Closed Bug 1366667 Opened 7 years ago Closed 7 years ago

update commit hook validation to validate backout messages

Categories

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

Product:
Developer Services
Component:
Mercurial: hg.mozilla.org
Type:
enhancement
Priority:
Not set
Severity:
normal

Tracking

(Not tracked)

Status:
RESOLVED FIXED

People

(Reporter: glob, Assigned: glob)

References

Details

Attachments

(4 files)

hghooks: fix pep8/style issues in commit-message.py (bug 1366667)
59 bytes, text/x-review-board-request
gps
: review+
Details
hghooks: remove b2g users from commit-message hook (bug 1366667)
59 bytes, text/x-review-board-request
gps
: review+
Details
hghooks: add mozautomation to venv (bug 1366667)
59 bytes, text/x-review-board-request
gps
: review+
Details
hghooks: validate backouts on vendored paths (bug 1366667)
59 bytes, text/x-review-board-request
gps
: review+
Details 
      No description provided.
i'm going to initially scope this to just servo/, because that's where it's important.

the existing commit hook's validation is very relaxed, allowing `revert` (and variations) in addition to `backout` (and variations):

> goodMessage = [re.compile(x, re.I) for x in [
>     r'bug [0-9]+',
>     r'no bug',
>     r'^(back(ing|ed)?\s+out|backout).*(\s+|\:)[0-9a-f]{12}',
>     r'^(revert(ed|ing)?).*(\s+|\:)[0-9a-f]{12}',
>     r'^add(ed|ing)? tag'
> ]]

this _looks like_ it will reject backouts that don't contain a node's sha, however as long as a backout references a bug number it will be allowed (the first regex in the list).

end result is we have a ton of backouts that look like:
> Revert "Bug 1349699 - Close GPU channel unconditionally (r=dvander)"

a lot, but not all, of these contain the backed out revision somewhere in the commit description.
Comment on attachment 8870823 [details]
hghooks: fix pep8/style issues in commit-message.py (bug 1366667)

https://reviewboard.mozilla.org/r/142390/#review146126

::: hghooks/mozhghooks/commit-message.py:56
(Diff revision 1)
>      return True
>  
>  
> -def isGoodMessage(c):
> +def is_good_message(c):
>      def message(fmt):
> -        print "\n\n************************** ERROR ****************************"
> +        print(

Strictly speaking we should use `ui.write()` in Mercurial code. But it isn't important to this change.

::: hghooks/mozhghooks/commit-message.py:99
(Diff revision 1)
> -    if dlower.startswith("merge") or dlower.startswith("merging") or dlower.startswith("automated merge"):
> +    if dlower.startswith("merge") or dlower.startswith("merging") \
> +            or dlower.startswith("automated merge"):

startswith() accepts a tuple, which would avoid the line continuation (I hate line continuations). I trust you to fix in flight or drop at your discretion.
Attachment #8870823 - Flags: review?(gps) → review+
Comment on attachment 8870824 [details]
hghooks: remove b2g users from commit-message hook (bug 1366667)

https://reviewboard.mozilla.org/r/142392/#review146128
Attachment #8870824 - Flags: review?(gps) → review+
Comment on attachment 8870825 [details]
hghooks: add mozautomation to venv (bug 1366667)

https://reviewboard.mozilla.org/r/142394/#review146130
Attachment #8870825 - Flags: review?(gps) → review+
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

https://reviewboard.mozilla.org/r/142396/#review146132

Aside from errant output in a test, this looks good!

A good follow-up would be to print a warning message when an invalid backout message is seen on non-vendored changesets. That should help us flush out issues with the parser and tools producing bad messages.

::: hghooks/tests/test-commit-messages.t:622
(Diff revision 1)
> +  added 1 changesets with 1 changes to 1 files
> +  
> +  
> +  ************************** ERROR ****************************
> +  Rev 169f75837913 has malformed backout message.
> +  test

Where does "test" come from?!
Attachment #8870826 - Flags: review?(gps) → review+
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

https://reviewboard.mozilla.org/r/142396/#review146132

> Where does "test" come from?!

it's ctx.user(), which is `test` in the testsuite.
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

i made some logic changes to add the warning; re-requesting review.
Attachment #8870826 - Flags: review+ → review?(gps)
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

https://reviewboard.mozilla.org/r/142396/#review146638

I like the new behavior. The implementation needs a few refinements. Teaches you to turn a follow-up suggestion into scope bloat :)

::: hghooks/mozhghooks/commit-message.py:71
(Diff revision 2)
> +            if not commitparser.parse_backouts(desc, strict=True):
> +                raise Exception('Rev {rev} has malformed backout message.')
> +            nodes, bugs = commitparser.parse_backouts(desc, strict=True)
> +            if not nodes:
> +                raise Exception('Rev {rev} is missing backed out revisions.')
> +        except Exception as e:

This assumes the exception is due to strict mode violations as opposed to some other failure raising an Exception.

Can we raise and catch an explicit type to avoid the potential for failure?

::: hghooks/mozhghooks/commit-message.py:76
(Diff revision 2)
> +        except Exception as e:
> +            # Reject invalid backout messages on vendored paths, warn otherwise.
> -    if is_vendor_ctx(c):
> +            if is_vendor_ctx(c):
> +                message(str(e))
> +                return False
> +            ui.write('Warning: %s\n' % str(e).format(rev=hex(c.node())[:12]))

nit: we don't capitalize the first word of messages, following Mercurial's convention.
Attachment #8870826 - Flags: review?(gps) → review-
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

https://reviewboard.mozilla.org/r/142396/#review146638

> nit: we don't capitalize the first word of messages, following Mercurial's convention.

as much as i like all-lowercase literally every other message generated by this hook has first word capitalisation.  i'd rather be consistent with the rest of the hook rather than mercurial.
Comment on attachment 8870826 [details]
hghooks: validate backouts on vendored paths (bug 1366667)

https://reviewboard.mozilla.org/r/142396/#review148830

::: hghooks/mozhghooks/commit-message.py:66
(Diff revision 3)
> +            if not commitparser.parse_backouts(desc, strict=True):
> +                raise ValueError('Rev {rev} has malformed backout message.')
> +            nodes, bugs = commitparser.parse_backouts(desc, strict=True)

I'm not a fan of the redundant calls here. But it shouldn't matter too much in practice.

::: hghooks/mozhghooks/commit-message.py:76
(Diff revision 3)
> +        except ValueError as e:
> +            # Reject invalid backout messages on vendored paths, warn otherwise.
> -    if is_vendor_ctx(c):
> +            if is_vendor_ctx(c):
> +                message(str(e))
> +                return False
> +            ui.write('Warning: %s\n' % str(e).format(rev=hex(c.node())[:12]))

c.hex()[:12]
Attachment #8870826 - Flags: review?(gps) → review+
Pushed by bjones@mozilla.com:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/ba5e85722a8f
hghooks: fix pep8/style issues in commit-message.py r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/31ad63af29c1
hghooks: remove b2g users from commit-message hook r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/b1b2649ec1e2
hghooks: add mozautomation to venv r=gps
https://hg.mozilla.org/hgcustom/version-control-tools/rev/bbc9bc7d1b5e
hghooks: validate backouts on vendored paths r=gps
Status: NEW → RESOLVED
Closed: 7 years ago
Resolution: --- → FIXED
You need to log in before you can comment on or make changes to this bug.

Attachment

General

Creator:
Created:
Updated:
Size: