{"thread":{"id":"38217","subject":"[PATCH] pre-push.sample: Remove unwanted `IFS=' '`.","startedAt":"2014-12-21T18:14:25Z","lastAt":"2014-12-23T02:08:59Z","messageCount":7,"participants":["Jim Hill","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"253891","messageId":"1419185665-19988-1-git-send-email-gjthill@gmail.com","threadId":"38217","inReplyTo":null,"subject":"[PATCH] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2014-12-21T18:14:25Z","receivedAt":"2014-12-21T18:14:25Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"---\n templates/hooks--pre-push.sample | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample\nindex 69e3c67..6187dbf 100755\n--- a/templates/hooks--pre-push.sample\n+++ b/templates/hooks--pre-push.sample\n@@ -24,7 +24,6 @@ url=\"$2\"\n \n z40=0000000000000000000000000000000000000000\n \n-IFS=' '\n while read local_ref local_sha remote_ref remote_sha\n do\n \tif [ \"$local_sha\" = $z40 ]\n-- \n2.2.1.212.g51be871\n"},{"id":"253892","messageId":"1419186337-20348-1-git-send-email-gjthill@gmail.com","threadId":"38217","inReplyTo":"1419185665-19988-1-git-send-email-gjthill@gmail.com","subject":"[PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2014-12-21T18:25:37Z","receivedAt":"2014-12-21T18:25:37Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"Signed-off-by: Jim Hill \n---\n templates/hooks--pre-push.sample | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample\nindex 69e3c67..6187dbf 100755\n--- a/templates/hooks--pre-push.sample\n+++ b/templates/hooks--pre-push.sample\n@@ -24,7 +24,6 @@ url=\"$2\"\n \n z40=0000000000000000000000000000000000000000\n \n-IFS=' '\n while read local_ref local_sha remote_ref remote_sha\n do\n \tif [ \"$local_sha\" = $z40 ]\n-- \n2.2.1.212.g51be871\n"},{"id":"253894","messageId":"xmqqtx0obzwm.fsf@gitster.dls.corp.google.com","threadId":"38217","inReplyTo":"1419186337-20348-1-git-send-email-gjthill@gmail.com","subject":"Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-21T18:50:33Z","receivedAt":"2014-12-21T18:50:33Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill writes:\n\n> Signed-off-by: Jim Hill \n> ---\n\nPlease clarify \"unwanted\" in the proposed commit log message.\n\nIt looks to me that the assignment very much deliberate. We know\nrefnames and 40-hex object names do not contain SP, and the hook is\nfed (quoting from Documentation/githooks.txt) like this:\n\n Information about what is to be pushed is provided on the hook's standard\n input with lines of the form:\n\n SP SP SP LF\n\nso setting IFS to SP alone smells as an attempt to ensure that the\n\"read\" in each loop iteration would split at SP and nothing else;\nAaron Schrab CC'ed who did the original in 87c86dd1 (Add sample\npre-push hook script, 2013-01-13).\n\nAlso you would notice by reading \"git shortlog\" of our history that\ns/Remove/remove/ on the subject line would avoid this entry stand out\namong others unnecessarily, but that is minor.\n\n> templates/hooks--pre-push.sample | 1 -\n> 1 file changed, 1 deletion(-)\n>\n> diff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample\n> index 69e3c67..6187dbf 100755\n> --- a/templates/hooks--pre-push.sample\n> +++ b/templates/hooks--pre-push.sample\n> @@ -24,7 +24,6 @@ url=\"$2\"\n> \n> z40=0000000000000000000000000000000000000000\n> \n> -IFS=' '\n> while read local_ref local_sha remote_ref remote_sha\n> do\n> \tif [ \"$local_sha\" = $z40 ]\n"},{"id":"253898","messageId":"CAEE75_0Ff7NfQYUPrA414N9E0AE6LsS2zs0kL=BJ25bjPgom_w@mail.gmail.com","threadId":"38217","inReplyTo":"xmqqtx0obzwm.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2014-12-21T19:12:45Z","receivedAt":"2014-12-21T19:12:45Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"I call it unwanted because the default works fine with the actual\ninput and explicitly limiting whitespace this way breaks most command\nsubstitution. For instance, attempting to check only new commits with\n\n range=\"$local_sha $(\n git for-each-ref --format='^%(refname)' refs/remotes/$remote\n )\"\n # ...\n commit=`git rev-list -n 1 --grep '\\bbad string\\b' $range`\n\n\nfails, because the newlines are passed verbatim instead of being\ntreated as whitespace.\n\nSee http://stackoverflow.com/a/27392839/1290731 for the motivation.\n\nSorry for the capsing, and also for presuming ESP.\n"},{"id":"253900","messageId":"1419189960-21264-1-git-send-email-gjthill@gmail.com","threadId":"38217","inReplyTo":"1419185665-19988-1-git-send-email-gjthill@gmail.com","subject":"[PATCH v2] pre-push.sample: remove unwanted `IFS=' '`.","fromName":"Jim Hill","fromEmail":"gjthill@gmail.com","sentAt":"2014-12-21T19:26:00Z","receivedAt":"2014-12-21T19:26:00Z","isPatch":true,"sender":{"key":"gjthill@gmail.com","avatar":"https://avatars.githubusercontent.com/u/80352?v=4"},"body":"Limiting the shell's word splitting breaks command substitution from\ne.g. `git rev-list` output, the motivating example is\n\n range=\"$local_sha $(\n git for-each-ref --format='^%(refname)' refs/remotes/$remote\n )\"\n # ...\n commit=`git rev-list -n 1 --grep '\\bbad string\\b' $range`\n\nwhich fails with IFS=' ' because newlines aren't valid in commit id's\n\nSigned-off-by: Jim Hill \n---\n templates/hooks--pre-push.sample | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample\nindex 69e3c67..6187dbf 100755\n--- a/templates/hooks--pre-push.sample\n+++ b/templates/hooks--pre-push.sample\n@@ -24,7 +24,6 @@ url=\"$2\"\n \n z40=0000000000000000000000000000000000000000\n \n-IFS=' '\n while read local_ref local_sha remote_ref remote_sha\n do\n \tif [ \"$local_sha\" = $z40 ]\n-- \n2.2.1.212.g51be871\n"},{"id":"253910","messageId":"xmqqlhm0botq.fsf@gitster.dls.corp.google.com","threadId":"38217","inReplyTo":"CAEE75_0Ff7NfQYUPrA414N9E0AE6LsS2zs0kL=BJ25bjPgom_w@mail.gmail.com","subject":"Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-21T22:49:53Z","receivedAt":"2014-12-21T22:49:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Hill writes:\n\n> I call it unwanted because the default works fine with the actual\n> input and explicitly limiting whitespace this way breaks most command\n> substitution.\n\nOK. I'd call that \"unnecessary\", not \"unwanted\", though.\n\nIt becomes unwanted only when somebody cuts and pastes and changes\nwhat happens inside the body of the loop without thinking what IFS\nassignment is doing.\n\nLeaving it to the default is not wrong per-se, but I think it is\nbetter to justify this change as protecting cut-and-paste people,\nwhich is its primary benefit as far as I can see.\n\nThanks for noticing.\n"},{"id":"253983","messageId":"xmqq7fxj5d8k.fsf@gitster.dls.corp.google.com","threadId":"38217","inReplyTo":"xmqqlhm0botq.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH w/signoff] pre-push.sample: Remove unwanted `IFS=' '`.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-12-23T02:08:59Z","receivedAt":"2014-12-23T02:08:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> Jim Hill writes:\n>\n>> I call it unwanted because the default works fine with the actual\n>> input and explicitly limiting whitespace this way breaks most command\n>> substitution.\n>\n> OK. I'd call that \"unnecessary\", not \"unwanted\", though.\n>\n> It becomes unwanted only when somebody cuts and pastes and changes\n> what happens inside the body of the loop without thinking what IFS\n> assignment is doing.\n>\n> Leaving it to the default is not wrong per-se, but I think it is\n> better to justify this change as protecting cut-and-paste people,\n> which is its primary benefit as far as I can see.\n>\n> Thanks for noticing.\n\nFYI, here is what I queued for today's integration cycle (you should\nbe able to find it in 'pu' branch).\n\n-- >8 --\nFrom: Jim Hill \nDate: Sun, 21 Dec 2014 11:26:00 -0800\nSubject: [PATCH] pre-push.sample: remove unnecessary and misleading IFS=' '\n\nThe sample hook explicitly sets IFS to SP and nothing else so that\nthe \"read\" used in the per-ref while loop that iterates over\n\" SP SP SP \" records,\nwhere we know refs and sha1s will not have SPs, would split them\ncorrectly.\n\nWhile this is not wrong per-se, it is not necessary; because we know\nthese fields do not contain HT or LF, either, we can simply leave\nIFS the default.\n\nThis will also prevent those who cut and paste from this sample from\ngetting bitten when they write things in the per-ref loop that need\nsplitting with the default $IFS (e.g. use $(git rev-list ...) to\nproduce one-record-per-line output).\n\nSigned-off-by: Jim Hill \nSigned-off-by: Junio C Hamano \n---\n templates/hooks--pre-push.sample | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/templates/hooks--pre-push.sample b/templates/hooks--pre-push.sample\nindex 69e3c67..6187dbf 100755\n--- a/templates/hooks--pre-push.sample\n+++ b/templates/hooks--pre-push.sample\n@@ -24,7 +24,6 @@ url=\"$2\"\n \n z40=0000000000000000000000000000000000000000\n \n-IFS=' '\n while read local_ref local_sha remote_ref remote_sha\n do\n \tif [ \"$local_sha\" = $z40 ]\n-- \n2.2.1-321-gd161b79\n"}]}