1071296 - Additional regressions found in hg 3.1.1 and pushlog html

Status: RESOLVED FIXED

(Developer Services :: Mercurial: hg.mozilla.org, defect)

Description

[hwine]

Splitting from bug 1070637, comment #9, as this in not a blocking issue:

Justin Wood (:Callek) reports:
> While [bug 1070637 comment 4]  certainly did fix it for the reported issue,
> there is a followup to do which regressed with this update:
> 
> https://hg.mozilla.org/releases/mozilla-beta/
> pushloghtml?changeset=d179d51ba70c
> 
> Note for example, that the specified cset landed on a branch (easy to verify
> by clicking down to hg's main view), however pushloghtml doesn't actually
> show that its a branch, while it used to.
> 
> Your change basically makes the "old mercurial used repo.branchtags() --
> while in new thats invalid" not raise a real exception that breaks things.
> 
> n-i to bkero and gps to figure out the best path forward here.

Comment 1

[hwine]

moving over correct n-i's

Comment 2

[Justin Wood (:Callek)]

Basically from that bugs Questions aimed at me, this was basically "pushes to branches that are not `default` used to show the branch in the UI of pushloghtml" where post-fix from that bug it only shows the tags applied on a cset.

This is not a blocking thing imo, but is probably good hygene in terms of UI correctness, and is a regression from pre-webhead-upgrade.

I admittedly don't know the code in question, nor do I plan to patch this myself. The mitigation in that bug is also entirely correct for a short-term fix (no traceback with pushlog data is far better than traceback without pushlog data at all.)

Comment 3

[Gregory Szorc [:gps]]

I think fix to pushlog to unbust compatibility with 3.1 (https://hg.mozilla.org/hgcustom/version-control-tools/rev/4fcea92fdba0) inadvertently broke display of branch info.

I'll look into this.

Comment 4

[Gregory Szorc [:gps]]

I just pushed a fix (with tests!) to https://reviewboard-dev.allizom.org/r/179/. ReviewBoard is in a funky state right now, so it didn't notify this bug.

Comment 5

[Gregory Szorc [:gps]]

Justin: if you wouldn't mind taking a look at the diff and verifying things look sane.

Basically, we should be inserting the branch name next to every commit on a branch that isn't "default."

Comment 6

[Justin Wood (:Callek)]

(In reply to Gregory Szorc [:gps] from comment #5)
> Justin: if you wouldn't mind taking a look at the diff and verifying things
> look sane.
> 
> Basically, we should be inserting the branch name next to every commit on a
> branch that isn't "default."

I always sigh when we drop old compat for merely the sake of dropping old compat. But since to my knowledge we're the only ones using this I won't block on that.

I also don't understand our test code, so unsure if that is right or not.

That said, assuming branchmap does what you claim I'm happy to stamp+ this.

Comment 7

[Gregory Szorc [:gps]]

Where did I drop compat in the proposed patch?

Comment 8

[Justin Wood (:Callek)]

(In reply to Gregory Szorc [:gps] from comment #7)
> Where did I drop compat in the proposed patch?

"This patch breaks compatibility with Mercurial < 2.9. That should
be fine."

+ the entire removal of `if repo.branchtags().get(branch) == ctx.node():` and similar sections

Comment 9

[Gregory Szorc [:gps]]

We don't care about 2.5.4 for web hosting any more. We'll soon have the SSH server running 3.1+ as well. The cost to continue to support 2.5.4 and similarly old releases is too high.

Comment 10

[Gregory Szorc [:gps]]

Attachment: MozReview Request: bz://1071296/gps

Comment 11

[Gregory Szorc [:gps]]

/r/117 - pushlog: add better tests for HTML output
/r/119 - pushlog: unbust branch display (bug 1071296)

Pull down these commits:

hg pull review -r 3f6af9007ed5f2d8268bd611bc335f162cc6a073

Comment 12

[Gregory Szorc [:gps]]

/r/117 - pushlog: add better tests for HTML output
/r/119 - pushlog: unbust branch display (bug 1071296)

Pull down these commits:

hg pull review -r 24c191c5f1dca0482e07c30a477c896629999d98

Comment 13

[Gregory Szorc [:gps]]

https://reviewboard.mozilla.org/r/115/#review217

I'm working on some pushlog fixes to reduce excessive bandwidth usage. I'd like the new pushlog test infrastructure in place so I can adequately test things. Please review quickly.

Comment 14

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/115/#review273

::: testing/http-request.py
(Diff revision 2)
> +import httplib

from __future__ import print_function

::: testing/http-request.py
(Diff revision 2)
> +        help='Display only headers in this list')

comma-separated list

::: testing/http-request.py
(Diff revision 2)
> +from optparse import OptionParser

Can't you use argparse? Or does that need to run on python < 2.7?

::: testing/xmldump.py
(Diff revision 2)
> +    if action == 'dump-element-with-class':

You could use xpath, and you wouldn't need "actions" with random params.

::: hgext/pushlog-legacy/pushlog-feed.py
(Diff revision 2)
> -        if hasattr(repo, 'branchtags'):
> +        if repo.branchmap().get(branch) == ctx.node():

For the record, since I looked up, branchmap() was added in mercurial 1.3.

Mostly nits, but I'd like to read answers before clicking "ship ip".

Comment 15

[Ben Kero [:bkero]]

https://reviewboard.mozilla.org/r/117/#review461

lgtm

Comment 16

[Ben Kero [:bkero]]

https://reviewboard.mozilla.org/r/119/#review469

Comment 17

[Gregory Szorc [:gps]]

/r/117 - pushlog: add better tests for HTML output
/r/119 - pushlog: unbust branch display (bug 1071296)

Pull down these commits:

hg pull review -r 268205876768205a0337b3b2fce81be1f3e4a33a

Comment 18

[Mike Hommey [:glandium]]

https://reviewboard.mozilla.org/r/115/#review529

This review would have been much easier if running tests in version-control-tools didn't insist on requiring docker even when trying to run tests that don't need it. Especially when on top of using docker, it downloads 1.2G+ of data (I can't tell exactly how much because that's when it failed because of insufficient disk space).

::: testing/http-request.py
(Diff revisions 2 - 3)
> -        help='Display only headers in this list')
> +        help='Display only headers in this list. Values can be comma delimited.')

s/can/must/

Comment 19

[Gregory Szorc [:gps]]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/7f3fb8d53510

Comment 20

[Gregory Szorc [:gps]]

Deployed.

URLs like https://hg.mozilla.org/releases/mozilla-release/pushloghtml?changeset=9f301ac8f453 now display the branches again.

Comment 22

[Gregory Szorc [:gps]]

Attachment: MozReview Request: pushlog: add better tests for HTML output

Comment 23

[Gregory Szorc [:gps]]

Attachment: MozReview Request: pushlog: unbust branch display (bug 1071296)