Skip to content
This repository has been archived by the owner on Sep 9, 2020. It is now read-only.

Update Pre Commit Hook Script #89

Open
wants to merge 5 commits into
base: master
Choose a base branch
from

Conversation

Marto32
Copy link

@Marto32 Marto32 commented Feb 7, 2017

This PR updates the pre-commit hook scripts:

  • pre-commit.git-lint.sh
  • pre-commit.hg-lint.sh

These changes are needed as the previous command to initialize git lint used xargs flags that were not available on OSX (namely --null and --no-run-if-empty). The updated scripts should perform the same functionality and will run on OSX.

This is addressed in issue #88

@coveralls
Copy link

coveralls commented Feb 7, 2017

Coverage Status

Changes Unknown when pulling 5727349 on Marto32:mm/pre-commit-updates into ** on sk-:master**.

Copy link
Owner

@sk- sk- left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Thanks for the change. Could you update the PR, so that the change retains the same functionality as before.

then
exit 0;
else
git lint;
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Note that this is not exactly the same as the original script. The original script was linting only files being commited.

Here it should read something like:

echo $DIFF_FILES | xargs -0 git lint

The same applies for the mercurial hook.

@@ -14,8 +14,13 @@
# limitations under the License.

# First part return the files being commited, excluding deleted files.
git diff-index -z --cached HEAD --name-only --diff-filter=ACMRTUXB |
xargs --null --no-run-if-empty git lint;
DIFF=$(git diff-index -z --cached HEAD --name-only --diff-filter=ACMRTUXB)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Could you rename to something like DIFF_FILES.

@coveralls
Copy link

coveralls commented Feb 8, 2017

Coverage Status

Changes Unknown when pulling 7092839 on Marto32:mm/pre-commit-updates into ** on sk-:master**.

@@ -14,8 +14,13 @@
# limitations under the License.

# First part return the files being commited, excluding deleted files.
git diff-index -z --cached HEAD --name-only --diff-filter=ACMRTUXB |
xargs --null --no-run-if-empty git lint;
DIFF_FILES=$(git diff-index -z --cached HEAD --name-only --diff-filter=ACMRTUXB)
Copy link
Owner

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Bash does not store new lines nor \0, so the above won't work. See http://stackoverflow.com/questions/6570531/assign-string-containing-null-character-0-to-a-variable-in-bash#answer-22985397

Apparently the only bullet proof solution is to save the result to a file, but that has the complications of deleting the temporary file even in case of errors.

Another alternative is to run the command twice. The first time to check whether the output is empty. And the second piped to xargs.

Copy link
Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Would it be possible to convert this to a python script and store those results as variables? Or does the hook have to be a shell script?

Copy link
Author

@Marto32 Marto32 Feb 14, 2017

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

@sk- I opted for the latter solution and used the variable to check if there is output, if there is, I reran the command and piped it into xargs. I tested the git script by running it manually line by line - works on OSX 10.11.6 (I did not test mercurial).

@coveralls
Copy link

coveralls commented Feb 14, 2017

Coverage Status

Changes Unknown when pulling 7d93f36 on Marto32:mm/pre-commit-updates into ** on sk-:master**.

@Marto32
Copy link
Author

Marto32 commented Mar 1, 2017

@sk- circling back on this

@avoltz
Copy link

avoltz commented Dec 8, 2018

Is this PR still going?

This is a different change, but it seems to be working for me on OSX mojave. I think -L 1 has the same effect as --no-run-if-empty, but I'm not 100% sure.

Let me know if you want me to send a PR.

diff --git a/scripts/pre-commit.git-lint.sh b/scripts/pre-commit.git-lint.sh
index d432bdb..d1141b1 100755
--- a/scripts/pre-commit.git-lint.sh
+++ b/scripts/pre-commit.git-lint.sh
@@ -15,7 +15,7 @@

 # First part return the files being commited, excluding deleted files.
 git diff-index -z --cached HEAD --name-only --diff-filter=ACMRTUXB |
-xargs --null --no-run-if-empty git lint;
+xargs -0 -L 1 git lint;

 if [ "$?" != "0" ]; then
   echo "There are some problems with the modified files.";
diff --git a/scripts/pre-commit.hg-lint.sh b/scripts/pre-commit.hg-lint.sh
index 4a7cacf..d82785f 100644
--- a/scripts/pre-commit.hg-lint.sh
+++ b/scripts/pre-commit.hg-lint.sh
@@ -19,7 +19,7 @@ if [ "$NO_VERIFY" != "" ]; then
 fi

 hg status --change $HG_NODE | cut -b 3- | tr '\n' '\0' |
-xargs --null --no-run-if-empty git-lint;
+xargs -0 -L 1 git-lint;

 if [ "$?" != "0" ]; then
   echo "There are some problems with the modified files.";

Sign up for free to subscribe to this conversation on GitHub. Already have an account? Sign in.
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants