1271494 - colourise the "review requests lack reviewers" message when pushing

Status: RESOLVED FIXED

(MozReview Graveyard :: General, defect)

Description

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

A kind of follow-up to bug 1164127: If the commit message does not contain anything like 'r?nick', there is no warning (that I can see).

For example, in bug 1268434 I mistakenly wrote 'c?cpearce'. I didn't notice the issue.
I "reminded" my reviewer about the review, so that's why he got to it.

And the worst part is that I was even allowed to autoland it, and it ended as 'c?cpearce r=cpearce' in the landed patch.

Comment 1

[:glob ✱]

(In reply to Gerald Squelart [:gerald] from comment #0)
> A kind of follow-up to bug 1164127: If the commit message does not contain
> anything like 'r?nick', there is no warning (that I can see).

there's a warning when you push to mozreivew:

> pushing to ssh://10.211.55.158:54994/test-repo
> searching for changes
> no changes found
> submitting 1 changesets for review
> 
> changeset:  4:078ae2366aba
> summary:    bug 1 c?level1
> review:     http://10.211.55.158:55556/r/4 (draft)
> 
> review id:  bz://1/dummy
> review url: http://10.211.55.158:55556/r/1 (draft)
> (review requests lack reviewers; visit review url to assign reviewers)
> 
> publish these review requests now (Yn)?

note the line "review requests lack reviewers".

> And the worst part is that I was even allowed to autoland it, and it ended
> as 'c?cpearce r=cpearce' in the landed patch.

the autoland dialog displays what it will rewrite the commit message as prior for exactly this reason.
the follow text is displayed at the top of the autoland dialog:

> About to land the following commits to ssh://hg.mozilla.org/hgcustom/version-control-tools.
> Please confirm these commit descriptions are correct before landing.
> If corrections are required please amend the commit message and try again.



given the "review requests lack reviewers" message was missed, i think the right thing here is to colourise it to make it stand out.

Comment 2

[:glob ✱]

Attachment: MozReview Request: hgext: make "review requests lack reviewers" more noticable (bug 1271494); r?gps

In order to make it stand out, reclassify the "review requests lack reviewers"
message as a warning, and ensure a colour for warnings is defined.

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

Comment 3

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Byron Jones ‹:glob› from comment #1)
> note the line "review requests lack reviewers".
It's very possible I missed the push comment.

(In reply to Byron Jones ‹:glob› from comment #2)
> In order to make it stand out, reclassify the "review requests lack
> reviewers"
> message as a warning, and ensure a colour for warnings is defined.
Colours may not show on some terminals, and it'd still be easy to miss it when we're pushing the Nth review on autopilot (type 'hg push review' and press enter twice).

Are there real-world scenarios where reviews would be requested without a reviewer attached?

I personally would be fine if that was a strong error/failure, i.e., it prevents pushing altogether.
At least I think it should be the default, with some optional way to force the push without reviewer.

Comment 4

[Gregory Szorc [:gps]]

Yes, there are real world scenarios for pushing without a reviewer. For example, someone may wish to push to use autoland to do the Try push. Or they may want to assign a reviewer later while having the code published somewhere.

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8750826 [details]
MozReview Request: hgext: make "review requests lack reviewers" more noticable (bug 1271494); r?gps

https://reviewboard.mozilla.org/r/51663/#review48605

::: hgext/reviewboard/client.py:486
(Diff revision 1)
> +        from hgext.color import _styles
> +        if 'ui.warning' not in _styles:
> +            _styles['ui.warning'] = 'red'

This concerns me because upstream keeps talking about moving color into core. But we have continuous integration, so it should all be fine.

There is an API to find loaded extensions and return their module name. You should use that.

try:
   color = extensions.find('color')
   ...
except Exception:

(It will raise ValueError if the extension isn't loaded IIRC.)

Having suggested that, I'm tempted to set ui.warning in `mach mercurial-setup` instead of doing it the hacky way here.

::: hgext/reviewboard/client.py:489
(Diff revision 1)
> +    # Ensure a colour for ui.warning is defined.
> +    try:
> +        from hgext.color import _styles
> +        if 'ui.warning' not in _styles:
> +            _styles['ui.warning'] = 'red'
> +    except:

Bareword `except` in Python is evil because it catches the branch of exception that don't derive from `Exception`, notable `KeyboardInterrupt` and `SystemExit`. This means if a ctrl+c occurs during this block, it is ignored. That's bad.

You should use `except Exception` to catch everything except the special exceptions.

Comment 6

[Gerald Squelart (he/him) (not at Mozilla since 2022-09-15)]

(In reply to Gregory Szorc [:gps] from comment #4)
> Yes, there are real world scenarios for pushing without a reviewer. For
> example, someone may wish to push to use autoland to do the Try push. Or
> they may want to assign a reviewer later while having the code published
> somewhere.

Makes sense, thank you.

I'd still be interested in having stronger barriers to catch my mistakes in my day-to-day work, whether it's opt-in or opt-out, and with ways around these for exceptions of course.
If you think there's no chance for that, I'll just drop it.
Otherwise I'm happy to open a new bug, or first chat about it in some forum if more appropriate...

Comment 7

[Gregory Szorc [:gps]]

We're trending to having some kind of dialog before you submit to catch obvious issues (failed lint, no reviewer, to suggest a reviewer, squash commits, etc). There's a lot we can cram into such a UI.

Comment 8

[:glob ✱]

https://reviewboard.mozilla.org/r/51663/#review48605

> This concerns me because upstream keeps talking about moving color into core. But we have continuous integration, so it should all be fine.
> 
> There is an API to find loaded extensions and return their module name. You should use that.
> 
> try:
>    color = extensions.find('color')
>    ...
> except Exception:
> 
> (It will raise ValueError if the extension isn't loaded IIRC.)
> 
> Having suggested that, I'm tempted to set ui.warning in `mach mercurial-setup` instead of doing it the hacky way here.

i did consider adding it to mercurial-setup, but i wanted to ensure it worked for all review board consumers (eg. mercurial-setup isn't a pre-requsite for hacking on mozreview).

Comment 9

[:glob ✱]

Comment on attachment 8750826 [details]
MozReview Request: hgext: make "review requests lack reviewers" more noticable (bug 1271494); r?gps

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

Comment 10

[Gregory Szorc [:gps]]

Comment on attachment 8750826 [details]
MozReview Request: hgext: make "review requests lack reviewers" more noticable (bug 1271494); r?gps

https://reviewboard.mozilla.org/r/51663/#review49267

Comment 11

[:glob ✱]

https://hg.mozilla.org/hgcustom/version-control-tools/rev/50521de1cae3