I wish this were merged on priority. But does this work on Windows?

You're using fs.existsSync at line no. 39 which is probably deprecated. Check.

About working on Windows: I don't use any OS specific code here so I can say with a high confidence that it should on Windows. I don't have any way of proving that, because I only have OSX and Linux.

About fs.existsSync deprecation: I had forgotten about that. Mostly because I don't really agree with it. Regardless, I won't debate the API so I removed it and wrote a manual check using statSync.

I'd like for it to be merged too, but I don't see much activity on this repo so I don't expect it to be merged any time soon.

Looks good to me.

Seems like I forgot to post this, but I already fixed the problems described above:

diff --git a/src/touch.js b/src/touch.js
index eee2237..bb7f5d2 100644
--- a/src/touch.js
+++ b/src/touch.js
@@ -1,5 +1,8 @@
 var common = require('./common');
 var fs = require('fs');
+var constants = require('constants');
+var O_CREAT = constants.O_CREAT || 0;
+var O_WRONLY = constants.O_WRONLY || 0;

 module.exports = _touch;

@@ -36,19 +39,21 @@ function _touch(opts, file) {
                common.error('cannot touch directory ' + file);
        }

-       if (fileExists(file) && fs.statSync(file).isDirectory()) {
+       var stat = tryToStat(file);
+
+       if (stat && stat.isDirectory()) {
                common.error('cannot touch directory ' + file);
        }

        // if the file doesn't already exist and the user has specified --no-create then
        // this script is finished
-       if (!fileExists(file) && opts.no_create) {
+       if (!stat && opts.no_create) {
                return;
        }

        // open the file and then close it. this will create it if it doesn't exist but will
        // not wipe out it's contents
-       fs.closeSync(fs.openSync(file, 'a+'));
+       fs.closeSync(fs.openSync(file, O_WRONLY | O_CREAT));

        //
        // Set timestamps
@@ -61,7 +66,6 @@ function _touch(opts, file) {

        // use reference file
        if (opts.reference) {
-               var stat = fs.statSync(file);
                mtime = stat.mtime;
                atime = stat.atime;
        } else if (opts.date) {
@@ -70,20 +74,19 @@ function _touch(opts, file) {
        }

        if (opts.atime) {
-               mtime = fs.statSync(file).mtime;
+               mtime = stat.mtime;
        } else if (opts.mtime) {
-               atime = fs.statSync(file).atime;
+               atime = stat.atime;
        }

        fs.utimesSync(file, atime, mtime);
 }

-function fileExists(filePath) {
+function tryToStat(filePath) {
        try {
-               fs.statSync(filePath, fs.R_OK | fs.W_OK);
-               return true;
+               return fs.statSync(filePath);
        } catch (e) {
-               return false;
+               return undefined;
        }
 }

I want to know what the purpose of this is and why it's necessary:

-       fs.closeSync(fs.openSync(file, 'a+'));
+       fs.closeSync(fs.openSync(file, O_WRONLY | O_CREAT));

@blockloop it is to imitate touch

I'm not sure that this specific piece will make much of a difference. The a+ flag translates to similar flags which accomplish the goal without having to require another module.

@blockloop thanks for the link! The link suggests that the a and w flags translate even more closely. Would it be better to use a over a+? I think we should keep dependencies at a minimum (see #122).

@TimothyGu could you provide an example where the behavior would differ between the a flag and O_WRONLY | O_CREAT?

I think the point I'm trying to make is that the purpose of this line of code is to Try to open FILE, creating it if necessary (as the comment in the GNU source says) and either of these flags appear solve that problem. The only other caveat is that we don't want to truncate the file and that is covered.

Would you be able to reformat the files to use a two-space indent instead of tabs? That'll keep things stylistically similar.

@nfischer practically, no, but I prefer to keep the syscalls exactly the same as coreutils. It doesn't matter much either way so I'm not going to insist on this.

I applied all of the discussed changes. I also added an .editorconfig file to avoid indent issues

Since I changed the indentation you'll want to append ?w=1 to see the actual changes in the most recent diff. I wish I would have changed the whitespaces as a separate commit.

Anything else need changing?

Could you rebase off master and squash your commits into one?

I fixed a bug with the README, so this should fix the merge conflict.

Done

LGTM 👍 @ariporad what do you think?

Although I don't think this handles every possible edge case, I think this handles enough of them that it's ready to be a feature. If there are any other significant edge cases, they'll be observed by users, and can be added as features/bug fixes down the road.

Other than the comments I made (mostly regarding the .editorconfig), LGTM!

The CI failed the first time, but seems to be passing now. I'll merge for now, and we can investigate more closely if the break occurs again.

👍 Great to see this feature merged back right in.

But why isn't the shelljs bumped up from 0.5.3 to something to reflect these changes?

@marvindanig You raise a great point! I've been more concerned with merging new changes and bug fixes, and less concerned with making a release. I think we will bump the version number once for when we make our next release. If you need the version number bumped right now (like if you're checking package.json in the git repo), then I can do that.

👍 Nya, I'm good. Just that I didn't realize about this feature being available until I came back to this repo again (today). Otherwise npm check updates would have reflected if it was versioned up.

Looking at the rate at which this library is used I think it would good to bake in semver right away.

Yeah, we're going to use 100% semver staring with he next release. (Because we're <1.0.0, it will be a minor bump).

On Mon, Jan 18, 2016 at 8:22 PM -0800, "Marvin Danig" notifications@github.com wrote:

Nya, I'm good. Just that I didn't realize about this feature being available until I came back to this repo again (today). Otherwise npm check updates would have reflected if it was versioned up.

Looking at the rate at which this library is used I think it would good to bake in semver right away.

—
Reply to this email directly or view it on GitHub.

^ Yup, I was thinking the same thing, and hopefully we'll hit v1.0 within the month (if we can add enough features).