{"thread":{"id":"37969","subject":"[PATCH] difftool: honor --trust-exit-code for builtin tools","startedAt":"2014-11-14T21:33:55Z","lastAt":"2014-11-17T22:15:37Z","messageCount":8,"participants":["David Aguilar","Junio C Hamano","Mikael Magnusson","Andreas Schwab","Aaron Schrab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"251915","messageId":"1416000835-79274-1-git-send-email-davvid@gmail.com","threadId":"37969","inReplyTo":null,"subject":"[PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-14T21:33:55Z","receivedAt":"2014-11-14T21:33:55Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"run_merge_tool() was not setting $status, which prevented the\nexit code for builtin tools from being forwarded to the caller.\n\nCapture the exit status and add a test to guarantee the behavior.\n\nReported-by: Adria Farres <14farresa@gmail.com>\nSigned-off-by: David Aguilar \n---\n git-mergetool--lib.sh | 1 +\n t/t7800-difftool.sh | 5 +++++\n 2 files changed, 6 insertions(+)\n\ndiff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\nindex a40d3df..2b66351 100644\n--- a/git-mergetool--lib.sh\n+++ b/git-mergetool--lib.sh\n@@ -221,6 +221,7 @@ run_merge_tool () {\n \telse\n \t\trun_diff_cmd \"$1\"\n \tfi\n+\tstatus=$?\n \treturn $status\n }\n \ndiff --git a/t/t7800-difftool.sh b/t/t7800-difftool.sh\nindex 69bde7a..ea35a02 100755\n--- a/t/t7800-difftool.sh\n+++ b/t/t7800-difftool.sh\n@@ -86,6 +86,11 @@ test_expect_success PERL 'difftool forwards exit code with --trust-exit-code' '\n \ttest_must_fail git difftool -y --trust-exit-code -t error branch\n '\n \n+test_expect_success PERL 'difftool forwards exit code with --trust-exit-code for built-ins' '\n+\ttest_config difftool.vimdiff.path false &&\n+\ttest_must_fail git difftool -y --trust-exit-code -t vimdiff branch\n+'\n+\n test_expect_success PERL 'difftool honors difftool.trustExitCode = true' '\n \ttest_config difftool.error.cmd false &&\n \ttest_config difftool.trustExitCode true &&\n-- \n2.2.0.rc1.23.gf570943.dirty\n"},{"id":"251916","messageId":"xmqqy4rd1mdw.fsf@gitster.dls.corp.google.com","threadId":"37969","inReplyTo":"1416000835-79274-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-14T21:51:39Z","receivedAt":"2014-11-14T21:51:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Aguilar writes:\n\n> run_merge_tool() was not setting $status, which prevented the\n> exit code for builtin tools from being forwarded to the caller.\n>\n> Capture the exit status and add a test to guarantee the behavior.\n>\n> Reported-by: Adria Farres <14farresa@gmail.com>\n> Signed-off-by: David Aguilar \n> ---\n> git-mergetool--lib.sh | 1 +\n> t/t7800-difftool.sh | 5 +++++\n> 2 files changed, 6 insertions(+)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index a40d3df..2b66351 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -221,6 +221,7 @@ run_merge_tool () {\n> \telse\n> \t\trun_diff_cmd \"$1\"\n> \tfi\n> +\tstatus=$?\n> \treturn $status\n> }\n\nThanks for a quick turn-around. As a hot-fix for what is already in\n-rc I am fine with this fix but the patch makes me wonder if $status\nas a global shell variable has any significance.\n\nI see that this shell function in its early part does this:\n\n\tstatus=0\n setup_tool \"$1\" || return 1\n\nwhich means that the caller of this function, instead of checking\nwhat is returned as the return value of the function like:\n\n\tif run_merge_tool ...\n then\n\t\t...\n\nrelies on the value of $status in its later part of the code like:\n\n\trun_merge_tool ...\n\t...\n\tif test \"$status\" = 0\n\tthen\n\t\t...\n\nthen we are already in trouble. And the latter form, if we had such\na flow in the code, is simply a bad taste.\n\nA cleaner fix might be to get rid of the extra $status variable from\nthis function and let the function return the result of its last\ncommand, either run_merge_cmd or run_diff_cmd, by either explicitly\nhaving \"return $?\" at the end, or not having that \"return $status\"\nline. But that relies on us not having any caller that relies on\nthe $status carried as a global variable around, so it will be more\nwork to convince ourselves that such a fix is correctly done. From\nmy cursory look, what I suggested above should be safe and correct,\nbut I do not want to risk an unnecessary and silly breakage this\nlate in the cycle.\n\nSo I'll queue this patch as-is for upcoming 2.2, but I think we\nwould want to revisit this issue after the release is done.\n"},{"id":"251918","messageId":"20141114215746.GB93845@gmail.com","threadId":"37969","inReplyTo":"xmqqy4rd1mdw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-14T21:57:47Z","receivedAt":"2014-11-14T21:57:47Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Fri, Nov 14, 2014 at 01:51:39PM -0800, Junio C Hamano wrote:\n> David Aguilar writes:\n> \n> > run_merge_tool() was not setting $status, which prevented the\n> > exit code for builtin tools from being forwarded to the caller.\n> >\n> > Capture the exit status and add a test to guarantee the behavior.\n> >\n> > Reported-by: Adria Farres <14farresa@gmail.com>\n> > Signed-off-by: David Aguilar \n> > ---\n> > git-mergetool--lib.sh | 1 +\n> > t/t7800-difftool.sh | 5 +++++\n> > 2 files changed, 6 insertions(+)\n> >\n> > diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> > index a40d3df..2b66351 100644\n> > --- a/git-mergetool--lib.sh\n> > +++ b/git-mergetool--lib.sh\n> > @@ -221,6 +221,7 @@ run_merge_tool () {\n> > \telse\n> > \t\trun_diff_cmd \"$1\"\n> > \tfi\n> > +\tstatus=$?\n> > \treturn $status\n> > }\n> \n> Thanks for a quick turn-around. As a hot-fix for what is already in\n> -rc I am fine with this fix but the patch makes me wonder if $status\n> as a global shell variable has any significance.\n> \n> I see that this shell function in its early part does this:\n> \n> \tstatus=0\n> setup_tool \"$1\" || return 1\n> \n> which means that the caller of this function, instead of checking\n> what is returned as the return value of the function like:\n> \n> \tif run_merge_tool ...\n> then\n> \t\t...\n> \n> relies on the value of $status in its later part of the code like:\n> \n> \trun_merge_tool ...\n> \t...\n> \tif test \"$status\" = 0\n> \tthen\n> \t\t...\n> \n> then we are already in trouble. And the latter form, if we had such\n> a flow in the code, is simply a bad taste.\n> \n> A cleaner fix might be to get rid of the extra $status variable from\n> this function and let the function return the result of its last\n> command, either run_merge_cmd or run_diff_cmd, by either explicitly\n> having \"return $?\" at the end, or not having that \"return $status\"\n> line. But that relies on us not having any caller that relies on\n> the $status carried as a global variable around, so it will be more\n> work to convince ourselves that such a fix is correctly done. From\n> my cursory look, what I suggested above should be safe and correct,\n> but I do not want to risk an unnecessary and silly breakage this\n> late in the cycle.\n> \n> So I'll queue this patch as-is for upcoming 2.2, but I think we\n> would want to revisit this issue after the release is done.\n\n\nThanks for the sug, I totally agree with that.\nI'll put a $status audit/rework for mergetool+difftool on my\ntodo list.\n\ncheers,\n-- \nDavid\n"},{"id":"251950","messageId":"CAHYJk3Q9tcS+o0hDnDz24ysSKkL6m16OmhyHuj=W88VQjTximw@mail.gmail.com","threadId":"37969","inReplyTo":"xmqqy4rd1mdw.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"Mikael Magnusson","fromEmail":"mikachu@gmail.com","sentAt":"2014-11-16T01:51:11Z","receivedAt":"2014-11-16T01:51:11Z","isPatch":true,"sender":{"key":"mikachu@gmail.com","avatar":null},"body":"On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano wrote:\n> David Aguilar writes:\n>\n>> run_merge_tool() was not setting $status, which prevented the\n>> exit code for builtin tools from being forwarded to the caller.\n>>\n>> Capture the exit status and add a test to guarantee the behavior.\n>>\n>> Reported-by: Adria Farres <14farresa@gmail.com>\n>> Signed-off-by: David Aguilar \n>> ---\n>> git-mergetool--lib.sh | 1 +\n>> t/t7800-difftool.sh | 5 +++++\n>> 2 files changed, 6 insertions(+)\n>>\n>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>> index a40d3df..2b66351 100644\n>> --- a/git-mergetool--lib.sh\n>> +++ b/git-mergetool--lib.sh\n>> @@ -221,6 +221,7 @@ run_merge_tool () {\n>> else\n>> run_diff_cmd \"$1\"\n>> fi\n>> + status=$?\n>> return $status\n>> }\n>\n> Thanks for a quick turn-around. As a hot-fix for what is already in\n> -rc I am fine with this fix but the patch makes me wonder if $status\n> as a global shell variable has any significance.\n\n$status is an alias for $? in zsh, and so cannot be assigned to. But\nother than that I don't think it holds any meaning and should be fine\nin a .sh script.\n\n-- \nMikael Magnusson\n"},{"id":"251951","messageId":"20141116023609.GA74487@gmail.com","threadId":"37969","inReplyTo":"CAHYJk3Q9tcS+o0hDnDz24ysSKkL6m16OmhyHuj=W88VQjTximw@mail.gmail.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"David Aguilar","fromEmail":"davvid@gmail.com","sentAt":"2014-11-16T02:36:10Z","receivedAt":"2014-11-16T02:36:10Z","isPatch":true,"sender":{"key":"davvid@gmail.com","avatar":"https://avatars.githubusercontent.com/u/13196?v=4"},"body":"On Sun, Nov 16, 2014 at 02:51:11AM +0100, Mikael Magnusson wrote:\n> On Fri, Nov 14, 2014 at 10:51 PM, Junio C Hamano wrote:\n> > David Aguilar writes:\n> >\n> >> run_merge_tool() was not setting $status, which prevented the\n> >> exit code for builtin tools from being forwarded to the caller.\n> >>\n> >> Capture the exit status and add a test to guarantee the behavior.\n> >>\n> >> Reported-by: Adria Farres <14farresa@gmail.com>\n> >> Signed-off-by: David Aguilar \n> >> ---\n> >> git-mergetool--lib.sh | 1 +\n> >> t/t7800-difftool.sh | 5 +++++\n> >> 2 files changed, 6 insertions(+)\n> >>\n> >> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> >> index a40d3df..2b66351 100644\n> >> --- a/git-mergetool--lib.sh\n> >> +++ b/git-mergetool--lib.sh\n> >> @@ -221,6 +221,7 @@ run_merge_tool () {\n> >> else\n> >> run_diff_cmd \"$1\"\n> >> fi\n> >> + status=$?\n> >> return $status\n> >> }\n> >\n> > Thanks for a quick turn-around. As a hot-fix for what is already in\n> > -rc I am fine with this fix but the patch makes me wonder if $status\n> > as a global shell variable has any significance.\n> \n> $status is an alias for $? in zsh, and so cannot be assigned to. But\n> other than that I don't think it holds any meaning and should be fine\n> in a .sh script.\n\n\nThanks for the heads-up ~ this is even more reason to cleanup\nthe script a bit.\n\nIf we still need a local variable for it in a few places then I'll\ncall it $rc instead, but it'll only be used for local things\nrather than its current global usage.\n-- \nDavid\n"},{"id":"251960","messageId":"m2a93ro8xo.fsf@linux-m68k.org","threadId":"37969","inReplyTo":"1416000835-79274-1-git-send-email-davvid@gmail.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2014-11-16T08:18:11Z","receivedAt":"2014-11-16T08:18:11Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"David Aguilar writes:\n\n> run_merge_tool() was not setting $status, which prevented the\n> exit code for builtin tools from being forwarded to the caller.\n>\n> Capture the exit status and add a test to guarantee the behavior.\n>\n> Reported-by: Adria Farres <14farresa@gmail.com>\n> Signed-off-by: David Aguilar \n> ---\n> git-mergetool--lib.sh | 1 +\n> t/t7800-difftool.sh | 5 +++++\n> 2 files changed, 6 insertions(+)\n>\n> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n> index a40d3df..2b66351 100644\n> --- a/git-mergetool--lib.sh\n> +++ b/git-mergetool--lib.sh\n> @@ -221,6 +221,7 @@ run_merge_tool () {\n> \telse\n> \t\trun_diff_cmd \"$1\"\n> \tfi\n> +\tstatus=$?\n> \treturn $status\n\nIf you want to return the last exit status at the end of a function you\ndon't need any return at all.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"},{"id":"251964","messageId":"xmqq4mtz10ef.fsf@gitster.dls.corp.google.com","threadId":"37969","inReplyTo":"CAHYJk3Q9tcS+o0hDnDz24ysSKkL6m16OmhyHuj=W88VQjTximw@mail.gmail.com","subject":"Re: [PATCH] difftool: honor --trust-exit-code for builtin tools","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-11-16T18:11:04Z","receivedAt":"2014-11-16T18:11:04Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Mikael Magnusson writes:\n\n>>> diff --git a/git-mergetool--lib.sh b/git-mergetool--lib.sh\n>>> index a40d3df..2b66351 100644\n>>> --- a/git-mergetool--lib.sh\n>>> +++ b/git-mergetool--lib.sh\n>>> @@ -221,6 +221,7 @@ run_merge_tool () {\n>>> else\n>>> run_diff_cmd \"$1\"\n>>> fi\n>>> + status=$?\n>>> return $status\n>>> }\n>>\n>> Thanks for a quick turn-around. As a hot-fix for what is already in\n>> -rc I am fine with this fix but the patch makes me wonder if $status\n>> as a global shell variable has any significance.\n>\n> $status is an alias for $? in zsh, and so cannot be assigned to. But\n> other than that I don't think it holds any meaning and should be fine\n> in a .sh script.\n\nThat is not what I meant by \"global ... significance\".\n\nThe question was if the codepath in the caller depends on this\nsetting the global variable here, or nobody looks at and depends on\nthe global variable we are setting here after this function returns.\n\nIt does not have any significance that a random shell implementation\nis not POSIX compliant. That would merely mean that such a shell\ncannot be used to run POSIX shell scripts like our Porcelain. I\nwould suspect that zsh has more \"posixly correct\" mode, with which\nit _can_ run POSIX shell scripts, and I would imagine that this\n\"$status is an alias $?\" business is disabled in that mode?\n\nMy quick glance across the codepaths in the callers of this funciton\nindicated that it should be safe not using this global variable, so\nmy answer to my original question was \"no there is no significance\".\nI think we can safely remove any mention of status from this shell\nfunction, i.e. if we remove initial assignment to 0, remove this new\nassignment and then remove the \"return $status\" at the end, the\ncaller would still be happy.\n"},{"id":"252008","messageId":"20141117221536.GG615@pug.qqx.org","threadId":"37969","inReplyTo":"xmqq4mtz10ef.fsf@gitster.dls.corp.google.com","subject":"Re: difftool: honor --trust-exit-code for builtin tools","fromName":"Aaron Schrab","fromEmail":"aaron@schrab.com","sentAt":"2014-11-17T22:15:37Z","receivedAt":"2014-11-17T22:15:37Z","isPatch":false,"sender":{"key":"aaron@schrab.com","avatar":"https://avatars.githubusercontent.com/u/39620?v=4"},"body":"At 10:11 -0800 16 Nov 2014, Junio C Hamano wrote:\n>It does not have any significance that a random shell implementation\n>is not POSIX compliant. That would merely mean that such a shell\n>cannot be used to run POSIX shell scripts like our Porcelain.\n\nRight, and I suspect that it's very rare for zsh to be used as /bin/sh. \nI've heard of people doing it just to see what would fail, but not of \nanybody doing that for regular use.\n\n>I would suspect that zsh has more \"posixly correct\" mode, with which\n>it _can_ run POSIX shell scripts, and I would imagine that this \n>\"$status is an alias $?\" business is disabled in that mode? \n\nYes, if zsh is invoked as either \"sh\" or \"ksh\" it attempts to emulate \nthe usual semantics of the named shell. One of the differences is that \n$status isn't special in the emulation modes.\n"}]}