From: David Aguilar <davvid@gmail.com>
Date: Mon, 05 Mar 2012 02:10:30 GMT
Subject: Re: [PATCH v2] mergetools: add support for DeltaWalker
Message-ID: <CAJDDKr6toChs8KpnAbjxeemBZT1AT1Uk9Ajk7DWBREuM6yFMzg@mail.gmail.com>
In-Reply-To: <CAFouetjZ0kQMnAPdNey5kSEmq+fmQqq+fC51hcT+9k6T8c6uOw@mail.gmail.com>

On Sat, Mar 3, 2012 at 7:50 PM, Tim Henigan <tim.henigan@gmail.com> wrote:
> On Sat, Mar 3, 2012 at 5:47 PM, Junio C Hamano <gitster@pobox.com> wrote:
>> David Aguilar <davvid@gmail.com> writes:
>>
>>>>> +merge_cmd () {
>>>>> +     if $base_present
>>>>> +     then
>>>>> +             "$merge_tool_path" "$LOCAL" "$REMOTE" "$BASE" -merged="$PWD/$MERGED"
>>>>> +     else
>>>>> +             "$merge_tool_path" "$LOCAL" "$REMOTE" -merged="$PWD/$MERGED"
>>>>> +     fi >/dev/null 2>&1
>>>>> +}
>>>
>>> Is the $PWD/ prefix strictly needed?  The rest of the mergetools use
>>> $MERGED as-is.  Does it work without it?
>>
>> Hrm, I didn't notice it but they do look fishy.  Thanks for good eyes.
>>
>> Tim?
>
> I ran a quick test using msysgit v1.7.9 on Win7 64-bit and found that
> it fails without '$PWD'.
>
> When '$PWD/' is removed from the '-merged' option, it results in a
> Java JRE crash and the conflict resolutions entered by the user are
> not written to the file.
>
> The JRE exception is 'EXCEPTION_ACCESS_VIOLATION (0xc0000005)'.  I
> posted the full text of the exception to a public location, in case
> anyone is interested [1].
>
> The format of the '-merged' option was copied directly from the
> DeltaWalker manual.  There was no explanation why '$PWD' is needed.
>
> [1]: https://gist.github.com/1970590

Thanks, that makes a lot of sense.  It looks like they have a bug.  Do
they know about it?

My naive guess is that they are using some inotify-like thing and
subscribing to a directory (per the stack trace).  Since
dirname("relative") is "" they crash.

So this bug probably wouldn't be present when merging a file in a
sub-directory, or ../ relative path.  Is this the case?

Can you mention this in a comment so that someone doesn't copy/paste
it into another tool in the future?

If they fix it then we may want to consider only supporting the newer
version and remove the $PWD.  Does the OS X version have this bug too?

I didn't notice any other production shell scripts in git using $PWD.
There is one use of `pwd` in git-stash.sh and it's used in a few
tests, as is $PWD, but $(pwd) is the most prevalent overall.  I don't
know the reason $PWD is avoided in the git shell scripts (though
portability is often the reason).

Deltawalker is not currently available on any of the platforms where
this would be a concern, but we may still want to use $(pwd) for
consistency with the other commands.  Does that work instead?
-- 
David