Patchwork D6082: phabricator: add a `--branch` flag to `hg phabsend`

login
register
mail settings
Submitter phabricator
Date March 7, 2019, 2 p.m.
Message ID <differential-rev-PHID-DREV-dxbzw27kl56tg2byq6ym-req@phab.mercurial-scm.org>
Download mbox | patch
Permalink /patch/39121/
State New
Headers show

Comments

phabricator - March 7, 2019, 2 p.m.
pulkit created this revision.
Herald added a subscriber: mercurial-devel.
Herald added a reviewer: hg-reviewers.

REVISION SUMMARY
  One of the existing problems with using phabricator for us is that there is no
  way to specify that the revision/patch/differential is intended for stable
  branch.
  
  This patch adds a new flag `--branch` to `hg phabsend` command. If the
  `--branch` is passed, `hg phabsend` will add a comment to the related
  differential saying that `The review is intended for a <branch-name> branch`.
  
  I looked into other API's which we can use or other properties which can be used
  to put the branch info and didn't found anything quite useful.
  
  Future improvement can be checking the branch of the patch and commenting that
  branch name.
  
  I am not sure how to add tests for this. I tested this patch while sending this
  patch.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

AFFECTED FILES
  hgext/phabricator.py

CHANGE DETAILS




To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - March 7, 2019, 2 p.m.
pulkit added a comment.


  This review is intended for `default` branch.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: mercurial-devel
phabricator - March 7, 2019, 5:35 p.m.
mharbison72 added a comment.


  To create a test, you need to `pip install pytest-vcr`, edit `auth.hgphab.phabtoken` at the top of the test, and use `--test-vcr` with your `hg phabsend` command.  See https://phab.mercurial-scm.org/rHGa641fd1a1196ae1c5cfd0374e10c618949c148cd for some background, and remember to purge your phabtoken from the changes before posting.  FWIW, I used a VirtualBox VM running Phabricator from Bitnami to figure things out before generating the test on phab.m-s.o to avoid extra noise.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 8, 2019, 10:30 p.m.
pulkit added a comment.


  @mharbison72 thanks for tips on adding test. Will add tests in next iteration.
  
  I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 8, 2019, 11:12 p.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D6082#88851, @pulkit wrote:
  
  > @mharbison72 thanks for tips on adding test. Will add tests in next iteration.
  >
  > I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php
  
  
  Seems worth a shot.  I don’t know anything about it, but presumably this would be rendered specially in the web UI, like the test plan, etc.  That sounds better than as a follow up comment.  It also seems natural enough that maybe it can be done unconditionally, instead of needing the argument.

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: mharbison72, mercurial-devel
phabricator - March 9, 2019, 4:48 p.m.
Kwan added a comment.


  In https://phab.mercurial-scm.org/D6082#88852, @mharbison72 wrote:
  
  > In https://phab.mercurial-scm.org/D6082#88851, @pulkit wrote:
  >
  > > @mharbison72 thanks for tips on adding test. Will add tests in next iteration.
  > >
  > > I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php
  >
  >
  > Seems worth a shot.  I don’t know anything about it, but presumably this would be rendered specially in the web UI, like the test plan, etc.  That sounds better than as a follow up comment.  It also seems natural enough that maybe it can be done unconditionally, instead of needing the argument.
  
  
  Yeah, it shows in the Diff Detail pane, like here <https://phabricator-dev.allizom.org/D1092?id=1765>.  Unfortunately I think it can be only set when using the creatediff endpoint, which is what I had to change my fork to do (though I could be wrong, the conduit docs are too sparse to be sure).

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: Kwan, mharbison72, mercurial-devel
phabricator - March 9, 2019, 6:27 p.m.
mharbison72 added a comment.


  In https://phab.mercurial-scm.org/D6082#88981, @Kwan wrote:
  
  > In https://phab.mercurial-scm.org/D6082#88852, @mharbison72 wrote:
  >
  > > In https://phab.mercurial-scm.org/D6082#88851, @pulkit wrote:
  > >
  > > > @mharbison72 thanks for tips on adding test. Will add tests in next iteration.
  > > >
  > > > I found that Differentials do have a branch field, maybe we can use that? https://secure.phabricator.com/source/phabricator/browse/master/src/applications/differential/customfield/DifferentialBranchField.php
  > >
  > >
  > > Seems worth a shot.  I don’t know anything about it, but presumably this would be rendered specially in the web UI, like the test plan, etc.  That sounds better than as a follow up comment.  It also seems natural enough that maybe it can be done unconditionally, instead of needing the argument.
  >
  >
  > Yeah, it shows in the Diff Detail pane, like here <https://phabricator-dev.allizom.org/D1092?id=1765>.  Unfortunately I think it can be only set when using the creatediff endpoint, which is what I had to change my fork to do (though I could be wrong, the conduit docs are too sparse to be sure).
  
  
  Yeah, that's kinda how I read the documentation too :/
  
  I wonder if we can use `parents.set` in `differential.revision.edit` on the first commit in the series.  Or add the branch name to the existing metadata wiith `differential.setdiffproperty`?  I've aliased `phabimport` to `!"%HG%" phabread $1 --stack | "%HG%" import --bypass -`, but maybe a first class phabimport that knows how to use the data would make the tooling easier?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: Kwan, mharbison72, mercurial-devel
phabricator - March 13, 2019, 6:37 p.m.
indygreg added a comment.


  While there may be a need for an explicit `--branch` argument, should we start by having `hg phabsend` automatically pick up the branch from the changeset? i.e. wouldn't we want branch selection to be automatic?

REPOSITORY
  rHG Mercurial

REVISION DETAIL
  https://phab.mercurial-scm.org/D6082

To: pulkit, #hg-reviewers
Cc: indygreg, Kwan, mharbison72, mercurial-devel

Patch

diff --git a/hgext/phabricator.py b/hgext/phabricator.py
--- a/hgext/phabricator.py
+++ b/hgext/phabricator.py
@@ -465,6 +465,7 @@ 
 
 @vcrcommand(b'phabsend',
          [(b'r', b'rev', [], _(b'revisions to send'), _(b'REV')),
+          (b'', b'branch', b'', _(b'comments review is for specific branch')),
           (b'', b'amend', True, _(b'update commit messages')),
           (b'', b'reviewer', [], _(b'specify reviewers')),
           (b'', b'confirm', None, _(b'ask for confirmation before sending'))],
@@ -496,6 +497,15 @@ 
 
     phabsend will check obsstore and the above association to decide whether to
     update an existing Differential Revision, or create a new one.
+
+    When working with multiple named branches, one can specify which branch your
+    reviews/differentials are intended for using --branch flag. Doing that,
+    phabsend will add a comment to your differential about that. For example::
+
+        `hg phabsend -r . --branch stable`
+
+    This will upload the parent of working directory and will add a comment on
+    related differential saying "This review is intented for stable branch".
     """
     revs = list(revs) + opts.get(b'rev', [])
     revs = scmutil.revrange(repo, revs)
@@ -524,6 +534,10 @@ 
     drevids = [] # [int]
     diffmap = {} # {newnode: diff}
 
+    bmsg = None
+    if opts.get(b'branch'):
+        bmsg = b"This review is intended for `%s` branch." % opts[b'branch']
+
     # Send patches one by one so we know their Differential Revision IDs and
     # can provide dependency relationship
     lastrevid = None
@@ -570,6 +584,12 @@ 
         drevids.append(newrevid)
         lastrevid = newrevid
 
+        if bmsg:
+            # comment on the new differential created the this review is for
+            # this certain branch
+            callconduit(repo, b'differential.createcomment',
+                        {b'revision_id': newrevid, b'message': bmsg})
+
     # Update commit messages and remove tags
     if opts.get(b'amend'):
         unfi = repo.unfiltered()