[PATCH] git-rebase--interactive.sh: LF terminate line sent to cut

[PATCH] git-rebase--interactive.sh: LF terminate line sent to cut

[Chris Johnsen]

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 <chris_johnsen@pobox.com>

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

Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut

[Brandon Casey]

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 <chris_johnsen@pobox.com>
> 
> ---
> 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

Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut

[Junio C Hamano]

Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> 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.

Re: [PATCH] git-rebase--interactive.sh: LF terminate line sent to cut

[Brandon Casey]

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

[PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}

[Chris Johnsen]

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 <chris_johnsen@pobox.com>

---

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

Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}

[Junio C Hamano]

Chris Johnsen <chris_johnsen@pobox.com> 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.

Re: [PATCH v2] git-rebase--interactive.sh: replace cut with ${v%% *}

[Chris Johnsen]

Junio C Hamano <gitster@pobox.com> wrote:
> Chris Johnsen <chris_johnsen@pobox.com> 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