( )⚙ D122 phabricator: add --amend option to phabsend

This is an archive of the discontinued Mercurial Phabricator instance.

Differential D122

phabricator: add --amend option to phabsend
ClosedPublic

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

Details

Reviewers
mitrandir
Group Reviewers
hg-reviewers
Commits
rHGfa3aa6c98bb7: phabricator: add --amend option to phabsend
Summary

Previously hg phabsend was imitating hg email and won't mutate
changesets. That works fine with reviewer-push workflow, reviewers run
phabread, import.

However, it does not work well with author-push workflow. Namely, the author
needs to run extra commands to get the right commit message, and remove the
local tag after push.

This patch solves those issues by adding the --amend option, so local
changesets will have the right commit message, and tags become unnecessary.

Test Plan

Given the following DAG:

o  17
o  16
| o  15
| @  14
|/
o  13
o  12

Run hg phabsend '(13::)-17' --amend, check the new DAG looks like:

o  21
| o  20
| @  19
|/
o  18
| o  17
| x  16
| x  13
|/
o  12

And commit messages are updated to contain the Differential Revision lines.
Use phabread to make sure Phabricator has the amended node recorded.

Also check phabsend . followed by a phabsend . --amend, the commit
message will be updated and the tag will be removed.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped

Event Timeline

quark created this revision.Jul 18 2017, 12:20 AM
mitrandir accepted this revision.Jul 18 2017, 10:31 AM
mitrandir added inline comments.
contrib/phabricator.py
426

I think it would be better to use differential.getcommitmessage for this. For example "reviewed by" field is a really nice thing to have in the commit message.

quark added inline comments.Jul 18 2017, 12:40 PM
contrib/phabricator.py
426

I was trying to make it behave closer to hg email. IIRC reviewers have some other plans to show "who accepts" information (that works for both Phabricator and email submits) on hgweb, so I didn't pay too much attention on "Reviewed By" here.

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 485.
quark edited the test plan for this revision. (Show Details)Aug 4 2017, 3:55 PM
quark updated this revision to Diff 553.
quark updated this revision to Diff 568.Aug 4 2017, 6:13 PM
quark updated this revision to Diff 789.Aug 11 2017, 2:37 PM
quark updated this revision to Diff 838.Aug 13 2017, 12:41 AM
Closed by commit rHGfa3aa6c98bb7: phabricator: add --amend option to phabsend (authored by quark). · Explain WhyAug 14 2017, 2:20 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/phabricator.py (72 lines)

Diff 553

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.node import bin, nullid
from mercurial.i18n import _ from mercurial.i18n import _
from mercurial import ( from mercurial import (
cmdutil,
context,
encoding, encoding,
error, error,
mdiff, mdiff,
obsolete, obsolete,
patch, patch,
registrar, registrar,
scmutil, scmutil,
tags, tags,
if revid is not None: if revid is not None:
# Update an existing Differential Revision # Update an existing Differential Revision
params['objectIdentifier'] = revid params['objectIdentifier'] = revid
revision = callconduit(repo, 'differential.revision.edit', params) revision = callconduit(repo, 'differential.revision.edit', params)
if not revision: if not revision:
raise error.Abort(_('cannot create revision for %s') % ctx) raise error.Abort(_('cannot create revision for %s') % ctx)
return revision return revision, diff
def userphids(repo, names): def userphids(repo, names):
"""convert user names to PHIDs""" """convert user names to PHIDs"""
query = {'constraints': {'usernames': names}} query = {'constraints': {'usernames': names}}
result = callconduit(repo, 'user.search', query) result = callconduit(repo, 'user.search', query)
# username not found is not an error of the API. So check if we have missed # username not found is not an error of the API. So check if we have missed
# some names here. # some names here.
data = result[r'data'] data = result[r'data']
resolved = set(entry[r'fields'][r'username'] for entry in data) resolved = set(entry[r'fields'][r'username'] for entry in data)
unresolved = set(names) - resolved unresolved = set(names) - resolved
if unresolved: if unresolved:
raise error.Abort(_('unknown username: %s') raise error.Abort(_('unknown username: %s')
% ' '.join(sorted(unresolved))) % ' '.join(sorted(unresolved)))
return [entry[r'phid'] for entry in data] return [entry[r'phid'] for entry in data]
@command('phabsend', @command('phabsend',
[('r', 'rev', [], _('revisions to send'), _('REV')), [('r', 'rev', [], _('revisions to send'), _('REV')),
('', 'amend', False, _('update commit messages')),
('', 'reviewer', [], _('specify reviewers'))], ('', 'reviewer', [], _('specify reviewers'))],
_('REV [OPTIONS]')) _('REV [OPTIONS]'))
def phabsend(ui, repo, *revs, **opts): def phabsend(ui, repo, *revs, **opts):
"""upload changesets to Phabricator """upload changesets to Phabricator
If there are multiple revisions specified, they will be send as a stack If there are multiple revisions specified, they will be send as a stack
with a linear dependencies relationship using the order specified by the with a linear dependencies relationship using the order specified by the
revset. revset.
For the first time uploading changesets, local tags will be created to If --amend is set, update commit messages so they have the
maintain the association. After the first time, phabsend will check ``Differential Revision`` URL, remove related tags. This is similar to what
obsstore and tags information so it can figure out whether to update an arcanist will do, and is more desired in author-push workflows. Otherwise,
existing Differential Revision, or create a new one. use local tags to record the ``Differential Revision`` association.
phabsend will check obsstore and the above association to decide whether to
update an existing Differential Revision, or create a new one.
""" """
revs = list(revs) + opts.get('rev', []) revs = list(revs) + opts.get('rev', [])
revs = scmutil.revrange(repo, revs) revs = scmutil.revrange(repo, revs)
if not revs: if not revs:
raise error.Abort(_('phabsend requires at least one changeset')) raise error.Abort(_('phabsend requires at least one changeset'))
if opts.get('amend'):
cmdutil.checkunfinished(repo)
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})
# {newnode: (oldnode, olddiff, olddrev} # {newnode: (oldnode, olddiff, olddrev}
oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs]) oldmap = getoldnodedrevmap(repo, [repo[r].node() for r in revs])
drevids = [] # [int]
diffmap = {} # {newnode: diff}
# 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, 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, diff = createdifferentialrevision(
oldnode, olddiff, actions) ctx, revid, lastrevid, oldnode, olddiff, actions)
diffmap[ctx.node()] = diff
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, if commit message
# does not have it already
m = _differentialrevisiondescre.search(ctx.description())
if not m or int(m.group(1)) != newrevid:
tagname = 'D%d' % newrevid tagname = 'D%d' % newrevid
tags.tag(repo, tagname, ctx.node(), message=None, user=None, tags.tag(repo, tagname, ctx.node(), message=None, user=None,
date=None, local=True) date=None, local=True)
else: else:
# Nothing changed. But still set "newrevid" so the next revision # Nothing changed. But still set "newrevid" so the next revision
# could depend on this one. # could depend on this one.
newrevid = revid newrevid = revid
action = _('skipped') action = _('skipped')
ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx, ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
ctx.description().split('\n')[0])) ctx.description().split('\n')[0]))
drevids.append(newrevid)
lastrevid = newrevid lastrevid = newrevid
# Update commit messages and remove tags
if opts.get('amend'):
unfi = repo.unfiltered()
drevs = callconduit(repo, 'differential.query', {'ids': drevids})
with repo.wlock(), repo.lock(), repo.transaction('phabsend'):
wnode = unfi['.'].node()
mapping = {} # {oldnode: [newnode]}
for i, rev in enumerate(revs):
old = unfi[rev]
drevid = drevids[i]
drev = [d for d in drevs if int(d[r'id']) == drevid][0]
newdesc = getdescfromdrev(drev)
mitrandirUnsubmitted
Not Done

I think it would be better to use differential.getcommitmessage for this. For example "reviewed by" field is a really nice thing to have in the commit message.

mitrandir: I think it would be better to use differential.getcommitmessage for this. For example "reviewed…
quarkAuthorUnsubmitted
Not Done

I was trying to make it behave closer to hg email. IIRC reviewers have some other plans to show "who accepts" information (that works for both Phabricator and email submits) on hgweb, so I didn't pay too much attention on "Reviewed By" here.

quark: I was trying to make it behave closer to `hg email`. IIRC reviewers have some other plans to…
# Make sure commit message contain "Differential Revision"
if old.description() != newdesc:
parents = [
mapping.get(old.p1().node(), (old.p1(),))[0],
mapping.get(old.p2().node(), (old.p2(),))[0],
]
new = context.metadataonlyctx(
repo, old, parents=parents, text=newdesc,
user=old.user(), date=old.date(), extra=old.extra())
newnode = new.commit()
mapping[old.node()] = [newnode]
# Update diff property
writediffproperties(unfi[newnode], diffmap[old.node()])
# Remove local tags since it's no longer necessary
tagname = 'D%d' % drevid
if tagname in repo.tags():
tags.tag(repo, tagname, nullid, message=None, user=None,
date=None, local=True)
scmutil.cleanupnodes(repo, mapping, 'phabsend')
if wnode in mapping:
unfi.setparents(mapping[wnode][0])
# Map from "hg:meta" keys to header understood by "hg import". The order is # Map from "hg:meta" keys to header understood by "hg import". The order is
# consistent with "hg export" output. # consistent with "hg export" output.
_metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'), _metanamemap = util.sortdict([(r'user', 'User'), (r'date', 'Date'),
(r'node', 'Node ID'), (r'parent', 'Parent ')]) (r'node', 'Node ID'), (r'parent', 'Parent ')])
def querydrev(repo, params, stack=False): def querydrev(repo, params, stack=False):
"""return a list of "Differential Revision" dicts """return a list of "Differential Revision" dicts