Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool

From: Junio C Hamano <gitster@pobox.com>
Date: Mar 7, 2013, 19:10 UTC
Message-ID: <7vboavm3fh.fsf@alter.siamese.dyndns.org>
In-Reply-To: <5138CAFE.2010602@bracey.fi>

Kevin Bracey <kevin@bracey.fi> writes:

Show 19 quoted lines

> On 07/03/2013 09:23, Junio C Hamano wrote:
>> If p4merge GUI labels one side clearly as "theirs" and the other
>> "ours", and the way we feed the inputs to it makes the side that is
>> actually "ours" appear in p4merge GUI labelled as "theirs", then I
>> do not think backward compatibility argument does not hold water. It
>> is just correcting a longstanding 3-4 year old bug in a tool that
>> nobody noticed.
>
> It's not quite that clear-cut. Some years ago, and before p4merge was
> added as a Git mergetool, P4Merge was changed so its main GUI text
> says "left" and "right" instead of "theirs" and "ours" when invoked
> manually.
>
> But it appears that's as far as they went. It doesn't seem any of its
> asymmetric diff display logic was changed; it works better with ours
> on the right, and the built-in help all remains written on the
> theirs/ours basis. And even little details like the icons imply it (a
> square for the base, a downward-pointing triangle for their incoming
> stuff, and a circle for the version we hold).

So in short, a user of p4merge can see that left side is intended as "theirs", even though recent p4merge sometimes calls it "left". And your description on the coloring (green vs blue) makes it clear that "left" and "theirs" are still intended to be synonyms.

If that is the case I would think you can still argue such a change as "correcting a 3-4-year old bug".

> Would it be going too far to also have "xxxtool.reverse" to choose the
> global default?

It would be a natural thing to do. I left it out because I thought it would go without saying, given that precedences already exist, e.g. mergetool.keepBackup etc.

> My only reservation is that I assume it would be implemented by
> swapping what's passed in $LOCAL and $REMOTE. Which seems a bit icky:
> $LOCAL="a.REMOTE.1234.c".

Doesn't the UI show the actual temporary filename? When merging my version of hello.c with your version, showing them as hello.LOCAL.c and hello.REMOTE.c is an integral part of the UI experience, I think, even if the GUI tool does not give its own labels (and behaviour differences as you mentioned for p4merge) to mark which side is theirs and which side is ours. The temporary file that holds their version should still be named with REMOTE, even when the mergetool.reverse option is in effect.

As to the name of the variable, I do not care too deeply about it myself, but I think keeping the current LOCAL and REMOTE would help people following the code, especially given the option is called "reverse", meaning that there is an internal convention that the order is "LOCAL and then REMOTE".

One thing to watch out for is from which temporary file we take the merged results. You can present the two sides swapped, but if the tool always writes the results out by updating the second file, the caller needs to be prepared to read from the one that gets changed.