phabricator: add a `--branch` flag to `hg phabsend`
Needs ReviewPublic

Authored by pulkit on Mar 7 2019, 8:59 AM.

Details

Reviewers
None
Group Reviewers
hg-reviewers
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.

Diff Detail

Repository
rHG Mercurial
Lint
Lint Skipped
Unit
Unit Tests Skipped
pulkit created this revision.Mar 7 2019, 8:59 AM
pulkit added a comment.Mar 7 2019, 8:59 AM

This review is intended for default branch.

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 a641fd1a1196 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.

pulkit added a comment.Mar 8 2019, 5:30 PM

@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

In 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.

Kwan added a subscriber: Kwan.Mar 9 2019, 11:48 AM
In 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. 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).

In D6082#88981, @Kwan wrote:
In 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. 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?

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?

pulkit updated this revision to Diff 14548.Mar 18 2019, 5:07 PM
pulkit edited the summary of this revision. (Show Details)Mar 18 2019, 5:36 PM
pulkit edited the summary of this revision. (Show Details)Mar 18 2019, 5:47 PM
In D6082#88981, @Kwan wrote:
In 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. 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.

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.

Kwan added a comment.EditedMar 19 2019, 8:22 AM
In D6082#89656, @pulkit wrote:
In D6082#88981, @Kwan wrote:
In 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. 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 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 binary file changes to mozilla-central.

In all its "glory"

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