patchgit-rebase--interactive.sh: LF terminate line sent to cut

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

On 09/17/2010 09:17 AM, Chris Johnsen wrote:

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

Brandon Casey <brandon.casey.ctr@nrlssc.navy.mil> writes:

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.

On 09/17/2010 01:38 PM, Junio C Hamano wrote:

Double agreed.

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. :)

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

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:

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:

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

Chris Johnsen <chris_johnsen@pobox.com> writes:

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.

Junio C Hamano <gitster@pobox.com> wrote:

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