Have hg commit hook check Noissue commits have linked review URL

[kzar]

Background

It is considered best practice at eyeo to add a link to the codereview for Noissue commits in the commit message. For an example ​see this commit:

Noissue - Remove devbuild update URL for Firefox builds

Review: https://codereview.adblockplus.org/29526585/

Unfortunately this is extremely easy to forget, I often do. Why not have a Mercurial hook check this for us? (For some context about the existing hg hooks see #3674.)

What to change

Have the server reject Noissue commits without a codereview URL, or some text to indicate there was none. Some examples of valid commit messages:

Noissue - Some short message

Review: https://codereview.adblockplus.org/29526585/

or

Noissue - Some short message

Review: None

or

Noissue - Some short message

Review: None
This is a long mention to further clarify the commit's purpose.

Questions

• What about commits produced by the release automation and other scripts? I guess we have to modify those to include "Review: None"
• Do we all agree about the syntax? How strict do we want to be, for example allowing the "Review: " prefix to be omitted?
• Does everyone agree about this hook being applied for all repositories, or should we have a whitelist?

[greiner]

I agree that enforcement of rules we impose shouldn't harm in this case. However, note that such rules are allowed to differ between project teams but yeah, ideally, we would agree on certain rules across all projects as we do with our coding styles.

[kvas]

I don't think that it would be a good idea to push this change onto all users of hg.adblockplus.org. Making it optional, enabled on a per-repository basis, could be useful though.
As for the syntax, perhaps just checking for "Review:" somewhere in the commit message of any Noissue commit would be enough?

[sergz]

Replying to kvas:

I don't think that it would be a good idea to push this change onto all users of hg.adblockplus.org. Making it optional, enabled on a per-repository basis, could be useful though.

IMO it definitely should be optional, however what about firstly enabling it everywhere and disable when something goes wrong, so we will know such special cases?

As for the syntax, perhaps just checking for "Review:" somewhere in the commit message of any Noissue commit would be enough?

Sounds legit.

[kvas]

I think at this point with most development having moved to GitLab and the remaining few repos to follow soon this ticket can be declared irrelevant. I'm closing it.