Closed
Bug 1287234
Opened 8 years ago
Closed 8 years ago
git mozreview configure broken with git cinnabar 0.4.0beta1 without git-cinnabar-helper
Categories
(MozReview Graveyard :: Integration: Git, defect)
MozReview Graveyard
Integration: Git
Tracking
(Not tracked)
RESOLVED
FIXED
People
(Reporter: glandium, Assigned: glandium)
Details
Attachments
(2 files)
$ git mozreview configure searching for appropriate review repository... using hg::https://reviewboard-hg.mozilla.org/reviewboard using hg::https://reviewboard-hg.mozilla.org/buildbot-configs using hg::https://reviewboard-hg.mozilla.org/build-puppet using hg::https://reviewboard-hg.mozilla.org/mozillapulse using hg::https://reviewboard-hg.mozilla.org/build-mozharness using hg::https://reviewboard-hg.mozilla.org/build-talos using hg::https://reviewboard-hg.mozilla.org/build-tools using hg::https://reviewboard-hg.mozilla.org/build-buildbotcustom using hg::https://reviewboard-hg.mozilla.org/version-control-tools using hg::https://reviewboard-hg.mozilla.org/gecko using hg::https://reviewboard-hg.mozilla.org/comm-central Traceback (most recent call last): File "/home/enes/.mozbuild/version-control-tools/git/commands/git-mozreview", line 845, in <module> sys.exit(main(sys.argv[1:])) File "/home/enes/.mozbuild/version-control-tools/git/commands/git-mozreview", line 831, in main return configure_command(args) File "/home/enes/.mozbuild/version-control-tools/git/commands/git-mozreview", line 643, in configure_command remote_url = data[hg_shas[i]].split()[0] IndexError: list index out of range
Assignee | ||
Comment 1•8 years ago
|
||
The commands get_output is run for may emit messages on stderr that are only noise for the git mozreview script, altering the expected output. With current git cinnabar tip of the release branch, a warning may be emitted when running `git cinnabar hg2git`, which prevents `git mozreview configure` from finding the right repository because the output contains more than the expected sha1s. Review commit: https://reviewboard.mozilla.org/r/64694/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64694/
Attachment #8771594 -
Flags: review?(gps)
Attachment #8771595 -
Flags: review?(gps)
Assignee | ||
Comment 2•8 years ago
|
||
When for some reason the output of git cinnabar hg2git doesn't fit the "one sha1 per line" model, the current code can go horribly wrong by splitting words instead of lines, assuming that anything that is not a 0000000000000000000000000000000000000000 is a valid sha1, assuming the output has the same number of lines as the input and still trying other remotes even when one was already found. This change makes it all more robust. Review commit: https://reviewboard.mozilla.org/r/64696/diff/#index_header See other reviews: https://reviewboard.mozilla.org/r/64696/
Updated•8 years ago
|
Attachment #8771594 -
Flags: review?(gps) → review+
Comment 3•8 years ago
|
||
Comment on attachment 8771594 [details] Bug 1287234 - Do not merge stdout and stderr in get_output. https://reviewboard.mozilla.org/r/64694/#review62090 I'm slightly nervous about this because the caller may want to present command output to help the user debug. But I suppose this is fine until we run into a bug where we need the output.
Updated•8 years ago
|
Attachment #8771595 -
Flags: review?(gps) → review+
Comment 4•8 years ago
|
||
Comment on attachment 8771595 [details] Bug 1287234 - Make finding remote mozreview repository a little more robust. https://reviewboard.mozilla.org/r/64696/#review62092 Nice use of zip(). Do you have a test case to reproduce? What I worry about is output from Git like the following: Warning: blah blah blah <commit0> <commit1> <commit2> This has a leading non-SHA-1 "header" message. <commit0> is on line 1 and will map up to Mercurial node 0. This is, of course, incorrect. Can you guarantee we won't see output like this? FWIW, I suppose this is a fundamental problem with `git cinnabar hg2git`. If hg2git echoed back the query value in each line, that would remove any ambiguity and make parsing yet more robust. I'll r+ this to unblock landing. But I'd like you to consider making `hg2git` output more robust to prevent callers from having to match line offsets.
Assignee | ||
Comment 5•8 years ago
|
||
(In reply to Gregory Szorc [:gps] from comment #4) > Comment on attachment 8771595 [details] > Bug 1287234 - Make finding remote mozreview repository a little more robust. > > https://reviewboard.mozilla.org/r/64696/#review62092 > > Nice use of zip(). > > Do you have a test case to reproduce? What I worry about is output from Git > like the following: > > Warning: blah blah blah > <commit0> > <commit1> > <commit2> ... which shouldn't happen anymore with the change in get_output :) > This has a leading non-SHA-1 "header" message. <commit0> is on line 1 and > will map up to Mercurial node 0. This is, of course, incorrect. Can you > guarantee we won't see output like this? > > FWIW, I suppose this is a fundamental problem with `git cinnabar hg2git`. If > hg2git echoed back the query value in each line, that would remove any > ambiguity and make parsing yet more robust. > > I'll r+ this to unblock landing. But I'd like you to consider making > `hg2git` output more robust to prevent callers from having to match line > offsets. We can make that an option, I guess. The tricky part either way is to make the mozreview code compatible with both cases
Pushed by mh@glandium.org: https://hg.mozilla.org/hgcustom/version-control-tools/rev/561e85f0921f Do not merge stdout and stderr in get_output. r=gps https://hg.mozilla.org/hgcustom/version-control-tools/rev/8c1ccd41d597 Make finding remote mozreview repository a little more robust. r=gps
Status: NEW → RESOLVED
Closed: 8 years ago
Resolution: --- → FIXED
Comment 7•8 years ago
|
||
(In reply to Mike Hommey [:glandium] from comment #5) > > I'll r+ this to unblock landing. But I'd like you to consider making > > `hg2git` output more robust to prevent callers from having to match line > > offsets. > > We can make that an option, I guess. The tricky part either way is to make > the mozreview code compatible with both cases Indeed. Since this is part of the setup code, I suppose we could version sniff cinnabar in the future. I'm sure you wouldn't mind getting users on the latest version given all the improvements you've been making to it :) But obviously that's for another day.
You need to log in
before you can comment on or make changes to this bug.
Description
•