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

From: David Aguilar <davvid@gmail.com>
To: Junio C Hamano <gitster@pobox.com>
Cc: Kevin Bracey <kevin@bracey.fi>,
	git@vger.kernel.org, Ciaran Jessup <ciaranj@gmail.com>,
	Scott Chacon <schacon@gmail.com>
Subject: Re: [PATCH 1/2] p4merge: swap LOCAL and REMOTE for mergetool
Date: Thu, 7 Mar 2013 11:50:36 -0800	[thread overview]
Message-ID: <CAJDDKr5-ttcU48r0-qTfov7q736Rj63rS33fTScSsvx53VG4pA@mail.gmail.com> (raw)
In-Reply-To: <7vboavm3fh.fsf@alter.siamese.dyndns.org>

On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Kevin Bracey <kevin@bracey.fi> writes:
>
>> 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".

I would prefer to treat this as a bugfix rather than introducing
a new set of configuration knobs if possible.  It really does
seem like a correction.

Users that want the traditional behavior can get that by
configuring a custom mergetool.p4merge.cmd, so we're not
completely losing the ability to get at the old behavior.

Users that want to see a reverse diff with difftool can
already say "--reverse", so there's even less reason to
have it there (though I know we're talking about mergetool only).

>> 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.

Medium NACK.  If we can do without configuration all the better.

I would much rather prefer to have the default/mainstream
behavior be the best out-of-the-box sans configuration.

The reasoning behind swapping them for p4merge makes sense
for p4merge only.  I don't think we're quite ready to declare
that all the merge tools need to be swapped or that we need a
mechanism for swapping the order.

>> 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.
-- 
David