syntax error in git-rebase while running t34* tests

classic Classic list List threaded Threaded
7 messages Options
Reply | Threaded
Open this post in threaded view
|

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Armin Kunaschik <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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

Junio C Hamano
Jeff King <[hidden email]> 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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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 <[hidden email]>
Signed-off-by: Jeff King <[hidden email]>
---
 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

--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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 <[hidden email]> 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 <[hidden email]>
> Signed-off-by: Jeff King <[hidden email]>
> ---
>  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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html
Reply | Threaded
Open this post in threaded view
|

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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to [hidden email]
More majordomo info at  http://vger.kernel.org/majordomo-info.html