Background

  1. #493074: Back-link to the commit as a comment on the related issue. introduced automated comments on issues upon a commit that references an issue ID.

Problem

  1. After committing a change, maintainers have to manually go back to the issue and to change its status to fixed.
  2. Sometimes maintainers forget to do this.

Goal

  1. Support powerful/smart commit messages to automatically change the status of an issue.

Details

  1. Powerful/smart commit messages is a very common feature in code collaboration systems.
  2. If a commit message contains the keyword "fix" and is directly followed by an issue ID, then the corresponding issue is automatically marked as fixed.
  3. This is implemented by Unfuddle, JIRA, and GitHub, whereas GitHub's implementation is the best in terms of DX.

Proposed solution

  1. Enhance the new back-link code to additionally scan for powerful/smart commit message commands.

    Only an explicit command that uses the powerful/smart commit message keyword and syntax triggers the behavior; i.e., explicit opt-in.

  2. Use the regular expression /(?:fix(es|ed|ing)?)\s+#(\d+)/i

    (Support all tenses of "fix".)

  3. Use preg_match_all() to allow to fix multiple issues with a single commit.

  4. Update the recommended commit message syntax:

    - Issue #123 by foo, bar: Fixed Baz breaks installer.
    + Fixed #123 by foo, bar: Baz breaks installer.
    

    Note: This is just the recommendation. As mentioned above, alternate tenses will be supported.

  5. Deploy the (already prepared) change to Dreditor.

CommentFileSizeAuthor
#18 537fef20c38aa5474d911275.png26.1 KBmarkhalliwell
Support from Acquia helps fund testing for Drupal Acquia logo

Comments

Kartagis’s picture

Issue summary: View changes
YesCT’s picture

Title: Having system bot automatically Fix issues » Have system bot automatically Fix issues when committed with "Fixes .." keyword in commit message like github
Issue summary: View changes

.

sun’s picture

Title: Have system bot automatically Fix issues when committed with "Fixes .." keyword in commit message like github » Automatically fix issues when commit message contains "Fixes .." keyword (powerful/smart commit messages)

This concept is known as "powerful commit messages" or "smart commit messages" — a mechanism to "auto-close issues through commits."

It is implemented by Unfuddle, JIRA, and GitHub, whereas GitHub has the best implementation in terms of DX, as it supports many alternative terms (Fix, Fixes, Fixed, Resolves, Resolved, Closes, Closed, etc).

Note that our current recommended commit message syntax is not compatible with this concept. We'd have to change it from

Issue #123 by foo, bar: Fixed Baz breaks installer.

to

Fix #123 by foo, bar: Baz breaks installer.
Fixes #123 by foo, bar: Baz breaks installer.
Fixed #123 by foo, bar: Baz breaks installer.

Adjusting Dreditor for that is trivial - happy to do so once this issue has consensus.

Kartagis’s picture

How about Issue #123 by foo fixed as well?

sun’s picture

@Kartagis: That would diverge from the informal standard of powerful commit messages as implemented everywhere else. The syntax is simple:

<keyword> <[#]issueID>

i.e., the issue ID to fix/close is immediately preceded by the keyword, delimited by [white-]space.

The idea is that a keyword that just happens to appear anywhere else in a commit message does not trigger the automatic behavior - i.e., if you just want to commit a change without changing the issue's status (for many possible reasons), then you simply ensure that your commit message does not use the "magic" syntax. Instead, you use "Prepare #123 …" or "Issue #123: Fixed Baz breaks installer."

marvil07’s picture

It is funny, yesterday's night I was talking about this with beejeebus, and he started some code for that.

This is naturally possible, and I'm willing to add it to upstream, but not sure if d.o would really like to use it.

If we got that route, I would suggest to do it with an strict syntax, so it does not happen without intention.
Also, as sun mentions, drupal.org commit message standard would need to change (but looking there it seems like start with "Fixed" is still possible), and I would rather avoid that.

In some issues, people use to do incremental additions in different commits(or follow-ups for other case), so IMHO differentiate between a simple commit reference and a fixing commit reference is important.

I guess a good way to do this(as another alternative besides the one mentioned by sun in comment 5) un-intrusively would be via another line, i.e.

Issue #1948566 by joachim: Document batch API callbacks using new standard

Fixes #1948566.

It's a little more work, but I guess a good trade off to avoid changing commit message standard(a lot of community time to catch up) and unintentional issue closing.

FTR, the regex could be something like ^Fixes #(\d+)$ (again strict to avoid unintentional changes).

Other open question would be if include it on one of the already added event processor plugins or via a new one.

markhalliwell’s picture

sun’s picture

@marvil07: I don't see a reason for why we cannot change the recommended commit message syntax/template. The PCRE simply changes into:

/(?:Fix|Fixe[s|d])\s+(#\d+)/i

This can appear anywhere in the commit message. If you want to continue to use the current commit message template, then you can put it into a separate line. But it's just reasonable to assume/predict that most people will switch to a commit message syntax/template that simply replaces the (noisy) "Issue" prefix with an "Fix"/"Fixed" prefix.

Note that the current commit message syntax/template is a recommendation, it is NOT a standard. Therefore, given a consensus based on a very reasonable incentive (like this), it can be changed easily (and quickly). The real meat of the recommendation is not affected by this change anyway, since the text before the issue #ID is ignored by all commit message parser implementations. That is, because in particular @Dries used a different prefix than everyone else for a very long time (a single dash/hyphen instead of "Issue").

Using the common standard syntax for powerful/smart commit messages additionally supplies a clean "migration" path. To actively use it, you need to change your commit message syntax. That prevents issue status changes that were not intended.

Last but not least, the above PCRE is meant to be executed via preg_match_all(); i.e., it should be possible to fix multiple issues through a single commit. That is natively supported by all implementations I'm aware of.

In any case, let's please not diverge from the usual implementation of powerful/smart commit messages. There's absolutely no reason/incentive for re-inventing this wheel with yet another Drupalism.

Mixologic’s picture

Is there a corresponding action that should happen on an issue for the verb "Reverts" ?

sun’s picture

@Mixologic: None of the implementations I'm aware of implement support for a "Revert" keyword.

It is reasonable to not support reverts, because the change (as well as the keyword) is about a particular commit and code change only. The effective resulting operation can be multi-fold — the originating issue may be re-opened (to active? or to needs work?), or the change was completely bogus and a bad idea (no status change), or the originating issue might be superseded by a new issue (no status change).

In other words, the powerful/smart commit message keyword would not be "revert", but instead "Revert to active #123 …" or "Needs work #123 …" etc, which (1) is too complex, (2) would be very rarely used (since git automatically creates a revert commit message), and thus (3) rather pointless to implement/support.

sun’s picture

Title: Automatically fix issues when commit message contains "Fixes .." keyword (powerful/smart commit messages) » Automatically fix issues when commit message contains "Fixes …" keyword (powerful/smart commit messages)

Started a PR for Dreditor to investigate necessary changes to its automated commit message suggestion in parallel:

https://github.com/dreditor/dreditor/pull/146

markhalliwell’s picture

Also FWIW, if anyone would like to test this change you can now download packaged PRs at https://dreditor.org/development/build

markhalliwell’s picture

RE: "Recommended commit message" (aka d.o standard/policy) and tools like Dreditor and Drush IQ that follow this. I think the generated prefix should focus on the verb tense ("Fixed #N" or "Fixes #N") as "Fix #N" implies that there is still needed action, not actions already taken (the commit that is being made).

Regardless of that, I still think this project should be able to handle all tenses of "fix" (expanding on #8):

/(?:fix(es|ed|ing)?)\s+#(\d+)/i
star-szr’s picture

sun’s picture

Title: Automatically fix issues when commit message contains "Fixes …" keyword (powerful/smart commit messages) » Automatically fix issues when commit message contains "Fixed …" keyword (powerful/smart commit messages)
Component: Miscellaneous » Code
Issue summary: View changes
Related issues: +#493074: Back-link to the commit as a comment on the related issue.

Rewrote issue summary.

@marvil07: You mentioned that this would be naturally (= easily?) possible…? :-)


Regarding imperative tense vs. past tense, I think we should defer that discussion to later and first get some working code in place. Nevertheless, I'm ambivalent:

I agree with @Cottser that every character matters, since commits are truncated to ~76 chars in git logs. OTOH, the prefix is not actually guilty; normally it's the "by" credits that cause the actual issue title to get hidden away, so it's rather pointless to try to "save" two chars in the prefix.

Thanks for those links. The best answer of all was definitely the one by Matt Quigley, which focuses on (1) consistency and (2) the driving question of whether your repository is truly distributed and commits in your repository are typically cherry-picked (as in e.g. the Linux kernel) or whether your repository is centralized and thus its history rather acts as a journal.

The latter is the case for Drupal (partially even enforced by architectural constructs like sequentially numbered hook_update_N() functions), and in terms of consistency, we've always used the past tense, so I'd simply recommend to keep on doing that and use "Fixed".

sun’s picture

Heh. I just adjusted the prefix to be journal-style by default in Dreditor, which revealed:

-Issue #2269385 by sun: Fixed DependencyInjection YamlFileLoader is out of date.
+Fixed #2269385 by sun: DependencyInjection YamlFileLoader is out of date.

Based on grammar, the past tense doesn't work so well with the "by" line… ;-)

-Fixed [thing] by [someone]: [subject].
+Fix [thing] by [someone]: [subject].

Oh my. I definitely did not and do not want to turn that silly prefix into a bikeshed. :-/

So just pointing it out. Personally, I don't care at all whether it's "Fix" or "Fixed". (I'd only disagree with "Fixes")


Still looking forward to @marvil07's and/or @beejeebus's feedback on how hard/simple it is to actually implement this on the server-side.

Anonymous’s picture

i'm willing to hack on this at DrupalCon Austin (and possibly before on the way there).

i'll just need some pointers from marvil07 re. getting a working dev environment.

i have a vague idea of how to do this, but will need pointers on some of the details.

markhalliwell’s picture

Agreed, this is starting to bikeshed. Let's not get hung up on verbiage/grammar. It's really tied to the status of the issue being changed which is "Fixed":

sun’s picture

Excellent point, @Mark Carver. The parity with the "Fixed" issue status trumps all other arguments. Case closed. :-)

marvil07’s picture

Last but not least, the above PCRE is meant to be executed via preg_match_all(); i.e., it should be possible to fix multiple issues through a single commit. That is natively supported by all implementations I'm aware of.

Let's start with one issue fixed at a time. I do see that case is usual(do not have data to prove that now). In any case those could be parameters of the event processor plugin.

Not sure about how many people uses "Fixed" prefix now, but let's assume unintentional issue changes for those cases are ok.

Thanks for the clarification on recommendation vs. standard.

On verb tense, it would matter to dreditor, I agree this should allow all fix tenses, but again reges is a configuration option in the plugin.

Still looking forward to @marvil07's and/or @beejeebus's feedback on how hard/simple it is to actually implement this on the server-side.

What's required here: a new versioncontrol event processor plugin ala "backlink as a coment" or "keep a list of commits related with an issue"(both inside versioncontrol_project already).

@beejeebus, feel free to ping me in irc(when I'm not in irc away status, aka not there).

As mentioned before, other open question would be if include it on one of the already added event processor plugins or via a new one, for now a separate one sounds like a better idea.

sun’s picture

@marvil07: Will a separate plugin cause separate "System message" comments? One for the commit back-link, and one for the issue status change?

Ideally, there'd be just a single issue comment/update that performs both operations (if applicable) in one go, IMO.

markhalliwell’s picture

markhalliwell’s picture

marvil07’s picture

@sun: yes, a new plugin will do a separate node update, so it will be a new comment.
It is possible to avoid that, but we will need to extend the functionality of the already added plugin instead, which should be ok too, as long as both things are independent from each other(i.e. can use one of them without the other and vice-versa), but sounds a little strange from the architecture perspective(plugins are about splitting individual functionality pieces to then be set up, in this case more than one at a time). Maybe we need a way to let a plugin call another one to solve this in a general way, not really sure but leaving a note to avoid forgetting about it.

t0xicCode’s picture

Any work on this front lately?

gonssal’s picture

This would make module maintainership a bit less painful, and it's already implemented in almost every collaborative and bug tracking software.

+1

drumm’s picture

#2676042: Automatically assign issue credit for committing patches depends on the person making the commit making a comment. A comment made by the committer stores the entity references that make up issue credit and attribution, and allows editing attribution. When implementing, we need to make sure that continues working.