Re: p4Merge bundled command and the behaviour with files (same name) added on different branches.

[Ciaran <ciaranj@gmail.com>]

On Thu, Apr 7, 2011 at 10:43 AM, David Aguilar <davvid@gmail.com> wrote:
> On Mon, Apr 04, 2011 at 09:55:41AM +0100, Ciaran wrote:
>> [...]
>> We would expect a 'both added' merge conflict (both the other branch,
>> and the master branch added the file named bar.txt, but with different
>> content.)  This is all good and right.  So in a system configured to
>> use p4merge as the mergetool, one fires up with 'git mergetool'
>>
>> What happens now is p4merge starts and tells us:
>>
>> Base: bar.txt.LOCAL.<num1>.txt
>> Left: bar.txt.LOCAL.<num1>.txt Differences from base: 0
>> Right: bar.txt.LOCAL.<num2>.txt Differences from base: 1
>> Merge: bar.txt Conflicts:0
>>
>> Presenting the left + right options on top of each other in the result
>> window (which may be correct) and leaving the save button disabled
>> (grayed out)
>>
>> If at this point one closes the window without editing the presented
>> (apparently merged) file, then nothing will be saved to disk and we
>> will see:
>>
>> bar.txt seems unchanged.
>> Was the merge successful? [y/n]
>>
>> In the console.  Which Git wise is correct, that is exactly right, the
>> p4merge tool hasn't made any actual changes to the underlying file.
>>
>> This behaviour seems confusing to me (the p4merge client behaviour,
>> *not* Git's)   I believe it is because in the case where there is no
>> logical base between two files the local one is arbritrarily chosen,
>> and p4merge *thinks* that this is equal to the merge result and has
>> nothing to persist.
>>
>> I have attached a patch that resolves the issue for me (e.g.
>> introduces the behaviour I expect) by passing a reference to an empty
>> file in the case where there is no meaningful base.  Unfortunately I
>> don't understand enough to say whether this change is correct or not
>> and would value feedback on it.
>>
>> Many thanks
>>  - Cj.
>
> Thanks.  If this patch were for actual consideration you would
> inline the patch instead of sending an attachment as described
> in Documentation/SubmittingPatches.  Marking the subject line
> with "[RFC PATCH]" lets us know that you're interested in
> feedback.  I have a few questions below.
Thank you for respnding, I wasn't sure on the etiquette (and quite
frankly nervous as it was about posting to the list ;) ), so please
accept my apologies.

>
>> index fb3f52b..3e486dc 100644
>> --- a/git-mergetool--lib.sh
>> +++ b/git-mergetool--lib.sh
>> @@ -262,7 +262,9 @@ run_merge_tool () {
>>                       if $base_present; then
>>                               "$merge_tool_path" "$BASE" "$LOCAL" "$REMOTE" "$MERGED"
>>                       else
>> -                             "$merge_tool_path" "$LOCAL" "$LOCAL" "$REMOTE" "$MERGED"
>> +                             touch ".empty"
>> +                             "$merge_tool_path" ".empty" "$LOCAL" "$REMOTE" "$MERGED"
>> +                             rm ".empty"
>>                       fi
>>                       check_unchanged
>>               else
>> --
>
> What if the user has a file called '.empty' in their repository?
Then it will get over-written ;)

>
> What if the user Ctrl-C's out of mergetool -- does a stale
> .empty file get left behind?
Yup, I imagine so.

>
> Does it work if we pass /dev/null instead?
> Is such a strategy portable to Windows?
I don't think so, that was my first try (in Windows.)

>
> If /dev/null doesn't work, would it be better if the
> empty file were given a different name?
> Maybe something like foo.EMPTY.<num>.txt?
I'm amenable to anything.   My patch was really an example, hoping to
prompt a conversation with someone who actually knows the working of
git / mergetool-lib :)

Presumably I can co-opt whatever logic drives the existing
local/remote/merged temporary file names to create an 'empty' filename
in the temporary folder, since this file will always be identical it
shouldn't matter if it hangs around/gets updated con-currently etc. ?

Thanks
- Cj.

> --
>                David
>