This is an archive of the discontinued Mercurial Phabricator instance.

Differential D121

phabricator: use Phabricator's last node information
ClosedPublic

Authored by quark on Jul 18 2017, 12:20 AM.

Details

Reviewers
mitrandir
Group Reviewers
hg-reviewers
Commits
rHG1664406a44d9: phabricator: use Phabricator's last node information
Summary

This makes it more strict when checking whether or not we should update a
Differential Revision. For example,

a) Alice updates D1 to content 1.
b) Bob updates D1 to content 2.
c) Alice tries to update D1 to content 1.

Previously, c) will do nothing because phabsend detects the patch is not
changed. A more correct behavior is to override Bob's update here, hence the
patch.

This also makes it possible to return a reaonsable "last node" when there is
no tags but only Differential Revision commit messages.

Test Plan
for i in A B C; do echo $i > $i; hg ci -m $i -A $i; done

hg phabsend 0::
# D40: created
# D41: created
# D42: created

echo 3 >> C; hg amend; hg phabsend .
# D42: updated

hg tag --local --hidden -r 2 -f D42
# move tag to the previous version

hg phabsend .
# D42: skipped (previously it would be "updated")

rm -rf .hg; hg init
hg phabread --stack D42 | hg import -
hg phabsend .
# D42: updated
hg tag --local --remove D42
hg commit --amend
hg phabsend .
# D42: updated (no new diff uploaded, previously it will upload a new diff)

The old diff object is now returned, which could be useful in the next
patch.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 18 2017, 12:20 AM
quark edited the test plan for this revision. (Show Details)Jul 18 2017, 12:22 AM
mitrandir accepted this revision.Jul 18 2017, 10:27 AM
quark abandoned this revision.Jul 18 2017, 5:37 PM
quark reclaimed this revision.Aug 1 2017, 4:39 PM
quark updated this revision to Diff 484.
quark edited the test plan for this revision. (Show Details)Aug 4 2017, 3:54 PM
quark added a child revision: D229: phabricator: update diff property even if we choose not to create a new diff.
quark updated this revision to Diff 550.
Closed by commit rHG1664406a44d9: phabricator: use Phabricator's last node information (authored by quark). · Explain WhyAug 4 2017, 6:00 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/phabricator.py (79 lines)

Diff 550

contrib/phabricator.py

if len(query[r'data']) == 0: if len(query[r'data']) == 0:
return None return None
repophid = encoding.strtolocal(query[r'data'][0][r'phid']) repophid = encoding.strtolocal(query[r'data'][0][r'phid'])
repo.ui.setconfig('phabricator', 'repophid', repophid) repo.ui.setconfig('phabricator', 'repophid', repophid)
return repophid return repophid
_differentialrevisiontagre = re.compile('\AD([1-9][0-9]*)\Z') _differentialrevisiontagre = re.compile('\AD([1-9][0-9]*)\Z')
_differentialrevisiondescre = re.compile( _differentialrevisiondescre = re.compile(
'^Differential Revision:\s*(.*)D([1-9][0-9]*)$', re.M) '^Differential Revision:\s*(?:.*)D([1-9][0-9]*)$', re.M)
def getoldnodedrevmap(repo, nodelist): def getoldnodedrevmap(repo, nodelist):
"""find previous nodes that has been sent to Phabricator """find previous nodes that has been sent to Phabricator
return {node: (oldnode or None, Differential Revision ID)} return {node: (oldnode, Differential diff, Differential Revision ID)}
for node in nodelist with known previous sent versions, or associated for node in nodelist with known previous sent versions, or associated
Differential Revision IDs. Differential Revision IDs. ``oldnode`` and ``Differential diff`` could
be ``None``.
Examines all precursors and their tags. Tags with format like "D1234" are Examines commit messages like "Differential Revision:" to get the
considered a match and the node with that tag, and the number after "D" association information.
(ex. 1234) will be returned.
If tags are not found, examine commit message. The "Differential Revision:" If such commit message line is not found, examines all precursors and their
line could associate this changeset to a Differential Revision. tags. Tags with format like "D1234" are considered a match and the node
with that tag, and the number after "D" (ex. 1234) will be returned.
The ``old node``, if not None, is guaranteed to be the last diff of
corresponding Differential Revision, and exist in the repo.
""" """
url, token = readurltoken(repo) url, token = readurltoken(repo)
unfi = repo.unfiltered() unfi = repo.unfiltered()
nodemap = unfi.changelog.nodemap nodemap = unfi.changelog.nodemap
result = {} # {node: (oldnode or None, drev)} result = {} # {node: (oldnode?, lastdiff?, drev)}
toconfirm = {} # {node: (oldnode, {precnode}, drev)} toconfirm = {} # {node: (force, {precnode}, drev)}
for node in nodelist: for node in nodelist:
ctx = unfi[node] ctx = unfi[node]
# For tags like "D123", put them into "toconfirm" to verify later # For tags like "D123", put them into "toconfirm" to verify later
precnodes = list(obsolete.allprecursors(unfi.obsstore, [node])) precnodes = list(obsolete.allprecursors(unfi.obsstore, [node]))
for n in precnodes: for n in precnodes:
if n in nodemap: if n in nodemap:
for tag in unfi.nodetags(n): for tag in unfi.nodetags(n):
m = _differentialrevisiontagre.match(tag) m = _differentialrevisiontagre.match(tag)
if m: if m:
toconfirm[node] = (n, set(precnodes), int(m.group(1))) toconfirm[node] = (0, set(precnodes), int(m.group(1)))
continue continue
# Check commit message (make sure URL matches) # Check commit message
m = _differentialrevisiondescre.search(ctx.description()) m = _differentialrevisiondescre.search(ctx.description())
if m: if m:
if m.group(1).rstrip('/') == url.rstrip('/'): toconfirm[node] = (1, set(precnodes), int(m.group(1)))
result[node] = (None, int(m.group(2)))
else:
unfi.ui.warn(_('%s: Differential Revision URL ignored - host '
'does not match config\n') % ctx)
# 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:
confirmed = {} # {drev: {oldnode}} drevs = [drev for force, precs, drev in toconfirm.values()]
drevs = [drev for n, precs, drev in toconfirm.values()] alldiffs = callconduit(unfi, 'differential.querydiffs',
diffs = callconduit(unfi, 'differential.querydiffs',
{'revisionIDs': drevs}) {'revisionIDs': drevs})
for diff in diffs.values(): getnode = lambda d: bin(encoding.unitolocal(
drev = int(diff[r'revisionID']) getdiffmeta(d).get(r'node', ''))) or None
oldnode = bin(encoding.unitolocal(getdiffmeta(diff).get(r'node'))) for newnode, (force, precset, drev) in toconfirm.items():
if node: diffs = [d for d in alldiffs.values()
confirmed.setdefault(drev, set()).add(oldnode) if int(d[r'revisionID']) == drev]
for newnode, (oldnode, precset, drev) in toconfirm.items():
if bool(precset & confirmed.get(drev, set())): # "precursors" as known by Phabricator
result[newnode] = (oldnode, drev) phprecset = set(getnode(d) for d in diffs)
else:
# 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):
tagname = 'D%d' % drev tagname = 'D%d' % drev
tags.tag(repo, tagname, nullid, message=None, user=None, tags.tag(repo, tagname, nullid, message=None, user=None,
date=None, local=True) date=None, local=True)
unfi.ui.warn(_('D%s: local tag removed - does not match ' unfi.ui.warn(_('D%s: local tag removed - does not match '
'Differential history\n') % drev) 'Differential history\n') % drev)
continue
# Find the last node using Phabricator metadata, and make sure it
# exists in the repo
oldnode = lastdiff = None
if diffs:
lastdiff = max(diffs, key=lambda d: int(d[r'id']))
oldnode = getnode(lastdiff)
if oldnode and oldnode not in nodemap:
oldnode = None
result[newnode] = (oldnode, lastdiff, drev)
return result return result
def getdiff(ctx, diffopts): def getdiff(ctx, diffopts):
"""plain-text diff without header (user, commit message, etc)""" """plain-text diff without header (user, commit message, etc)"""
output = util.stringio() output = util.stringio()
for chunk, _label in patch.diffui(ctx.repo(), ctx.p1().node(), ctx.node(), for chunk, _label in patch.diffui(ctx.repo(), ctx.p1().node(), ctx.node(),
None, opts=diffopts): None, opts=diffopts):
raise error.Abort(_('phabsend requires at least one changeset')) raise error.Abort(_('phabsend requires at least one changeset'))
actions = [] actions = []
reviewers = opts.get('reviewer', []) reviewers = opts.get('reviewer', [])
if reviewers: if reviewers:
phids = userphids(repo, reviewers) phids = userphids(repo, reviewers)
actions.append({'type': 'reviewers.add', 'value': phids}) actions.append({'type': 'reviewers.add', 'value': phids})
oldnodedrev = getoldnodedrevmap(repo, [repo[r].node() for r in revs]) # {newnode: (oldnode, olddiff, olddrev}
oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
# Send patches one by one so we know their Differential Revision IDs and # Send patches one by one so we know their Differential Revision IDs and
# can provide dependency relationship # can provide dependency relationship
lastrevid = None lastrevid = None
for rev in revs: for rev in revs:
ui.debug('sending rev %d\n' % rev) ui.debug('sending rev %d\n' % rev)
ctx = repo[rev] ctx = repo[rev]
# Get Differential Revision ID # Get Differential Revision ID
oldnode, revid = oldnodedrev.get(ctx.node(), (None, None)) oldnode, olddiff, revid = oldmap.get(ctx.node(), (None, None, None))
if oldnode != ctx.node(): if oldnode != ctx.node():
# Create or update Differential Revision # Create or update Differential Revision
revision = createdifferentialrevision(ctx, revid, lastrevid, revision = createdifferentialrevision(ctx, revid, lastrevid,
oldnode, actions) oldnode, actions)
newrevid = int(revision[r'object'][r'id']) newrevid = int(revision[r'object'][r'id'])
if revid: if revid:
action = _('updated') action = _('updated')
else: else: