Page MenuHomePhabricator

phabricator: add a `phabimport` command
ClosedPublic

Authored by mharbison72 on Feb 20 2020, 8:10 PM.

Details

Summary

I've had alias.phabimport = $hg phabread --stack $1 | $hg import --bypass -
for a while now, and I suspect others do too. That's limited though, in that it
can't use the information on Phabricator to restore it in the original location,
so I'm making it a first class command.

This doesn't do anything ambitious like that- this is mostly a simplification of
hg import to get the equivalent of the alias mentioned above. The --bypass
option is hardcoded to be enabled and the message about amending rejects
dropped (rejects aren't created with --bypass), because editing patches on
Phabricator seems like an unusual workflow.

This will need other options, like --obsolete and --secret. I think these
would be more useful as config settings, as I imagine the workflows are pretty
fixed depending on roles. Reviewers who don't queue patches probably never want
--obsolete, but may need --secret. Reviewers who do will want the former,
but not the latter. I left --stack as an option, but that should probably be
a config knob too (or at least default to on)- if the point of this is to avoid
rejects, it doesn't make sense to skip dependencies in most cases.

Evolve is going to need a fix to its wrapping of cmdutil.tryimportone(), as it
currently assumes opts has an obsolete key. It's worked around for now.

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

mharbison72 created this revision.Feb 20 2020, 8:10 PM
mharbison72 updated this revision to Diff 20426.Tue, Mar 3, 1:25 PM
pulkit added a subscriber: pulkit.Wed, Mar 4, 7:56 AM
pulkit added inline comments.
hgext/phabricator.py
1737

I think we should abort if there are any rejects.

mharbison72 added inline comments.Wed, Mar 4, 9:49 AM
hgext/phabricator.py
1737

When the bypass option is provided to tryimportone(), it looks like you can't get rejects and PatchErrors are converted to abort[1]. I'll clarify the commit message.

Is there some graceful transaction close/release/rollback function that needs to be called before it is finalized by exiting the with block? Having to run hg recover whenever a patch fails to apply will get annoying.

[1] https://www.mercurial-scm.org/repo/hg/file/5.3/mercurial/cmdutil.py#l1885

mharbison72 edited the summary of this revision. (Show Details)Wed, Mar 4, 9:57 AM
mharbison72 updated this revision to Diff 20450.
durin42 accepted this revision as: durin42.Wed, Mar 4, 1:56 PM
durin42 added a subscriber: durin42.

I like this, but have a question about a pony I kind of want as a follow-up. Will let someone else queue...

hgext/phabricator.py
1708

Would it be reasonable to accept revspec*s* here so I could type a series of patch IDs and have them all land?

mharbison72 added inline comments.Wed, Mar 4, 2:27 PM
hgext/phabricator.py
1708

I'm not quite sure the context. I didn't realize this until recently, but apparently + is a valid operator like with revsets. That said, I strung together two heads with common ancestors, +, and --stack in that unlanded pytype series you have (not sure how that got so branchy), and querydrev() seemed to return them in an order that wouldn't apply. But if I swapped the order on the command line, it worked. So tread carefully if patches are unrelated.

Are you looking to skip some initial patches (which --stack doesn't allow, but maybe - would), and apply the rest? Or are you trying to grab separate series all at once?

durin42 added inline comments.Wed, Mar 4, 2:41 PM
hgext/phabricator.py
1708

Are you looking to skip some initial patches (which --stack doesn't allow, but maybe - would), and apply the rest? Or are you trying to grab separate series all at once?

My typical workflow is I explicitly enumerate the patches I want to apply, partially because I've had enough misfires with phab's stack-querying operators that I don't trust myself with them anymore. Typically I'm grabbing one series at a time, but sometimes for a pile of trivial fixes I'll do unrelated changes in a single go.

mharbison72 added inline comments.Wed, Mar 4, 3:33 PM
hgext/phabricator.py
1708

My typical workflow is I explicitly enumerate the patches I want to apply, partially because I've had enough misfires with phab's stack-querying operators that I don't trust myself with them anymore.

Sounds like you'd prefer a phabimport.stack = False config option also then?

My understanding of revsets is that -r abc -r def -r ... essentially gets converted to (abc) + (def) + ..., so we should be able to do the same thing pretty easily here, if there's a way to distinguish each DREVSPEC from the other options.

pulkit added inline comments.Fri, Mar 6, 2:58 AM
hgext/phabricator.py
1737

I think if you raise error.Abort(), it should be rolled back automatically but not sure.

mharbison72 added inline comments.Fri, Mar 6, 11:01 AM
hgext/phabricator.py
1737

That's what I thought too, but not what I saw when raising error.Abort(). I can't reproduce that now though, so I'll cleanup the commit message and resend.

mharbison72 edited the summary of this revision. (Show Details)Fri, Mar 6, 11:03 AM
mharbison72 updated this revision to Diff 20554.
martinvonz accepted this revision.Fri, Mar 13, 8:22 PM
This revision is now accepted and ready to land.Fri, Mar 13, 8:22 PM
This revision was automatically updated to reflect the committed changes.