This is an archive of the discontinued Mercurial Phabricator instance.

Differential D8311

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

Authored by mharbison72 on Mar 20 2020, 5:24 PM.

Details

Reviewers
Alphare
Group Reviewers
hg-reviewers
Commits
rHG5f9c917e3b50: phabricator: teach `getoldnodedrevmap()` to handle folded reviews
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.

Diff Detail

Repository
rHG Mercurial
Lint
Automatic diff as part of commit; lint not applicable.
Unit
Automatic diff as part of commit; unit tests not applicable.

Event Timeline

mharbison72 created this revision.Mar 20 2020, 5:24 PM
Alphare requested changes to this revision.Mar 25 2020, 2:46 PM
Alphare added a subscriber: Alphare.

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

This revision now requires changes to proceed.Mar 25 2020, 2:46 PM
mharbison72 added a subscriber: marmoute.Mar 25 2020, 11:42 PM
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 <module>
    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 <lambda>
    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]
mharbison72 added a comment.Mar 29 2020, 12:53 AM
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.

Alphare accepted this revision.Mar 31 2020, 4:33 AM
mharbison72 added a commit: rHG5f9c917e3b50: phabricator: teach `getoldnodedrevmap()` to handle folded reviews.Apr 1 2020, 11:23 AM
This revision was not accepted when it landed; it landed in state Needs Review.
Closed by commit rHG5f9c917e3b50: phabricator: teach `getoldnodedrevmap()` to handle folded reviews (authored by mharbison72). · Explain Why
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (43 lines)

Diff 20945

hgext/phabricator.py

# Double check if tags are genuine by collecting all old nodes from # Double check if tags are genuine by collecting all old nodes from
# Phabricator, and expect precursors overlap with it. # Phabricator, and expect precursors overlap with it.
if toconfirm: if toconfirm:
drevs = [drev for force, precs, drev in toconfirm.values()] drevs = [drev for force, precs, drev in toconfirm.values()]
alldiffs = callconduit( alldiffs = callconduit(
unfi.ui, b'differential.querydiffs', {b'revisionIDs': drevs} 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(): for newnode, (force, precset, drev) in toconfirm.items():
diffs = [ diffs = [
d for d in alldiffs.values() if int(d[b'revisionID']) == drev d for d in alldiffs.values() if int(d[b'revisionID']) == drev
] ]
# "precursors" as known by Phabricator # local predecessors known by Phabricator
phprecset = {getnode(d) for d in diffs} phprecset = {n for d in diffs for n in getnodes(d, precset)}
# Ignore if precursors (Phabricator and local repo) do not overlap, # Ignore if precursors (Phabricator and local repo) do not overlap,
# and force is not set (when commit message says nothing) # 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 tagname = b'D%d' % drev
tags.tag( tags.tag(
repo, repo,
tagname, tagname,
nullid, nullid,
message=None, message=None,
user=None, user=None,
date=None, date=None,
local=True, local=True,
) )
unfi.ui.warn( unfi.ui.warn(
_( _(
b'D%d: local tag removed - does not match ' b'D%d: local tag removed - does not match '
b'Differential history\n' b'Differential history\n'
) )
% drev % drev
) )
continue continue
# Find the last node using Phabricator metadata, and make sure it # Find the last node using Phabricator metadata, and make sure it
# exists in the repo # exists in the repo
oldnode = lastdiff = None oldnode = lastdiff = None
if diffs: if diffs:
lastdiff = max(diffs, key=lambda d: int(d[b'id'])) 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): if oldnode and not has_node(oldnode):
oldnode = None oldnode = None
result[newnode] = (oldnode, lastdiff, drev) result[newnode] = (oldnode, lastdiff, drev)
return result return result
# in Phabricator. # in Phabricator.
m = _differentialrevisiondescre.search(ctx.description()) m = _differentialrevisiondescre.search(ctx.description())
if not m: if not m:
return b'\n\n'.join([ctx.description(), uri]) return b'\n\n'.join([ctx.description(), uri])
return _differentialrevisiondescre.sub(uri, ctx.description()) 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): def getdiffmeta(diff):
"""get commit metadata (date, node, user, p1) from a diff object """get commit metadata (date, node, user, p1) from a diff object
The metadata could be "hg:meta", sent by phabsend, like: The metadata could be "hg:meta", sent by phabsend, like:
"properties": { "properties": {
"hg:meta": { "hg:meta": {
"branch": "default", "branch": "default",