Patchwork D347: rebase: rewrite _computeobsoletenotrebased

login
register
mail settings
Submitter phabricator
Date Aug. 11, 2017, 9:02 a.m.
Message ID <differential-rev-PHID-DREV-afgdktfmojzmqwusozc7-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/22868/
State Superseded
Headers show

Comments

phabricator - Aug. 11, 2017, 9:02 a.m.
quark created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The old code stores successors of all related nodes together, which works
  fine if destination is unique. A future patch would make destination
  non-unique so let's change the implementation to test successors for
  rebaseset separately.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D347

AFFECTED FILES
  hgext/rebase.py

CHANGE DETAILS




To: quark, #hg-reviewers
Cc: mercurial-devel
phabricator - Aug. 29, 2017, 6:16 p.m.
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:1494
> +        successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
> +        if len(successors) == 1:
> +            # prune

Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A comment would help.

How about something like this?

  successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
  successors = successors[1:] # allsuccessors() includes the start nodes
  if not successors:
      # prune
  ...

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D347

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Aug. 29, 2017, 7:11 p.m.
quark added inline comments.

INLINE COMMENTS

> martinvonz wrote in rebase.py:1494
> Oh, allsuccessors() includes "srcnode" in the output! It took me a while to figure that out. A comment would help.
> 
> How about something like this?
> 
>   successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
>   successors = successors[1:] # allsuccessors() includes the start nodes
>   if not successors:
>       # prune
>   ...

That copies the list therefore slightly less performant. I'll add a comment.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D347

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel
phabricator - Aug. 30, 2017, 4:18 a.m.
martinvonz added inline comments.

INLINE COMMENTS

> rebase.py:1495
> +        if len(successors) == 1:
> +            # obsutil.allsuccessors returns node itself if it finds obsmarkers.
> +            # When the list only contains one element, it means there is at

"returns node itself if it finds obsmarkers" is technically correct, but but also returns it if there are no obsmarkers :-)

However, "When the list only contains one element, it means there is at least one obsmarker with srcnode as predecessor" is not correct; it does not mean that there is at least one obsmarker, it means that there are no successors.

Or did I misunderstand either the text or what allsuccessors() does?

I'll rephrase it in flight unless you tell me soon that I misunderstood.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D347

To: quark, #hg-reviewers
Cc: martinvonz, mercurial-devel

Patch

diff --git a/hgext/rebase.py b/hgext/rebase.py
--- a/hgext/rebase.py
+++ b/hgext/rebase.py
@@ -1436,31 +1436,23 @@ 
     obsolete => None entries in the mapping indicate nodes with no successor"""
     obsoletenotrebased = {}
 
-    # Build a mapping successor => obsolete nodes for the obsolete
-    # nodes to be rebased
-    allsuccessors = {}
-    cl = repo.changelog
-    for r in rebaseobsrevs:
-        node = cl.node(r)
-        for s in obsutil.allsuccessors(repo.obsstore, [node]):
-            try:
-                allsuccessors[cl.rev(s)] = cl.rev(node)
-            except LookupError:
-                pass
-
-    if allsuccessors:
-        # Look for successors of obsolete nodes to be rebased among
-        # the ancestors of dest
-        ancs = cl.ancestors([dest],
-                            stoprev=min(allsuccessors),
-                            inclusive=True)
-        for s in allsuccessors:
-            if s in ancs:
-                obsoletenotrebased[allsuccessors[s]] = s
-            elif (s == allsuccessors[s] and
-                  allsuccessors.values().count(s) == 1):
-                # plain prune
-                obsoletenotrebased[s] = None
+    cl = repo.unfiltered().changelog
+    nodemap = cl.nodemap
+    destnode = cl.node(dest)
+    for srcrev in rebaseobsrevs:
+        srcnode = cl.node(srcrev)
+        # XXX: more advanced APIs are required to handle split correctly
+        successors = list(obsutil.allsuccessors(repo.obsstore, [srcnode]))
+        if len(successors) == 1:
+            # prune
+            obsoletenotrebased[srcrev] = None
+        else:
+            for succnode in successors:
+                if succnode == srcnode or succnode not in nodemap:
+                    continue
+                if cl.isancestor(succnode, destnode):
+                    obsoletenotrebased[srcrev] = nodemap[succnode]
+                    break
 
     return obsoletenotrebased