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 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 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 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.

Pushed by bjones@mozilla.com: https://hg.mozilla.org/hgcustom/version-control-tools/rev/caa7a2f87663 mozautomation: rework backout parser r=gps