Make pre-commit Git hook POSIX compliant and executable - #629
Conversation
This allows for symlinking it in .git/hooks/ directly[1] or executing it from within an already existing `pre-commit` hook/script without calling out to an interpreter in there[2], thus keeping up with any upstream changes to it automatically. Follow-up to e3b39b7 / #622 [1] `ln -s ../../scripts/git/pre-commit .git/hooks/pre-commit` [2] `./scripts/git/pre-commit` vs. `/bin/sh ./scripts/git/pre-commit`
On a lot of systems using bashisms in /bin/sh scripts will be fine, since /bin/sh is a symlink to /bin/bash, but some systems symlink it to the more lightweight (and more strictly POSIX) /bin/dash or some other shell. Safest thing is to not assume a bashism will work. What has been changed: 1) `[[ … ]]` is a bashism. Replaced with `[ … ]`. 2) `-eq` is for numeric operations. When a variable is quoted, it becomes a string and uses `=` for comparison. 3) `&&` is a shell thing, but does not exist in `test`'s syntax. (`[ … ]` is shorthand for `test …`.) `-a` is the "and" when talking `test`.
96ccf6c to
abd7970
Compare
|
This doesn't seem to have worked, because it's included a POT file with only the creation date changed. 😕 |
|
Yeah, in the first of the two commits. Which is how I noticed the error in the hook script and made the second commit. I actually rebased it originally to remove the edit to the .pot, but I guess it snuck in again when I re-rebased it to fix a mistake in the |
|
Oh, so the I've tested your changes out and they work for me. 👍 Though I have discovered that the hook isn't executed if I commit through GitHub Desktop, because it isn't finding xgettext in my PATH, so now I've got to figure why its PATH is different... |
Yeah, exactly. And actually, on that note - it will probably be better to negate the checks. That is, use |
|
Hm. No. I guess that won't work the way I naïvely imagined it. |
|
At half past ten in the evening I couldn't think of a way to do what I wanted to do without making the script significantly more complex. The script works now (for me :)), so merging it as it is now. |
2 participants
See individual commit messages for reasoning.