Patchwork merge: improve conflict markers by pointing to introrev

login
register
mail settings
Submitter Simon Farnsworth
Date March 4, 2016, 4:31 p.m.
Message ID <f9c8bf605e5b96530f6a.1457109100@devvm631.lla1.facebook.com>
Download mbox | patch
Permalink /patch/13605/
State Changes Requested
Delegated to: Yuya Nishihara
Headers show

Comments

Simon Farnsworth - March 4, 2016, 4:31 p.m.
# HG changeset patch
# User Simon Farnsworth <simonfar@fb.com>
# Date 1457034054 0
#      Thu Mar 03 19:40:54 2016 +0000
# Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445
# Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab
merge: improve conflict markers by pointing to introrev

If you're working in a fast moving repository, the nodes chosen for conflict
markers are apparently nonsensical - they're the revisions you're actually
merging, even if the file in conflict did not change in those revisions.

Change the conflict markers to find the introrev for local and other, and
use that as the node instead. This means that the conflict markers now point
to the commits in which the conflicting file was last changed.
Yuya Nishihara - March 7, 2016, 1:22 p.m.
On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:
> # HG changeset patch
> # User Simon Farnsworth <simonfar@fb.com>
> # Date 1457034054 0
> #      Thu Mar 03 19:40:54 2016 +0000
> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445
> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab
> merge: improve conflict markers by pointing to introrev
> 
> If you're working in a fast moving repository, the nodes chosen for conflict
> markers are apparently nonsensical - they're the revisions you're actually
> merging, even if the file in conflict did not change in those revisions.
> 
> Change the conflict markers to find the introrev for local and other, and
> use that as the node instead. This means that the conflict markers now point
> to the commits in which the conflicting file was last changed.

The default conflict marker includes {tags} and {bookmarks}. I don't think
they should be changed to the introrev. Perhaps we'll need new template keyword
for the introrev.
Simon Farnsworth - March 7, 2016, 1:58 p.m.
On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:



>On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:

>> # HG changeset patch

>> # User Simon Farnsworth <simonfar@fb.com>

>> # Date 1457034054 0

>> #      Thu Mar 03 19:40:54 2016 +0000

>> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445

>> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab

>> merge: improve conflict markers by pointing to introrev

>> 

>> If you're working in a fast moving repository, the nodes chosen for conflict

>> markers are apparently nonsensical - they're the revisions you're actually

>> merging, even if the file in conflict did not change in those revisions.

>> 

>> Change the conflict markers to find the introrev for local and other, and

>> use that as the node instead. This means that the conflict markers now point

>> to the commits in which the conflicting file was last changed.

>

>The default conflict marker includes {tags} and {bookmarks}. I don't think

>they should be changed to the introrev. Perhaps we'll need new template keyword

>for the introrev.


Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?

My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.

I'm interested in other use cases I've not considered - if you'd prefer to discuss on IRC, I'm "farnz" on #mercurial, and I tend to be around from 1100 to 1800 UTC Monday to Friday.

Simon
Yuya Nishihara - March 7, 2016, 2:56 p.m.
On Mon, 7 Mar 2016 13:58:29 +0000, Simon Farnsworth wrote:
> On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
> 
> 
> 
> >On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:
> >> # HG changeset patch
> >> # User Simon Farnsworth <simonfar@fb.com>
> >> # Date 1457034054 0
> >> #      Thu Mar 03 19:40:54 2016 +0000
> >> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445
> >> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab
> >> merge: improve conflict markers by pointing to introrev
> >> 
> >> If you're working in a fast moving repository, the nodes chosen for conflict
> >> markers are apparently nonsensical - they're the revisions you're actually
> >> merging, even if the file in conflict did not change in those revisions.
> >> 
> >> Change the conflict markers to find the introrev for local and other, and
> >> use that as the node instead. This means that the conflict markers now point
> >> to the commits in which the conflicting file was last changed.
> >
> >The default conflict marker includes {tags} and {bookmarks}. I don't think
> >they should be changed to the introrev. Perhaps we'll need new template keyword
> >for the introrev.
> 
> Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?
> 
> My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.

If you're merging two bookmarked branches, {bookmarks} can provide which side
is which branch. I think that's why the conflict marker shows {bookmarks}.

Does the bookmark of the introrev provide something interesting?
Simon Farnsworth - March 7, 2016, 4:12 p.m.
On 07/03/2016, 14:56, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:


>On Mon, 7 Mar 2016 13:58:29 +0000, Simon Farnsworth wrote:

>> On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

>> 

>> 

>> 

>> >On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:

>> >> # HG changeset patch

>> >> # User Simon Farnsworth <simonfar@fb.com>

>> >> # Date 1457034054 0

>> >> #      Thu Mar 03 19:40:54 2016 +0000

>> >> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445

>> >> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab

>> >> merge: improve conflict markers by pointing to introrev

>> >> 

>> >> If you're working in a fast moving repository, the nodes chosen for conflict

>> >> markers are apparently nonsensical - they're the revisions you're actually

>> >> merging, even if the file in conflict did not change in those revisions.

>> >> 

>> >> Change the conflict markers to find the introrev for local and other, and

>> >> use that as the node instead. This means that the conflict markers now point

>> >> to the commits in which the conflicting file was last changed.

>> >

>> >The default conflict marker includes {tags} and {bookmarks}. I don't think

>> >they should be changed to the introrev. Perhaps we'll need new template keyword

>> >for the introrev.

>> 

>> Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?

>> 

>> My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.

>

>If you're merging two bookmarked branches, {bookmarks} can provide which side

>is which branch. I think that's why the conflict marker shows {bookmarks}.


My assumption is that you already know which side is which bookmark - and I'm enhancing merge state to have descriptions of "local" and "other" that will help with that.

>

>Does the bookmark of the introrev provide something interesting?


Depends on your workflow; it does if you're in the habit of creating bookmarks at interesting intermediate states, as then you'll get told that this conflict is with the interesting intermediate state, not with the latest state.

I can see the point of giving the conflict markers both nodes, though (introrev and merge point), and defaulting to tags and bookmarks from the merge node, not the introrev node. I'll look into this for a V2.

Simon
Pierre-Yves David - March 8, 2016, 12:41 p.m.
On 03/07/2016 05:12 PM, Simon Farnsworth wrote:
> On 07/03/2016, 14:56, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>
>
>> On Mon, 7 Mar 2016 13:58:29 +0000, Simon Farnsworth wrote:
>>> On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>>>
>>>
>>>
>>>> On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:
>>>>> # HG changeset patch
>>>>> # User Simon Farnsworth <simonfar@fb.com>
>>>>> # Date 1457034054 0
>>>>> #      Thu Mar 03 19:40:54 2016 +0000
>>>>> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445
>>>>> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab
>>>>> merge: improve conflict markers by pointing to introrev
>>>>>
>>>>> If you're working in a fast moving repository, the nodes chosen for conflict
>>>>> markers are apparently nonsensical - they're the revisions you're actually
>>>>> merging, even if the file in conflict did not change in those revisions.
>>>>>
>>>>> Change the conflict markers to find the introrev for local and other, and
>>>>> use that as the node instead. This means that the conflict markers now point
>>>>> to the commits in which the conflicting file was last changed.
>>>>
>>>> The default conflict marker includes {tags} and {bookmarks}. I don't think
>>>> they should be changed to the introrev. Perhaps we'll need new template keyword
>>>> for the introrev.
>>>
>>> Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?
>>>
>>> My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.
>>
>> If you're merging two bookmarked branches, {bookmarks} can provide which side
>> is which branch. I think that's why the conflict marker shows {bookmarks}.
>
> My assumption is that you already know which side is which bookmark - and I'm enhancing merge state to have descriptions of "local" and "other" that will help with that.

It is not clear to me that "you already know which side is which 
bookmark", people get confused easily especially on large merge. 
Furthermore when they use command overlay that hide them the underlying 
Mercurial invocation (like your employer do).

>> Does the bookmark of the introrev provide something interesting?
>
> Depends on your workflow; it does if you're in the habit of creating bookmarks at interesting intermediate states, as then you'll get told that this conflict is with the interesting intermediate state, not with the latest state.
>
> I can see the point of giving the conflict markers both nodes, though (introrev and merge point), and defaulting to tags and bookmarks from the merge node, not the introrev node. I'll look into this for a V2.

My general opinion on this "using introrev" business is that it is a red 
herring. Revision touching a file are likely to be numberous so print 
the latest one is going to marginaly help in some case and confuse user 
in all the other. I would rather see provided an easy way to see the 
"what revision touched that file on each side" information than trying 
to try to fit very complexe information into the limited conflict marker 
entry.
Simon Farnsworth - March 8, 2016, 1:24 p.m.
On 08/03/2016, 12:41, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:


>On 03/07/2016 05:12 PM, Simon Farnsworth wrote:

>> On 07/03/2016, 14:56, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

>>

>>

>>> On Mon, 7 Mar 2016 13:58:29 +0000, Simon Farnsworth wrote:

>>>> On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:

>>>>

>>>>

>>>>

>>>>> On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:

>>>>>> # HG changeset patch

>>>>>> # User Simon Farnsworth <simonfar@fb.com>

>>>>>> # Date 1457034054 0

>>>>>> #      Thu Mar 03 19:40:54 2016 +0000

>>>>>> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445

>>>>>> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab

>>>>>> merge: improve conflict markers by pointing to introrev

>>>>>>

>>>>>> If you're working in a fast moving repository, the nodes chosen for conflict

>>>>>> markers are apparently nonsensical - they're the revisions you're actually

>>>>>> merging, even if the file in conflict did not change in those revisions.

>>>>>>

>>>>>> Change the conflict markers to find the introrev for local and other, and

>>>>>> use that as the node instead. This means that the conflict markers now point

>>>>>> to the commits in which the conflicting file was last changed.

>>>>>

>>>>> The default conflict marker includes {tags} and {bookmarks}. I don't think

>>>>> they should be changed to the introrev. Perhaps we'll need new template keyword

>>>>> for the introrev.

>>>>

>>>> Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?

>>>>

>>>> My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.

>>>

>>> If you're merging two bookmarked branches, {bookmarks} can provide which side

>>> is which branch. I think that's why the conflict marker shows {bookmarks}.

>>

>> My assumption is that you already know which side is which bookmark - and I'm enhancing merge state to have descriptions of "local" and "other" that will help with that.

>

>It is not clear to me that "you already know which side is which 

>bookmark", people get confused easily especially on large merge. 

>Furthermore when they use command overlay that hide them the underlying 

>Mercurial invocation (like your employer do).


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.

By pulling back to the introrev for the commit, you get a clearer conflict marker - you're now seeing a commit that actually touched the file in conflict, instead of an apparently random commit from later in the tree.

>

>>> Does the bookmark of the introrev provide something interesting?

>>

>> Depends on your workflow; it does if you're in the habit of creating bookmarks at interesting intermediate states, as then you'll get told that this conflict is with the interesting intermediate state, not with the latest state.

>>

>> I can see the point of giving the conflict markers both nodes, though (introrev and merge point), and defaulting to tags and bookmarks from the merge node, not the introrev node. I'll look into this for a V2.

>

>My general opinion on this "using introrev" business is that it is a red 

>herring. Revision touching a file are likely to be numberous so print 

>the latest one is going to marginaly help in some case and confuse user 

>in all the other. I would rather see provided an easy way to see the 

>"what revision touched that file on each side" information than trying 

>to try to fit very complexe information into the limited conflict marker 

>entry.


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)


I can get an interesting view in TortoiseHG by filtering for 'conflicts("set:unresolved()")' or doing hg log -r 'conflicts("set:resolved()")' when I'm finished resolving to see which commits I should reference in my merge message.

Simon
Yuya Nishihara - March 8, 2016, 2:20 p.m.
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}"}
Pierre-Yves David - March 8, 2016, 3:29 p.m.
On 03/08/2016 01:24 PM, Simon Farnsworth wrote:
> On 08/03/2016, 12:41, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>
>
>> On 03/07/2016 05:12 PM, Simon Farnsworth wrote:
>>> On 07/03/2016, 14:56, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>>>
>>>
>>>> On Mon, 7 Mar 2016 13:58:29 +0000, Simon Farnsworth wrote:
>>>>> On 07/03/2016, 13:22, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@tcha.org> wrote:
>>>>>
>>>>>
>>>>>
>>>>>> On Fri, 4 Mar 2016 08:31:40 -0800, Simon Farnsworth wrote:
>>>>>>> # HG changeset patch
>>>>>>> # User Simon Farnsworth <simonfar@fb.com>
>>>>>>> # Date 1457034054 0
>>>>>>> #      Thu Mar 03 19:40:54 2016 +0000
>>>>>>> # Node ID f9c8bf605e5b96530f6a4636c78f9c444767c445
>>>>>>> # Parent  9d6c19c8413a039aff8399d6b1db573cb6610fab
>>>>>>> merge: improve conflict markers by pointing to introrev
>>>>>>>
>>>>>>> If you're working in a fast moving repository, the nodes chosen for conflict
>>>>>>> markers are apparently nonsensical - they're the revisions you're actually
>>>>>>> merging, even if the file in conflict did not change in those revisions.
>>>>>>>
>>>>>>> Change the conflict markers to find the introrev for local and other, and
>>>>>>> use that as the node instead. This means that the conflict markers now point
>>>>>>> to the commits in which the conflicting file was last changed.
>>>>>>
>>>>>> The default conflict marker includes {tags} and {bookmarks}. I don't think
>>>>>> they should be changed to the introrev. Perhaps we'll need new template keyword
>>>>>> for the introrev.
>>>>>
>>>>> Why do you think that {tags} and {bookmarks} should be on the nodes being merged, not on the introrevs?
>>>>>
>>>>> My reasoning for changing the node itself (hence also changing {tags} and {bookmarks}) is that when you're dealing with a merge conflict, you're not interested in the merge heads themselves - in the common cases, you either know exactly which nodes are being merged (because you're merging two trees of your own work together), or one is familiar (your own work), and the other is random (because you don't know the state of the remote tree at the point you did "hg pull"). Having the tags and bookmarks in the conflict marker come from the introrev means that I get told if the conflict happened at an interesting point in the tree. For slow-moving trees, this is uninteresting information - the introrev of a conflict is the same as the merge revision, because I simply don't get many conflicts in the first place; it's once you get into fast moving trees that conflicts get interesting, and in those cases, the merge nodes are pseudo-random anyway.
>>>>
>>>> If you're merging two bookmarked branches, {bookmarks} can provide which side
>>>> is which branch. I think that's why the conflict marker shows {bookmarks}.
>>>
>>> My assumption is that you already know which side is which bookmark - and I'm enhancing merge state to have descriptions of "local" and "other" that will help with that.
>>
>> It is not clear to me that "you already know which side is which
>> bookmark", people get confused easily especially on large merge.
>> Furthermore when they use command overlay that hide them the underlying
>> Mercurial invocation (like your employer do).
>
> 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.
>
> By pulling back to the introrev for the commit, you get a clearer conflict marker - you're now seeing a commit that actually touched the file in conflict, instead of an apparently random commit from later in the tree.
>
>>
>>>> Does the bookmark of the introrev provide something interesting?
>>>
>>> Depends on your workflow; it does if you're in the habit of creating bookmarks at interesting intermediate states, as then you'll get told that this conflict is with the interesting intermediate state, not with the latest state.
>>>
>>> I can see the point of giving the conflict markers both nodes, though (introrev and merge point), and defaulting to tags and bookmarks from the merge node, not the introrev node. I'll look into this for a V2.
>>
>> My general opinion on this "using introrev" business is that it is a red
>> herring. Revision touching a file are likely to be numberous so print
>> the latest one is going to marginaly help in some case and confuse user
>> in all the other. I would rather see provided an easy way to see the
>> "what revision touched that file on each side" information than trying
>> to try to fit very complexe information into the limited conflict marker
>> entry.
>
> 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):

My point is that there is no such things as "the three interesting 
points of a three ways merge". The last changeset touching a file have 
no reason to be much more interesting than the second last one, etc…
Simon Farnsworth - March 8, 2016, 6:04 p.m.
On 08/03/2016, 15:29, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:


>On 03/08/2016 01:24 PM, Simon Farnsworth wrote:

>> On 08/03/2016, 12:41, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:

>>>My general opinion on this "using introrev" business is that it is a red

>>> herring. Revision touching a file are likely to be numberous so print

>>> the latest one is going to marginaly help in some case and confuse user

>>> in all the other. I would rather see provided an easy way to see the

>>> "what revision touched that file on each side" information than trying

>>> to try to fit very complexe information into the limited conflict marker

>>> entry.

>>

>> 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):

>

>My point is that there is no such things as "the three interesting 

>points of a three ways merge". The last changeset touching a file have 

>no reason to be much more interesting than the second last one, etc…


In the context of Mercurial revsets, I disagree; we have the revset language for starting from boundary points and getting to interesting places in-between. It is unlikely that changes before the three way merge base are interesting, because that's the common ancestor point; similarly, changes after the most recent changes to a file in either side of the merge are unlikely to be interesting; if you have all three points, you can write (as I demonstrated in the context you've snipped) a revset expression that gets you all the interesting commits for a human merge.

You can then decide how to put these points together - do you ask Mercurial for the DAG between the base and both edge nodes, do you filter based on files touched (not just the file in merge conflict - you might, for example, want every commit touching a file that you think of as part of the same project as the file in conflict, for hints about the broader direction of development at the time), do you perhaps want to see some descendants of these nodes (and if so, which ones)?

Simon
Simon Farnsworth - March 8, 2016, 6:40 p.m.
On 08/03/2016, 14:20, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@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.

Simon
Pierre-Yves David - March 8, 2016, 7:45 p.m.
On 03/08/2016 06:40 PM, Simon Farnsworth wrote:
> On 08/03/2016, 14:20, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@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.
Simon Farnsworth - March 9, 2016, 2:36 p.m.
On 08/03/2016, 19:45, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:


>On 03/08/2016 06:40 PM, Simon Farnsworth wrote:

>> On 08/03/2016, 14:20, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@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.

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?

Simon
timeless - March 9, 2016, 2:57 p.m.
On Wed, Mar 9, 2016 at 9:36 AM, Simon Farnsworth <simonfar@fb.com> wrote:
> 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.
>
> 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.

I like this. It's what I moved histedit to.
I have half a mind to do something similar for rebase (although there
are other things I'm considering first, and it isn't a priority).
Pierre-Yves David - March 9, 2016, 8:28 p.m.
On 03/09/2016 02:36 PM, Simon Farnsworth wrote:
> On 08/03/2016, 19:45, "Pierre-Yves David" <pierre-yves.david@ens-lyon.org> wrote:
>
>
>> On 03/08/2016 06:40 PM, Simon Farnsworth wrote:
>>> On 08/03/2016, 14:20, "Yuya Nishihara" <youjah@gmail.com on behalf of yuya@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,
Danek Duvall - March 11, 2016, 8:02 p.m.
Pierre-Yves David wrote:

> On 03/09/2016 02:36 PM, Simon Farnsworth wrote:
>
> >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

I would really be sorry to not have the description.  I'm not usually
working in a situation where the tag or branch information would be useful,
and we don't use bookmarks, and I haven't memorized all the changeset IDs,
but the descriptions would usually give me a pretty good idea of what part
of the tree that changeset is from.  The author of that changeset might be
nice, too, though in my case it'll usually not be completely
distinguishing.

As long as it's all configurable, though, I'm okay with configuring it to
my own preferences.

Danek

Patch

diff --git a/mercurial/filemerge.py b/mercurial/filemerge.py
--- a/mercurial/filemerge.py
+++ b/mercurial/filemerge.py
@@ -515,13 +515,13 @@ 
 
 _defaultconflictlabels = ['local', 'other']
 
-def _formatlabels(repo, fcd, fco, fca, labels):
+def _formatlabels(repo, fcl, fco, fca, labels):
     """Formats the given labels using the conflict marker template.
 
     Returns a list of formatted labels.
     """
-    cd = fcd.changectx()
-    co = fco.changectx()
+    cl = fcl.repo()[fcl.introrev()]
+    co = fco.repo()[fco.introrev()]
     ca = fca.changectx()
 
     ui = repo.ui
@@ -530,7 +530,7 @@ 
 
     pad = max(len(l) for l in labels)
 
-    newlabels = [_formatconflictmarker(repo, cd, tmpl, labels[0], pad),
+    newlabels = [_formatconflictmarker(repo, cl, tmpl, labels[0], pad),
                  _formatconflictmarker(repo, co, tmpl, labels[1], pad)]
     if len(labels) > 2:
         newlabels.append(_formatconflictmarker(repo, ca, tmpl, labels[2], pad))
@@ -563,6 +563,7 @@ 
 
     ui = repo.ui
     fd = fcd.path()
+    fcl = fcd.changectx().filectx(orig)
     binary = fcd.isbinary() or fco.isbinary() or fca.isbinary()
     symlink = 'l' in fcd.flags() + fco.flags()
     changedelete = fcd.isabsent() or fco.isabsent()
@@ -621,7 +622,7 @@ 
         if not labels:
             labels = _defaultconflictlabels
         if markerstyle != 'basic':
-            labels = _formatlabels(repo, fcd, fco, fca, labels)
+            labels = _formatlabels(repo, fcl, fco, fca, labels)
 
         if premerge and mergetype == fullmerge:
             r = _premerge(repo, fcd, fco, fca, toolconf, files, labels=labels)
diff --git a/tests/test-keyword.t b/tests/test-keyword.t
--- a/tests/test-keyword.t
+++ b/tests/test-keyword.t
@@ -1080,7 +1080,7 @@ 
   bar
   =======
   foo
-  >>>>>>> other: 85d2d2d732a5  - test: simplemerge
+  >>>>>>> other: 7e422f1c943c  - test: 5foo
 
 resolve to local, m must contain hash of last change (local parent)