{"thread":{"id":"25129","subject":"[PATCH] git-rebase--interactive.sh: LF terminate line sent to cut","startedAt":"2010-09-17T14:17:43Z","lastAt":"2010-09-18T05:25:16Z","messageCount":7,"participants":["Chris Johnsen","Brandon Casey","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"150879","messageId":"60d13fc6a7d5b1b08f35f91b2d90eb7c13922390.1284733059.git.chris_johnsen@pobox.com","threadId":"25129","inReplyTo":null,"subject":"[PATCH] git-rebase--interactive.sh: LF terminate line sent to cut","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2010-09-17T14:17:43Z","receivedAt":"2010-09-17T14:17:43Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"Some versions of cut do not cope well with lines that do not end in\nan LF. Add '\\n' to the printf format string to ensure that the\ngenerated output ends in a LF.\n\nI found this problem when t3404's \"avoid unnecessary reset\" failed\ndue to the \"rebase -i\" not avoiding updating the tested timestamp.\n\nOn a Mac OS X 10.4.11 system:\n\n % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1\n cut: stdin: Illegal byte sequence\n % printf '%s\\n' 'foo bar' | /usr/bin/cut -d ' ' -f 1\n foo\n\nSigned-off-by: Chris Johnsen \n\n---\nIt looks like the cut on my system is derived from FreeBSD. It is\nprobably an old version though (possibly too old to care about).\n\nThe cut from GNU coreutils does not to have this problem, so using\nit serves as a workaround.\n---\n git-rebase--interactive.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex eb2dff5..834460a 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -626,7 +626,7 @@ skip_unnecessary_picks () {\n \t\tcase \"$fd,$command\" in\n \t\t3,pick|3,p)\n \t\t\t# pick a commit whose parent is current $ONTO -> skip\n-\t\t\tsha1=$(printf '%s' \"$rest\" | cut -d ' ' -f 1)\n+\t\t\tsha1=$(printf '%s\\n' \"$rest\" | cut -d ' ' -f 1)\n \t\t\tcase \"$(git rev-parse --verify --quiet \"$sha1\"^)\" in\n \t\t\t\"$ONTO\"*)\n \t\t\t\tONTO=$sha1\n-- \n1.7.3.rc2\n"},{"id":"150881","messageId":"XhMLJaG8mUbh4rzLnU3IrGDXbMd9-p7UFO6kn9Uke7n_H4NNOG6glg@cipher.nrlssc.navy.mil","threadId":"25129","inReplyTo":"60d13fc6a7d5b1b08f35f91b2d90eb7c13922390.1284733059.git.chris_johnsen@pobox.com","subject":"Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2010-09-17T15:10:31Z","receivedAt":"2010-09-17T15:10:31Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"On 09/17/2010 09:17 AM, Chris Johnsen wrote:\n> Some versions of cut do not cope well with lines that do not end in\n> an LF. Add '\\n' to the printf format string to ensure that the\n> generated output ends in a LF.\n> \n> I found this problem when t3404's \"avoid unnecessary reset\" failed\n> due to the \"rebase -i\" not avoiding updating the tested timestamp.\n> \n> On a Mac OS X 10.4.11 system:\n> \n> % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1\n> cut: stdin: Illegal byte sequence\n> % printf '%s\\n' 'foo bar' | /usr/bin/cut -d ' ' -f 1\n> foo\n\n\nOr we could write it like:\n\n sha1=${rest%% *}\n\nwhich I wish I had changed it to in the first place when I made some\nrecent modifications. The '%%' notation avoids the whole newline issue\nby not even spawning 'cut'. We are already using this construct in\ngit-filter-branch.sh and git-instaweb.sh, though those are not the\nmost visible scripts in git.\n\nDoes the above work on your FreeBSD system?\n\n-Brandon\n\n\n> Signed-off-by: Chris Johnsen \n> \n> ---\n> It looks like the cut on my system is derived from FreeBSD. It is\n> probably an old version though (possibly too old to care about).\n> \n> The cut from GNU coreutils does not to have this problem, so using\n> it serves as a workaround.\n> ---\n> git-rebase--interactive.sh | 2 +-\n> 1 files changed, 1 insertions(+), 1 deletions(-)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index eb2dff5..834460a 100755\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -626,7 +626,7 @@ skip_unnecessary_picks () {\n> \t\tcase \"$fd,$command\" in\n> \t\t3,pick|3,p)\n> \t\t\t# pick a commit whose parent is current $ONTO -> skip\n> -\t\t\tsha1=$(printf '%s' \"$rest\" | cut -d ' ' -f 1)\n> +\t\t\tsha1=$(printf '%s\\n' \"$rest\" | cut -d ' ' -f 1)\n> \t\t\tcase \"$(git rev-parse --verify --quiet \"$sha1\"^)\" in\n> \t\t\t\"$ONTO\"*)\n> \t\t\t\tONTO=$sha1\n"},{"id":"150901","messageId":"7vsk182p2q.fsf@alter.siamese.dyndns.org","threadId":"25129","inReplyTo":"XhMLJaG8mUbh4rzLnU3IrGDXbMd9-p7UFO6kn9Uke7n_H4NNOG6glg@cipher.nrlssc.navy.mil","subject":"Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-17T18:38:05Z","receivedAt":"2010-09-17T18:38:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Casey writes:\n\n> Or we could write it like:\n>\n> sha1=${rest%% *}\n>\n> which I wish I had changed it to in the first place when I made some\n> recent modifications.\n\nAgreed; the less use of 'cut' we see, the better ;-)\n\nAs to portability guideline in our shell script:\n\n ${param#word} ${param##word} ${param%word} ${param%%word}\n\nare permissible POSIX constructs (together with more traditional -/=/?/+), and\ntheir use is encouraged over 'cut', 'expr', etc. [*1*]\n\n ${param:ofs} ${param:ofs:len} ${param/pattern/string}\n\nare bashisms we avoid (unless of course in the bash completion script).\n\nWe do not seem to use ${#param}, not because it is forbidden, but I think\nbecause it is not very useful without ${param:ofs:len}.\n\n\n[Footnote]\n\n*1* In 2005 back when I took over the git maintenance, I used to be a lot\nmore conservative/traditionalist and as a result, you may see overused\n\"expr\" in contrib/examples/ and \"git log -p -- '*.sh'\" output. But we\nhave been eradicating them a bit by bit for the past few years.\n"},{"id":"150904","messageId":"VzbuextQE2-OASqyG4sJxmg1IuyBq5BWWiDERv0h-YQdVcnL8Enurg@cipher.nrlssc.navy.mil","threadId":"25129","inReplyTo":"7vsk182p2q.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut","fromName":"Brandon Casey","fromEmail":"brandon.casey.ctr@nrlssc.navy.mil","sentAt":"2010-09-17T18:59:34Z","receivedAt":"2010-09-17T18:59:34Z","isPatch":true,"sender":{"key":"brandon.casey.ctr@nrlssc.navy.mil","avatar":null},"body":"On 09/17/2010 01:38 PM, Junio C Hamano wrote:\n\n> Agreed; the less use of 'cut' we see, the better ;-)\n\nDouble agreed.\n\n> As to portability guideline in our shell script:\n> \n> ${param#word} ${param##word} ${param%word} ${param%%word}\n> \n> are permissible POSIX constructs (together with more traditional -/=/?/+), and\n> their use is encouraged over 'cut', 'expr', etc. [*1*]\n> \n> ${param:ofs} ${param:ofs:len} ${param/pattern/string}\n> \n> are bashisms we avoid (unless of course in the bash completion script).\n\nIt's been a long while since I've reviewed Documentation/CodingGuidelines,\nbut these are indeed in there, and have been for a very long time. Maybe\nI should refresh my memory more often. :)\n\n> We do not seem to use ${#param}, not because it is forbidden, but I think\n> because it is not very useful without ${param:ofs:len}.\n\nCodingGuidelines does say \"No strlen ${#parameter}\", so that could be part\nof the reason. But like you say, it's not very useful without ${param:ofs:len}.\n\n-Brandon\n"},{"id":"150910","messageId":"0eafa42f1da5f66465a1eb9da170416363cf72e0.1284759770.git.chris_johnsen@pobox.com","threadId":"25129","inReplyTo":"7vsk182p2q.fsf@alter.siamese.dyndns.org","subject":"[PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2010-09-17T21:42:51Z","receivedAt":"2010-09-17T21:42:51Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"Some versions of cut do not cope well with lines that do not end in\nan LF. In this case, we can completely avoid cut by using the\n${var%% *} parameter expansion (suggested by Brandon Casey).\n\nI found this problem when t3404's \"avoid unnecessary reset\" failed\ndue to the \"rebase -i\" not avoiding updating the tested timestamp.\n\nOn a Mac OS X 10.4.11 system:\n\n % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1\n cut: stdin: Illegal byte sequence\n\nSigned-off-by: Chris Johnsen \n\n---\n\nBrandon Casey wrote:\n> Or we could write it like:\n>\n> sha1=${rest%% *}\n>\n> Does the above work on your FreeBSD system?\n\nYes, as Junio points out, ${var%% *} is portable enough for Git.\nAfter this change t3404 passes here without GNU cut available.\n\nJunio C Hamano wrote:\n> Agreed; the less use of 'cut' we see, the better ;-)\n\nIt seems like the other uses of cut in git-rebase--interactive.sh\nwould be more awkward if they were replaced with equivalent\nprocessing done in-shell with parameter expansions. Eliminating them\nshould probably wait until after 1.7.3, if at all.\n---\n git-rebase--interactive.sh | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex eb2dff5..a27952d 100755\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -626,7 +626,7 @@ skip_unnecessary_picks () {\n \t\tcase \"$fd,$command\" in\n \t\t3,pick|3,p)\n \t\t\t# pick a commit whose parent is current $ONTO -> skip\n-\t\t\tsha1=$(printf '%s' \"$rest\" | cut -d ' ' -f 1)\n+\t\t\tsha1=${rest%% *}\n \t\t\tcase \"$(git rev-parse --verify --quiet \"$sha1\"^)\" in\n \t\t\t\"$ONTO\"*)\n \t\t\t\tONTO=$sha1\n-- \n1.7.3.rc2\n"},{"id":"150911","messageId":"7v8w302fu1.fsf@alter.siamese.dyndns.org","threadId":"25129","inReplyTo":"0eafa42f1da5f66465a1eb9da170416363cf72e0.1284759770.git.chris_johnsen@pobox.com","subject":"Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-09-17T21:57:42Z","receivedAt":"2010-09-17T21:57:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Chris Johnsen writes:\n\n> It seems like the other uses of cut in git-rebase--interactive.sh\n> would be more awkward if they were replaced with equivalent\n> processing done in-shell with parameter expansions...\n\nMore importantly, they are fed output from rev-list and do not have\nbreakage you observed on your Mac OS box, do they?\n\nIOW, I don't see anything that needs fixing in other uses.\n\nIn any case, thanks for the fix.\n"},{"id":"150925","messageId":"AANLkTi=9rDR0chmPrjK3eAKgg_ECbAjcUYhvP_GELdvc@mail.gmail.com","threadId":"25129","inReplyTo":"7v8w302fu1.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}","fromName":"Chris Johnsen","fromEmail":"chris_johnsen@pobox.com","sentAt":"2010-09-18T05:25:16Z","receivedAt":"2010-09-18T05:25:16Z","isPatch":true,"sender":{"key":"chris_johnsen@pobox.com","avatar":"https://avatars.githubusercontent.com/u/107071?v=4"},"body":"Junio C Hamano wrote:\n> Chris Johnsen writes:\n>> It seems like the other uses of cut in git-rebase--interactive.sh\n>> would be more awkward if they were replaced with equivalent\n>> processing done in-shell with parameter expansions...\n>\n> More importantly, they are fed output from rev-list and do not have\n> breakage you observed on your Mac OS box, do they?\n>\n> IOW, I don't see anything that needs fixing in other uses.\n\nRight, the other uses of cut do not cause any problems on my system.\n\nAny remaining reason to change them would be along the lines of your\n\"the less of 'cut' we see, the better\" and the possible efficency of\nin-shell processing (e.g. for msys/cygwin).\n\n-- \nChris\n"}]}