910658 - mach mercurial-setup should add "qnew = -Ue" to defaults section

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[Jonas Finnemann Jensen (:jonasfj)]

First of, as new to hg, MQ and FF development, mach mercurial-setup is pretty awesome :)

But it might be nice to add:
    [defaults]
    qnew = -Ue

Or at the very least "qnew = -U" to avoid having patches without names.
This is in line with guidelines from:
https://developer.mozilla.org/en-US/docs/Mercurial_FAQ#How_can_I_generate_a_patch_for_somebody_else_to_check-in_for_me.3F

Comment 1

[:Ms2ger (he/him; ⌚ UTC+1/+2)]

Can't say I like -e, but -U is probably good.

Comment 2

[Gregory Szorc [:gps]]

I'm not sure I like -e either, but -U is good.

This should be trivial to implement. Code lives in /tools/mercurial/hgsetup in mozilla-central.

Comment 3

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: mach-mercurial-setup-qnew-set-currentuser.patch

Yeah, okay... -e is probably annoying in the long run :)

I've attached a quick patch for testing if -U or --currentuser is set and offering to set it if not.

Comment 4

[Gregory Szorc [:gps]]

Comment on attachment 797835 [details] [diff] [review]
mach-mercurial-setup-qnew-set-currentuser.patch

Review of attachment 797835 [details] [diff] [review]:
-----------------------------------------------------------------

This looks good.

If you wanted to take this to the next level, you could probably import argparse to perform the command argument parsing. This might be a little more robust than relying on a regexp to parse options. Ideally we'd load up the Mercurial API to parse the arguments, but then we have to worry about GPL licensing issues and whether the Mercurial API is importable. I don't want to go there now.

::: tools/mercurial/hgsetup/wizard.py
@@ +73,5 @@
>  
> +QNEWCURRENTUSER_INFO = '''
> +The mercurial queues command |hg qnew|, which creates new patches in your patch
> +queue does not set patch author information by default. Author information
> +should be included when uploading for review. 

Nit: Trailing whitespace

Comment 5

[Jonas Finnemann Jensen (:jonasfj)]

Attachment: mach-mercurial-setup-qnew-set-currentuser.patch

I haven't seen argparse before, so I just had to try it out.
And while I'm sure it's awesome for writing shell scripts, I'm not sure it's more robust in this use case.
The `ArgumentParser` from argparse apparently prints an error/usage message and exists non-zero on argument failures.
There is `ArgumentParser.parse_known_args` which won't complain about unknown arguments, but still exits zero on '-Ue'.
I might have overlooked something, but libraries that might exit(2) intentionally are probably not the best fit here.

Granted if someone has a default section featuring:
`qnew =  --exclude 'my\ -Unlike\ named\ folder/'`
we will not detect missing -U argument.
But I can't of the top my head, see any cases where we might break something by prefixing the existing arguments with "-U ".
There's probably special cases with special extensions and very special configuration, but that's why the user is offered a diff.

Anyways, I fixed the trailing whitespace...

Comment 6

[Gregory Szorc [:gps]]

I thought there was an API somewhere in argparse that offered just the low-level arg parsing bits. But yeah, there are some sys.exits buried in there. Not exactly the most reusable API :/

Let's get this checked in.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 799810 [details] [diff] [review]
mach-mercurial-setup-qnew-set-currentuser.patch

Review of attachment 799810 [details] [diff] [review]:
-----------------------------------------------------------------

https://hg.mozilla.org/integration/mozilla-inbound/rev/d9ad02ac2fcb

Comment 8

[Ed Morley [:emorley]]

https://hg.mozilla.org/mozilla-central/rev/d9ad02ac2fcb