[PATCH] merge: improve conflict markers by pointing to introrev

[Pierre-Yves David]

On 03/09/2016 02:36 PM, Simon Farnsworth wrote:
> On 08/03/2016, 19:45, "Pierre-Yves David" <pierre-yves.david at ens-lyon.org> wrote:
>
>
>> On 03/08/2016 06:40 PM, Simon Farnsworth wrote:
>>> On 08/03/2016, 14:20, "Yuya Nishihara" <youjah at gmail.com on behalf of yuya at tcha.org> wrote:
>>>> On Tue, 8 Mar 2016 13:24:02 +0000, Simon Farnsworth wrote:
>>>>> I'm reworking this so that conflict markers can access both the merge heads and the introrev to get you tags and bookmarks from both the merge side and the introrev. The issue this patch tries to address is that if you've got a large, fast moving, monolithic repository, the conflict markers are confusing (and we've had complaints that Mercurial doesn't get them right) - you're working on the backend C++ codebase, but (for some reason that's beyond you), you've got a conflict that references Android work only.
>>>>
>>>> [snip]
>>>>
>>>>> That's the goal of the extra revset predicates (separate patch series); by providing you with an easy route to find the three interesting points of a three way merge on a file (introrev of the local and other parts of the merge, and the base node), you have the tools to write a revset alias that gets you all the commits that touched unresolved files (for example):
>>>>>
>>>>> With
>>>>>
>>>>> [revsetalias]
>>>>> conflicts($1) = conflict("base",$1)::(conflict("other",$1) or conflict("local",$1)) and file($1)
>>>>
>>>> FWIW, if a file name and conflict side are provided, conflict marker can be
>>>> build by a template expression.
>>>>
>>>>    {revset("conflict(%s, %s)", conflictside, "path:{file}") % "{node|short}"}
>>>
>>> Not at the point Mercurial wants to build the conflict marker, unfortunately; conflict markers are created when we merge the files together and hit a conflict, but merge conflicts are not recorded until we've completed merging files.
>>
>> This is getting confusing, let's talk about it by voice tomorrow.
>
> Pierre-Yves and I talked about this over lunch.
>
> His objection is that introrev and merge parent are both random points; introrev is the last commit to change the file in conflict, not necessarily the commit (or commits) that caused the conflict. Using introrev runs the risk of increasing user confusion because it increases the sense that the conflict marker is telling you about the commit that caused the conflict.

Yeah, basically, I personally feel like the benefit of having more 
chance to be right (because sometimes we'll get it right) will be 
counter balanced by the extra confusion when we dont. In addition it get 
harder to report useful "branch/bookmark" destination for the user to 
seen something familiar in the conflict. I would rather consistently 
display the merge destination (sometime explicitly picked and consistent 
accross all files),

That said, my gut feeling that the benefit are not worth the drawback, 
but experimenting with this behind an experimental flag seems reasonable.

> For reference, a current default set of conflict markers looks like:
> <<<<<<< local: 7c459574949d tag book branch - user: commit 7c459574949d description first line
> =======
>>>>>>>> other: 6de9c4bc3d38 tag book branch - user: commit 6de9c4bc3d38 description first line
>
>
> We agreed that the cause of the confusion is that the "user: commit description first line" section; it gives the impression that the commit you're being pointed at is the commit that causes the conflict, right up until you hit a merge where that's not true and get confused (to the point where I've had users tell me that git's behaviour is better - yet git does the same as current hg).
>
> My thought is that we should change this, so a default set of commit markers look like:
> <<<<<<< local: merging 7c459574949d tag bookmark branch - last change 4dbc2cd1399a
> =======
>>>>>>>> other: merging 6de9c4bc3d38 tag bookmark branch - last change 00025129f739
>
>
> The idea is that by only giving you locations, and not details, you won't feel that the conflict marker is telling you about the commit that caused the conflict, but instead is giving you locations to help you find out why there's a conflict here.
>
> Thoughts?

- I've mixed feeling about dropping the description is often more useful 
than hash, especially during rebase because you can know what changeset 
you are merging in this case. But dropping it free a lot of space

- Adding both node is an interresting idea (not 100% sold yet).

I would suggest: "merging at 7c459574949d" to make it clearer that this 
is not "merging with 7c459574949d"

Cheers,

-- 
Pierre-Yves David