Closed Bug 1039164 Opened 10 years ago Closed 10 years ago

`mach mercurial-setup' fails when presented with duplicate mercurial configuration options.

Categories

(Firefox for Android Graveyard :: General, defect)

defect
Not set
minor

Tracking

(Not tracked)

RESOLVED FIXED
Firefox 33

People

(Reporter: ckitching, Assigned: ckitching)

Details

Attachments

(1 file, 2 obsolete files)

When running `mach mercurial-setup` with an hgrc that contains more than one instance of a particular setting in a particular section it throws an exception and dies (gracefully-ish).

The error given by mach in this situation isn't great, even though it secretly knows exactly where and how you screwed up:

Error importing existing Mercurial config!
Parsing failed with several errors.
First error at line 12.
If using quotes, they must wrap the entire string.


I propose modifying this to instead print the actual error objects that mach has collected internally (complete with their nice human-readable error messages) so users with configurations as badly-written as my own can repair them.
Attachment #8456667 - Attachment is obsolete: true
Under this patch, the output is the much more helpful:

Error importing existing Mercurial config!
Parsing failed with several errors:

Duplicate keyword name at line 12.
Duplicate keyword name at line 93.

If using quotes, they must wrap the entire string.


(After which I was able to figure out my stupid and fix my hgrc :P )

Naturally, feel three to throw this away: I doubt I've followed the proper convention for code in this area, but hope that it's at least illustrative of what I hope to achieve here.
Attachment #8456668 - Flags: review?(gps)
Comment on attachment 8456668 [details] [diff] [review]
Include all errors in the ConfigObjError's message

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

Oh, it's saving the useful data - that's nice!

You have the right idea but are patching the wrong file: the file modified in this patch is an upstream package and shouldn't be touched.

The place you want to patch is https://hg.mozilla.org/mozilla-central/file/869971ad9fd6/tools/mercurial/hgsetup/wizard.py#l157. It looks like you should be able to print e.errors.
Attachment #8456668 - Flags: review?(gps) → feedback+
(In reply to Gregory Szorc [:gps] from comment #3)

Thanks for the swift review! (at a slightly crazy time, too)

> Oh, it's saving the useful data - that's nice!
> 
> You have the right idea but are patching the wrong file: the file modified
> in this patch is an upstream package and shouldn't be touched.

*whistles innocently*

Admittedly, I just grepped m-c for the error. I really have no idea what I'm doing :P.

> The place you want to patch is
> https://hg.mozilla.org/mozilla-central/file/869971ad9fd6/tools/mercurial/
> hgsetup/wizard.py#l157. It looks like you should be able to print e.errors.

Okay, let's try that again. I have a feeling you're about to find a reason for my for loop not to exist, too, but maybe I've overestimated the coefficeint of "import antigravity".
Attachment #8456668 - Attachment is obsolete: true
Attachment #8456689 - Flags: review?(gps)
Comment on attachment 8456689 [details] [diff] [review]
Print all errorspassed to mercurial setup wizard.

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

::: tools/mercurial/hgsetup/wizard.py
@@ +160,5 @@
> +            msg = 'Error importing existing Mercurial config!\n\n'
> +            for error in e.errors:
> +                msg = msg + "%s\n" % error.message
> +
> +            print(msg)

You could make this a bit prettier.

First, you can do += on strings in Python.

But you don't need that.

Just print() as you go.
Attachment #8456689 - Flags: review?(gps) → review+
Excellent! Thanks.

Apologies: my first patch in Python land. I really should sit down and learn Python properly. I hear it's a good one.

Anyway, tidied up as per your suggestion and landed:

https://hg.mozilla.org/integration/fx-team/rev/e69dde0fa09f
https://hg.mozilla.org/mozilla-central/rev/e69dde0fa09f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Created:
Updated:
Size: