( )⚙ D36 phabricator: verify local tags before trusting them

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D36

phabricator: verify local tags before trusting them
ClosedPublic

Authored by quark on Jul 11 2017, 11:56 AM.

Details

Reviewers
durin42
Commits
rHGe48082e0a8d5: phabricator: verify local tags before trusting them
Summary

Previously we trust local tags blindly and that could cause wrong
Differential Revision to be updated, when people switch between Phabricator
instances.

This patch adds verification logic to detect such issue and remove
problematic tags. For example, a tag "D19" was on node "X", the code will
fetch all diffs attached to D19, and check if nodes server-side overlaps
with nodes in precursors. If they do not overlap, create a new Differential
Revision.

Test Plan

Use a test Phabricator instance, send patches using hg phabsend, then
change the local tag manually to a wrong Differential Revision number.
Amend the patch and send again. Make sure the tag gets ignored and deleted.

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

quark created this revision.Jul 11 2017, 11:56 AM
durin42 accepted this revision.Jul 14 2017, 12:00 PM
This revision is now accepted and ready to land.Jul 14 2017, 12:00 PM
Closed by commit rHGe48082e0a8d5: phabricator: verify local tags before trusting them (authored by quark). · Explain WhyJul 14 2017, 1:25 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/phabricator.py (31 lines)

Diff 141

contrib/phabricator.py

""" """
from __future__ import absolute_import from __future__ import absolute_import
import json import json
import re import re
from mercurial.node import bin, nullid
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
encoding, encoding,
error, error,
mdiff, mdiff,
obsolete, obsolete,
patch, patch,
registrar, registrar,
If tags are not found, examine commit message. The "Differential Revision:" If tags are not found, examine commit message. The "Differential Revision:"
line could associate this changeset to a Differential Revision. line could associate this changeset to a Differential Revision.
""" """
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 or None, drev)}
toconfirm = {} # {node: (oldnode, {precnode}, drev)}
for node in nodelist: for node in nodelist:
ctx = unfi[node] ctx = unfi[node]
# Check tags like "D123" # For tags like "D123", put them into "toconfirm" to verify later
for n in obsolete.allprecursors(unfi.obsstore, [node]): precnodes = list(obsolete.allprecursors(unfi.obsstore, [node]))
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:
result[node] = (n, int(m.group(1))) toconfirm[node] = (n, set(precnodes), int(m.group(1)))
continue continue
# Check commit message # Check commit message
m = _differentialrevisiondescre.search(ctx.description()) m = _differentialrevisiondescre.search(ctx.description())
if m: if m:
result[node] = (None, int(m.group(1))) result[node] = (None, int(m.group(1)))
# Double check if tags are genuine by collecting all old nodes from
# Phabricator, and expect precursors overlap with it.
if toconfirm:
confirmed = {} # {drev: {oldnode}}
drevs = [drev for n, precs, drev in toconfirm.values()]
diffs = callconduit(unfi, 'differential.querydiffs',
{'revisionIDs': drevs})
for diff in diffs.values():
drev = int(diff[r'revisionID'])
oldnode = bin(encoding.unitolocal(getdiffmeta(diff).get(r'node')))
if node:
confirmed.setdefault(drev, set()).add(oldnode)
for newnode, (oldnode, precset, drev) in toconfirm.items():
if bool(precset & confirmed.get(drev, set())):
result[newnode] = (oldnode, drev)
else:
tagname = 'D%d' % drev
tags.tag(repo, tagname, nullid, message=None, user=None,
date=None, local=True)
unfi.ui.warn(_('D%s: local tag removed - does not match '
'Differential history\n') % 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):
output.write(chunk) output.write(chunk)