[PATCH] pre-push.sample: Remove unwanted `IFS=' '`.

[PATCH] pre-push.sample: Remove unwanted `IFS=' '`.

[Jim Hill]

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

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
index 69e3c67..6187dbf 100755
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -24,7 +24,6 @@ url="$2"
 
 z40=0000000000000000000000000000000000000000
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
-- 
2.2.1.212.g51be871

[PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.

[Jim Hill]

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

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
index 69e3c67..6187dbf 100755
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -24,7 +24,6 @@ url="$2"
 
 z40=0000000000000000000000000000000000000000
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
-- 
2.2.1.212.g51be871

Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.

[Junio C Hamano]

Jim Hill <gjthill@gmail.com> writes:

> Signed-off-by: Jim Hill <gjthill@gmail.com>
> ---

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.

>  templates/hooks--pre-push.sample | 1 -
>  1 file changed, 1 deletion(-)
>
> diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
> index 69e3c67..6187dbf 100755
> --- a/templates/hooks--pre-push.sample
> +++ b/templates/hooks--pre-push.sample
> @@ -24,7 +24,6 @@ url="$2"
>  
>  z40=0000000000000000000000000000000000000000
>  
> -IFS=' '
>  while read local_ref local_sha remote_ref remote_sha
>  do
>  	if [ "$local_sha" = $z40 ]

Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.

[Jim Hill]

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.

Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.

[Junio C Hamano]

Jim Hill <gjthill@gmail.com> writes:

> I call it unwanted because the default works fine with the actual
> input and explicitly limiting whitespace this way breaks most command
> substitution.

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.

Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.

[Junio C Hamano]

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

> Jim Hill <gjthill@gmail.com> writes:
>
>> I call it unwanted because the default works fine with the actual
>> input and explicitly limiting whitespace this way breaks most command
>> substitution.
>
> 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.

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(-)

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
index 69e3c67..6187dbf 100755
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -24,7 +24,6 @@ url="$2"
 
 z40=0000000000000000000000000000000000000000
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
-- 
2.2.1-321-gd161b79

[PATCH v2] pre-push.sample: remove unwanted `IFS=' '`.

[Jim Hill]

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(-)

diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample
index 69e3c67..6187dbf 100755
--- a/templates/hooks--pre-push.sample
+++ b/templates/hooks--pre-push.sample
@@ -24,7 +24,6 @@ url="$2"
 
 z40=0000000000000000000000000000000000000000
 
-IFS=' '
 while read local_ref local_sha remote_ref remote_sha
 do
 	if [ "$local_sha" = $z40 ]
-- 
2.2.1.212.g51be871