syntax error in git-rebase while running t34* tests

Hello,

I noticed in my environment (AIX, ksh) that all rebase tests don't work. git-rebase terminates with git-rebase[6]: Syntax error at line 3 : `newline or ;' is not expected. Digging deeper I found the problem in git-rebase--interactive line 85

<snip>
strategy_args=${strategy:+--strategy=$strategy}
eval '
        for strategy_opt in '"$strategy_opts"'
        do
                strategy_args="$strategy_args -X$(git rev-parse
--sq-quote "${strategy_opt#--}")"
        done
'
<snip>

This snippet fails when $strategy_opts is empty or undefined... and my eval seems not to like this. The for loop runs fine, even with empty strategy_opts, but not inside eval. I fail to see why eval is really necessary here. Things can probably be done without eval, but I don't feel to see the big picture around this code.

My quick and dirty hotfix is to place a test -n "$strategy_opts" && in front of the eval. The tests run fine after this change.

What do you think?

Armin

Armin Kunaschik <megabreit@googlemail.com> writes:

It is necessary to work correctly with any strategy option with $IFS in it, I would think. The calling script "git-rebase" accumulates --strategy-option values after passing each of them through "rev-parse --sq-quote" for that reason.

which means that eval'ed string here:

I do not see why "test -n &&" is necessary here, and would be very hesitant to accept a change that nobody understands why it works.

On Tue, May 10, 2016 at 12:11:59PM -0700, Junio C Hamano wrote:

I think it is clear why it works. If $strategy_opts is empty, then the code we generate looks like:

  for strategy_opt in
  do
          ...
  done

and some shells (apparently) are not happy with no content after the "in". If the variable is empty, there is no need to run the loop at all, so it is OK to skip it the eval entirely.

-Peff

Jeff King <peff@peff.net> writes:

Ah, of course.  Thanks.

On Tue, May 10, 2016 at 01:53:56PM -0700, Junio C Hamano wrote:

Here it is as a patch and commit message.

-- >8 --
Subject: [PATCH] rebase--interactive: avoid empty list in shell for-loop

The $strategy_opts variable contains a space-separated list of strategy options, each individually shell-quoted. To loop over each, we "unwrap" them by doing an eval like:

  eval '
    for opt in '"$strategy_opts"'
    do
       ...
    done
  '

Note the quoting that means we expand $strategy_opts inline in the code to be evaluated (which is the right thing because we want the IFS-split and de-quoting). If the variable is empty, however, we ask the shell to eval the following code:

  for opt in
  do
     ...
  done

without anything between "in" and "do". Most modern shells are happy to treat that like a noop, but reportedly ksh88 on AIX considers it a syntax error. So let's catch the case that the variable is empty and skip the eval altogether (since we know the loop would be a noop anyway).

Reported-by: Armin Kunaschik <megabreit@googlemail.com>
Signed-off-by: Jeff King <peff@peff.net>
---
 git-rebase--interactive.sh | 1 +
 1 file changed, 1 insertion(+)

Hi,

On Tue, 10 May 2016, Jeff King wrote:

Looks obviously correct to me.

I had a look at our other shell scripts and it looks as if there is only one more candidate for this issue: git-bisect.sh has a couple of 'for arg in "$@"' constructs. But from a cursory look, it appears that none of these "$@" can be empty lists because at least one parameter is passed to those functions (check_expected_revs() is only called from bisect_state() with 1 or 2 parameters, bisect_skip() makes no sense without parameters, and bisect_state() has another for loop if it got 2 parameters).

So I think we're fine.

Ciao, Dscho

On Wed, May 11, 2016 at 03:28:35PM +0200, Johannes Schindelin wrote:

Thanks.

I'm not even sure that:

  for arg in "$@"

is a problem if "$@" is empty. The issue here is the eval, in which we generate syntactically funny code and expect the shell to interpret it. It's possible a shell could get the more mundane case wrong, but I'd expect most to get it right.

I did some brief grepping around myself, but didn't find any other interesting cases. That doesn't mean much, though; tracking down what content actually makes it into some of our "eval" calls would be pretty time-consuming. So I'd rely on people like Armin to report failures in the test suite.

-Peff