This is an archive of the discontinued Mercurial Phabricator instance.

Differential D38

phabricator: allow specifying reviewers on phabsend
ClosedPublic

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

Details

Reviewers
durin42
phillco
Group Reviewers
hg-reviewers
Commits
rHGb7a75b9a3386: phabricator: allow specifying reviewers on phabsend
Summary

Sometimes people want to specify reviewer explicitly for a stack. The
webpage only allows changing reviewer for one revision at a time. This patch
adds a --reviewer flag to make it easier to specify reviewers.

Test Plan

On a test Phabricator instance, enable differential.allow-self-accept,
assign myself as a reviewer and make sure it works. Also try an invalid
username and make sure it raises.

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
phillco accepted this revision.Jul 13 2017, 3:58 PM
phillco added a subscriber: phillco.

lgtm

This revision is now accepted and ready to land.Jul 13 2017, 3:58 PM
quark edited edge metadata.Jul 14 2017, 4:35 PM
quark added a reviewer: hg-reviewers.
quark requested review of this revision.
durin42 accepted this revision.Jul 14 2017, 5:03 PM
This revision is now accepted and ready to land.Jul 14 2017, 5:03 PM
Closed by commit rHGb7a75b9a3386: phabricator: allow specifying reviewers on phabsend (authored by quark). · Explain WhyJul 17 2017, 2:30 PM
This revision was automatically updated to reflect the committed changes.

Revision Contents
Changeset List

PathPackages
Mcontrib/phabricator.py (33 lines)

Diff 207

contrib/phabricator.py

'user': ctx.user(), 'user': ctx.user(),
'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):
"""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.
""" """
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)
transactions.append({'type': 'update', 'value': diff[r'phid']}) transactions.append({'type': 'update', 'value': diff[r'phid']})
# 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},
{'type': 'summary', 'value': ' '}] {'type': 'summary', 'value': ' '}]
if actions:
transactions += actions
# Parse commit message and update related fields. # Parse commit message and update related fields.
desc = ctx.description() desc = ctx.description()
info = callconduit(repo, 'differential.parsecommitmessage', info = callconduit(repo, 'differential.parsecommitmessage',
{'corpus': desc}) {'corpus': desc})
for k, v in info[r'fields'].items(): for k, v in info[r'fields'].items():
if k in ['title', 'summary', 'testPlan']: if k in ['title', 'summary', 'testPlan']:
transactions.append({'type': k, 'value': v}) transactions.append({'type': k, 'value': v})
params = {'transactions': transactions} params = {'transactions': transactions}
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
def userphids(repo, names):
"""convert user names to PHIDs"""
query = {'constraints': {'usernames': names}}
result = callconduit(repo, 'user.search', query)
# username not found is not an error of the API. So check if we have missed
# some names here.
data = result[r'data']
resolved = set(entry[r'fields'][r'username'] for entry in data)
unresolved = set(names) - resolved
if unresolved:
raise error.Abort(_('unknown username: %s')
% ' '.join(sorted(unresolved)))
return [entry[r'phid'] for entry in data]
@command('phabsend', @command('phabsend',
[('r', 'rev', [], _('revisions to send'), _('REV'))], [('r', 'rev', [], _('revisions to send'), _('REV')),
('', '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 For the first time uploading changesets, local tags will be created to
maintain the association. After the first time, phabsend will check maintain the association. After the first time, phabsend will check
obsstore and tags information so it can figure out whether to update an obsstore and tags information so it can figure out whether to update an
existing Differential Revision, or create a new one. 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'))
actions = []
reviewers = opts.get('reviewer', [])
if reviewers:
phids = userphids(repo, reviewers)
actions.append({'type': 'reviewers.add', 'value': phids})
oldnodedrev = getoldnodedrevmap(repo, [repo[r].node() for r in revs]) oldnodedrev = 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, revid = oldnodedrev.get(ctx.node(), (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) 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:
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