902529 - mercurial-setup should not just check ~/.hgrc but also ~/mercurial.ini on Windows

Status: RESOLVED FIXED

(Firefox Build System :: Mach Core, enhancement)

Description

[:Gijs (he/him)]

Attachment: my-mercurial.ini

When I run this:

gkruitbosch@GKRUITBOSCH-PC /e/dev/ux-central
$ hg qnew testme

gkruitbosch@GKRUITBOSCH-PC /e/dev/ux-central
$ hg log -l1
changeset:   142036:df1440500467
tag:         qtip
tag:         testme
tag:         tip
user:        Gijs Kruitbosch <[e-mail]>
date:        Wed Aug 07 19:34:55 2013 +0200
summary:     [mq]: testme

Obviously hg config has my username and so on in my mercurial config. However running ./mach mercurial-setup gives:

$ ./mach mercurial-setup
<snip>
You don't have a username defined in your Mercurial config file. In order to
send patches to Mozilla, you'll need to attach a name and email address. If you
aren't comfortable giving us your full name, pseudonames are acceptable.

What is your name?

(at which point I ctrl-c)

I'm guessing it's not reading/writing mercurial.ini on windows, which is where my config is.

I've attached my mercurial.ini, sans my bugzilla credentials because bzexport doesn't like reading them from profiles on Windows.

Comment 1

[:Gijs (he/him)]

Attachment: Also check mercurial.ini on Windows

My python skills are a bit rusty. I'm sure determining which hgrc is more used than the other one can be done better than this, but this is what I ended up doing. Please be ruthless in review. :-)

Comment 3

[Gregory Szorc [:gps]]

Comment on attachment 795920 [details] [diff] [review]
Also check mercurial.ini on Windows

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

I'm not even sure why Mercurial on Windows supports an extra config file name. Seems silly to me. I'd almost prefer we just detect mercurial.ini as unsupported and ask the user to rename it to hgrc.

But, that's not very friendly and arguably mean, so maybe we shouldn't do that. Then again...

Fix the formatting and I'll probably r+ this.

::: tools/mercurial/hgsetup/config.py
@@ +20,5 @@
>          """Create a new instance, optionally from an existing hgrc file."""
>  
> +        # Figure out what file we're using:
> +        picky_infiles = []
> +        if len(infiles) > 1:

len(None) raises an Exception!

@@ +21,5 @@
>  
> +        # Figure out what file we're using:
> +        picky_infiles = []
> +        if len(infiles) > 1:
> +          picky_infiles = filter(os.path.isfile, infiles)

Nit: Always use 4 space indent in Python.

@@ +24,5 @@
> +        if len(infiles) > 1:
> +          picky_infiles = filter(os.path.isfile, infiles)
> +          if len(infiles) > 0:
> +            picky_infiles = map(lambda filename: (os.path.getsize(filename), filename), picky_infiles)
> +            picky_infiles = [max(picky_infiles, key=lambda tup: tup[0])]

Simple lambas like this are typically implemented with comprehensions. e.g.

picky_infiles = [f for f in picky_infiles if os.path.getsize(f) > 0]

Comment 4

[:Gijs (he/him)]

Attachment: Also check mercurial.ini on Windows

The previous patch was actually embarrassingly broken (but worked for me because mercurial.ini comes first in the list anyway) because of typos and logic fail on my part. I've fixed the brokenness (I think) and now use 4-space indenting, and I swapped one lambda for a list comprehension.

(In reply to Gregory Szorc [:gps] from comment #3)
> @@ +24,5 @@
> > +        if len(infiles) > 1:
> > +          picky_infiles = filter(os.path.isfile, infiles)
> > +          if len(infiles) > 0:
> > +            picky_infiles = map(lambda filename: (os.path.getsize(filename), filename), picky_infiles)
> > +            picky_infiles = [max(picky_infiles, key=lambda tup: tup[0])]
> 
> Simple lambas like this are typically implemented with comprehensions. e.g.
> 
> picky_infiles = [f for f in picky_infiles if os.path.getsize(f) > 0]

The second of these AFAIK can't easily be switched; I actually want to get the largest of the files passed (as a proxy for "which of these is the user most likely to want us to write to"). I then want the filename rather than the filesize (yes, the quoted code above is (part of what's) broken). I hope the current version does make sense. If there's a better way to do this, please let me know! :-)

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 796624 [details] [diff] [review]
Also check mercurial.ini on Windows

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

This is effectively r+. But since you're still learning Python, I'd like to see the final version before committing.

Thank you for staying the course.

::: tools/mercurial/hgsetup/config.py
@@ +22,5 @@
> +        if infiles is not None and len(infiles) > 0:
> +            # If files were specified, figure out what file we're using:
> +            if len(infiles) > 1:
> +                picky_infiles = filter(os.path.isfile, infiles)
> +                if len(picky_infiles) > 0:

In Python, |if <list>| returns true if the list has elements, so you don't need len().

@@ +24,5 @@
> +            if len(infiles) > 1:
> +                picky_infiles = filter(os.path.isfile, infiles)
> +                if len(picky_infiles) > 0:
> +                    picky_infiles = [(os.path.getsize(path), path) for path in picky_infiles]
> +                    infiles = [max(picky_infiles, key=lambda tup: tup[0])[1]]

This could be written a few different ways. Since the list is short and you aren't worried about algorithmic complexity, you could just sort the list and then just grab the first element:

picky_infiles = sorted(picky_infiles)

sorted() takes cmp and key optional arguments to control sorting. But the default sort for tuples (sort by index) should work fine here.

Most Python programmers also use operator.itemgetter() (http://docs.python.org/2/library/operator.html#operator.itemgetter) to obtain an item from an object instead of using the simple lambda you used. e.g.

  max(pick_infiles, key=itemgetter(0))

::: tools/mercurial/mach_commands.py
@@ +26,5 @@
>          from hgsetup.wizard import MercurialSetupWizard
>  
>          wizard = MercurialSetupWizard(self._context.state_dir)
> +        config_paths = ['~/.hgrc']
> +        if sys.platform == "win32" or sys.platform == "cygwin":

Nit: Single quotes for strings unless the string contains a single quote, then use double quotes. If you find yourself escaping quotes everywhere, use triple quotes.

Comment 6

[:Gijs (he/him)]

Attachment: Also check mercurial.ini on Windows,

As sorted for tuples uses the first item, so does max(), it seems (tested quickly on the cmdline interpreter). So I presume just getting rid of the lambda entirely is fine. Judging by the style in mach_commands, I presumed (for config.py) that you'd expect me to use separate lines for import statements.

Comment 7

[Gregory Szorc [:gps]]

Comment on attachment 804316 [details] [diff] [review]
Also check mercurial.ini on Windows,

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

Please commit with "DONTBUILD (NPOTB)" in the commit message so you don't trigger automation jobs.

::: tools/mercurial/mach_commands.py
@@ +26,5 @@
>          from hgsetup.wizard import MercurialSetupWizard
>  
>          wizard = MercurialSetupWizard(self._context.state_dir)
> +        config_paths = ['~/.hgrc']
> +        if sys.platform == 'win32' or sys.platform == 'cygwin':

Nit: if sys.platform in ('win32', 'cygwin'):

Comment 8

[:Gijs (he/him)]

Landed with nit fixed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/9801fa05621f

Comment 9

[Ryan VanderMeulen (PTO, back July 27)]

https://hg.mozilla.org/mozilla-central/rev/9801fa05621f