On Thu, Mar 7, 2013 at 11:10 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote: > Kevin Bracey <kevin@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html