patchpre-push.sample: Remove unwanted `IFS=' '`.

---
 templates/hooks--pre-push.sample | 1 -
 1 file changed, 1 deletion(-)

Signed-off-by: Jim Hill <gjthill@gmail.com>
---
 templates/hooks--pre-push.sample | 1 -
 1 file changed, 1 deletion(-)

Jim Hill <gjthill@gmail.com> writes:

Please clarify "unwanted" in the proposed commit log message.

It looks to me that the assignment very much deliberate. We know refnames and 40-hex object names do not contain SP, and the hook is fed (quoting from Documentation/githooks.txt) like this:

    Information about what is to be pushed is provided on the hook's standard
    input with lines of the form:

      <local ref> SP <local sha1> SP <remote ref> SP <remote sha1> LF

so setting IFS to SP alone smells as an attempt to ensure that the "read" in each loop iteration would split at SP and nothing else; Aaron Schrab CC'ed who did the original in 87c86dd1 (Add sample pre-push hook script, 2013-01-13).

Also you would notice by reading "git shortlog" of our history that s/Remove/remove/ on the subject line would avoid this entry stand out among others unnecessarily, but that is minor.

I call it unwanted because the default works fine with the actual input and explicitly limiting whitespace this way breaks most command substitution. For instance, attempting to check only new commits with

    range="$local_sha $(
            git for-each-ref --format='^%(refname)' refs/remotes/$remote
    )"
    # ...
    commit=`git rev-list -n 1 --grep '\bbad string\b' $range`

fails, because the newlines are passed verbatim instead of being treated as whitespace.

See http://stackoverflow.com/a/27392839/1290731 for the motivation.

Sorry for the capsing, and also for presuming ESP.

Jim Hill <gjthill@gmail.com> writes:

OK.  I'd call that "unnecessary", not "unwanted", though.

It becomes unwanted only when somebody cuts and pastes and changes what happens inside the body of the loop without thinking what IFS assignment is doing.

Leaving it to the default is not wrong per-se, but I think it is better to justify this change as protecting cut-and-paste people, which is its primary benefit as far as I can see.

Thanks for noticing.

Junio C Hamano <gitster@pobox.com> writes:

FYI, here is what I queued for today's integration cycle (you should be able to find it in 'pu' branch).

-- >8 --
From: Jim Hill <gjthill@gmail.com>
Date: Sun, 21 Dec 2014 11:26:00 -0800
Subject: [PATCH] pre-push.sample: remove unnecessary and misleading IFS=' '

The sample hook explicitly sets IFS to SP and nothing else so that the "read" used in the per-ref while loop that iterates over "<localref> SP <localsha1> SP <remoteref> SP <remotesha>" records, where we know refs and sha1s will not have SPs, would split them correctly.

While this is not wrong per-se, it is not necessary; because we know these fields do not contain HT or LF, either, we can simply leave IFS the default.

This will also prevent those who cut and paste from this sample from getting bitten when they write things in the per-ref loop that need splitting with the default $IFS (e.g. use $(git rev-list ...) to produce one-record-per-line output).

Signed-off-by: Jim Hill <gjthill@gmail.com>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 templates/hooks--pre-push.sample | 1 -
 1 file changed, 1 deletion(-)

Limiting the shell's word splitting breaks command substitution from e.g. `git rev-list` output, the motivating example is

    range="$local_sha $(
            git for-each-ref --format='^%(refname)' refs/remotes/$remote
    )"
    # ...
    commit=`git rev-list -n 1 --grep '\bbad string\b' $range`

which fails with IFS=' ' because newlines aren't valid in commit id's

Signed-off-by: Jim Hill <gjthill@gmail.com>
---
 templates/hooks--pre-push.sample | 1 -
 1 file changed, 1 deletion(-)