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

login
register
mail settings
Submitter Paul Morelle
Date Oct. 6, 2017, 4:17 p.m.
Message ID <18c8fa9b75ba1c7b0dfd.1507306637@taranis.localdomain>
Download mbox | patch
Permalink /patch/24611/
State Accepted
Headers show

Comments

Paul Morelle - Oct. 6, 2017, 4:17 p.m.
# HG changeset patch
# User Paul Morelle <paul.morelle@octobus.net>
# Date 1507212785 -7200
#      Thu Oct 05 16:13:05 2017 +0200
# Node ID 18c8fa9b75ba1c7b0dfd984cf78d35d0a467ab24
# Parent  ae82f66cd58f85264e756f7a718ae9fbae5f17db
# EXP-Topic issue-5540
# Available At https://bitbucket.org/octobus/mercurial-devel/
#              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r 18c8fa9b75ba
strip: take branch into account when selecting update target (issue5540)

Test contributed by Matt Harbison

Keep the same behavior in most cases (i.e. first parent of the first root of
stripped changsets), but if the branch differs from wdir's, try to find another
parent of stripped commits that is on the same branch.
Matt Harbison - Oct. 7, 2017, 5:01 a.m.
On Fri, 06 Oct 2017 12:17:17 -0400, Paul Morelle  
<paul.morelle@octobus.net> wrote:

> # HG changeset patch
> # User Paul Morelle <paul.morelle@octobus.net>
> # Date 1507212785 -7200
> #      Thu Oct 05 16:13:05 2017 +0200
> # Node ID 18c8fa9b75ba1c7b0dfd984cf78d35d0a467ab24
> # Parent  ae82f66cd58f85264e756f7a718ae9fbae5f17db
> # EXP-Topic issue-5540
> # Available At https://bitbucket.org/octobus/mercurial-devel/
> #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r  
> 18c8fa9b75ba
> strip: take branch into account when selecting update target (issue5540)
>
> Test contributed by Matt Harbison
>
> Keep the same behavior in most cases (i.e. first parent of the first  
> root of
> stripped changsets), but if the branch differs from wdir's, try to find  
> another
> parent of stripped commits that is on the same branch.
>
> diff -r ae82f66cd58f -r 18c8fa9b75ba hgext/strip.py
> --- a/hgext/strip.py	Thu Oct 05 15:11:34 2017 +0200
> +++ b/hgext/strip.py	Thu Oct 05 16:13:05 2017 +0200
> @@ -60,10 +60,18 @@
> def _find_update_target(repo, revs):
>      urev, p2 = repo.changelog.parents(revs[0])
> +    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?   
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.

>     return urev
> diff -r ae82f66cd58f -r 18c8fa9b75ba tests/test-strip.t
> --- a/tests/test-strip.t	Thu Oct 05 15:11:34 2017 +0200
> +++ b/tests/test-strip.t	Thu Oct 05 16:13:05 2017 +0200
> @@ -941,6 +941,146 @@
>    abort: boom
>    [255]
> +test stripping a working directory parent doesn't switch named branches
> +
> +  $ hg log -G
> +  @  changeset:   1:eca11cf91c71
> +  |  tag:         tip
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     commitB
> +  |
> +  o  changeset:   0:105141ef12d0
> +     user:        test
> +     date:        Thu Jan 01 00:00:00 1970 +0000
> +     summary:     commitA
> +
> +
> +  $ hg branch new-branch
> +  marked working directory as branch new-branch
> +  (branches are permanent and global, did you want a bookmark?)
> +  $ hg ci -m "start new branch"
> +  $ echo 'foo' > foo.txt
> +  $ hg ci -Aqm foo
> +  $ hg up default
> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ echo 'bar' > bar.txt
> +  $ hg ci -Aqm bar
> +  $ hg up new-branch
> +  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  $ hg merge default
> +  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
> +  (branch merge, don't forget to commit)
> +  $ hg ci -m merge
> +  $ hg log -G
> +  @    changeset:   5:4cf5e92caec2
> +  |\   branch:      new-branch
> +  | |  tag:         tip
> +  | |  parent:      3:f62c6c09b707
> +  | |  parent:      4:35358f982181
> +  | |  user:        test
> +  | |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  | |  summary:     merge
> +  | |
> +  | o  changeset:   4:35358f982181
> +  | |  parent:      1:eca11cf91c71
> +  | |  user:        test
> +  | |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  | |  summary:     bar
> +  | |
> +  o |  changeset:   3:f62c6c09b707
> +  | |  branch:      new-branch
> +  | |  user:        test
> +  | |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  | |  summary:     foo
> +  | |
> +  o |  changeset:   2:b1d33a8cadd9
> +  |/   branch:      new-branch (glob)

Windows doesn't like this glob, and Linux seems OK without it too.

> +  |    user:        test
> +  |    date:        Thu Jan 01 00:00:00 1970 +0000
> +  |    summary:     start new branch
> +  |
> +  o  changeset:   1:eca11cf91c71
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     commitB
> +  |
> +  o  changeset:   0:105141ef12d0
> +     user:        test
> +     date:        Thu Jan 01 00:00:00 1970 +0000
> +     summary:     commitA
> +
> +
> +  $ hg strip -r 35358f982181

If I comment out the commit for the merge above the DAG and use --force on  
this strip, it updates to eca11cf91c71.  I'd expect it to still update to  
f62c6c09b707, since that's the same named branch.

> +  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
> +  saved backup bundle to  
> $TESTTMP/issue4736/.hg/strip-backup/35358f982181-a6f020aa-backup.hg  
> (glob)
> +  $ hg log -G
> +  @  changeset:   3:f62c6c09b707
> +  |  branch:      new-branch
> +  |  tag:         tip
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     foo
> +  |
> +  o  changeset:   2:b1d33a8cadd9
> +  |  branch:      new-branch
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     start new branch
> +  |
> +  o  changeset:   1:eca11cf91c71
> +  |  user:        test
> +  |  date:        Thu Jan 01 00:00:00 1970 +0000
> +  |  summary:     commitB
> +  |
> +  o  changeset:   0:105141ef12d0
> +     user:        test
> +     date:        Thu Jan 01 00:00:00 1970 +0000
> +     summary:     commitA
> +
Yuya Nishihara - Oct. 8, 2017, 6:02 a.m.
On Sat, 07 Oct 2017 01:01:08 -0400, Matt Harbison wrote:
> On Fri, 06 Oct 2017 12:17:17 -0400, Paul Morelle  
> <paul.morelle@octobus.net> wrote:
> 
> > # HG changeset patch
> > # User Paul Morelle <paul.morelle@octobus.net>
> > # Date 1507212785 -7200
> > #      Thu Oct 05 16:13:05 2017 +0200
> > # Node ID 18c8fa9b75ba1c7b0dfd984cf78d35d0a467ab24
> > # Parent  ae82f66cd58f85264e756f7a718ae9fbae5f17db
> > # EXP-Topic issue-5540
> > # Available At https://bitbucket.org/octobus/mercurial-devel/
> > #              hg pull https://bitbucket.org/octobus/mercurial-devel/ -r  
> > 18c8fa9b75ba
> > strip: take branch into account when selecting update target (issue5540)
> >
> > Test contributed by Matt Harbison
> >
> > Keep the same behavior in most cases (i.e. first parent of the first  
> > root of
> > stripped changsets), but if the branch differs from wdir's, try to find  
> > another
> > parent of stripped commits that is on the same branch.
> >
> > diff -r ae82f66cd58f -r 18c8fa9b75ba hgext/strip.py
> > --- a/hgext/strip.py	Thu Oct 05 15:11:34 2017 +0200
> > +++ b/hgext/strip.py	Thu Oct 05 16:13:05 2017 +0200
> > @@ -60,10 +60,18 @@
> > def _find_update_target(repo, revs):

Nit: _findupdatetarget() per our coding style.

https://www.mercurial-scm.org/wiki/CodingStyle#Naming_conventions

> >      urev, p2 = repo.changelog.parents(revs[0])
> > +    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.

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.

> 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.
Paul Morelle - Oct. 9, 2017, 11:08 a.m.
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?
>> 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.
Yuya Nishihara - Oct. 9, 2017, 1:40 p.m.
On Mon, 9 Oct 2017 13:08:29 +0200, Paul Morelle 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.

I meant destupdate() could provide hints to determine the desired behavior,
and some corner cases which we might have to take care. IIUC, we roughly need
a destupdate() equivalent which doesn't see the revisions to be stripped.

> "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?

+1, and Mads said it might be more useful, in ff2c89ebf5d4.
Matt Harbison - Oct. 10, 2017, 2:15 a.m.
On Mon, 09 Oct 2017 07:08:29 -0400, Paul Morelle  
<paul.morelle@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.
Paul Morelle - Oct. 10, 2017, 7:54 a.m.
On 10/10/2017 04:15 AM, Matt Harbison wrote:
> 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. 
Hello Matt,

Thank you for thinking about this again.
Yes, prune will have to be patched too, using the same strategy, and I
will do that once strip is fixed.
I have added a test for the branch merge case, and the tests are running
right now. I will push a V3 when it's done.

Patch

diff -r ae82f66cd58f -r 18c8fa9b75ba hgext/strip.py
--- a/hgext/strip.py	Thu Oct 05 15:11:34 2017 +0200
+++ b/hgext/strip.py	Thu Oct 05 16:13:05 2017 +0200
@@ -60,10 +60,18 @@ 
 
 def _find_update_target(repo, revs):
     urev, p2 = repo.changelog.parents(revs[0])
+    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)
 
     return urev
 
diff -r ae82f66cd58f -r 18c8fa9b75ba tests/test-strip.t
--- a/tests/test-strip.t	Thu Oct 05 15:11:34 2017 +0200
+++ b/tests/test-strip.t	Thu Oct 05 16:13:05 2017 +0200
@@ -941,6 +941,146 @@ 
   abort: boom
   [255]
 
+test stripping a working directory parent doesn't switch named branches
+
+  $ hg log -G
+  @  changeset:   1:eca11cf91c71
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     commitB
+  |
+  o  changeset:   0:105141ef12d0
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     commitA
+  
+
+  $ hg branch new-branch
+  marked working directory as branch new-branch
+  (branches are permanent and global, did you want a bookmark?)
+  $ hg ci -m "start new branch"
+  $ echo 'foo' > foo.txt
+  $ hg ci -Aqm foo
+  $ hg up default
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ echo 'bar' > bar.txt
+  $ hg ci -Aqm bar
+  $ hg up new-branch
+  1 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  $ hg merge default
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+  (branch merge, don't forget to commit)
+  $ hg ci -m merge
+  $ hg log -G
+  @    changeset:   5:4cf5e92caec2
+  |\   branch:      new-branch
+  | |  tag:         tip
+  | |  parent:      3:f62c6c09b707
+  | |  parent:      4:35358f982181
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     merge
+  | |
+  | o  changeset:   4:35358f982181
+  | |  parent:      1:eca11cf91c71
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     bar
+  | |
+  o |  changeset:   3:f62c6c09b707
+  | |  branch:      new-branch
+  | |  user:        test
+  | |  date:        Thu Jan 01 00:00:00 1970 +0000
+  | |  summary:     foo
+  | |
+  o |  changeset:   2:b1d33a8cadd9
+  |/   branch:      new-branch (glob)
+  |    user:        test
+  |    date:        Thu Jan 01 00:00:00 1970 +0000
+  |    summary:     start new branch
+  |
+  o  changeset:   1:eca11cf91c71
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     commitB
+  |
+  o  changeset:   0:105141ef12d0
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     commitA
+  
+
+  $ hg strip -r 35358f982181
+  0 files updated, 0 files merged, 1 files removed, 0 files unresolved
+  saved backup bundle to $TESTTMP/issue4736/.hg/strip-backup/35358f982181-a6f020aa-backup.hg (glob)
+  $ hg log -G
+  @  changeset:   3:f62c6c09b707
+  |  branch:      new-branch
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     foo
+  |
+  o  changeset:   2:b1d33a8cadd9
+  |  branch:      new-branch
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     start new branch
+  |
+  o  changeset:   1:eca11cf91c71
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     commitB
+  |
+  o  changeset:   0:105141ef12d0
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     commitA
+  
+
+  $ hg pull -u $TESTTMP/issue4736/.hg/strip-backup/35358f982181-a6f020aa-backup.hg
+  pulling from $TESTTMP/issue4736/.hg/strip-backup/35358f982181-a6f020aa-backup.hg (glob)
+  searching for changes
+  adding changesets
+  adding manifests
+  adding file changes
+  added 2 changesets with 1 changes to 1 files
+  1 files updated, 0 files merged, 0 files removed, 0 files unresolved
+
+  $ hg strip -k -r 35358f982181
+  saved backup bundle to $TESTTMP/issue4736/.hg/strip-backup/35358f982181-a6f020aa-backup.hg (glob)
+  $ hg log -G
+  @  changeset:   3:f62c6c09b707
+  |  branch:      new-branch
+  |  tag:         tip
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     foo
+  |
+  o  changeset:   2:b1d33a8cadd9
+  |  branch:      new-branch
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     start new branch
+  |
+  o  changeset:   1:eca11cf91c71
+  |  user:        test
+  |  date:        Thu Jan 01 00:00:00 1970 +0000
+  |  summary:     commitB
+  |
+  o  changeset:   0:105141ef12d0
+     user:        test
+     date:        Thu Jan 01 00:00:00 1970 +0000
+     summary:     commitA
+  
+  $ hg diff
+  diff -r f62c6c09b707 bar.txt
+  --- /dev/null	Thu Jan 01 00:00:00 1970 +0000
+  +++ b/bar.txt	Thu Jan 01 00:00:00 1970 +0000
+  @@ -0,0 +1,1 @@
+  +bar
+
 Use delayedstrip to strip inside a transaction
 
   $ cd $TESTTMP