git mozreview configure broken with git cinnabar 0.4.0beta1 without git-cinnabar-helper

RESOLVED FIXED

Status

MozReview
Integration: Git
RESOLVED FIXED
a year ago
a year ago

People

(Reporter: glandium, Assigned: glandium)

Tracking

Details

MozReview Requests

Submitter Diff Changes Open Issues Last Updated
Loading...
Error loading review requests:

Attachments

(2 attachments)

(Assignee)

Description

a year ago
$ 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

a year ago
Created attachment 8771594 [details]
Bug 1287234 - Do not merge stdout and stderr in get_output.

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

a year ago
Created attachment 8771595 [details]
Bug 1287234 - Make finding remote mozreview repository a little more robust.

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/
Attachment #8771594 - Flags: review?(gps) → review+
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.
Attachment #8771595 - Flags: review?(gps) → review+
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

a year 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

Comment 6

a year ago
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
Last Resolved: a year ago
Resolution: --- → FIXED
(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.