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)
Firefox for Android Graveyard
General
Tracking
(Not tracked)
RESOLVED
FIXED
Firefox 33
People
(Reporter: ckitching, Assigned: ckitching)
Details
Attachments
(1 file, 2 obsolete files)
1.20 KB,
patch
|
gps
:
review+
|
Details | Diff | Splinter Review |
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.
Assignee | ||
Comment 1•10 years ago
|
||
Assignee | ||
Updated•10 years ago
|
Attachment #8456667 -
Attachment is obsolete: true
Assignee | ||
Comment 2•10 years ago
|
||
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 3•10 years ago
|
||
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+
Assignee | ||
Comment 4•10 years ago
|
||
(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 5•10 years ago
|
||
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+
Assignee | ||
Comment 6•10 years ago
|
||
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
Comment 7•10 years ago
|
||
https://hg.mozilla.org/mozilla-central/rev/e69dde0fa09f
Status: NEW → RESOLVED
Closed: 10 years ago
Resolution: --- → FIXED
Target Milestone: --- → Firefox 33
Updated•3 years ago
|
Product: Firefox for Android → Firefox for Android Graveyard
You need to log in
before you can comment on or make changes to this bug.
Description
•