Closed Bug 1413628 Opened 7 years ago Closed 7 years ago

Review repo discovery, HTTP submission broken by Mercurial 4.4

Categories

(MozReview Graveyard :: Integration: Mercurial, defect)

Product:
MozReview Graveyard
Component:
Integration: Mercurial
Type:
defect
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: gps, Assigned: gps)

References

Details

Attachments

(12 files)

reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: add output from Mercurial 4.3 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: register config options (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: don't add review id during normal commits (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: allow divergence in test output (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: add new output in Mercurial 4.4 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: adapt HTTP client code to Mercurial 4.4 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: adapt discovery repo code to Mercurial 4.4 (bug 1413628);
59 bytes, text/x-review-board-request
Details
reviewboard: mark extension as compatible with Mercurial 4.4 (bug 1413628);
59 bytes, text/x-review-board-request
Details 
Mercurial 4.4 breaks the "autoreview" repository feature in the reviewboard client extension.

hgext/reviewboard/tests/test-repo-discovery.t demonstrates the failure.
HTTP based review submission is also borked in 4.4.
Summary: Review repo discovery broken by Mercurial 4.4 → Review repo discovery, HTTP submission broken by Mercurial 4.4
Comment on attachment 8924346 [details]
reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);

https://reviewboard.mozilla.org/r/195590/#review201020
Attachment #8924346 - Flags: review+
Comment on attachment 8924347 [details]
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

https://reviewboard.mozilla.org/r/195592/#review201022
Attachment #8924347 - Flags: review+
Comment on attachment 8924348 [details]
reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);

https://reviewboard.mozilla.org/r/195594/#review201024

::: commit-message-7b9e8:4
(Diff revision 3)
> +reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628); r?glob
> +
> +Mercurial 4.3+ will issue a devel warning if you pass a default
> +argument to look up the config value if an option with a static

"if" vs "of"?
Attachment #8924348 - Flags: review+
Comment on attachment 8924349 [details]
reviewboard: add output from Mercurial 4.3 (bug 1413628);

https://reviewboard.mozilla.org/r/195596/#review201028

::: hgext/reviewboard/tests/test-multiple-precursors.t:63
(Diff revision 3)
>    created new head
>    $ echo 'foo3' > foo3
>    $ hg commit -A -m 'Bug 1 - Foo 3'
>    adding foo3
>    $ hg debugobsolete -d '0 0' 0b3e14fe3ff19019110705e72dcf563c0ef551f6 8cfe623ffa827c2382e41f8cd479005984c94e76 02a9514408b763f7534d4f250046b513799ffe4c
> +  obsoleted 1 changesets (?)

huh, I don't remember seeing the optional thing before, interesting.
Attachment #8924349 - Flags: review+
Comment on attachment 8924350 [details]
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

https://reviewboard.mozilla.org/r/195598/#review201030
Attachment #8924350 - Flags: review+
Comment on attachment 8924378 [details]
reviewboard: don't add review id during normal commits (bug 1413628);

https://reviewboard.mozilla.org/r/195656/#review201032

LGTM, that being said I didn't review the test changes as thoroughly as usual, not feeling my sharpest right now.

::: commit-message-ea964:20
(Diff revision 2)
> +submit time. Other than semantically changing the meaning of the
> +embedded time in the review id from commit time to review submit
> +time and adding some overhead to 1st time review submission, this

This would affect the ability to submit some reviews with a dirty working directory, that would normally have already had the `MozReview-Commit-ID`, no?

Not saying this change shouldn't be done, maybe you should call that out explicitly in the message though if it's the case?
Attachment #8924378 - Flags: review+
Comment on attachment 8924351 [details]
reviewboard: register config options (bug 1413628);

https://reviewboard.mozilla.org/r/195600/#review201036

::: hgext/reviewboard/client.py:99
(Diff revision 3)
> +# Mercurial 4.3 introduced the config registrar. 4.4 requires config
> +# items to be registered to avoid a devel warning.
> +if util.safehasattr(registrar, 'configitem'):
> +    # Imports get evaluated in block scope. So the if above has no impact.
> +    try:
> +        from mercurial import configitems
> +    except ImportError:
> +        pass

Seems like it's probably worth making the way you import `configitems` and the comments you use consistent between `client.py` and `server.py`, is there a reason you're changing things slightly?
Attachment #8924351 - Flags: review+
Comment on attachment 8924417 [details]
reviewboard: allow divergence in test output (bug 1413628);

https://reviewboard.mozilla.org/r/195706/#review201044

::: hgext/reviewboard/tests/test-specify-reviewers.t:547
(Diff revision 1)
>    remote: added 1 changesets with 1 changes to 1 files
>    remote: recorded push in pushlog
>    submitting 1 changesets for review
>    unrecognized reviewer: cthulhu
>    
> -  changeset:  28:c0f7a0ead8a3
> +  changeset:  2[58]:c0f7a0ead8a3 (re)

I think the obviousness of what you're doing below (e.g. `(29|32)`) would make things more readable up here too. So `(25|28)` in this case... applying that here and to the other lines would go a long way to make me feel less dirty about this change heh.
Attachment #8924417 - Flags: review+
Comment on attachment 8924418 [details]
reviewboard: add new output in Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195708/#review201048
Attachment #8924418 - Flags: review+
Comment on attachment 8924419 [details]
reviewboard: adapt HTTP client code to Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195710/#review201052

::: commit-message-cf98c:1
(Diff revision 1)
> +reviewboard: adopt HTTP client code to Mercurial 4.4 (bug 1413628); r?glob

"adopt" seems like a strange choice here... "adapt" what you were intending?

::: hgext/reviewboard/client.py:152
(Diff revision 1)
>          url = '%s/%s' % (remote._url, httpcommand)
> -        request = remote.requestbuilder(url, data=data,
> +
> +        # TRACKING hg44: 4.4 renamed requestbuilder to _requestbuilder and
> +        # urlopener to _urlopener.
> +        if util.safehasattr(remote, '_requestbuilder'):
> +            rb = remote._requestbuilder

I'd rather you use something more verbose than `rb` here, since we're dealing with a review board focused extension, where `rb` means review board colloquially. When you called `rb(...)` below it made me do a double take.

::: hgext/reviewboard/client.py:153
(Diff revision 1)
> -        request = remote.requestbuilder(url, data=data,
> +
> +        # TRACKING hg44: 4.4 renamed requestbuilder to _requestbuilder and
> +        # urlopener to _urlopener.
> +        if util.safehasattr(remote, '_requestbuilder'):
> +            rb = remote._requestbuilder
> +            opener = remote._urlopener

ooo, depending on `_<method>`s now, fun heh.
Attachment #8924419 - Flags: review+
Comment on attachment 8924420 [details]
reviewboard: adapt discovery repo code to Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195712/#review201056

::: commit-message-81f89:1
(Diff revision 1)
> +reviewboard: adopt discovery repo code to Mercurial 4.4 (bug 1413628); r?glob

Ah again, "adopt", maybe it was intended... Using it this way still feels strange to me, I think "adapt" works better.
Attachment #8924420 - Flags: review+
Comment on attachment 8924421 [details]
reviewboard: mark extension as compatible with Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195714/#review201058
Attachment #8924421 - Flags: review+
Comment on attachment 8924349 [details]
reviewboard: add output from Mercurial 4.3 (bug 1413628);

https://reviewboard.mozilla.org/r/195596/#review201028

> huh, I don't remember seeing the optional thing before, interesting.

I think it is somewhat new in the Mercurial test harness.

I updated the harness yesterday.

It has a new ``#testcases`` syntax where you can declare variations of the test to run. You can then change behavior of individual variations by ``#if variation`` tests. e.g. Mercurial's ``test-amend.t`` runs the test with obsolescence on and off. Individual lines can be associated with specific variations by ending the line with e.g. ``(variation !)``.

The test harness also now colorizes output in terminals.
Comment on attachment 8924351 [details]
reviewboard: register config options (bug 1413628);

https://reviewboard.mozilla.org/r/195600/#review201036

> Seems like it's probably worth making the way you import `configitems` and the comments you use consistent between `client.py` and `server.py`, is there a reason you're changing things slightly?

No good reason. I'll fix this.
Comment on attachment 8924417 [details]
reviewboard: allow divergence in test output (bug 1413628);

https://reviewboard.mozilla.org/r/195706/#review201044

> I think the obviousness of what you're doing below (e.g. `(29|32)`) would make things more readable up here too. So `(25|28)` in this case... applying that here and to the other lines would go a long way to make me feel less dirty about this change heh.

I'll just change this to use the new ``(hg44 !)`` and ``(no-hg44 !)`` syntax. That makes it much more clear where to expect divergence.
Pushed by gszorc@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/dfc18bac081c
reviewboard: drop support for Mercurial 3.9 and 4.0 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/12eae325c3eb
reviewboard: update backup bundle output for 4.3 compatibility ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/755a7e8e6bea
reviewboard: avoid devel warning in Mercurial 4.3 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/2b3225ab0a7b
reviewboard: add output from Mercurial 4.3 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/1f364ea3077e
reviewboard: mark extensions as compatible with 4.3 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/f04c4f84af3f
reviewboard: don't add review id during normal commits ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/4c5c51ffeeec
reviewboard: register config options ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/d06315ca4400
reviewboard: allow divergence in test output ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/3f23bf6aa130
reviewboard: add new output in Mercurial 4.4 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/6afb468448ed
reviewboard: adapt HTTP client code to Mercurial 4.4 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/9cef2e1bee13
reviewboard: adapt discovery repo code to Mercurial 4.4 ; r=smacleod
https://hg.mozilla.org/hgcustom/version-control-tools/rev/a5a9773c6571
reviewboard: mark extension as compatible with Mercurial 4.4 ; r=smacleod
Status: ASSIGNED → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
Attachment #8924346 - Flags: review?(glob)
Attachment #8924347 - Flags: review?(glob)
Attachment #8924348 - Flags: review?(glob)
Attachment #8924349 - Flags: review?(glob)
Attachment #8924350 - Flags: review?(glob)
Attachment #8924351 - Flags: review?(glob)
Attachment #8924378 - Flags: review?(glob)
Attachment #8924417 - Flags: review?(glob)
Attachment #8924418 - Flags: review?(glob)
Attachment #8924419 - Flags: review?(glob)
Attachment #8924420 - Flags: review?(glob)
Attachment #8924421 - Flags: review?(glob)
Blocks: 1414963
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: