{"thread":{"id":"42277","subject":"syntax error in git-rebase while running t34* tests","startedAt":"2016-05-10T17:29:51Z","lastAt":"2016-05-11T13:36:16Z","messageCount":7,"participants":["Armin Kunaschik","Junio C Hamano","Jeff King","Johannes Schindelin"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"286082","messageId":"CALR6jEiF9Ooi1f0O3KG0wYmN0KRWBQTNarXx79-wBD2E-8q2jA@mail.gmail.com","threadId":"42277","inReplyTo":null,"subject":"syntax error in git-rebase while running t34* tests","fromName":"Armin Kunaschik","fromEmail":"megabreit@googlemail.com","sentAt":"2016-05-10T17:29:51Z","receivedAt":"2016-05-10T17:29:51Z","isPatch":false,"sender":{"key":"megabreit@googlemail.com","avatar":"https://avatars.githubusercontent.com/u/10429218?v=4"},"body":"Hello,\n\nI noticed in my environment (AIX, ksh) that all rebase tests don't work.\ngit-rebase terminates with\ngit-rebase[6]: Syntax error at line 3 : `newline or ;' is not expected.\nDigging deeper I found the problem in git-rebase--interactive line 85\n\n\nstrategy_args=${strategy:+--strategy=$strategy}\neval '\n for strategy_opt in '\"$strategy_opts\"'\n do\n strategy_args=\"$strategy_args -X$(git rev-parse\n--sq-quote \"${strategy_opt#--}\")\"\n done\n'\n\n\nThis snippet fails when $strategy_opts is empty or undefined... and my\neval seems not to like this.\nThe for loop runs fine, even with empty strategy_opts, but not inside eval.\nI fail to see why eval is really necessary here. Things can probably\nbe done without eval, but I\ndon't feel to see the big picture around this code.\n\nMy quick and dirty hotfix is to place a\ntest -n \"$strategy_opts\" &&\nin front of the eval.\nThe tests run fine after this change.\n\nWhat do you think?\n\nArmin\n"},{"id":"286096","messageId":"xmqq60ulpw1s.fsf@gitster.mtv.corp.google.com","threadId":"42277","inReplyTo":"CALR6jEiF9Ooi1f0O3KG0wYmN0KRWBQTNarXx79-wBD2E-8q2jA@mail.gmail.com","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T19:11:59Z","receivedAt":"2016-05-10T19:11:59Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Armin Kunaschik writes:\n\n> I fail to see why eval is really necessary here.\n\nIt is necessary to work correctly with any strategy option with $IFS\nin it, I would think. The calling script \"git-rebase\" accumulates\n--strategy-option values after passing each of them through\n\"rev-parse --sq-quote\" for that reason.\n\nwhich means that eval'ed string here:\n\n> My quick and dirty hotfix is to place a\n> test -n \"$strategy_opts\" &&\n> in front of the eval.\n> The tests run fine after this change.\n>\n> What do you think?\n\nI do not see why \"test -n &&\" is necessary here, and would be very\nhesitant to accept a change that nobody understands why it works.\n"},{"id":"286111","messageId":"20160510204758.GB19958@sigill.intra.peff.net","threadId":"42277","inReplyTo":"xmqq60ulpw1s.fsf@gitster.mtv.corp.google.com","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T20:47:58Z","receivedAt":"2016-05-10T20:47:58Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 12:11:59PM -0700, Junio C Hamano wrote:\n\n> Armin Kunaschik writes:\n> \n> > I fail to see why eval is really necessary here.\n> \n> It is necessary to work correctly with any strategy option with $IFS\n> in it, I would think. The calling script \"git-rebase\" accumulates\n> --strategy-option values after passing each of them through\n> \"rev-parse --sq-quote\" for that reason.\n> \n> which means that eval'ed string here:\n> \n> > My quick and dirty hotfix is to place a\n> > test -n \"$strategy_opts\" &&\n> > in front of the eval.\n> > The tests run fine after this change.\n> >\n> > What do you think?\n> \n> I do not see why \"test -n &&\" is necessary here, and would be very\n> hesitant to accept a change that nobody understands why it works.\n\nI think it is clear why it works. If $strategy_opts is empty, then the\ncode we generate looks like:\n\n for strategy_opt in\n do\n ...\n done\n\nand some shells (apparently) are not happy with no content after the\n\"in\". If the variable is empty, there is no need to run the loop at all,\nso it is OK to skip it the eval entirely.\n\n-Peff\n"},{"id":"286112","messageId":"xmqqy47hmy6z.fsf@gitster.mtv.corp.google.com","threadId":"42277","inReplyTo":"20160510204758.GB19958@sigill.intra.peff.net","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-05-10T20:53:56Z","receivedAt":"2016-05-10T20:53:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I think it is clear why it works. If $strategy_opts is empty, then the\n> code we generate looks like:\n>\n> for strategy_opt in\n> do\n> ...\n> done\n\nAh, of course. Thanks.\n"},{"id":"286115","messageId":"20160510210709.GD19958@sigill.intra.peff.net","threadId":"42277","inReplyTo":"xmqqy47hmy6z.fsf@gitster.mtv.corp.google.com","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-10T21:07:10Z","receivedAt":"2016-05-10T21:07:10Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 10, 2016 at 01:53:56PM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > I think it is clear why it works. If $strategy_opts is empty, then the\n> > code we generate looks like:\n> >\n> > for strategy_opt in\n> > do\n> > ...\n> > done\n> \n> Ah, of course. Thanks.\n\nHere it is as a patch and commit message.\n\n-- >8 --\nSubject: [PATCH] rebase--interactive: avoid empty list in shell for-loop\n\nThe $strategy_opts variable contains a space-separated list\nof strategy options, each individually shell-quoted. To loop\nover each, we \"unwrap\" them by doing an eval like:\n\n eval '\n for opt in '\"$strategy_opts\"'\n do\n ...\n done\n '\n\nNote the quoting that means we expand $strategy_opts inline\nin the code to be evaluated (which is the right thing\nbecause we want the IFS-split and de-quoting). If the\nvariable is empty, however, we ask the shell to eval the\nfollowing code:\n\n for opt in\n do\n ...\n done\n\nwithout anything between \"in\" and \"do\". Most modern shells\nare happy to treat that like a noop, but reportedly ksh88 on\nAIX considers it a syntax error. So let's catch the case\nthat the variable is empty and skip the eval altogether\n(since we know the loop would be a noop anyway).\n\nReported-by: Armin Kunaschik \nSigned-off-by: Jeff King \n---\n git-rebase--interactive.sh | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\nindex 9ea3075..1c6dfb6 100644\n--- a/git-rebase--interactive.sh\n+++ b/git-rebase--interactive.sh\n@@ -82,6 +82,7 @@ rewritten_pending=\"$state_dir\"/rewritten-pending\n cr=$(printf \"\\015\")\n \n strategy_args=${strategy:+--strategy=$strategy}\n+test -n \"$strategy_opts\" &&\n eval '\n \tfor strategy_opt in '\"$strategy_opts\"'\n \tdo\n-- \n2.8.2.660.ge43c418\n"},{"id":"286263","messageId":"alpine.DEB.2.20.1605111518470.4092@virtualbox","threadId":"42277","inReplyTo":"20160510210709.GD19958@sigill.intra.peff.net","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-05-11T13:28:35Z","receivedAt":"2016-05-11T13:28:35Z","isPatch":false,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Tue, 10 May 2016, Jeff King wrote:\n\n> On Tue, May 10, 2016 at 01:53:56PM -0700, Junio C Hamano wrote:\n> \n> > Jeff King writes:\n> > \n> > > I think it is clear why it works. If $strategy_opts is empty, then the\n> > > code we generate looks like:\n> > >\n> > > for strategy_opt in\n> > > do\n> > > ...\n> > > done\n> > \n> > Ah, of course. Thanks.\n> \n> Here it is as a patch and commit message.\n> \n> -- >8 --\n> Subject: [PATCH] rebase--interactive: avoid empty list in shell for-loop\n> \n> The $strategy_opts variable contains a space-separated list\n> of strategy options, each individually shell-quoted. To loop\n> over each, we \"unwrap\" them by doing an eval like:\n> \n> eval '\n> for opt in '\"$strategy_opts\"'\n> do\n> ...\n> done\n> '\n> \n> Note the quoting that means we expand $strategy_opts inline\n> in the code to be evaluated (which is the right thing\n> because we want the IFS-split and de-quoting). If the\n> variable is empty, however, we ask the shell to eval the\n> following code:\n> \n> for opt in\n> do\n> ...\n> done\n> \n> without anything between \"in\" and \"do\". Most modern shells\n> are happy to treat that like a noop, but reportedly ksh88 on\n> AIX considers it a syntax error. So let's catch the case\n> that the variable is empty and skip the eval altogether\n> (since we know the loop would be a noop anyway).\n> \n> Reported-by: Armin Kunaschik \n> Signed-off-by: Jeff King \n> ---\n> git-rebase--interactive.sh | 1 +\n> 1 file changed, 1 insertion(+)\n> \n> diff --git a/git-rebase--interactive.sh b/git-rebase--interactive.sh\n> index 9ea3075..1c6dfb6 100644\n> --- a/git-rebase--interactive.sh\n> +++ b/git-rebase--interactive.sh\n> @@ -82,6 +82,7 @@ rewritten_pending=\"$state_dir\"/rewritten-pending\n> cr=$(printf \"\\015\")\n> \n> strategy_args=${strategy:+--strategy=$strategy}\n> +test -n \"$strategy_opts\" &&\n> eval '\n> \tfor strategy_opt in '\"$strategy_opts\"'\n> \tdo\n\nLooks obviously correct to me.\n\nI had a look at our other shell scripts and it looks as if there is only\none more candidate for this issue: git-bisect.sh has a couple of 'for arg\nin \"$@\"' constructs. But from a cursory look, it appears that none of\nthese \"$@\" can be empty lists because at least one parameter is passed to\nthose functions (check_expected_revs() is only called from bisect_state()\nwith 1 or 2 parameters, bisect_skip() makes no sense without parameters,\nand bisect_state() has another for loop if it got 2 parameters).\n\nSo I think we're fine.\n\nCiao,\nDscho\n"},{"id":"286265","messageId":"20160511133615.GA19356@sigill.intra.peff.net","threadId":"42277","inReplyTo":"alpine.DEB.2.20.1605111518470.4092@virtualbox","subject":"Re: syntax error in git-rebase while running t34* tests","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-05-11T13:36:16Z","receivedAt":"2016-05-11T13:36:16Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, May 11, 2016 at 03:28:35PM +0200, Johannes Schindelin wrote:\n\n> Looks obviously correct to me.\n\nThanks.\n\n> I had a look at our other shell scripts and it looks as if there is only\n> one more candidate for this issue: git-bisect.sh has a couple of 'for arg\n> in \"$@\"' constructs. But from a cursory look, it appears that none of\n> these \"$@\" can be empty lists because at least one parameter is passed to\n> those functions (check_expected_revs() is only called from bisect_state()\n> with 1 or 2 parameters, bisect_skip() makes no sense without parameters,\n> and bisect_state() has another for loop if it got 2 parameters).\n> \n> So I think we're fine.\n\nI'm not even sure that:\n\n for arg in \"$@\"\n\nis a problem if \"$@\" is empty. The issue here is the eval, in which we\ngenerate syntactically funny code and expect the shell to interpret it.\nIt's possible a shell could get the more mundane case wrong, but I'd\nexpect most to get it right.\n\nI did some brief grepping around myself, but didn't find any other\ninteresting cases. That doesn't mean much, though; tracking down what\ncontent actually makes it into some of our \"eval\" calls would be pretty\ntime-consuming. So I'd rely on people like Armin to report failures in\nthe test suite.\n\n-Peff\n"}]}