[PATCH 2 of 2 V2] strip: take branch into account when selecting update target (issue5540)

[Matt Harbison]

On Mon, 09 Oct 2017 07:08:29 -0400, Paul Morelle  
<paul.morelle at octobus.net> wrote:

> On 10/08/2017 08:02 AM, Yuya Nishihara wrote:
>>>> +    current_branch = repo[None].branch()
>>>>      if (util.safehasattr(repo, 'mq') and p2 != nullid
>>>>          and p2 in [x.node for x in repo.mq.applied]):
>>>>          urev = p2
>>>> +    elif current_branch != repo[urev].branch():
>>>> +        revset = "(parents(%ln::parents(wdir())) -
>>>> %ln::parents(wdir()))" \
>>>> +                 + " and branch(%s)"
>>>> +        branch_targets = repo.revs(revset, revs, revs,  
>>>> current_branch)
>>>> +        if branch_targets:
>>>> +            cl = repo.changelog
>>>> +            urev = min(cl.node(r) for r in branch_targets)
>>> Should this be max() instead of min(), to get the node closest to wdir?
>> or just 'max(parents(%ln::...)'. Sorting sha1 hash doesn't make sense.
> I agree with the fact that sorting on sha1 hashes doesn't really make  
> sense.
> However, this reproduces the same (mis-)behavior as the replaced code
> https://www.mercurial-scm.org/repo/hg/file/tip/hgext/strip.py#l199,
> which takes the first item of the sorted array, hence the min.
>> Perhaps, destutil.destupdate() will give more hints about tricky cases.  
>> I
>> don't know how "hg prune" resolve the update destination, but I think  
>> that
>> would be similar to what "hg strip" should do.
> destutil.destupdate() is about updating to descendants while here we are
> updating to ancestors, so we can't reuse it.
>
> "hg prune" does nothing smart about branches, and has the same issue as
> the current strip code (and we should probably reuse the new strip code
> for prune too).
>
> Since using the highest revision number seems more appropriate, does
> everyone agree to use a max in a V3?

After thinking about this more, I'm not sure what the right behavior is.   
It sounds like it would be useful to address this and prune at the same  
time in a follow up, without side tracking this.  But I think the branch  
change with an uncommitted merge needs to be fixed here.

>>> Updating way back might be surprising.  I didn't try making a more
>>> elaborate test, so maybe not.  I'm thinking, for example, developer A
>>> keeps pulling and merging from developer B, both using the same named
>>> branch.  Then strip the first merge.