Added support for symbolic links to scripts

[shdwjk]

Also changed path parameter to not hide global path instance.

[coveralls]

Coverage decreased (-0.4%) to 93.578% when pulling 4bea73c7a4fee3a1099d633880fa404f882d84c2 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage decreased (-0.4%) to 93.578% when pulling 4bea73c7a4fee3a1099d633880fa404f882d84c2 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage decreased (-0.4%) to 93.578% when pulling 4bea73c7a4fee3a1099d633880fa404f882d84c2 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage decreased (-0.4%) to 93.578% when pulling 4bea73c7a4fee3a1099d633880fa404f882d84c2 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage increased (+0.6%) to 94.545% when pulling 52c3db665f682d033f88180304d7517d9e736cd1 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage increased (+0.6%) to 94.545% when pulling 52c3db665f682d033f88180304d7517d9e736cd1 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[coveralls]

Coverage increased (+0.6%) to 94.545% when pulling 52c3db665f682d033f88180304d7517d9e736cd1 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[shdwjk]

Woot. All tests passing. =D The polyfil function I used for path.isAbsolute() is a bit simplistic and doesn't support windows, but since windows isn't supported now, I don't feel like that's a problem... I should probably make sure there is a test for absolute paths...

[coveralls]

Coverage increased (+0.6%) to 94.545% when pulling 9a9762fdcbb24eee1dfd3f61f79a126b9c900f6f on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[tarmolov]

Could you explain why you need symlinks in .githooks directories?

[shdwjk]

Currently, this is how I'm using it:

> tree
.
├── __scripts__
│   ├── buildAndCommitDistBundle.sh
│   └── runTests.sh
├── post-commit
│   └── 01.runTests -> ../__scripts__/runTests.sh
└── pre-push
    ├── 01.runTests -> ../__scripts__/runTests.sh
    └── 02.buildAndCommitDistBundle -> ../__scripts__/buildAndCommitDistBundle.sh

3 directories, 5 files

[shdwjk]

I created #45 where I talked about the value of symlinks.

[tarmolov]

So, you run this commands in two ways:

• manually
• hooks

It's a good use case.

We usually put all commands to run tests in package.json or Makefile and then write a simple shell scripts to run needed command.

It's more flexible solution because you have a point of extension in hook without changing main scripts. Maybe you embrace such approach as well and git-hooks can keep living without symlink support? :)

[shdwjk]

Those examples I only run as part of the hooks. Here's an example of one of them:

> cat runTests.sh
#!/usr/bin/env bash

SOURCE="${BASH_SOURCE[0]}"
while [ -h "$SOURCE" ]; do # resolve $SOURCE until the file is no longer a symlink
  DIR="$( cd -P "$( dirname "$SOURCE" )" && pwd )"
  SOURCE="$(readlink "$SOURCE")"
  [[ $SOURCE != /* ]] && SOURCE="$DIR/$SOURCE" # if $SOURCE was a relative symlink, we need to resolve it relative to the path where the symlink file was located
done
DIR="$( cd -P "$( dirname "$SOURCE" )" && pwd )"

cd $DIR/../../
if hash yarn 2>/dev/null; then
    yarn test
elif hash npm 2>/dev/null; then
    npm test
else
    echo "ERROR: Please install yarn or npm to build package correctly before commit."
    exit 1
fi

exit $?

In this case, I'm wanting to have tests run automatically when I commit (but not prevent the commit). I want to have tests run before pushing and have failing tests fail the push (and also prevent the build).

I can foresee adding more scripts that are executed for other hooks, and likely linked from multiple of them, I just haven't written them yet. =D

It's true, I could solve this in other ways, but this is the most straightforward method, and mimics other similar systems (init.d run levels). It allows writing the code in one place without duplication that could lead to variation in behavior. Besides, it seems like reasonable expected behavior, so adding support for it follows the principle of least surprise.

Is there some reason you don't want support for it?

[tarmolov]

I've just wanted to double check that this feature is really needed for you :)

I'll take a look to your pr soon. Is it urgent for you? Or you can live without this feature for a while?

[shdwjk]

Not urgent. I'm using it from my for for the Proof of Concept work in my current project. It will be a few weeks before it moves to the wider development group.

Thanks!

[tarmolov]

I think it's better to add getFileStat to fs-helpers like this:

    /**
     * @param {String} filepath
     * @returns {Boolean}
     */
    getFileStat: function (filepath) {
        var stat = fs.lstatSync(filepath);

        if (stat.isSymbolicLink()) {
            try {
                var symlinkPath = fs.readlinkSync(filepath);
                var newFilePath = path.join(path.dirname(filepath), symlinkPath);
                return helpers.getFileStat(newFilePath);
            } catch (err) {
                return stat;
            }
        } else {
            return stat;
        }
    },

And use it everywhere instead of fs.lstatSync.

[shdwjk]

Extracting the logic dealing with stat does seem like a good idea since it's duplicated in isExecutable().

Simplistically adding what you have above and changing isFileOrValidSymlink() to use it:

    /**
     * @param {String} filepath
     * @returns {Boolean}
     */
    isFileOrValidSymlink: function (filepath) {
        var stats = this.getFileStat(filepath);
        return stats.isFile();
    },

Failed 3 of the tests, the 2 for absolute path symbolic links, and one other which is dependent on absolute paths. I think you've got an assumption that path.join() will throw if the path doesn't exist and thus will allow the above to fallback on the passed in path and assume it is absolute.

Moving the logic I had duplicated across isFileOrValidSymlink() and isExecutable() does pass those tests.

    /**
     * @param {String} filepath
     * @returns {Boolean}
     */
    getFileStat: function (filepath) {
        var stats = fs.lstatSync(filepath);

        if (stats.isSymbolicLink()){
            var linkPath = fs.readlinkSync(filepath);
            linkPath = ((path.isAbsolute || helpers.isAbsolutePathUnix)(linkPath) ?
                linkPath :
                path.resolve(path.dirname(filepath), linkPath)
            );
            stats = fs.lstatSync(linkPath);
        }
        return stats;
    },

Do you see room for improvement in this function?

I'll commit these changes.

[coveralls]

Coverage increased (+0.5%) to 94.444% when pulling 2944334754920b642bd20e2981f4dc352b668a27 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.

[shdwjk]

I refined this slightly so that it doesn't assume the depth on a symlink. Also added detecting and ignoring broken symlinks and made sure symlink delete order wouldn't crash removeDir().

[coveralls]

Coverage decreased (-0.3%) to 93.636% when pulling 9aa1cbe1612ba7a7e34c068a61efa989b5a5f958 on shdwjk:SymLinkSupport into 52c04e2b04c8c6fc0c76f6e00809cf9fee413ee6 on tarmolov:master.