Re: [PATCH] contrib: added git-diffall

From: David Aguilar <davvid@gmail.com>
Date: Feb 22, 2012, 09:15 UTC
Message-ID: <CAJDDKr72rPqp1z-12Ht3Q2UaeUVFutKwoOgkD1G+SbhsBs+p1A@mail.gmail.com>
In-Reply-To: <7vbooregj6.fsf@alter.siamese.dyndns.org>

On Tue, Feb 21, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 22 quoted lines

> Tim Henigan <tim.henigan@gmail.com> writes:
>
>> There is no specific reason it must be bash.  I changed from
>> "#!/bin/sh" to "#!/bin/bash -e" due to a bug report from a user on
>> Ubuntu [1].  The user reported that:
>>
>>     "If you use /bin/sh on ubuntu you get the dash shell instead of bash shell.
>>     This causes git_merge_tool_path to fail. The error isn't trapped,
>> so it exits
>>     without displaying anything and without cleaning up."
>>
>> Given that all the other scripts distributed with git use /bin/sh, I
>> will change this script to match.
>
> You need to dig back to that bug report deeper and find out what exactly
> is causing the "failure", before blindly allowing /bin/dash to be used.
>
> I think the above function name is a typo of get_merge_tool_path that is
> borrowed from git-mergetool--lib.sh, but nothing in the function jumps as
> a blatant bash-ism at me from a quick reading.
>
> David, any idea on this?

I don't see any bash-isms there myself, either. We should keep this stuff without bash-isms. I haven't had time to read these patches in depth yet but will try to read the re-roll.

Can we ask the github user to elaborate on what exactly was erroring out? Does dash not handle || inside $()? We can only make wild guesses without their help.

The only hint from the pull request is "silent exit with no results". Do we do that? There are a few code paths where we do "exit 1" but that's only under error conditions.

We haven't had any reports about git-mergetool/difftool, which use these functions... are we certain the problem was not some other bash-isms in the script?

-- 
David