Closed Bug 902529 Opened 6 years ago Closed 6 years ago

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

Categories

(Firefox Build System :: Mach Core, enhancement)

x86_64
Windows 7
enhancement
Not set

Tracking

(Not tracked)

RESOLVED FIXED
mozilla27

People

(Reporter: Gijs, Assigned: Gijs)

References

Details

Attachments

(2 files, 2 obsolete files)

Attached file 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 <gijskruitbosch@gmail.com>
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.
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. :-)
Attachment #795920 - Flags: review?(gps)
Assignee: nobody → gijskruitbosch+bugs
Duplicate of this bug: 909923
Blocks: 794580
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]
Attachment #795920 - Flags: review?(gps) → feedback+
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! :-)
Attachment #795920 - Attachment is obsolete: true
Attachment #796624 - Flags: review?(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.
Attachment #796624 - Flags: review?(gps) → feedback+
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.
Attachment #804316 - Flags: review?(gps)
Attachment #796624 - Attachment is obsolete: true
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'):
Attachment #804316 - Flags: review?(gps) → review+
Landed with nit fixed:

remote:   https://hg.mozilla.org/integration/fx-team/rev/9801fa05621f
Status: NEW → ASSIGNED
Whiteboard: [fixed-in-fx-team]
https://hg.mozilla.org/mozilla-central/rev/9801fa05621f
Status: ASSIGNED → RESOLVED
Closed: 6 years ago
Resolution: --- → FIXED
Whiteboard: [fixed-in-fx-team]
Target Milestone: --- → mozilla27
Product: Core → Firefox Build System
You need to log in before you can comment on or make changes to this bug.