Add suport for "Fixes XXXX - ..." commit messages

[fhd]

Background

#3674 adds a hook that posts a comment to issues referenced in commits. For example if you push a commit with the message "Issue 1234 - Hello world" the hook will post a comment containing a link to your commit in issue 1234.

We now want to expand this hook so that commits with a message like "Fixes XXXX - ..." are also supported. They should trigger the same automatic comment link when they are pushed. In addition, if they are pushed (or later merged) into the branch with the "master" bookmark, the referenced issue should be closed. Depending on the issue's module the correct default milestone should also be assigned.

What to change

Adjust the sitescripts/hg/bin/update_issue.py script to support the new "Fixes XXXX - Some change" format of commit messages. Add a "pushkey" hook that tracks the movement of "master" bookmark and closes the issues as the bookmark passes over them.

Add support for a new sitescripts.ini section for configuring the default milestone names (supporting regexp with case-insensitive matching) for the various modules. For example:

[hg_module_milestones]
platform=adblock-plus(-[\d\.]+)?-for-chrome-opera-safari(-next)?
Adblock-Plus-for-Firefox=adblock-plus(-[\d\.]+)?-for-firefox(-next)?
...

1. Adjust the existing hook to also post comment to the referenced issue do the "Fixes..." commits.
2. Add a new hook that detects the movement of the "master" bookmark and detects which commits are now included in it. For each such commit with a "Fixes..." marker in the commit message:
   • If the issue's module matches one of the hg_module_milestones options, the given option's regexp value matches an open milestone, and the issue hasn't already been assigned a milestone, then assign the issue the first milestone that the regexp matches.
   • If the issue isn't already closed mark it as closed.

Notes

• These actions should be performed in as few calls as possible to avoid creating too much email spam.
• The "Issue XXXX - ..." format should still be supported as before.
• The issue should only be assigned a milestone and/or closed when the "master" bookmark passed over that commit. A more precise way to say it would be: when the "master" bookmark is moved, all "Fixes ..." commits that are now ancestors of the commit with the bookmark and were not ancestors of the commit that had the bookmark before should be closed (by ancestor here we mean the commit itself or any of its parent commits or any of their parents commits and so forth).
• The comment with a link to the commit should be posted when the commit is pushed regardless of the branch/bookmark.
• If the issue hasn't already been assigned a milestone and one of the hg_module_milestones options matches the issue's module, but the regexp does not match any open milestones the commit should be rejected. A message explaining that no matching milestones could be found should be displayed.

Also, the filter buglinks defined in sitescripts.templateFilters will have to be adjusted in order to recognize the new commit message format.

[fhd]

Leaving as non-ready since this is probably a bit controversial :)

[kzar]

I think automatically adding a comment about a commit is really useful, but automatically closing the issue might be going too far. Sometimes I commit a change that hasn't finished solving the issue yet. Often an issue might require changes to multiple repositories for example. Often the number of code reviews for an issue matches the number of commits required to fix the issue, but sometimes not.

I think to make this work we would need a special format of commit message that indicates that it fixes the related issue, but I'm not sure if that's worth the trouble.

[sebastian]

I agree with kzar; occasionally it requires more than one commit to fix an issue, automatically closing the issue in that case would be inconvenient and dangerous. On the other hand, having the issue automatically closed is quite convenient most of the time. Having different syntax for the commit message to either indicate whether the change fixes the whole issue, or whether there is more to come, sounds like a good idea.

[greiner]

I can confirm that a ticket might have multiple commits associated with it so closing it after a single commit makes sense for a lot but not all cases.

However, I'm against introducing yet another naming convention if it doesn't add any value. In this case it would merely replace having to remember to close a ticket with having to remember writing a specially crafted commit message.

[philll]

I'm pretty much in favor of having the option to both just reference or close a tickets, by some syntax like "refs #1234" or "fixes #1234". Remembering this should be very trivial and also help standardizing our commit messages.

[andrey]

It would make sense to conform to GitHub syntax: ​https://help.github.com/articles/closing-issues-via-commit-messages/ as it is used as a mirror.

[fhd]

Well we don't use GitHub issues, so there's no really strong case to complying with their format. That said, their logic does make sense at first glance.

But greiner has a point: Is it _much_ better having to explicitly instruct an issue to be closed to the commit message? I guess it can be handy, but OTOH, it could also be error prone.

[kzar]

My vote is to just add one special word for when you want your commit to automatically close an issue e.g "Fixes 1234 - Some message". That's simple to reason about and no one is forced to use it. If you don't want to use the feature, or you don't want the issue to be closed you can carry on using the existing syntax e.g "Issue 1234 - Some message".

(I would personally find that pretty useful as well, at the moment I'm on a fairly slow connection and loading the issue tracker just to mark an issue as closed really feels like a waste of time.)

[trev]

Our workflow requires specifying milestone when closing issues - at least for some modules. If autoclosing is supposed to be useful it will have to set the milestone as well.

[kzar]

Replying to trev:

Our workflow requires specifying milestone when closing issues - at least for some modules. If autoclosing is supposed to be useful it will have to set the milestone as well.

I guess that's possible, generally something like modulename-next is the right milestone. (I guess when your pushing a fix after code-freeze it's down to you to fix the milestone.)

[kzar]

I've updated the issue, hope you don't mind. I'm not precious about any of the details, just trying to move things forward as I'd really love this feature to be added!

[sebastian]

I don't have a strong opinion but FWIW perhaps it's best if we just stick to manually closing issues, rather than introducing inconsistent syntax for commit messages. Also for some modules closed issues needs to be assigned to a milestone, so that you still have to manually update the issue, anyway.

[kzar]

(Well I already made automatic milestone assignment part of the issue.)

[sebastian]

Keep in mind that we already have logic parsing the commit messages (e.g. to link the issue number in the generated changelogs). So if we introduce an incompatible syntax here we also need to adapt that code. However, as indicated before, I find the suggested syntax ("Fixed XXXX" vs "Issue XXXX") somewhat inconsistent anyway.

I don't really have any suggestion I'm convinced of. But an alternative would be closing issues with a directive in the end of the commit message or potentially even on the second line. That way the syntax will remain consistent and backwards compatible at least. I wouldn't insist though.

[fhd]

Adding Vasily, I think this is his kind of stuff.

Keep in mind that we already have logic parsing the commit messages (e.g. to link the issue number in the generated changelogs). So if we introduce an incompatible syntax here we also need to adapt that code.

I presume the code will all go into update_issues.py.

I don't really have any suggestion I'm convinced of. But an alternative would be closing issues with a directive in the end of the commit message or potentially even on the second line. That way the syntax will remain consistent and backwards compatible at least. I wouldn't insist though.

Not sure about the syntax either. Adding some directive to the end of the message seems very technical but then again, this kind of magic should be triggered explicitly. I'm slightly worried that this might be too much magic but OTOH, the logic Dave suggested makes sense to me, and we could avoid common mistakes (like forgetting to set the milestone).

[trev]

Replying to fhd:

Keep in mind that we already have logic parsing the commit messages (e.g. to link the issue number in the generated changelogs). So if we introduce an incompatible syntax here we also need to adapt that code.

I presume the code will all go into update_issues.py.

I think that Sebastian meant changelog generation in createNightlies.py (or to be more precise: buglinks filter in templateFilters.py). This would have to be adapted as well in order to recognize the new format. I added a sentence on that to the issue description.

[kzar]

I'm pretty happy with the issue description as it is, thinking of marking this as ready now. Does anyone have any strong objections?

[sebastian]

As for the syntax, fine with me. However, I think there is a flaw with the milestones. When the release has already been scheduled it's not Adblock-Plus-for-Platform-next anymore, but Adblock-Plus-Version-for-Platform. So I guess we have to retrieve all open milestones and find one that matches either pattern.

[kzar]

Replying to sebastian:

As for the syntax, fine with me. However, I think there is a flaw with the milestones. When the release has already been scheduled it's not Adblock-Plus-for-Platform-next anymore, but Adblock-Plus-Version-for-Platform. So I guess we have to retrieve all open milestones and find one that matches either pattern.

You're right it's not perfect. The way I figure it, pushing after code freeze is pretty rare, so it doesn't matter too much that we have to remember to go back and fix the milestone manually when doing so. Currently we _always_ have to go and set the correct milestone no matter what, so this will still make things easier.

(This way it's also easier to check for mistakes, we can just check the Adblock-Plus-for-Platform-next milestone to make sure it's empty of issues. Any assigned should be fixed. At the moment it's harder to search for issues that we forgot to assign to any milestone.)

[sebastian]

Well, we have to handle that scenario anyway, and there are following options:

1. As I suggested above, correctly identify scheduled milestones. This would be most convenient.
2. Reject the push if any commit message uses "Fixes XXXX" but there isn't a *-next milestone. That way people won't forget to take care manually at least.
3. Just close the issue if we cannot find the corresponding *-next milestone. This is obviously the simplest approach, but it would still require some code to be implemented. If we simply ignore that scenario the hook would potentially error out.

Even if we don't often land changes after the release got scheduled, even more likely it is that people will forget to assign the milestone manually, if they don't have to otherwise. Also I'm not sure how rare that scenario actually is. Sometimes we schedule releases even before feature freeze. And even if we don't there is usually at least one more bug fix before the release.

[kzar]

Ah it didn't occur to me that the -next milestone might not have been created yet. Yea, I agree with you then and would vote for option 2 if option 1 isn't possible. Either way we would avoid mistakes.

[fhd]

I would vote for option three actually, as long as it also prints a warning. Couldn't we just change our release checklists to rename the -next milestones right after tagging? Then we'd always have a -next milestone except while the release is being performed on release day.

Something else I thought of today: What should happen to "Fixes" commits that are pushed to a branch other than master? (We sometimes have that scenario, I think the most recent case was the e10s branch of the adblockplus repository.) IMHO it'd make most sense to ignore those commits, just printing a warning.

[kzar]

Replying to fhd:

I would vote for option three actually, as long as it also prints a warning.

Also sounds good to me, as long as there's a warning then I wouldn't forget to do it.

What should happen to "Fixes" commits that are pushed to a branch other than master? ... IMHO it'd make most sense to ignore those commits, just printing a warning.

Yea, sounds good to me. I didn't think of that either!

[sebastian]

Replying to kzar:

Ah it didn't occur to me that the -next milestone might not have been created yet. Yea, I agree with you then and would vote for option 2 if option 1 isn't possible. Either way we would avoid mistakes.

Sounds good to me. Let's see if we can detect the corresponding milestone without too many hacks. And if that doesn't work resort to option 2.

Replying to fhd:

I would vote for option three actually, as long as it also prints a warning. Couldn't we just change our release checklists to rename the -next milestones right after tagging? Then we'd always have a -next milestone except while the release is being performed on release day.

This would be just a hack to prevent the hook from throwing an error, and IMO the worst option.
If something lands, it will be included with the next release, regardless whether that release has already been scheduled or not. So the issue needs to be moved into the milestone that will be released next, and not into a new milestone. By having the hook automatically assign the issue to the new *-next milestone it will create an incorrect representation of the situation, and likely nobody will notice.

Something else I thought of today: What should happen to "Fixes" commits that are pushed to a branch other than master? (We sometimes have that scenario, I think the most recent case was the e10s branch of the adblockplus repository.) IMHO it'd make most sense to ignore those commits, just printing a warning.

Good point. FWIW, I think it makes most sense to close issues as fixed as soon as they land in the branch, and set the milestone as soon as the branch got merged. Otherwise, it will be quite difficult to work with branches if nobody knows what's already fixed in there and what isn't. But for the matter of progress, let's simply don't touch issues for changes in other branches for now.

[kzar]

OK I've updated the description again to account for those. LGTS?

[sebastian]

How about making the configuration take a list of patterns?

[hg_module_milestones]
platform =
  Adblock-Plus-*-for-Chrome-Opera-Safari
  Adblock-Plus-for-Chrome-Opera-Safari-next

Or perhaps a regexp?

[hg_module_milestones]
platform = Adblock-Plus(-[\d\.]+)?-for-Chrome-Opera-Safari(-next)?

That way we don't need any magic in the hook.

[kzar]

Hmm cool idea, definitely better than having some magic. I think I'd prefer the regexp.

[kvas]

"The issue should only be assigned a milestone and or closed if the commit was pushed to the master branch".

However, we're not using mercurial branches, we're using bookmarks. The difference is that unlike with branches it's not possible to determine whether or not a particular commit ever had some bookmark on it. So there's no such thing as "pushing to the master branch" with our workflow. From the point of view of mercurial what happens is (a) some commits get pushed and then (b) the master bookmark gets moved. This creates two problems: (1) when we see the new commits, the bookmark is still in the old place (2) when it moves we only see the original point and the final point so we need to figure out which commits are now included in the "master".

The first problem can be solved by using a second hook: pushkey (I found it from this ​stackoverflow post and it seems to work solidly in my testing).

The second problem gets a bit tricky if the history is not linear. The only robust (in the face of branching and nonlinear moves of bookmarks, which we cannot exclude) way to determine which commits are to be processed for "Fixes" messages is to look at the commit that had the bookmark before and all its ancestors, compare it with the new commit that has the bookmark and all its ancestors and calculate the difference. This has reasonable performance out of the box and we can also optimise to not go through most of the common ancestors (which is the bulk of the commits) to make it properly efficient.

This would give us the following behaviour: when a branch (in bookmark sense) is merged to the master, all of its commits would be searched for "Fixes" messages and processed. Also if a commit is directly done on the master, it gets processed right away. This seems like what we would want.

I adjusted the description to be more precise about all the things described above.

[kvas]

Make module milestone regexps case-insensitive to be tolerant to small changes in milestone naming.

[kvas]

Change the regexps to lowercase in configuration example to make it more clear that the matching is case insensitive.

[abpbot]

A commit referencing this issue has landed:
​https://hg.adblockplus.org/sitescripts/rev/5a86ec1e5dbc