Issue 7334 - Remove handling of "Fixes XXXX - ..." commit messages

Issue 7334 - Remove handling of "Fixes XXXX - ..." commit messages

Repository: https://hg.adblockplus.org/sitescripts
Base revision: 518c14e605e3

[Vasily Kuznetsov, 2019-04-18 16:04:22]

Hey Matze!

WebExt team started using "Fixes XXXX ..." commit messages to refer to GitLab
tickets but when such changes land in the Mercurial repositories existing hooks
might link them to Trac tickets (which would be a mistake).

After discussing with Sebastian we decided to remove support for "Fixes ..."
messages in Mercurial hooks (there's no great inconvenience from this since the
hook that closes tickets was never active anyway).

It was a bit more work than just rolling back the old changes: some useful
commits have landed on top since, we have changed the coding style and the way
we do testing. I also didn't want to roll back the README, which didn't exist
before the "Fixes ..." support was introduced, so edited it to reflect the
reduced functionality. I ended up with a somewhat modernized version of original
commit by Felix [1].

Let me know what you think.

Cheers,
Vasily

[1]: https://hg.adblockplus.org/sitescripts/rev/83e487d99eda

[mathias, 2019-04-20 19:19:23]

The approach makes sense and the patch-set looks promising to me. Do you want me
to do the actual review as well?

[Vasily Kuznetsov, 2019-04-21 09:26:02]

On 2019/04/20 19:19:23, mathias wrote:
> The approach makes sense and the patch-set looks promising to me. Do you want
me
> to do the actual review as well?

Sure. Especially for these more DevOpsy kinds of things you're a perfect
reviewer IMO.

[mathias, 2019-05-13 12:23:43]

I like this patch-set very much. Not sure if I am missing something, i.e. if I
am a bit out of practice here, but I find it's pretty straightforward :)

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
File sitescripts/hg/bin/update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:30: ISSUE_NUMBER_REGEX =
re.compile(r'\bissue\s+(\d+)\b', re.I)
Shouldn't this one include the separating dash? I.e. `Issue 1337 - Foo bar baz`?
Same goes for the NOISSUE_REGEX below, though that is not part of this patch-set
(yet).

[Vasily Kuznetsov, 2019-05-13 15:14:15]

> I like this patch-set very much.
No wonder, it deletes code! :D

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
File sitescripts/hg/bin/update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:30: ISSUE_NUMBER_REGEX =
re.compile(r'\bissue\s+(\d+)\b', re.I)
On 2019/05/13 12:23:43, mathias wrote:
> Shouldn't this one include the separating dash? I.e. `Issue 1337 - Foo bar
baz`?
> Same goes for the NOISSUE_REGEX below, though that is not part of this
patch-set
> (yet).

Yeah I see what you mean, but I'd rather leave it as it was for now because:

- This commit is supposed to undo the change that added "fixes xxx" notation,
making "issue xxx" more precise is a bit out of scope.
- If we add the space and the dash to the regex, it will break the processing of
multiple references: "Issue 1, Issue 2 - foobar". We could fix it, of course, 
but that's extra work that doesn't seem justified at this point.

[mathias, 2019-05-14 09:32:02]

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/READ...
File sitescripts/hg/README.md (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/READ...
sitescripts/hg/README.md:44: The hook requires `Jinja2` to be importable by the
Python that runs `hg`.
The "to be importable by the Python that runs `hg`" part seems excessive and
could get dropped without replacement.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
File sitescripts/hg/bin/update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:16: """A changegroup (or pretxnchangegroup)
hook for Trac integration.
Why do you insist on naming the type(s) of hook?

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:30: ISSUE_NUMBER_REGEX =
re.compile(r'\bissue\s+(\d+)\b', re.I)
On 2019/05/13 15:14:15, Vasily Kuznetsov wrote:
> On 2019/05/13 12:23:43, mathias wrote:
> > Shouldn't this one include the separating dash? I.e. `Issue 1337 - Foo bar
> baz`?
> > Same goes for the NOISSUE_REGEX below, though that is not part of this
> patch-set
> > (yet).
> 
> Yeah I see what you mean, but I'd rather leave it as it was for now because:
> 
> - This commit is supposed to undo the change that added "fixes xxx" notation,
> making "issue xxx" more precise is a bit out of scope.
> - If we add the space and the dash to the regex, it will break the processing
of
> multiple references: "Issue 1, Issue 2 - foobar". We could fix it, of course, 
> but that's extra work that doesn't seem justified at this point.

Acknowledged.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:95: ui.status('updating
{}\n'.format(issue_url))
This should not be in the `try` block. It should either come before the `try`
and the call to `_post_comment` OR use past tense and be in an `else` block.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:101: changegroup_hook = hook
This should not be necessary at all:

    hg@hg-2:~$ grep changegroup_hook web/*/.hg/hgrc | wc -l
    0

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
File sitescripts/hg/test/test_update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:25: """Very simple mock of a Mercurial
repository."""
If you call the class "MercurialRepositoryMock" there will be no comment needed.
Especially not something that tells the reader how simple it is.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:30: change_mock.hex.return_value =
'000011112222'
This magic number string should be a constant.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:80: assert '000011112222' in comment
This magic number string should be a constant.

[Vasily Kuznetsov, 2019-05-14 10:29:00]

Hi Matze!

Thanks for further comments. I've addressed everything. Let me know if anything
else seems wrong.

Cheers,
Vasily

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/READ...
File sitescripts/hg/README.md (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/READ...
sitescripts/hg/README.md:44: The hook requires `Jinja2` to be importable by the
Python that runs `hg`.
On 2019/05/14 09:32:01, mathias wrote:
> The "to be importable by the Python that runs `hg`" part seems excessive and
> could get dropped without replacement.

Done.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
File sitescripts/hg/bin/update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:16: """A changegroup (or pretxnchangegroup)
hook for Trac integration.
On 2019/05/14 09:32:02, mathias wrote:
> Why do you insist on naming the type(s) of hook?

This is the intended use and I haven't checked if it can work in other contexts,
so it seems like a useful bit of information here.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:95: ui.status('updating
{}\n'.format(issue_url))
On 2019/05/14 09:32:02, mathias wrote:
> This should not be in the `try` block. It should either come before the `try`
> and the call to `_post_comment` OR use past tense and be in an `else` block. 

Makes sense. Done.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/bin/...
sitescripts/hg/bin/update_issues.py:101: changegroup_hook = hook
On 2019/05/14 09:32:01, mathias wrote:
> This should not be necessary at all:
> 
>     hg@hg-2:~$ grep changegroup_hook web/*/.hg/hgrc | wc -l
>     0

👍
Done!

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
File sitescripts/hg/test/test_update_issues.py (right):

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:25: """Very simple mock of a Mercurial
repository."""
On 2019/05/14 09:32:02, mathias wrote:
> If you call the class "MercurialRepositoryMock" there will be no comment
needed.
> Especially not something that tells the reader how simple it is.

Done.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:30: change_mock.hex.return_value =
'000011112222'
On 2019/05/14 09:32:02, mathias wrote:
> This magic number string should be a constant.

Done.

https://codereview.adblockplus.org/30048555/diff/30048556/sitescripts/hg/test...
sitescripts/hg/test/test_update_issues.py:80: assert '000011112222' in comment
On 2019/05/14 09:32:02, mathias wrote:
> This magic number string should be a constant.

Done.

[mathias, 2019-05-14 10:31:14]

LGTM. Thank you!