D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[mharbison72 (Matt Harbison)]

Closed by commit rHG5f9c917e3b50: phabricator: teach `getoldnodedrevmap()` to 
handle folded reviews (authored by mharbison72).
This revision was automatically updated to reflect the committed changes.
This revision was not accepted when it landed; it landed in state "Needs 
Review".

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D8311?vs=20907&id=20945

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8311/new/

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

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -483,18 +483,23 @@
 alldiffs = callconduit(
 unfi.ui, b'differential.querydiffs', {b'revisionIDs': drevs}
 )
-getnode = lambda d: bin(getdiffmeta(d).get(b'node', b'')) or None
+
+def getnodes(d, precset):
+# Ignore other nodes that were combined into the Differential
+# that aren't predecessors of the current local node.
+return [n for n in getlocalcommits(d) if n in precset]
+
 for newnode, (force, precset, drev) in toconfirm.items():
 diffs = [
 d for d in alldiffs.values() if int(d[b'revisionID']) == drev
 ]
 
-# "precursors" as known by Phabricator
-phprecset = {getnode(d) for d in diffs}
+# local predecessors known by Phabricator
+phprecset = {n for d in diffs for n in getnodes(d, precset)}
 
 # Ignore if precursors (Phabricator and local repo) do not overlap,
 # and force is not set (when commit message says nothing)
-if not force and not bool(phprecset & precset):
+if not force and not phprecset:
 tagname = b'D%d' % drev
 tags.tag(
 repo,
@@ -519,7 +524,20 @@
 oldnode = lastdiff = None
 if diffs:
 lastdiff = max(diffs, key=lambda d: int(d[b'id']))
-oldnode = getnode(lastdiff)
+oldnodes = getnodes(lastdiff, precset)
+
+# If this commit was the result of `hg fold` after submission,
+# and now resubmitted with --fold, the easiest thing to do is
+# to leave the node clear.  This only results in creating a new
+# diff for the _same_ Differential Revision if this commit is
+# the first or last in the selected range.
+# If this commit is the result of `hg split` in the same
+# scenario, there is a single oldnode here (and multiple
+# newnodes mapped to it).  That makes it the same as the normal
+# case, as the edges of the newnode range cleanly maps to one
+# oldnode each.
+if len(oldnodes) == 1:
+oldnode = oldnodes[0]
 if oldnode and not has_node(oldnode):
 oldnode = None
 
@@ -1667,6 +1685,21 @@
 return _differentialrevisiondescre.sub(uri, ctx.description())
 
 
+def getlocalcommits(diff):
+"""get the set of local commits from a diff object
+
+See ``getdiffmeta()`` for an example diff object.
+"""
+props = diff.get(b'properties') or {}
+commits = props.get(b'local:commits') or {}
+if len(commits) > 1:
+return {bin(c) for c in commits.keys()}
+
+# Storing the diff metadata predates storing `local:commits`, so continue
+# to use that in the --no-fold case.
+return {bin(getdiffmeta(diff).get(b'node', b'')) or None}
+
+
 def getdiffmeta(diff):
 """get commit metadata (date, node, user, p1) from a diff object
 



To: mharbison72, #hg-reviewers, Alphare
Cc: marmoute, Alphare, Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[mharbison72 (Matt Harbison)]

mharbison72 updated this revision to Diff 20907.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST UPDATE
  https://phab.mercurial-scm.org/D8311?vs=20851&id=20907

BRANCH
  default

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8311/new/

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

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -483,18 +483,23 @@
 alldiffs = callconduit(
 unfi.ui, b'differential.querydiffs', {b'revisionIDs': drevs}
 )
-getnode = lambda d: bin(getdiffmeta(d).get(b'node', b'')) or None
+
+def getnodes(d, precset):
+# Ignore other nodes that were combined into the Differential
+# that aren't predecessors of the current local node.
+return [n for n in getlocalcommits(d) if n in precset]
+
 for newnode, (force, precset, drev) in toconfirm.items():
 diffs = [
 d for d in alldiffs.values() if int(d[b'revisionID']) == drev
 ]
 
-# "precursors" as known by Phabricator
-phprecset = {getnode(d) for d in diffs}
+# local predecessors known by Phabricator
+phprecset = {n for d in diffs for n in getnodes(d, precset)}
 
 # Ignore if precursors (Phabricator and local repo) do not overlap,
 # and force is not set (when commit message says nothing)
-if not force and not bool(phprecset & precset):
+if not force and not phprecset:
 tagname = b'D%d' % drev
 tags.tag(
 repo,
@@ -519,7 +524,20 @@
 oldnode = lastdiff = None
 if diffs:
 lastdiff = max(diffs, key=lambda d: int(d[b'id']))
-oldnode = getnode(lastdiff)
+oldnodes = getnodes(lastdiff, precset)
+
+# If this commit was the result of `hg fold` after submission,
+# and now resubmitted with --fold, the easiest thing to do is
+# to leave the node clear.  This only results in creating a new
+# diff for the _same_ Differential Revision if this commit is
+# the first or last in the selected range.
+# If this commit is the result of `hg split` in the same
+# scenario, there is a single oldnode here (and multiple
+# newnodes mapped to it).  That makes it the same as the normal
+# case, as the edges of the newnode range cleanly maps to one
+# oldnode each.
+if len(oldnodes) == 1:
+oldnode = oldnodes[0]
 if oldnode and not has_node(oldnode):
 oldnode = None
 
@@ -1667,6 +1685,21 @@
 return _differentialrevisiondescre.sub(uri, ctx.description())
 
 
+def getlocalcommits(diff):
+"""get the set of local commits from a diff object
+
+See ``getdiffmeta()`` for an example diff object.
+"""
+props = diff.get(b'properties') or {}
+commits = props.get(b'local:commits') or {}
+if len(commits) > 1:
+return {bin(c) for c in commits.keys()}
+
+# Storing the diff metadata predates storing `local:commits`, so continue
+# to use that in the --no-fold case.
+return {bin(getdiffmeta(diff).get(b'node', b'')) or None}
+
+
 def getdiffmeta(diff):
 """get commit metadata (date, node, user, p1) from a diff object
 



To: mharbison72, #hg-reviewers, Alphare
Cc: marmoute, Alphare, Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.


  In D8311#124447 , @mharbison72 
wrote:
  
  > In D8311#124387 , @Alphare 
wrote:
  >
  >> Could you add a comment about what happens if you `hg split` either the 
base or the tip of the range?
  >
  > I think what happens is each `newnode` key maps to a single precursor, so 
it's like a regular amend case.  It just happens that multiple `newnode` keys 
have the same `oldnode` in their value tuple.  But all we care about is the 
first and last `newnode` in the range.
  > That said, I tried creating a simple test where I amended in a new file to 
the last commit of the last test, split it with the internal extension, and... 
Somehow I ended up with a weird state where the first half of the split is 
pruned...
  
  Disregard this.  What happened was that I split the commit in a way that 
recreated the content of the pre-amend commit.  Because the timestamp is 
locked, it ended up reusing the existing (obsolete) commit.  I guess there's no 
noise added to `{extras}` when splitting.  With that out of the way, I 
confirmed my original thoughts above.

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8311/new/

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

To: mharbison72, #hg-reviewers, Alphare
Cc: marmoute, Alphare, Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[mharbison72 (Matt Harbison)]

mharbison72 added a comment.
mharbison72 added a subscriber: marmoute.


  In D8311#124387 , @Alphare wrote:
  
  > Could you add a comment about what happens if you `hg split` either the 
base or the tip of the range?
  
  I think what happens is each `newnode` key maps to a single precursor, so 
it's like a regular amend case.  It just happens that multiple `newnode` keys 
have the same `oldnode` in their value tuple.  But all we care about is the 
first and last `newnode` in the range.
  
  That said, I tried creating a simple test where I amended in a new file to 
the last commit of the last test, split it with the internal extension, and... 
Somehow I ended up with a weird state where the first half of the split is 
pruned, making the second half orphaned.  I tried `obslog` on it, and it raises 
an AssertionError because of a cycle.  I assume this is a known problem?  Is 
there some other trivial split that works?  (It's really hard getting the 
interactive sequence right in these *.t files, and I'm basically doing it by 
trial and error.)
  
  CC @marmoute for obsmarker wizardry
  
$ hg obslog . --config extensions.evolve=/d/hg-evolve/hgext3rd/evolve
@  222d8ec1e644 (14) four: extend the fold range
|
** unknown exception encountered, please report by visiting
** https://mercurial-scm.org/wiki/BugTracker
** Python 2.7.15 (v2.7.15:ca079a3ea3, Apr 30 2018, 16:30:26) [MSC v.1500 64 
bit (AMD64)]
** Mercurial Distributed SCM (version 5.3.1+389-9127f584b52b)
** Extensions loaded: phabricator, evolve
Traceback (most recent call last):
  File "d:\mercurial\hg", line 43, in 
dispatch.run()
  File "d:\mercurial\mercurial\dispatch.py", line 111, in run
status = dispatch(req)
  File "d:\mercurial\mercurial\dispatch.py", line 254, in dispatch
ret = _runcatch(req) or 0
  File "d:\mercurial\mercurial\dispatch.py", line 428, in _runcatch
return _callcatch(ui, _runcatchfunc)
  File "d:\mercurial\mercurial\dispatch.py", line 437, in _callcatch
return scmutil.callcatch(ui, func)
  File "d:\mercurial\mercurial\scmutil.py", line 152, in callcatch
return func()
  File "d:\mercurial\mercurial\dispatch.py", line 418, in _runcatchfunc
return _dispatch(req)
  File "d:\mercurial\mercurial\dispatch.py", line 1182, in _dispatch
lui, repo, cmd, fullargs, ui, options, d, cmdpats, cmdoptions
  File "d:\mercurial\mercurial\dispatch.py", line 866, in runcommand
ret = _runcommand(ui, options, cmd, d)
  File "d:\mercurial\mercurial\dispatch.py", line 1193, in _runcommand
return cmdfunc()
  File "d:\mercurial\mercurial\dispatch.py", line 1179, in 
d = lambda: util.checksignature(func)(ui, *args, **strcmdopt)
  File "d:\mercurial\mercurial\util.py", line 1854, in check
return func(*args, **kwargs)
  File "d:/hg-evolve/hgext3rd\evolve\obshistory.py", line 101, in 
cmdobshistory
return _debugobshistorygraph(ui, repo, revs, opts)
  File "d:/hg-evolve/hgext3rd\evolve\obshistory.py", line 432, in 
_debugobshistorygraph
compat.displaygraph(ui, repo, walker, displayer, edges)
  File "d:\mercurial\mercurial\logcmdutil.py", line 1042, in displaygraph
for rev, type, ctx, parents in dag:
  File "d:/hg-evolve/hgext3rd\evolve\obshistory.py", line 330, in 
_obshistorywalker
assert cycle
AssertionError
[1]

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8311/new/

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

To: mharbison72, #hg-reviewers, Alphare
Cc: marmoute, Alphare, Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[Raphaël Gomès]

This revision now requires changes to proceed.
Alphare added a comment.
Alphare requested changes to this revision.


  Could you add a comment about what happens if you `hg split` either the base 
or the tip of the range?

REPOSITORY
  rHG Mercurial

CHANGES SINCE LAST ACTION
  https://phab.mercurial-scm.org/D8311/new/

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

To: mharbison72, #hg-reviewers, Alphare
Cc: Alphare, Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel

D8311: phabricator: teach `getoldnodedrevmap()` to handle folded reviews

[mharbison72 (Matt Harbison)]

mharbison72 created this revision.
Herald added subscribers: mercurial-devel, Kwan.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  The tricky part here is reasoning through all of the possible predecessor
  scenarios.  In the typical case of submitting a folded range and then
  resubmitting it (also folded), filtering the list of commits for the diff 
stored
  on Phabricator through the local predecessor list for each single node will
  result in the typical 1:1 mapping to the old node.
  
  There are edge cases like using `hg fold` within the range prior to
  resubmitting, that will result in mapping to multiple old nodes.  In that 
case,
  the first direct predecessor is needed for the base of the diff, and the last
  direct predecessor is needed for the head of the diff in order to make sure 
that
  the entire range is included in the diff content.  And none of this matters 
for
  commits in the middle of the range, as they are never used.
  
  Fortunately the only crucial thing here is the `drev` number for each node.  
For
  these complicated cases where there are multiple old nodes, simply ignore them
  all.  This will cause `createdifferentialrevision()` to generate a new diff
  (within the same Differential), and avoids complicating the code.

REPOSITORY
  rHG Mercurial

BRANCH
  default

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

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -483,18 +483,23 @@
 alldiffs = callconduit(
 unfi.ui, b'differential.querydiffs', {b'revisionIDs': drevs}
 )
-getnode = lambda d: bin(getdiffmeta(d).get(b'node', b'')) or None
+
+def getnodes(d, precset):
+# Ignore other nodes that were combined into the Differential
+# that aren't predecessors of the current local node.
+return [n for n in getlocalcommits(d) if n in precset]
+
 for newnode, (force, precset, drev) in toconfirm.items():
 diffs = [
 d for d in alldiffs.values() if int(d[b'revisionID']) == drev
 ]
 
-# "precursors" as known by Phabricator
-phprecset = {getnode(d) for d in diffs}
+# local predecessors known by Phabricator
+phprecset = {n for d in diffs for n in getnodes(d, precset)}
 
 # Ignore if precursors (Phabricator and local repo) do not overlap,
 # and force is not set (when commit message says nothing)
-if not force and not bool(phprecset & precset):
+if not force and not phprecset:
 tagname = b'D%d' % drev
 tags.tag(
 repo,
@@ -519,7 +524,15 @@
 oldnode = lastdiff = None
 if diffs:
 lastdiff = max(diffs, key=lambda d: int(d[b'id']))
-oldnode = getnode(lastdiff)
+oldnodes = getnodes(lastdiff, precset)
+
+# If this commit was the result of `hg fold` after submission,
+# and now resubmitted with --fold, the easiest thing to do is
+# to leave the node clear.  This only results in creating a new
+# diff for the _same_ Differential Revision if this commit is
+# the first or last in the selected range.
+if len(oldnodes) == 1:
+oldnode = oldnodes[0]
 if oldnode and not has_node(oldnode):
 oldnode = None
 
@@ -1667,6 +1680,21 @@
 return _differentialrevisiondescre.sub(uri, ctx.description())
 
 
+def getlocalcommits(diff):
+"""get the set of local commits from a diff object
+
+See ``getdiffmeta()`` for an example diff object.
+"""
+props = diff.get(b'properties') or {}
+commits = props.get(b'local:commits') or {}
+if len(commits) > 1:
+return {bin(c) for c in commits.keys()}
+
+# Storing the diff metadata predates storing `local:commits`, so continue
+# to use that in the --no-fold case.
+return {bin(getdiffmeta(diff).get(b'node', b'')) or None}
+
+
 def getdiffmeta(diff):
 """get commit metadata (date, node, user, p1) from a diff object
 



To: mharbison72, #hg-reviewers
Cc: Kwan, mercurial-devel
___
Mercurial-devel mailing list
Mercurial-devel@mercurial-scm.org
https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel