[PATCH] merge: extract _resolvetrivial() function

Mads Kiilerich mads at kiilerich.com
Thu Dec 11 13:56:16 CST 2014


On 12/11/2014 07:58 PM, Matt Mackall wrote:
> On Thu, 2014-12-11 at 00:43 +0100, Mads Kiilerich wrote:
>> On 12/10/2014 10:18 PM, Martin von Zweigbergk wrote:
>>> # HG changeset patch
>>> # User Martin von Zweigbergk <martinvonz at google.com>
>>> # Date 1417643428 28800
>>> #      Wed Dec 03 13:50:28 2014 -0800
>>> # Node ID 9b8dc07b08a2721d1db6f218bee06880a40d168d
>>> # Parent  07fcb956212223676b35055d39dddac88ca990b6
>>> merge: extract _resolvetrivial() function
>>>
>>> We would eventually like to move the resolution of modify/delete and
>>> delete/modify conflicts to the resolve phase. However, we don't want
>>> to move the checks for identical content that were added in
>>> 902554884335 (merge: before cd/dc prompt, check that changed side
>>> really changed, 2014-12-01). Let's instead move these out to a new
>>> _resolvetrivial() function that processes the actions from
>>> manifestmerge() and replaces any false cd/dc conflicts. The function
>>> will also provide a natural place for us to later add code for
>>> resolving false 'm' conflicts.
>> Hmm ... it doesn't sound wrong ... but so far we don't have any use of
>> it and it doesn't add much value by itself. It could perhaps wait until
>> we have some use of it and know it is going in the right direction?
>>
>> Resolving 'm' conflicts in this function seems to imply that we
>> duplicate a lot of manifestmerge in this function. Meh.
> No, it just means moving this line out of filemerge:
>
> http://www.selenic.com/hg/file/416c133145ee/mercurial/filemerge.py#l381

Agreed, assuming we want it to show up as something that is a resolved 
merge, instead of showing the simpler actions actions usually computed 
in manifestmerge 
http://www.selenic.com/hg/file/416c133145ee/mercurial/merge.py#l425 . I 
assume there is a reason we don't postpone all of these to filemerge in 
the first place?

>
>> More important, I would like to somehow move this "changed but same"
>> check into manifestmerge, before scheduling any wrong actions. Bid merge
>> needs that so it avoids picking unnecessarily hard actions - especially
>> merge actions, but also cd/dc. It would be an unfortunate layering
>> violation to consider actual file content in manifestmerge and it could
>> be costly to retrieve file content an extra time and at the wrong time
>> ... but that seems to be what it takes. This patch will not help us much
>> going in that direction ... if we should end up going in that direction.
> Seems this will actually give us an opportunity to defer those expensive
> comparisons. If our bids on a file are:
>
> keep
> merge
>
> ..we can just jump to keep without discovering whether the merge is
> actually trivial, right?

Yes, but especially when recovering from a criss cross merge, we will 
very rarely have a "same filelog node" situation. There will have been 
merges, and assuming things have been resolved "correctly", we will have 
a "different filelog node but same content" situation on one side. We 
want to pick that side. Currently we cannot.

Having the "changed but same" check in manifestmerge will _give_ us the 
"keep/merge" situation that bid merge can handle optimally. We will need 
that before the bid auction.

> However, when we're trying to pick between two merges, we obviously want
> to go with the trivial one, so we might want a two-pass approach.

Yes, we can implement the "changed but same" check for 'm', 'cd' and 
'cd' in/before bid merge too. That would be pretty much the same as we 
already have to do after bid merge (for cd/dc) or in filemerge (for m). 
I think it would be simpler and cleaner to make the first pass in 
manifestmerge handle "changed but same" too. I assume that is how we 
would do it if we had a "clean content" hash in the index. The question 
is how much we will let the lack of it influence the architecture and 
how value it adds to be able to defer it.

/Mads


More information about the Mercurial-devel mailing list