This is an archive of the discontinued Mercurial Phabricator instance.

Differential D229

phabricator: update diff property even if we choose not to create a new diff
ClosedPublic

Authored by quark on Aug 4 2017, 3:55 PM.

Details

Reviewers
durin42
Group Reviewers
hg-reviewers
Commits
rHGf100354cce52: phabricator: update diff property even if we choose not to create a new diff
Summary

The diff property contains metadata like "HG Node". Previously we skip
uploading a new diff if we are sure that the old patch and new patch have a
same content. That has issues when a pusher adds an obsmarker using the old
"HG Node" stored in the old diff.

This patch adds logic to update the diff property so "HG Node" gets updated
to prevent that issue.

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.Aug 4 2017, 3:55 PM
durin42 accepted this revision.Aug 4 2017, 5:37 PM
This revision is now accepted and ready to land.Aug 4 2017, 5:37 PM
Closed by commit rHGf100354cce52: phabricator: update diff property even if we choose not to create a new diff (authored by quark). · Explain WhyAug 4 2017, 6:01 PM
This revision was automatically updated to reflect the committed changes.
yuja added a subscriber: yuja.Aug 12 2017, 11:28 PM
yuja added inline comments.
contrib/phabricator.py
280

writediffproperties() is called twice. Perhaps one of them should be deleted.

Revision Contents
Changeset List

Diff 566

contrib/phabricator.py

'date': '%d %d' % ctx.date(), 'date': '%d %d' % ctx.date(),
'node': ctx.hex(), 'node': ctx.hex(),
'parent': ctx.p1().hex(), 'parent': ctx.p1().hex(),
}), }),
} }
callconduit(ctx.repo(), 'differential.setdiffproperty', params) callconduit(ctx.repo(), 'differential.setdiffproperty', params)
def createdifferentialrevision(ctx, revid=None, parentrevid=None, oldnode=None, def createdifferentialrevision(ctx, revid=None, parentrevid=None, oldnode=None,
actions=None): olddiff=None, actions=None):
"""create or update a Differential Revision """create or update a Differential Revision
If revid is None, create a new Differential Revision, otherwise update If revid is None, create a new Differential Revision, otherwise update
revid. If parentrevid is not None, set it as a dependency. revid. If parentrevid is not None, set it as a dependency.
If oldnode is not None, check if the patch content (without commit message If oldnode is not None, check if the patch content (without commit message
and metadata) has changed before creating another diff. and metadata) has changed before creating another diff.
If actions is not None, they will be appended to the transaction. If actions is not None, they will be appended to the transaction.
""" """
repo = ctx.repo() repo = ctx.repo()
if oldnode: if oldnode:
diffopts = mdiff.diffopts(git=True, context=1) diffopts = mdiff.diffopts(git=True, context=1)
oldctx = repo.unfiltered()[oldnode] oldctx = repo.unfiltered()[oldnode]
neednewdiff = (getdiff(ctx, diffopts) != getdiff(oldctx, diffopts)) neednewdiff = (getdiff(ctx, diffopts) != getdiff(oldctx, diffopts))
else: else:
neednewdiff = True neednewdiff = True
transactions = [] transactions = []
if neednewdiff: if neednewdiff:
diff = creatediff(ctx) diff = creatediff(ctx)
writediffproperties(ctx, diff) writediffproperties(ctx, diff)
yujaUnsubmitted
Not Done

writediffproperties() is called twice. Perhaps one of them should be deleted.

yuja: `writediffproperties()` is called twice. Perhaps one of them should be deleted.
transactions.append({'type': 'update', 'value': diff[r'phid']}) transactions.append({'type': 'update', 'value': diff[r'phid']})
else:
# Even if we don't need to upload a new diff because the patch content
# does not change. We might still need to update its metadata so
# pushers could know the correct node metadata.
assert olddiff
diff = olddiff
writediffproperties(ctx, diff)
# Use a temporary summary to set dependency. There might be better ways but # Use a temporary summary to set dependency. There might be better ways but
# I cannot find them for now. But do not do that if we are updating an # I cannot find them for now. But do not do that if we are updating an
# existing revision (revid is not None) since that introduces visible # existing revision (revid is not None) since that introduces visible
# churns (someone edited "Summary" twice) on the web page. # churns (someone edited "Summary" twice) on the web page.
if parentrevid and revid is None: if parentrevid and revid is None:
summary = 'Depends on D%s' % parentrevid summary = 'Depends on D%s' % parentrevid
transactions += [{'type': 'summary', 'value': summary}, transactions += [{'type': 'summary', 'value': summary},
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, olddiff, revid = oldmap.get(ctx.node(), (None, 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, olddiff, 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:
action = _('created') action = _('created')
# Create a local tag to note the association # Create a local tag to note the association
tagname = 'D%d' % newrevid tagname = 'D%d' % newrevid