# [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut 7 messages from 2010-09-17 to 2010-09-18. Participants: Chris Johnsen, Brandon Casey, Junio C Hamano. Thread: https://gitlist.dev/t/25129 ## Chris Johnsen, 2010-09-17 14:17 Subject: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut Message-ID: <60d13fc6a7d5b1b08f35f91b2d90eb7c13922390.1284733059.git.chris_johnsen@pobox.com> URL: https://gitlist.dev/e/60d13fc6a7d5b1b08f35f91b2d90eb7c13922390.1284733059.git.chris_johnsen%40pobox.com ``` Some versions of cut do not cope well with lines that do not end in an LF. Add '\n' to the printf format string to ensure that the generated output ends in a LF. I found this problem when t3404's "avoid unnecessary reset" failed due to the "rebase -i" not avoiding updating the tested timestamp. On a Mac OS X 10.4.11 system: % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1 cut: stdin: Illegal byte sequence % printf '%s\n' 'foo bar' | /usr/bin/cut -d ' ' -f 1 foo Signed-off-by: Chris Johnsen --- It looks like the cut on my system is derived from FreeBSD. It is probably an old version though (possibly too old to care about). The cut from GNU coreutils does not to have this problem, so using it serves as a workaround. --- git-rebase--interactive.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index eb2dff5..834460a 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -626,7 +626,7 @@ skip_unnecessary_picks () { case "$fd,$command" in 3,pick|3,p) # pick a commit whose parent is current $ONTO -> skip - sha1=$(printf '%s' "$rest" | cut -d ' ' -f 1) + sha1=$(printf '%s\n' "$rest" | cut -d ' ' -f 1) case "$(git rev-parse --verify --quiet "$sha1"^)" in "$ONTO"*) ONTO=$sha1 -- 1.7.3.rc2 ``` ## Brandon Casey, 2010-09-17 15:10 Subject: Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut Message-ID: URL: https://gitlist.dev/e/XhMLJaG8mUbh4rzLnU3IrGDXbMd9-p7UFO6kn9Uke7n_H4NNOG6glg%40cipher.nrlssc.navy.mil In-Reply-To: <60d13fc6a7d5b1b08f35f91b2d90eb7c13922390.1284733059.git.chris_johnsen@pobox.com> ``` On 09/17/2010 09:17 AM, Chris Johnsen wrote: > Some versions of cut do not cope well with lines that do not end in > an LF. Add '\n' to the printf format string to ensure that the > generated output ends in a LF. > > I found this problem when t3404's "avoid unnecessary reset" failed > due to the "rebase -i" not avoiding updating the tested timestamp. > > On a Mac OS X 10.4.11 system: > > % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1 > cut: stdin: Illegal byte sequence > % printf '%s\n' 'foo bar' | /usr/bin/cut -d ' ' -f 1 > foo Or we could write it like: sha1=${rest%% *} which I wish I had changed it to in the first place when I made some recent modifications. The '%%' notation avoids the whole newline issue by not even spawning 'cut'. We are already using this construct in git-filter-branch.sh and git-instaweb.sh, though those are not the most visible scripts in git. Does the above work on your FreeBSD system? -Brandon > Signed-off-by: Chris Johnsen > > --- > It looks like the cut on my system is derived from FreeBSD. It is > probably an old version though (possibly too old to care about). > > The cut from GNU coreutils does not to have this problem, so using > it serves as a workaround. > --- > git-rebase--interactive.sh | 2 +- > 1 files changed, 1 insertions(+), 1 deletions(-) > > diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh > index eb2dff5..834460a 100755 > --- a/git-rebase--interactive.sh > +++ b/git-rebase--interactive.sh > @@ -626,7 +626,7 @@ skip_unnecessary_picks () { > case "$fd,$command" in > 3,pick|3,p) > # pick a commit whose parent is current $ONTO -> skip > - sha1=$(printf '%s' "$rest" | cut -d ' ' -f 1) > + sha1=$(printf '%s\n' "$rest" | cut -d ' ' -f 1) > case "$(git rev-parse --verify --quiet "$sha1"^)" in > "$ONTO"*) > ONTO=$sha1 ``` ## Junio C Hamano, 2010-09-17 18:38 Subject: Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut Message-ID: <7vsk182p2q.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsk182p2q.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Brandon Casey writes: > Or we could write it like: > > sha1=${rest%% *} > > which I wish I had changed it to in the first place when I made some > recent modifications. Agreed; the less use of 'cut' we see, the better ;-) As to portability guideline in our shell script: ${param#word} ${param##word} ${param%word} ${param%%word} are permissible POSIX constructs (together with more traditional -/=/?/+), and their use is encouraged over 'cut', 'expr', etc. [*1*] ${param:ofs} ${param:ofs:len} ${param/pattern/string} are bashisms we avoid (unless of course in the bash completion script). We do not seem to use ${#param}, not because it is forbidden, but I think because it is not very useful without ${param:ofs:len}. [Footnote] *1* In 2005 back when I took over the git maintenance, I used to be a lot more conservative/traditionalist and as a result, you may see overused "expr" in contrib/examples/ and "git log -p -- '*.sh'" output. But we have been eradicating them a bit by bit for the past few years. ``` ## Brandon Casey, 2010-09-17 18:59 Subject: Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut Message-ID: URL: https://gitlist.dev/e/VzbuextQE2-OASqyG4sJxmg1IuyBq5BWWiDERv0h-YQdVcnL8Enurg%40cipher.nrlssc.navy.mil In-Reply-To: <7vsk182p2q.fsf@alter.siamese.dyndns.org> ``` On 09/17/2010 01:38 PM, Junio C Hamano wrote: > Agreed; the less use of 'cut' we see, the better ;-) Double agreed. > As to portability guideline in our shell script: > > ${param#word} ${param##word} ${param%word} ${param%%word} > > are permissible POSIX constructs (together with more traditional -/=/?/+), and > their use is encouraged over 'cut', 'expr', etc. [*1*] > > ${param:ofs} ${param:ofs:len} ${param/pattern/string} > > are bashisms we avoid (unless of course in the bash completion script). It's been a long while since I've reviewed Documentation/CodingGuidelines, but these are indeed in there, and have been for a very long time. Maybe I should refresh my memory more often. :) > We do not seem to use ${#param}, not because it is forbidden, but I think > because it is not very useful without ${param:ofs:len}. CodingGuidelines does say "No strlen ${#parameter}", so that could be part of the reason. But like you say, it's not very useful without ${param:ofs:len}. -Brandon ``` ## Chris Johnsen, 2010-09-17 21:42 Subject: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *} Message-ID: <0eafa42f1da5f66465a1eb9da170416363cf72e0.1284759770.git.chris_johnsen@pobox.com> URL: https://gitlist.dev/e/0eafa42f1da5f66465a1eb9da170416363cf72e0.1284759770.git.chris_johnsen%40pobox.com In-Reply-To: <7vsk182p2q.fsf@alter.siamese.dyndns.org> ``` Some versions of cut do not cope well with lines that do not end in an LF. In this case, we can completely avoid cut by using the ${var%% *} parameter expansion (suggested by Brandon Casey). I found this problem when t3404's "avoid unnecessary reset" failed due to the "rebase -i" not avoiding updating the tested timestamp. On a Mac OS X 10.4.11 system: % printf '%s' 'foo bar' | /usr/bin/cut -d ' ' -f 1 cut: stdin: Illegal byte sequence Signed-off-by: Chris Johnsen --- Brandon Casey wrote: > Or we could write it like: > > sha1=${rest%% *} > > Does the above work on your FreeBSD system? Yes, as Junio points out, ${var%% *} is portable enough for Git. After this change t3404 passes here without GNU cut available. Junio C Hamano wrote: > Agreed; the less use of 'cut' we see, the better ;-) It seems like the other uses of cut in git-rebase--interactive.sh would be more awkward if they were replaced with equivalent processing done in-shell with parameter expansions. Eliminating them should probably wait until after 1.7.3, if at all. --- git-rebase--interactive.sh | 2 +- 1 files changed, 1 insertions(+), 1 deletions(-) diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh index eb2dff5..a27952d 100755 --- a/git-rebase--interactive.sh +++ b/git-rebase--interactive.sh @@ -626,7 +626,7 @@ skip_unnecessary_picks () { case "$fd,$command" in 3,pick|3,p) # pick a commit whose parent is current $ONTO -> skip - sha1=$(printf '%s' "$rest" | cut -d ' ' -f 1) + sha1=${rest%% *} case "$(git rev-parse --verify --quiet "$sha1"^)" in "$ONTO"*) ONTO=$sha1 -- 1.7.3.rc2 ``` ## Junio C Hamano, 2010-09-17 21:57 Subject: Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *} Message-ID: <7v8w302fu1.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8w302fu1.fsf%40alter.siamese.dyndns.org In-Reply-To: <0eafa42f1da5f66465a1eb9da170416363cf72e0.1284759770.git.chris_johnsen@pobox.com> ``` Chris Johnsen writes: > It seems like the other uses of cut in git-rebase--interactive.sh > would be more awkward if they were replaced with equivalent > processing done in-shell with parameter expansions... More importantly, they are fed output from rev-list and do not have breakage you observed on your Mac OS box, do they? IOW, I don't see anything that needs fixing in other uses. In any case, thanks for the fix. ``` ## Chris Johnsen, 2010-09-18 05:25 Subject: Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *} Message-ID: URL: https://gitlist.dev/e/AANLkTi%3D9rDR0chmPrjK3eAKgg_ECbAjcUYhvP_GELdvc%40mail.gmail.com In-Reply-To: <7v8w302fu1.fsf@alter.siamese.dyndns.org> ``` Junio C Hamano wrote: > Chris Johnsen writes: >> It seems like the other uses of cut in git-rebase--interactive.sh >> would be more awkward if they were replaced with equivalent >> processing done in-shell with parameter expansions... > > More importantly, they are fed output from rev-list and do not have > breakage you observed on your Mac OS box, do they? > > IOW, I don't see anything that needs fixing in other uses. Right, the other uses of cut do not cause any problems on my system. Any remaining reason to change them would be along the lines of your "the less of 'cut' we see, the better" and the possible efficency of in-shell processing (e.g. for msys/cygwin). -- Chris ```