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
phabricator - March 19, 2019, 11:24 a.m.
pulkit 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).
  
  
  Last night I tried different endpoints and looks like creatediff is the only one which can be used to set the branch. I have asked on their discourse https://discourse.phabricator-community.org/t/conduit-api-to-set-branch-of-a-differential/2521.
  
  Talking about creatediff, can it be used instead of createrawdiff? It it's possible can you paste the changes you did your fork, maybe we need the exact same changes.
  
  In https://phab.mercurial-scm.org/D6082#89250, @indygreg wrote:
  
  > 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?
  
  
  Yes I agree. The next version of this patch will make `hg phabsend` automatically pick up the branch from changeset.

REPOSITORY
  rHG Mercurial

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

To: pulkit, #hg-reviewers
Cc: indygreg, Kwan, mharbison72, mercurial-devel
phabricator - March 19, 2019, 12:22 p.m.
Kwan added a comment.


  In https://phab.mercurial-scm.org/D6082#89656, @pulkit wrote:
  
  > 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).
  >
  >
  > Last night I tried different endpoints and looks like creatediff is the only one which can be used to set the branch. I have asked on their discourse https://discourse.phabricator-community.org/t/conduit-api-to-set-branch-of-a-differential/2521.
  >
  > Talking about creatediff, can it be used instead of createrawdiff? It it's possible can you paste the changes you did your fork, maybe we need the exact same changes.
  
  
  Oh man, yeah sure, let me just commit the WIP and push.  It's a lot of extra code to wrangle the data into creatediff form.  Bear in mind this wasn't exactly ready to have other eyes on it, and it's possible not all the changes are actually necessary (particularly the urlencoding ones, _so_ much fun having multiple different ways <https://secure.phabricator.com/T12447#216803> to submit the same data to Conduit :/). I also have not downstreamed all my py3 compat changes yet.
  Frankly I thought this amount of extra code probably wouldn't fly for submitting to the official version (and my coding style is probably questionable).
  
  On the plus side this did let me successfully submit <https://phabricator.services.mozilla.com/D23114?id=76558> binary file changes to mozilla-central.
  
  In all it's "glory" <https://bitbucket.org/KwanEsq/phabsend-moz/commits/branch/beta>
  
  I've also started to write up what I've learnt about the API for the wiki there, to document things the official docs are silent on, like what the "changes" parameter to the creatediff endpoint is actually supposed to be.  Hopefully that'd help anyone else who wanted to use such APIs in future (or who even wanted to make changes to mine).

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()