Git pre-commit hook doesn't work with BSD grep.

[adriantrunzo]

The pre-commit hook was recently changed in commit bd5445c, but unfortunately the use of grep -z (after git-diff) doesn't seem valid for BSD grep. To note I am on macOS 10.13.1 and have verified the following on both the system bash (GNU bash, version 3.2.57(1)-release (x86_64-apple-darwin17) and brew installed bash (GNU bash, version 4.4.12(1)-release (x86_64-apple-darwin16.3.0).

In this case the -z option is meant to handle null-terminated input lines created with git-diff -z (and later parsed by xargs -0). However, grep -z on macOS/BSD is defined as:

-Z, -z, --decompress
             Force grep to behave as zgrep.

That definition is obviously not want we want, and unfortunately seems to be subject to it's own issues, namely if you try to use the -z option you will see grep: invalid option -- z.

To make the pre-commit hook work in my setup I changed the grep -z to grep -a --null, which is not quite functionally equivalent (treating input as text vs null-terminated lines) but close enough to get things working. I believe the -a and --null options are the same across most grep versions.

[Flet]

@ttencate thoughts on this?

[ttencate]

Indeed -z/--null-data is not in the POSIX standard, my bad!

The -Z/--null flag is also nonstandard, but it is not what we need anyway: it outputs a null byte after the filename, not after each matched line. At least, that's what it does on my system, Arch Linux with GNU grep 3.1:

  -Z, --null
         Output  a  zero  byte  (the  ASCII  NUL character)
         instead of the character that normally  follows  a
         file  name.   For example, grep -lZ outputs a zero
         byte after each file name  instead  of  the  usual
         newline.     [...]

Instead of nulls, we could use newlines (\n) as the delimiter, which is almost as safe as \0:

git diff --name-only --cached --relative | grep '\.jsx\?$' | xargs-r -d'\n' -t standard

But then we'd need the -d/--delimiter argument to xargs, which is also nonstandard. In fact, xargs -0 was never POSIX to begin with...

Here's a useful reference: Properly escaping output from pipe in xargs. If your xargs doesn't support any change in delimiter, the only choice is to pass it escaped input. Based on that, and the example in the POSIX xargs manpage, I believe this should be 100% portable:

git diff --name-only --cached --relative | grep '\.jsx\?$' | sed 's/[^[:alnum:]]/\\&/g' | xargs-r -E '' -t standard

I also added -E '' because POSIX leaves it unspecified whether eofstr defaults to _ or to null.

[adriantrunzo]

I had misread the definition of -Z/--null, thanks for the explanation. The links you provided are quite helpful for understanding escaping output and xargs usage.

That sed example from the POSIX docs for xargs fits well here and is actually pretty readable. I've tested that updated sequence in my setup and it is working as expected. I believe the xargs-r function needs attention, though, as the hook fails there (I can bypass xargs-r and it's fine). I am guessing because we changed the delimiter? In conjunction with the updated git diff ... line above, I changed xargs-r to the following:

# Ensure all JavaScript files staged for commit pass standard code style
function xargs-r() {
  # Portable version of "xargs -r". The -r flag is a GNU extension that
  # prevents xargs from running if there are no input files.
  if IFS= read -r path; then
    { echo "$path"; cat; } | xargs $@
  fi
}

By default read should use a newline so I removed -d ''. Additionally, it doesn't seem like we need to echo a \0 anymore. The POSIX doc for echo (http://pubs.opengroup.org/onlinepubs/009695399/utilities/echo.html) states "Implementations shall not support any options.", so -n is absent in my update as well.

I appreciate you guys looking into this issue with me. This code is definitely stretching my understanding of xargs, sed and bash input/output handling so let me know if I am off the mark.

[ttencate]

Ah, I missed the required changes in xargs-r, thanks. But I think it should be read -r -d"\n" because we don't want read to split on anything but newlines.

Your handling of echo looks right to me: it appends a newline by default, which is exactly what we need.

[adriantrunzo]

Makes sense to be more explicit. I think it needs to be read -r -d $'\n' to conform to ANSI-C quoting: https://www.gnu.org/software/bash/manual/html_node/ANSI_002dC-Quoting.html.

Should I open a pull request with all of these changes?

[naturalethic]

Until someone finds the best canonical solution, this is working for us:

#!/bin/bash

git diff --name-only --relative --cached | grep '\.jsx\?$' | xargs -t standard
if [[ $? -ne 0 ]]; then
  echo 'JavaScript Standard Style errors were detected. Aborting commit.'
  exit 1
fi

[stale]

This issue has been automatically marked as stale because it has not had recent activity. It will be closed if no further activity occurs.

[feross]

@naturalethic @ttencate Do you think we ought to mention @naturalethic's workaround in the readme for OS X users? We can also recommend that the user install GNU grep with brew install grep which gives them a ggrep binary which works as intended.

Is that a better approach? Can one of you make a decision about which approach is preferred and send a PR to add it to the docs? 😇

[ttencate]

@feross Ideally, pre-commit hook scripts should be portable across platforms without requiring anyone to install anything (at least on POSIX platforms). I think @adriantrunzo's #1025 contains all our combined efforts and no known issues, so why not just merge it?

[feross]

@ttencate Good point, didn't see that there was already a PR for this. Merged.