1413628 - Review repo discovery, HTTP submission broken by Mercurial 4.4

Status: RESOLVED FIXED

(MozReview Graveyard :: Integration: Mercurial, defect)

Description

[Gregory Szorc [:gps]]

Mercurial 4.4 breaks the "autoreview" repository feature in the reviewboard client extension.

hgext/reviewboard/tests/test-repo-discovery.t demonstrates the failure.

Comment 1

[Gregory Szorc [:gps]]

Attachment: reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);

Our policy is to support the last year's worth of releases of
Mercurial. 4.4 comes out today. So that means our minimum supported version
should be 4.1.

Review commit: https://reviewboard.mozilla.org/r/195590/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195590/

Comment 2

[Gregory Szorc [:gps]]

Attachment: reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

It looks like Mercurial changed the backup bundle naming in 4.3.
Adjust globs accordingly.

This fixes most test failures when run against Mercurial 4.3.

Review commit: https://reviewboard.mozilla.org/r/195592/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195592/

Comment 3

[Gregory Szorc [:gps]]

Attachment: reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);

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
default value.

There doesn't appear to be a good way to suppress the warning in
code aside from not passing the default argument. So we change
the code.

Review commit: https://reviewboard.mozilla.org/r/195594/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195594/

Comment 4

[Gregory Szorc [:gps]]

Attachment: reviewboard: add output from Mercurial 4.3 (bug 1413628);

It looks like `hg debugobsolete` can be chatty in Mercurial 4.3. Add
it as optional output in a test.

Review commit: https://reviewboard.mozilla.org/r/195596/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195596/

Comment 5

[Gregory Szorc [:gps]]

Attachment: reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

All tests now pass on 4.3.

Strictly speaking, we're not sure if the server works with 4.3 because
we haven't upgraded the server to Mercurial 4.3. But we need to bump the
compatibility of both extensions so the test runner runs tests for 4.3.

Review commit: https://reviewboard.mozilla.org/r/195598/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195598/

Comment 6

[Gregory Szorc [:gps]]

Attachment: reviewboard: register config options (bug 1413628);

Mercurial 4.4 softly requires config options be registered via a central
registrar. If they aren't, a devel warning is issued. The test runner
enables all devel warnings by default (which is a good idea). This
means we get new output and test failures for attempting to access a
config option that isn't registered.

When options are registered, Mercurial also issues a devel warning when
the caller accessing the option specifies a default value. This is
because the default value is defined as part of the option and doesn't
need to be specified at the call site.

Porting an extension to this mechanism is a bit annoying because old
Mercurial versions need to specify a default value (since the option
isn't registered) and new Mercurial versions would issue a warning
if a default argument is specified. The workaround is to set the
default value of "dynamicdefault." This special value means "the
default is dynamic" and ui.config* functions won't issue a devel
warning if a default value is specified.

When support for older versions of Mercurial are dropped, the proper
default value can be registered and the "default" argument can be
dropped from ui.config* callers.

This commit registers all config options used by the reviewboard
extension. By doing so, most test failures in 4.4 due to new
"devel-warn:" lines go away. This leaves a handful of legitimate
test failures.

Review commit: https://reviewboard.mozilla.org/r/195600/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195600/

Comment 7

[Gregory Szorc [:gps]]

Attachment: reviewboard: don't add review id during normal commits (bug 1413628);

The extension adds a MozReview-Commit-ID line to commit messages so
each changeset created on the client is tracked. We currently add
this line during local `hg commit` operations and on push/submit,
as needed.

Mercurial 4.4 changes some APIs around and introduces more cases
where commits can be created. This introduces more places where we
would need to monkeypatch functions to modify the commit message.

Ideally we wouldn't care. However, the test harness needs to run on
multiple Mercurial versions. And getting consistent output on multiple
Mercurial versions will be a lot of work.

Since the shelf life of the MozReview extension is limited since the
service is going away, we take one of the easier roads out of this
problem by removing the code that adds the review id during local
commit. This means we'll defer to rewriting commit messages until
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
shouldn't matter that much.

Review commit: https://reviewboard.mozilla.org/r/195656/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195656/

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8924347 [details]
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195592/diff/1-2/

Comment 9

[Gregory Szorc [:gps]]

Comment on attachment 8924348 [details]
reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195594/diff/1-2/

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8924349 [details]
reviewboard: add output from Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195596/diff/1-2/

Comment 11

[Gregory Szorc [:gps]]

Comment on attachment 8924350 [details]
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195598/diff/1-2/

Comment 12

[Gregory Szorc [:gps]]

Comment on attachment 8924351 [details]
reviewboard: register config options (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195600/diff/1-2/

Comment 13

[Gregory Szorc [:gps]]

Attachment: reviewboard: allow divergence in test output (bug 1413628);

Mercurial 4.4 is a bit more efficient at not creating intermediate
commits in some scenarios. This results in revision numbers not being
identical in some scenarios.

We add some regular expressions on lines that diverge when run under
4.4.

Review commit: https://reviewboard.mozilla.org/r/195706/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195706/

Comment 14

[Gregory Szorc [:gps]]

Attachment: reviewboard: add new output in Mercurial 4.4 (bug 1413628);

Mercurial 4.4 will print this message. Add it as optional output.

Review commit: https://reviewboard.mozilla.org/r/195708/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195708/

Comment 15

[Gregory Szorc [:gps]]

Attachment: reviewboard: adapt HTTP client code to Mercurial 4.4 (bug 1413628);

The HTTP peer API was overhauled in Mercurial 4.4. Some attributes
were renamed. Teach the code to look for the new attributes.

This eliminates a test failure in test-push-http.t.

Review commit: https://reviewboard.mozilla.org/r/195710/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195710/

Comment 16

[Gregory Szorc [:gps]]

Attachment: reviewboard: adapt discovery repo code to Mercurial 4.4 (bug 1413628);

Mercurial 4.4 renamed some httppeer attributes. Add some code to look
for both names and set the appropriate one.

This fixes a test failure in test-repo-discovery.t.

Review commit: https://reviewboard.mozilla.org/r/195712/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195712/

Comment 17

[Gregory Szorc [:gps]]

Attachment: reviewboard: mark extension as compatible with Mercurial 4.4 (bug 1413628);

With the previous commit, all tests now pass with Mercurial 4.4. We
can mark the extension as compatible.

Review commit: https://reviewboard.mozilla.org/r/195714/diff/#index_header
See other reviews: https://reviewboard.mozilla.org/r/195714/

Comment 18

[Gregory Szorc [:gps]]

Comment on attachment 8924346 [details]
reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195590/diff/1-2/

Comment 19

[Gregory Szorc [:gps]]

Comment on attachment 8924347 [details]
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195592/diff/2-3/

Comment 20

[Gregory Szorc [:gps]]

Comment on attachment 8924348 [details]
reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195594/diff/2-3/

Comment 21

[Gregory Szorc [:gps]]

Comment on attachment 8924349 [details]
reviewboard: add output from Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195596/diff/2-3/

Comment 22

[Gregory Szorc [:gps]]

Comment on attachment 8924350 [details]
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195598/diff/2-3/

Comment 23

[Gregory Szorc [:gps]]

Comment on attachment 8924378 [details]
reviewboard: don't add review id during normal commits (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195656/diff/1-2/

Comment 24

[Gregory Szorc [:gps]]

Comment on attachment 8924351 [details]
reviewboard: register config options (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195600/diff/2-3/

Comment 25

[Gregory Szorc [:gps]]

HTTP based review submission is also borked in 4.4.

Comment 26

[Steven MacLeod [:smacleod]]

Comment on attachment 8924346 [details]
reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);

https://reviewboard.mozilla.org/r/195590/#review201020

Comment 27

[Steven MacLeod [:smacleod]]

Comment on attachment 8924347 [details]
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

https://reviewboard.mozilla.org/r/195592/#review201022

Comment 28

[Steven MacLeod [:smacleod]]

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"?

Comment 29

[Steven MacLeod [:smacleod]]

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.

Comment 30

[Steven MacLeod [:smacleod]]

Comment on attachment 8924350 [details]
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

https://reviewboard.mozilla.org/r/195598/#review201030

Comment 31

[Steven MacLeod [:smacleod]]

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?

Comment 32

[Steven MacLeod [:smacleod]]

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?

Comment 33

[Steven MacLeod [:smacleod]]

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.

Comment 34

[Steven MacLeod [:smacleod]]

Comment on attachment 8924418 [details]
reviewboard: add new output in Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195708/#review201048

Comment 35

[Steven MacLeod [:smacleod]]

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.

Comment 36

[Steven MacLeod [:smacleod]]

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.

Comment 37

[Steven MacLeod [:smacleod]]

Comment on attachment 8924421 [details]
reviewboard: mark extension as compatible with Mercurial 4.4 (bug 1413628);

https://reviewboard.mozilla.org/r/195714/#review201058

Comment 38

[Gregory Szorc [:gps]]

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 39

[Gregory Szorc [:gps]]

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 40

[Gregory Szorc [:gps]]

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.

Comment 41

[Gregory Szorc [:gps]]

Comment on attachment 8924346 [details]
reviewboard: drop support for Mercurial 3.9 and 4.0 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195590/diff/2-3/

Comment 42

[Gregory Szorc [:gps]]

Comment on attachment 8924347 [details]
reviewboard: update backup bundle output for 4.3 compatibility (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195592/diff/3-4/

Comment 43

[Gregory Szorc [:gps]]

Comment on attachment 8924348 [details]
reviewboard: avoid devel warning in Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195594/diff/3-4/

Comment 44

[Gregory Szorc [:gps]]

Comment on attachment 8924349 [details]
reviewboard: add output from Mercurial 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195596/diff/3-4/

Comment 45

[Gregory Szorc [:gps]]

Comment on attachment 8924350 [details]
reviewboard: mark extensions as compatible with 4.3 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195598/diff/3-4/

Comment 46

[Gregory Szorc [:gps]]

Comment on attachment 8924378 [details]
reviewboard: don't add review id during normal commits (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195656/diff/2-3/

Comment 47

[Gregory Szorc [:gps]]

Comment on attachment 8924351 [details]
reviewboard: register config options (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195600/diff/3-4/

Comment 48

[Gregory Szorc [:gps]]

Comment on attachment 8924417 [details]
reviewboard: allow divergence in test output (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195706/diff/1-2/

Comment 49

[Gregory Szorc [:gps]]

Comment on attachment 8924418 [details]
reviewboard: add new output in Mercurial 4.4 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195708/diff/1-2/

Comment 50

[Gregory Szorc [:gps]]

Comment on attachment 8924419 [details]
reviewboard: adapt HTTP client code to Mercurial 4.4 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195710/diff/1-2/

Comment 51

[Gregory Szorc [:gps]]

Comment on attachment 8924420 [details]
reviewboard: adapt discovery repo code to Mercurial 4.4 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195712/diff/1-2/

Comment 52

[Gregory Szorc [:gps]]

Comment on attachment 8924421 [details]
reviewboard: mark extension as compatible with Mercurial 4.4 (bug 1413628);

Review request updated; see interdiff: https://reviewboard.mozilla.org/r/195714/diff/1-2/

Comment 53

[Pulsebot]

Pushed by [e-mail]:
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