1365860 - rework backout parsing

Status: RESOLVED FIXED

(Developer Services :: General, task)

Description

[:glob ✱]

with bug 1365181 needing to parse backouts we need to rework the backout parser slightly:
- add support for more commonly used forms
- tighten up parsing of some other forms

Comment 1

[:glob ✱]

Attachment: mozautomation: rework backout parser (bug 1365860)

Rework the backout parser slightly to both expand the phrases recognised by the
parser, and require N nodes are provided when the description says that N nodes
were backed out.

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

Comment 2

[Gregory Szorc [:gps]]

Comment on attachment 8868907 [details]
mozautomation: rework backout parser (bug 1365860)

https://reviewboard.mozilla.org/r/140540/#review144322

This seems like a pretty significant improvement! I couldn't have done the regexp work any better myself, even at my hayday when I had Perl and PCRE fully paged in :)

r- for some usability issues.

I won't hold you to it because it is scope bloat, but if you can find some time to write tests covering "backout" appearing outside the summary line, it would be appreciated.

::: pylib/mozautomation/mozautomation/commitparser.py:195
(Diff revision 1)
> -def parse_backouts(s):
> +def is_backout(commit_desc):
> +    """Returns True if the commit description appears to contain a backout."""
> +    return BACKOUT_KEYWORD_RE.match(commit_desc) is not None

Can we get a clearer docstring (or underscore prefix the name) to indicate where this is suitable? Since it looks at the entire commit message, I worry about "backout" in the message body and not the summary line giving false positives. (I imagine it is relatively common to use BACKOUT_KEYWORD_RE to describe a backout).

::: pylib/mozautomation/mozautomation/commitparser.py:228
(Diff revision 1)
> -                word = word.strip(',')
> -                if SHORT_RE.match(word):
> +        if expected != len(nodes):
> +            return None

I understand wanting to have strict conformance for validation reasons (e.g. in commit hooks). But I don't want e.g. backouts not being detected in revsets, hg.mo UI, etc because of a superficial error in the message. Can we make this behavior conditional?

Comment 3

[:glob ✱]

Comment on attachment 8868907 [details]
mozautomation: rework backout parser (bug 1365860)

https://reviewboard.mozilla.org/r/140540/#review144322

heh, thanks - i _do_ like regex'ing and globbing.

will expand the tests.

> Can we get a clearer docstring (or underscore prefix the name) to indicate where this is suitable? Since it looks at the entire commit message, I worry about "backout" in the message body and not the summary line giving false positives. (I imagine it is relatively common to use BACKOUT_KEYWORD_RE to describe a backout).

i'll update the docstring.

it only looks at the first line because it's anchored to the start of the string (and ri.M isn't set).

it's split out an public because i plan on using this in a commit hook to reject malformed backout messages.
eg.

  if is_backout(commit_desc) and not parse_backout(commit_desc):
    # reject

Comment 4

[:glob ✱]

Comment on attachment 8868907 [details]
mozautomation: rework backout parser (bug 1365860)

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

Comment 5

[Gregory Szorc [:gps]]

Comment on attachment 8868907 [details]
mozautomation: rework backout parser (bug 1365860)

https://reviewboard.mozilla.org/r/140540/#review144438

Ahhh, this is much, much better! And your plan for the commit message hook change looks good as well.

Comment 6

[Pulsebot]

Pushed by [e-mail]:
https://hg.mozilla.org/hgcustom/version-control-tools/rev/caa7a2f87663
mozautomation: rework backout parser r=gps