( )⚙ D7513 phabricator: fix processing of tags/desc in getoldnodedrevmap()

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D7513

phabricator: fix processing of tags/desc in getoldnodedrevmap()
ClosedPublic

Authored by dlax on Nov 23 2019, 5:47 AM.

Details

Reviewers
pulkit
Group Reviewers
hg-reviewers
Commits
rHG16b607e9f714: phabricator: fix processing of tags/desc in getoldnodedrevmap()
Summary

It seems that the previous logic was wrong (it essentially comes
from changeset 3ab0d5767b54 where the result got accumulated instead of
early returned).
First of all, the "continue" in first "if m:" is useless because we're
at the end of the loop. Then, the algorithm seems weird because we will
process all predecessors of a node and possibly override
toconfirm[node] for each of these having a tag (maybe this doesn't
happen, but still). Finally, we would also override toconfirm[node]
when the "Differential Revision: " is found in changeset description.
Maybe this is not a big deal when there is no mix of local tag and
changeset description update?

The logic is changed so that the loop on predecessors stops upon first
match of a tag and so that the changeset description is only checked if
no tag was found. Therefore, toconfirm[node] is only set once.

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

dlax created this revision.Nov 23 2019, 5:47 AM
pulkit accepted this revision.Dec 10 2019, 10:43 AM
This revision is now accepted and ready to land.Dec 10 2019, 10:43 AM

Revision Contents
Changeset List

PathPackages
Mhgext/phabricator.py (15 lines)

Diff 18580

hgext/phabricator.py

# For tags like "D123", put them into "toconfirm" to verify later # For tags like "D123", put them into "toconfirm" to verify later
precnodes = list(obsutil.allpredecessors(unfi.obsstore, [node])) precnodes = list(obsutil.allpredecessors(unfi.obsstore, [node]))
for n in precnodes: for n in precnodes:
if has_node(n): if has_node(n):
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] = (0, set(precnodes), int(m.group(1))) toconfirm[node] = (0, set(precnodes), int(m.group(1)))
continue break
else:
continue # move to next predecessor
break # found a tag, stop
else:
# Check commit message # Check commit message
m = _differentialrevisiondescre.search(ctx.description()) m = _differentialrevisiondescre.search(ctx.description())
if m: if m:
toconfirm[node] = (1, set(precnodes), int(m.group('id'))) toconfirm[node] = (1, set(precnodes), int(m.group('id')))
# 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}
) )