1345817 - Tree closure hghook gives confusing error message if "CLOSED TREE" used when treestatus down

Status: NEW

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

Description

[Ed Morley [:emorley]]

Split out of bug 1345584.

If treestatus is working and reports that a tree is closed, users can add "CLOSED TREE" to the commit message to override. In this case, the user is shown:

```
remote: FOO is CLOSED! Reason: BAR
remote: But you included the magic words.  Hope you had permission!
```

Similarly, in the "tree status isn't working so fail closed by default" case, the user can also add "CLOSED TREE" to override. However in this case they get:

```
remote: ************************** ERROR ****************************
remote: Error accessing https://api.pub.build.mozilla.org/treestatus/trees/comm-esr52-seamonkey :
remote: HTTP Error 404: NOT FOUND
remote: Unable to check if the tree is open - treating as if CLOSED.
remote: To push regardless, include "CLOSED TREE" in your push comment.
remote: *************************************************************
```

...even though though the push succeeded.

The error message here:
https://hg.mozilla.org/hgcustom/version-control-tools/file/06154c69e174/hghooks/mozhghooks/treeclosure.py#l56

...needs splitting into two, like occurs here:
https://hg.mozilla.org/hgcustom/version-control-tools/file/default/hghooks/mozhghooks/treeclosure.py#l40

Comment 1

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Attachment: hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

Previously, if the treestatus service was unreachable, the error message displayed mentioned the need to add "CLOSED TREE" to override, even if the commit message already included "CLOSED TREE".

This was confusing to people, so this patch changes the messaging to mention the need for "CLOSED TREE" only if "CLOSED TREE" wasn't part of the commit message. If it was part of the message, it mentions that instead.

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

https://reviewboard.mozilla.org/r/183448/#review188694

I'm not asking you to write tests for this because it is probably a bit of work, but do we not have test coverage for this case?

::: hghooks/mozhghooks/treeclosure.py:63
(Diff revision 1)
> -                   "To push regardless, include \"%s\" in your push comment." % (url, err, magicwords))
>          if repo.changectx('tip').description().find(magicwords) == -1:
> +            printError("%sTo push regardless, include \"%s\" in your push comment." % (error_text, magicwords))
>              return False
> +
> +        print "%sBut you included the magic words. Hope you had permission!" % error_text

Shouldn't this be `printError()`?

Comment 3

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

https://reviewboard.mozilla.org/r/183448/#review189042

::: hghooks/mozhghooks/treeclosure.py:63
(Diff revision 1)
> -                   "To push regardless, include \"%s\" in your push comment." % (url, err, magicwords))
>          if repo.changectx('tip').description().find(magicwords) == -1:
> +            printError("%sTo push regardless, include \"%s\" in your push comment." % (error_text, magicwords))
>              return False
> +
> +        print "%sBut you included the magic words. Hope you had permission!" % error_text

Not if I'm copying what happened in the non-exception case up around line 46.

Comment 4

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

https://reviewboard.mozilla.org/r/183448/#review188694

Yes there are and they're now failing. I'll fix them up tomorrow.

Comment 5

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

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

Comment 6

[Wes Kocher (:KWierso) (Not reading bugmail; email directly if needed)]

Okay, fixed up the syntax error that was causing the tests to fail. With that fixed, the tests all pass with no changes to them, so I guess this case is totally untested?

Comment 7

[Gregory Szorc [:gps]]

(In reply to Wes Kocher (:KWierso) from comment #6)
> Okay, fixed up the syntax error that was causing the tests to fail. With
> that fixed, the tests all pass with no changes to them, so I guess this case
> is totally untested?

It is probably totally untested. Not surprising.

Comment 8

[Gregory Szorc [:gps]]

Comment on attachment 8912065 [details]
hooks: Make the treeclosure hook's network error case make more sense (bug 1345817)

https://reviewboard.mozilla.org/r/183448/#review189184