syntax error in git-rebase while running t34* tests

syntax error in git-rebase while running t34* tests

[Armin Kunaschik]

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

Re: syntax error in git-rebase while running t34* tests

[Junio C Hamano]

Armin Kunaschik <megabreit@googlemail.com> writes:

> I fail to see why eval is really necessary here.

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:

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

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.

Re: syntax error in git-rebase while running t34* tests

[Jeff King]

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

> Armin Kunaschik <megabreit@googlemail.com> writes:
> 
> > I fail to see why eval is really necessary here.
> 
> 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:
> 
> > 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?
> 
> 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.

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

Re: syntax error in git-rebase while running t34* tests

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> 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

Ah, of course.  Thanks.

Re: syntax error in git-rebase while running t34* tests

[Jeff King]

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

> Jeff King <peff@peff.net> writes:
> 
> > 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
> 
> Ah, of course.  Thanks.

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

diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
index 9ea3075..1c6dfb6 100644
--- a/git-rebase--interactive.sh
+++ b/git-rebase--interactive.sh
@@ -82,6 +82,7 @@ rewritten_pending="$state_dir"/rewritten-pending
 cr=$(printf "\015")
 
 strategy_args=${strategy:+--strategy=$strategy}
+test -n "$strategy_opts" &&
 eval '
 	for strategy_opt in '"$strategy_opts"'
 	do
-- 
2.8.2.660.ge43c418

Re: syntax error in git-rebase while running t34* tests

[Johannes Schindelin]

Hi,

On Tue, 10 May 2016, Jeff King wrote:

> On Tue, May 10, 2016 at 01:53:56PM -0700, Junio C Hamano wrote:
> 
> > Jeff King <peff@peff.net> writes:
> > 
> > > 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
> > 
> > Ah, of course.  Thanks.
> 
> 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(+)
> 
> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh
> index 9ea3075..1c6dfb6 100644
> --- a/git-rebase--interactive.sh
> +++ b/git-rebase--interactive.sh
> @@ -82,6 +82,7 @@ rewritten_pending="$state_dir"/rewritten-pending
>  cr=$(printf "\015")
>  
>  strategy_args=${strategy:+--strategy=$strategy}
> +test -n "$strategy_opts" &&
>  eval '
>  	for strategy_opt in '"$strategy_opts"'
>  	do

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

Re: syntax error in git-rebase while running t34* tests

[Jeff King]

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

> Looks obviously correct to me.

Thanks.

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

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