All of lore.kernel.org
 help / color / mirror / Atom feed
From: Junio C Hamano <gitster@pobox.com>
To: Jim Hill <gjthill@gmail.com>
Cc: git@vger.kernel.org, Aaron Schrab <aaron@schrab.com>
Subject: Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.
Date: Mon, 22 Dec 2014 18:08:59 -0800	[thread overview]
Message-ID: <xmqq7fxj5d8k.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <xmqqlhm0botq.fsf@gitster.dls.corp.google.com> (Junio C. Hamano's message of "Sun, 21 Dec 2014 14:49:53 -0800")

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

  reply	other threads:[~2014-12-23  2:09 UTC|newest]

Thread overview: 7+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2014-12-21 18:14 [PATCH] pre-push.sample: Remove unwanted `IFS=' '` Jim Hill
2014-12-21 18:25 ` [PATCH w/signoff] " Jim Hill
2014-12-21 18:50   ` Junio C Hamano
2014-12-21 19:12     ` Jim Hill
2014-12-21 22:49       ` Junio C Hamano
2014-12-23  2:08         ` Junio C Hamano [this message]
2014-12-21 19:26 ` [PATCH v2] pre-push.sample: remove " Jim Hill

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:
  https://en.wikipedia.org/wiki/Posting_style#Interleaved_style

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \
    --in-reply-to=xmqq7fxj5d8k.fsf@gitster.dls.corp.google.com \
    --to=gitster@pobox.com \
    --cc=aaron@schrab.com \
    --cc=git@vger.kernel.org \
    --cc=gjthill@gmail.com \
    /path/to/YOUR_REPLY

  https://kernel.org/pub/software/scm/git/docs/git-send-email.html

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.