Patchwork [2,of,3] phabricator: add phabsend command to send a stack

login
register
mail settings
Submitter Jun Wu
Date July 3, 2017, 3:10 a.m.
Message ID <edb97a6c5f1065227180.1499051419@x1c>
Download mbox | patch
Permalink /patch/21947/
State Accepted
Headers show

Comments

Jun Wu - July 3, 2017, 3:10 a.m.
# HG changeset patch
# User Jun Wu <quark@fb.com>
# Date 1499051289 25200
#      Sun Jul 02 20:08:09 2017 -0700
# Node ID edb97a6c5f1065227180e41010076ad09697b408
# Parent  dde88a5ead698208226301a62a5f2aa2629d7839
# Available At https://bitbucket.org/quark-zju/hg-draft
#              hg pull https://bitbucket.org/quark-zju/hg-draft -r edb97a6c5f10
phabricator: add phabsend command to send a stack

The `phabsend` command is intended to provide `hg email`-like experience -
sending a stack, setup dependency information and do not amend existing
changesets.

It uses differential.createrawdiff and differential.revision.edit Conduit
API to create or update a Differential Revision.

Local tags like `D123` are written indicating certain changesets were sent
to Phabricator. The `phabsend` command will use obsstore and tags
information to decide whether to update or create Differential Revisions.
Phillip Cohen - July 3, 2017, 8:38 p.m.
> +    """
> +    revs = list(revs) + opts.get('rev', [])
> +    revs = scmutil.revrange(repo, revs)
> +

I'd add something like this here:

if not revs:
    raise error.Abort(_('must specify at least one changeset with -r'))

On Sun, Jul 2, 2017 at 8:10 PM, Jun Wu <quark@fb.com> wrote:
> # HG changeset patch
> # User Jun Wu <quark@fb.com>
> # Date 1499051289 25200
> #      Sun Jul 02 20:08:09 2017 -0700
> # Node ID edb97a6c5f1065227180e41010076ad09697b408
> # Parent  dde88a5ead698208226301a62a5f2aa2629d7839
> # Available At https://bitbucket.org/quark-zju/hg-draft
> #              hg pull https://bitbucket.org/quark-zju/hg-draft -r edb97a6c5f10
> phabricator: add phabsend command to send a stack
>
> The `phabsend` command is intended to provide `hg email`-like experience -
> sending a stack, setup dependency information and do not amend existing
> changesets.
>
> It uses differential.createrawdiff and differential.revision.edit Conduit
> API to create or update a Differential Revision.
>
> Local tags like `D123` are written indicating certain changesets were sent
> to Phabricator. The `phabsend` command will use obsstore and tags
> information to decide whether to update or create Differential Revisions.
>
> diff --git a/contrib/phabricator.py b/contrib/phabricator.py
> --- a/contrib/phabricator.py
> +++ b/contrib/phabricator.py
> @@ -7,4 +7,11 @@
>  """simple Phabricator integration
>
> +This extension provides a ``phabsend`` command which sends a stack of
> +changesets to Phabricator without amending commit messages.
> +
> +By default, Phabricator requires ``Test Plan`` which might prevent some
> +changeset from being sent. The requirement could be disabled by changing
> +``differential.require-test-plan-field`` config server side.
> +
>  Config::
>
> @@ -15,4 +22,8 @@ Config::
>      # API token. Get it from https://$HOST/conduit/login/
>      token = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx
> +
> +    # Repo callsign. If a repo has a URL https://$HOST/diffusion/FOO, then its
> +    # callsign is "FOO".
> +    callsign = FOO
>  """
>
> @@ -20,9 +31,16 @@ from __future__ import absolute_import
>
>  import json
> +import re
>
>  from mercurial.i18n import _
>  from mercurial import (
> +    encoding,
>      error,
> +    mdiff,
> +    obsolete,
> +    patch,
>      registrar,
> +    scmutil,
> +    tags,
>      url as urlmod,
>      util,
> @@ -97,2 +115,161 @@ def debugcallconduit(ui, repo, name):
>      s = json.dumps(result, sort_keys=True, indent=2, separators=(',', ': '))
>      ui.write('%s\n' % s)
> +
> +def getrepophid(repo):
> +    """given callsign, return repository PHID or None"""
> +    # developer config: phabricator.repophid
> +    repophid = repo.ui.config('phabricator', 'repophid')
> +    if repophid:
> +        return repophid
> +    callsign = repo.ui.config('phabricator', 'callsign')
> +    if not callsign:
> +        return None
> +    query = callconduit(repo, 'diffusion.repository.search',
> +                        {'constraints': {'callsigns': [callsign]}})
> +    if len(query[r'data']) == 0:
> +        return None
> +    repophid = encoding.strtolocal(query[r'data'][0][r'phid'])
> +    repo.ui.setconfig('phabricator', 'repophid', repophid)
> +    return repophid
> +
> +_differentialrevisionre = re.compile('\AD([1-9][0-9]*)\Z')
> +
> +def getmapping(ctx):
> +    """return (node, associated Differential Revision ID) or (None, None)
> +
> +    Examines all precursors and their tags. Tags with format like "D1234" are
> +    considered a match and the node with that tag, and the number after "D"
> +    (ex. 1234) will be returned.
> +    """
> +    unfi = ctx.repo().unfiltered()
> +    nodemap = unfi.changelog.nodemap
> +    for n in obsolete.allprecursors(unfi.obsstore, [ctx.node()]):
> +        if n in nodemap:
> +            for tag in unfi.nodetags(n):
> +                m = _differentialrevisionre.match(tag)
> +                if m:
> +                    return n, int(m.group(1))
> +    return None, None
> +
> +def getdiff(ctx, diffopts):
> +    """plain-text diff without header (user, commit message, etc)"""
> +    output = util.stringio()
> +    for chunk, _label in patch.diffui(ctx.repo(), ctx.p1().node(), ctx.node(),
> +                                      None, opts=diffopts):
> +        output.write(chunk)
> +    return output.getvalue()
> +
> +def creatediff(ctx):
> +    """create a Differential Diff"""
> +    repo = ctx.repo()
> +    repophid = getrepophid(repo)
> +    # Create a "Differential Diff" via "differential.createrawdiff" API
> +    params = {'diff': getdiff(ctx, mdiff.diffopts(git=True, context=32767))}
> +    if repophid:
> +        params['repositoryPHID'] = repophid
> +    diff = callconduit(repo, 'differential.createrawdiff', params)
> +    if not diff:
> +        raise error.Abort(_('cannot create diff for %s') % ctx)
> +    return diff
> +
> +def writediffproperties(ctx, diff):
> +    """write metadata to diff so patches could be applied losslessly"""
> +    params = {
> +        'diff_id': diff[r'id'],
> +        'name': 'hg:meta',
> +        'data': json.dumps({
> +            'user': ctx.user(),
> +            'date': '%d %d' % ctx.date(),
> +        }),
> +    }
> +    callconduit(ctx.repo(), 'differential.setdiffproperty', params)
> +
> +def createdifferentialrevision(ctx, revid=None, parentrevid=None):
> +    """create or update a Differential Revision
> +
> +    If revid is None, create a new Differential Revision, otherwise update
> +    revid. If parentrevid is not None, set it as a dependency.
> +    """
> +    repo = ctx.repo()
> +    diff = creatediff(ctx)
> +    writediffproperties(ctx, diff)
> +
> +    transactions = [{'type': 'update', 'value': diff[r'phid']}]
> +
> +    # 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
> +    # existing revision (revid is not None) since that introduces visible
> +    # churns (someone edited "Summary" twice) on the web page.
> +    if parentrevid and revid is None:
> +        summary = 'Depends on D%s' % parentrevid
> +        transactions += [{'type': 'summary', 'value': summary},
> +                         {'type': 'summary', 'value': ' '}]
> +
> +    # Parse commit message and update related fields.
> +    desc = ctx.description()
> +    info = callconduit(repo, 'differential.parsecommitmessage',
> +                       {'corpus': desc})
> +    for k, v in info[r'fields'].items():
> +        if k in ['title', 'summary', 'testPlan']:
> +            transactions.append({'type': k, 'value': v})
> +
> +    params = {'transactions': transactions}
> +    if revid is not None:
> +        # Update an existing Differential Revision
> +        params['objectIdentifier'] = revid
> +
> +    revision = callconduit(repo, 'differential.revision.edit', params)
> +    if not revision:
> +        raise error.Abort(_('cannot create revision for %s') % ctx)
> +
> +    return revision
> +
> +@command('phabsend',
> +         [('r', 'rev', [], _('revisions to send'), _('REV'))],
> +         _('REV [OPTIONS]'))
> +def phabsend(ui, repo, *revs, **opts):
> +    """upload changesets to Phabricator
> +
> +    If there are multiple revisions specified, they will be send as a stack
> +    with a linear dependencies relationship using the order specified by the
> +    revset.
> +
> +    For the first time uploading changesets, local tags will be created to
> +    maintain the association. After the first time, phabsend will check
> +    obsstore and tags information so it can figure out whether to update an
> +    existing Differential Revision, or create a new one.
> +    """
> +    revs = list(revs) + opts.get('rev', [])
> +    revs = scmutil.revrange(repo, revs)
> +
> +    # Send patches one by one so we know their Differential Revision IDs and
> +    # can provide dependency relationship
> +    lastrevid = None
> +    for rev in revs:
> +        ui.debug('sending rev %d\n' % rev)
> +        ctx = repo[rev]
> +
> +        # Get Differential Revision ID
> +        oldnode, revid = getmapping(ctx)
> +        if oldnode != ctx.node():
> +            # Create or update Differential Revision
> +            revision = createdifferentialrevision(ctx, revid, lastrevid)
> +            newrevid = int(revision[r'object'][r'id'])
> +            if revid:
> +                action = _('updated')
> +            else:
> +                action = _('created')
> +
> +            # Create a local tag to note the association
> +            tagname = 'D%d' % newrevid
> +            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
> +                     date=None, local=True)
> +        else:
> +            # Nothing changed. But still set "newrevid" so the next revision
> +            # could depend on this one.
> +            newrevid = revid
> +            action = _('skipped')
> +
> +        ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
> +                                            ctx.description().split('\n')[0]))
> +        lastrevid = newrevid
> _______________________________________________
> Mercurial-devel mailing list
> Mercurial-devel@mercurial-scm.org
> https://www.mercurial-scm.org/mailman/listinfo/mercurial-devel
Jun Wu - July 3, 2017, 10:33 p.m.
Excerpts from Phillip Cohen's message of 2017-07-03 13:38:34 -0700:
> > +    """
> > +    revs = list(revs) + opts.get('rev', [])
> > +    revs = scmutil.revrange(repo, revs)
> > +
> 
> I'd add something like this here:
> 
> if not revs:
>     raise error.Abort(_('must specify at least one changeset with -r'))
 
Good point. I'll send a follow up.

Patch

diff --git a/contrib/phabricator.py b/contrib/phabricator.py
--- a/contrib/phabricator.py
+++ b/contrib/phabricator.py
@@ -7,4 +7,11 @@ 
 """simple Phabricator integration
 
+This extension provides a ``phabsend`` command which sends a stack of
+changesets to Phabricator without amending commit messages.
+
+By default, Phabricator requires ``Test Plan`` which might prevent some
+changeset from being sent. The requirement could be disabled by changing
+``differential.require-test-plan-field`` config server side.
+
 Config::
 
@@ -15,4 +22,8 @@  Config::
     # API token. Get it from https://$HOST/conduit/login/
     token = cli-xxxxxxxxxxxxxxxxxxxxxxxxxxxx
+
+    # Repo callsign. If a repo has a URL https://$HOST/diffusion/FOO, then its
+    # callsign is "FOO".
+    callsign = FOO
 """
 
@@ -20,9 +31,16 @@  from __future__ import absolute_import
 
 import json
+import re
 
 from mercurial.i18n import _
 from mercurial import (
+    encoding,
     error,
+    mdiff,
+    obsolete,
+    patch,
     registrar,
+    scmutil,
+    tags,
     url as urlmod,
     util,
@@ -97,2 +115,161 @@  def debugcallconduit(ui, repo, name):
     s = json.dumps(result, sort_keys=True, indent=2, separators=(',', ': '))
     ui.write('%s\n' % s)
+
+def getrepophid(repo):
+    """given callsign, return repository PHID or None"""
+    # developer config: phabricator.repophid
+    repophid = repo.ui.config('phabricator', 'repophid')
+    if repophid:
+        return repophid
+    callsign = repo.ui.config('phabricator', 'callsign')
+    if not callsign:
+        return None
+    query = callconduit(repo, 'diffusion.repository.search',
+                        {'constraints': {'callsigns': [callsign]}})
+    if len(query[r'data']) == 0:
+        return None
+    repophid = encoding.strtolocal(query[r'data'][0][r'phid'])
+    repo.ui.setconfig('phabricator', 'repophid', repophid)
+    return repophid
+
+_differentialrevisionre = re.compile('\AD([1-9][0-9]*)\Z')
+
+def getmapping(ctx):
+    """return (node, associated Differential Revision ID) or (None, None)
+
+    Examines all precursors and their tags. Tags with format like "D1234" are
+    considered a match and the node with that tag, and the number after "D"
+    (ex. 1234) will be returned.
+    """
+    unfi = ctx.repo().unfiltered()
+    nodemap = unfi.changelog.nodemap
+    for n in obsolete.allprecursors(unfi.obsstore, [ctx.node()]):
+        if n in nodemap:
+            for tag in unfi.nodetags(n):
+                m = _differentialrevisionre.match(tag)
+                if m:
+                    return n, int(m.group(1))
+    return None, None
+
+def getdiff(ctx, diffopts):
+    """plain-text diff without header (user, commit message, etc)"""
+    output = util.stringio()
+    for chunk, _label in patch.diffui(ctx.repo(), ctx.p1().node(), ctx.node(),
+                                      None, opts=diffopts):
+        output.write(chunk)
+    return output.getvalue()
+
+def creatediff(ctx):
+    """create a Differential Diff"""
+    repo = ctx.repo()
+    repophid = getrepophid(repo)
+    # Create a "Differential Diff" via "differential.createrawdiff" API
+    params = {'diff': getdiff(ctx, mdiff.diffopts(git=True, context=32767))}
+    if repophid:
+        params['repositoryPHID'] = repophid
+    diff = callconduit(repo, 'differential.createrawdiff', params)
+    if not diff:
+        raise error.Abort(_('cannot create diff for %s') % ctx)
+    return diff
+
+def writediffproperties(ctx, diff):
+    """write metadata to diff so patches could be applied losslessly"""
+    params = {
+        'diff_id': diff[r'id'],
+        'name': 'hg:meta',
+        'data': json.dumps({
+            'user': ctx.user(),
+            'date': '%d %d' % ctx.date(),
+        }),
+    }
+    callconduit(ctx.repo(), 'differential.setdiffproperty', params)
+
+def createdifferentialrevision(ctx, revid=None, parentrevid=None):
+    """create or update a Differential Revision
+
+    If revid is None, create a new Differential Revision, otherwise update
+    revid. If parentrevid is not None, set it as a dependency.
+    """
+    repo = ctx.repo()
+    diff = creatediff(ctx)
+    writediffproperties(ctx, diff)
+
+    transactions = [{'type': 'update', 'value': diff[r'phid']}]
+
+    # 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
+    # existing revision (revid is not None) since that introduces visible
+    # churns (someone edited "Summary" twice) on the web page.
+    if parentrevid and revid is None:
+        summary = 'Depends on D%s' % parentrevid
+        transactions += [{'type': 'summary', 'value': summary},
+                         {'type': 'summary', 'value': ' '}]
+
+    # Parse commit message and update related fields.
+    desc = ctx.description()
+    info = callconduit(repo, 'differential.parsecommitmessage',
+                       {'corpus': desc})
+    for k, v in info[r'fields'].items():
+        if k in ['title', 'summary', 'testPlan']:
+            transactions.append({'type': k, 'value': v})
+
+    params = {'transactions': transactions}
+    if revid is not None:
+        # Update an existing Differential Revision
+        params['objectIdentifier'] = revid
+
+    revision = callconduit(repo, 'differential.revision.edit', params)
+    if not revision:
+        raise error.Abort(_('cannot create revision for %s') % ctx)
+
+    return revision
+
+@command('phabsend',
+         [('r', 'rev', [], _('revisions to send'), _('REV'))],
+         _('REV [OPTIONS]'))
+def phabsend(ui, repo, *revs, **opts):
+    """upload changesets to Phabricator
+
+    If there are multiple revisions specified, they will be send as a stack
+    with a linear dependencies relationship using the order specified by the
+    revset.
+
+    For the first time uploading changesets, local tags will be created to
+    maintain the association. After the first time, phabsend will check
+    obsstore and tags information so it can figure out whether to update an
+    existing Differential Revision, or create a new one.
+    """
+    revs = list(revs) + opts.get('rev', [])
+    revs = scmutil.revrange(repo, revs)
+
+    # Send patches one by one so we know their Differential Revision IDs and
+    # can provide dependency relationship
+    lastrevid = None
+    for rev in revs:
+        ui.debug('sending rev %d\n' % rev)
+        ctx = repo[rev]
+
+        # Get Differential Revision ID
+        oldnode, revid = getmapping(ctx)
+        if oldnode != ctx.node():
+            # Create or update Differential Revision
+            revision = createdifferentialrevision(ctx, revid, lastrevid)
+            newrevid = int(revision[r'object'][r'id'])
+            if revid:
+                action = _('updated')
+            else:
+                action = _('created')
+
+            # Create a local tag to note the association
+            tagname = 'D%d' % newrevid
+            tags.tag(repo, tagname, ctx.node(), message=None, user=None,
+                     date=None, local=True)
+        else:
+            # Nothing changed. But still set "newrevid" so the next revision
+            # could depend on this one.
+            newrevid = revid
+            action = _('skipped')
+
+        ui.write(_('D%s: %s - %s: %s\n') % (newrevid, action, ctx,
+                                            ctx.description().split('\n')[0]))
+        lastrevid = newrevid