Back-link to the commit as a comment on the related issue.

How do you imagine a "direct link", and what should a preliminary user interface for this feature provide? Version Control API itself offers the repository link for issue/case/bug trackers which you can enter in the repository edit form, if such a link template is present then Commit Log will link Drupal-style issue numbers ("#12345 by jpetso: blah blah") to their corresponding issue URLs.

There's no database table capturing such relations though (so backlinks from the issue are not currently possible), and the issue number format in the commit message is also not extensible. I will gladly accept patches that change these shortcomings.

Oh, and I think the main API module is appropriate as target for this feature request; after all, commit<->issue relations are one of the primary use cases.

Right, that's approximately what I expected. Valid request, although instead of a content type we might just have a table relating two values, the version control operation (a.k.a. vc_op_id) and the issue (a.k.a. nid). Then provide a block that appears in issue nodes and links to the operation URL returned by versioncontrol_get_url_commit_view(). Also provide a setting that the admin can customize for the regular expression in the commit message in order to correctly retrieve the issue id.

This functionality can and should be implemented as an extension module, and is probably pretty easy to implement, using hook_versioncontrol_operation() with the regular expression determining the issue id from $operation['message']. Still takes some work, of course, but all the infrastructure is there and the requirements are straightforward. I'm tagging it with the "Newbie" tag because this one seems like an ideal way to get familar with Version Control API.

new features on the 2.x branch

I have marked #782588: Add support for issue nid hyperlinking based on commit as a duplicate of this issue

tagging with views integration

I don't think this issue has anything to do with views integration, that's probably the issue over at #782588: Add support for issue nid hyperlinking based on commit.

This issue is about providing a new feature for drupal.org and VC API, VC Project and Project issue module to integrate and provide a link, or followup message when a commit is parsed that contains a reference to the issue NID. For example making the commit at http://drupal.org/cvs?commit=467500 would automatically post a followup issue comment with a link to, or meta data about the commit.

Since this is a new feature, I'm going to move this to phase 3 for now. If we can get this done by the launch, that'd be awesome, but I don't want to hold up phase 2 on a new feature.

Note that this will probably be implemented with hook_versioncontrol_entity_commit_insert() and/or hook_versioncontrol_entity_commit_update().

Note: I think we just want a block on issue nodes that shows all commits that reference the issue, not a new comment for every commit that references it. I could be wrong, but it seems like a comment is overkill.

In fact, I'm tempted to call this duplicate with #443000: When viewing an issue, display a list of commits that reference that issue # and move that over here. ;)

I would dearly love to see this.

Subscribe! :D

I really really really want this:

dww says this is the place to be. :)

@webchick, Why (and how) would commits be attached to an individual comment?

Wouldn't it be more useful to have all the commits that reference an issue be listed in a single place?

Well, I meant they'd be interspersed in the view in chronological order, along with the comments. I'm only so good with firebug. :) They might even be comments from "System message," or the committer, or or whatever.

I think a block showing all commits at the top of the page or something is also useful for those who want an overview of what happened in an issue. But for the purposes of the conversation about a change (commits are a BIG part of that conversation, esp. if/when we move to per-issue repos), having the commits in-context is also super helpful.

I would be happy with either of the above though. I just like to think big. :)

After using github issues I thought that making comments would be a great idea and even marking an issue as fixed (and/or having a setting for the auto-fixed part). The most common case for issues is to have a single commit that fixes the issue.

If there are more then one commit having them be interspersed in chronological order (as webchick wrote) seems like the best idea as you can easily see the conversation related to commits. It's also extremely simple to implement. We can add things like blocks later if we want.

Only question then, should this be a drupalorg module addition or general "Version Control / Project* integration" addition? I'd be happy to write code for this.

Should probably provide per-project setting for marking issue as fixed on commit and the formatting of the commit/detection of issue reference may have more proper/better ways to be done. I'll leave it up to others to tinker, but this should provide the basic structure.

Since this is being done generically in versioncontrol_project I figured we cannot assume we have uid like we should be able to on d.o thus the if ($operation['uid']).

Also as part of formatting we might want to add label. I just followed webchick's example for this patch.

This looks just lovely.

I love it, for sure

Cool, progress... thanks!

I haven't tested, but here's my review of the patch itself:

A) Huge -1 to automatically marking an issue fixed just because it's referenced in a commit (per-project setting or not). Imagine that in conjunction with per-issue repos. ;) Certainly when working on patches with feature branches that's a bad idea. Even for core, usually the issue needs to move to some "to be ported" state, or whatever, not just marked fixed. The only reason this works at all on Github is because there's special syntax you use if you actually want to resolve an issue via a commit. That's really a totally separate feature that belongs in a separate issue. Please don't try to introduce that in here, or we're going to bungle both and delay things unnecessarily.

B) (minor nit) It'd be nice (and conform to our coding standards) if the code comments used proper grammar. The 1st comment mentions "a issue". The 2nd is full of errors. We want something like:

// If the Drupal user is known then format the author as a link and post the
// comment as the detected user. Otherwise format the author as the VCS
// username without a link and post the comment as the auto-followup user.

C) The code seems needlessly confusing with how it's handling $changes['comment'] and the t() placeholders. Can't we just save the placeholder values in a separate array so it's easier to grok what's happening? I thought it was a bug, then re-read and saw "oh, wait, no there's just tricky stuff happening" but I'm too tired to tell if it's actually a bug or not. There's no reason to make this complex. A separate array is basically 0 cost and is a win in saved developer time and more robust code.

At first I wasn't sure I liked auto-commenting at all, and would have rather seen this more like an activity stream (e.g. along with notifications about edits to the issue summary, etc). But, in practice, I basically always comment on an issue once I commit something for it, so it seems like a win to just automate that. And it's definitely easier to implement this as a comment since we don't yet have a way to render all the comments interspersed with other activity notifications.

In summary: yay! ;)

Thanks,
-Derek

A) Didn't know you could close issue from commit on github, but making a special syntax seems reasonable. Although, I would have to say that per-issue repos, feature branches, and core represent the minority. Defaulting to disabled, but allowing contrib projects to do so since most work on single commit to fix issue wouldn't be a bad idea. I'm fine with making a separate issue since there are many options to discuss. Created #1291886: Support marking an issue as fixed via a commit.

B) Yea noticed the bad comments after uploading. heh

C) Sure, not biggy. Actually went back and forth when writing. My primary concern was that there was a better way to do the comment message maybe even using same formatter used in commitlog?

Fixed comments, removed auto-fixed, and moved t() parameters to $comment.

Sorry for the outdated api file, I just fixed the chunk related to operations so you can take a look at it and the operation class.

Thanks for the effort here!

Definitely appreciate the separate issue for A -- I'll reply there.

Now I'm just bikesheding, but wouldn't $placeholders be a more accurate name for this array than $comment?

Also, is $operation['revision'] already formatted as a link or is it just the text of the revision? Given that you're referring to it via !revision I'm assuming it's coming through as a link, but I just wanted to double check. (Oh, I noticed marvil07 just replied as I was typing this -- maybe that's what he's concerned about).

I've got no other visual concerns with the patch.

Anyone got a test site that's attached to Git that can try this? I have no idea what's the status of our Git test environment(s), puppet builds for VMs and all that stuff. Probably worth pinging sdboyer in #drupal-vcs in IRC to see what's up and where/how to test this before deployment.

Thanks,
-Derek

Just a side note that I'd still like to follow this up with a second issue that introduces a new table that stores associated commits with issue nids, and provides a block or format for adding to issues that lists ALL commit messages that reference the issue number in a single place.

This is looking like a great complement and a great start on a much needed feature though.

Agreed with mikey_p. Comments are hard to scan, but a block doesn't give the context that you'd get from a comment followup, so having both seems really useful.

I'm also +1 to a mapping table and a block. However, on d.o I'd like to avoid the block rendering in a whole sidebar region, since then we lose tons of screen real estate on the whole issue just for a tiny bit of info at the top of the page. I'd rather that "block" was floated to the right of the "Jump to" links or something. Anyway, that's out of scope for arguing about here, just wanted to mention it while it's on my mind. ;)

D) I just realized this patch isn't going to handle the case of a single commit that references multiple issues. I do that a fair bit -- e.g. if there's a separate issue to fix a bug introduced in an earlier feature, I do stuff like:

[#1234567] by dww: Fixed a bug in the foozbangle introduced in #1234000.

To me, that commit is relevant to both #1234567 and #1234000. Not sure if it should generate an auto-comment in both -- it probably should. Certainly I'd want to see the commit in the block of commits for both issues.

$operation['revision'] is not currently rendered as a link, but as I noted I was looking for tips on how to render all that stuff. Specifically with that a function to render it as a link, but otherwise I can construct it manually.

Looks like things outstanding are:
- Update hook and usage since api documentation was out of date.
- Render revision as a link.
- Deal with multiple issues in single commit.

Left some @todos which marvil07 sounds like he is willing to fix.

EDIT: missed the $operation['message'] in preg_match_all()

This is a stretch to exploit, but technically this is a XSS vulnerability:

+          $placeholders['!committer'] = $operation->committer;

In the other code path it's okay b/c l() check_plain()'s for you. But you should check_plain() here since we're using !committer. Sorry I missed this earlier.

Otherwise, looks good to me. Still haven't tested anything, so no idea if it actually works. ;)

What's new on the patch:

• Minor change on the regular expression: add word boundary at the end.
• Make the comment aware of the operation autorship.
• Use theme_username.
• Use operation repository webviewer_url_handler plugin to generate the link to the commit.
• Get label names to show.
• Use check plain's as dww suggested.

Also adding a diff against last patch state.

I also did not really tried this patch on a proper environment :-(

In the line $changes['comment'] = t('Commit !revision on @labels by !committer:

@message

"', $placeholders);, there is an unmatched and unneeded double quote.

+++ b/versioncontrol_project.module
@@ -508,6 +508,59 @@ function versioncontrol_project_versioncontrol_entity_commit_insert($operation)
+          $label_names = $label->name;

Assume you wanted $label_names[] = $label->name;

+++ b/versioncontrol_project.module
@@ -508,6 +508,59 @@ function versioncontrol_project_versioncontrol_entity_commit_insert($operation)
+            $changes['comment'] = t('Commit !revision on @labels authored by !author, committed by !committer: <pre>@message</pre>', $placeholders);

Have to see how this works out after groups.drupal.org/node/161659, but probably fine for now.

+++ b/versioncontrol_project.module
@@ -508,6 +508,59 @@ function versioncontrol_project_versioncontrol_entity_commit_insert($operation)
+          $placeholders['!committer'] = $operation->committer;

I believe this is where dww wanted check_plain which makes sense.

Powered by Dreditor.

Thanks for the corrections, we really need to test this on a proper environment ;-)

Seems good and agree we need to test somewhere. Not sure how we proceed.

Good call on using theme('username'). However, you're using it wrong. ;)

+          $committer->name = check_plain($operation->committer);
+          $placeholders['!committer'] = theme('username', $committer);

In Drupal's never ending quest to confuse you and get you to not handle strings correctly, theme_username() takes care of the check_plain() for you. If you can access user profiles, you get run through l() (which in turn does the check_plain()) and if not, theme_username() does the check_plain() itself. Fun, huh? ;) So your patch is double-escaping the usernames.

While I'm wearing my security team hat, it occurs to me that we should be checking node_access() here, too. Just because I put "#12345" into a commit message and could push that to your site doesn't mean I should actually have permission to view or comment on issue #12345. So, after node_load() we should both check $node->status and call node_access().

Thanks,
-Derek

Really happy to see this being picked up...but unfortunately, the approach here is wrong. Implementing the insert hook is not the way to do it - that hook is fired when a commit is recorded in the database, not when it initially arrives into the repo. The latter necessitates the former, but the former does not imply the latter - when we resync a repo (which we do fairly regularly for various reasons), we blow away all commit records then rebuild them from scratch. So every single commit in the history gets reinserted, and this hook is retriggered for each one. Imagine the cavalcade of automated issue comments :)

For when the commit truly first arrives in the repo, we have #1092062: Introduce framework for generic repository history synchronization. There are a couple smallish bugs related to merging that back in, but at this point the only thing that's blocking it is the fact that I don't have a git-dev to really test it on properly. re: that, I got git-dev just about set up again three weeks ago, but did it on an old Gentoo vm, instead of the new centos stagingvm2. Which is now temporarily borked because of an internal OSL/yum repo availability question. That's supposed to be resolved today, at which point I can resume efforts to bring git-dev back online.

That being said, I expect most of the actual logic here can be picked up and dropped into another hook implementation (and eventually shifted into a reposync event plugin). The hook to be implemented will be hook_versioncontrol_code_arrival(VersioncontrolRepository $repository, VersioncontrolEvent $event) (invocation here).

While I'm wearing my security team hat, it occurs to me that we should be checking node_access() here, too. Just because I put "#12345" into a commit message and could push that to your site doesn't mean I should actually have permission to view or comment on issue #12345. So, after node_load() we should both check $node->status and call node_access().

Agreed. And another reason why the event is very important - the relevant user to check for perms is *not* the commit's author/committer, but the person responsible for the network operation (the push). Which is not really accessible from the commit itself in its insert hook, but is a built-in part of VersioncontrolEvent: http://drupalcode.org/project/versioncontrol.git/blob/gsoc-activity:/inc... - though we should really add a getter method for it and maybe return a full user object.

And btw, a big reason we make this an optional plugin on a per-repo basis (rather than a straight hook) is so that we can have different rules for different repos - such as confining closing/responding to issues to pushes made to a main repository, but never a per-issue repo. That's a little further off, though, so it's fine to do it as a straight hook for now.

And, reading more/further back - my ultimate preference would be to thread the commits in chronologically with the comment times as separate bits of information. That is what github does, though they go by committer timestamp, which changes when you rebase to the time you rebased, so commits almost always end up stacking in a single place in their interface rather than being interleaved. That'll be much more useful in a per-issue repo/pull request context, though, so I see no problem with auto-commenting as an intermediary solution, as getting together a good framework for interleaving the data could be a bit complex.

As for highlighting names in the commit messages...well, maybe. That data is a bit fuzzy, though, and I'd prefer to either a) read it from git-notes or b) use actual commit metadata to show who authored/committed the commit or c) just not highlight them at all. If we want to do that highlighting, we'd need a whole separate place to store these db records with the commit message analysis already performed, otherwise we'd be doing potentially expensive mysql searches for username matches on the fly. Minimum one string-based matching query against {users} per commit shown on a page.

Summary:
- use theme('username') instead of calling theme_username() directly
- get rid of check_plain() from data passed to theme('username') [possibly just use user_load()?]
- move comment code to hook_versioncontrol_code_arrival(VersioncontrolRepository $repository, VersioncontrolEvent $event)
- perform a node access check before posting comment

I would vote for changing the comment user to the push event user (that was what I was looking for originally).

I want to note that posting a comment doesn't give you access to view a node right? So the viewing part seems off, but not being able to post is definitely a good idea.

@sdboyer: Not sure what you are referencing with "highlighting names." The only usernames that will be links are those that are already known based on data passed to the hook. Additionally, any look-ups are already performed and will only be done when comment is posted note repeatedly.

@dww: thanks for the clarification :-), so yeah, we need to remove it for strings that are then used in theme('username') (hint: not the last one)

@sdboyer: agree on waiting for git-dev to test history sync branch properly and use pusher to review permissions instead of author/committer. agree on using a plugin to implement this logic so we can be more granular too for d.o and easily change it when git notes was official attribution.
IMO boombatower is right on his comment about highlighting messages, we are using what's on the operation itself already(committer_uid/author_uid), and that's is performed once(on the history sync hook world) just before saving the comment.

Other notes:

• I forgot the git backend store emails on author/committer operation data members, which means that we need to use author_name/commiter_name stored by git backend on its table for operation.
• Random though(almost offtopic): maybe a versioncontrol_project_git module is needed to separate specific logic for git(i.e. use author_name/commiter_name directly)
• Other solution(optimal IMHO) to avoid wait for testing on a staging site is to create more tests here which tries tricky(I mean involves lots of dependencies) things like this feature so we do not depend on it to add features to modules.

In conclusion, I guess we need to postpone this :-/

@boombatower sorry, that was based on what now seems like a misreading of the 'jhodgdon'-linkification in #13. Initially it I mistook it for reading the commit message contents, discovering d.o usernames, and linkifying them. Not easy. If it's the actual author of the git commit itself, though, it's easy.

@marvil07 Why would we need to access the Git-specific data? We use the mapped uid in {versioncontrol_operations}.author and/or {versioncontrol_operations}.committer, and then just use that to load a user account. What's the problem?

We gotta wait for the staging site for final testing regardless, but I have no problem (AT ALL) with adding more tests.

Alright, I am more and more convinced we really need this. Due to a git accident, half a dozen commits were missing from D8 all weekend from issues that were marked fixed. I only noticed by accident. This issue would help prevent stuff like that from ever happening. :) Can we get the backreferences from node/foo/commits in first, and maybe iterate on the username linkage as followup?

@xjm: Do you mean you want to move forward with #443000: When viewing an issue, display a list of commits that reference that issue # first and then try to sort out all this auto-commenting stuff as the next step? I'm +1 to that strategy. The block seems a lot easier to implement, especially in terms of dealing with legacy issues/commits/comments -- it's easy to do a one-time processing of the commit history searching for issue nids and creating a mapping table that can be used immediately to populate this block on all existing issues. It's more complicated to go back in and create new comments in issues for existing commits. Maybe we don't even want that, but then it's confusing if some issues have those auto-commit comments and others don't, whereas all issues with commits associated could have the block immediately. Anyway, if that's what you have in mind, please join the party over at #443000...

Happy carnival, ;)
-Derek

Might as well throw this out there -- while you're waiting for this patch to land, I have another one for Dreditor that does almost the same thing using AJAX: #1437400: Get and insert link to last commit

Specifically, my patch grabs the last commit (because for practical use, that's typically all you're interested in anyway) and inserts a link into the comment textarea at your cursor position. I've been using it practically every day for awhile now.

That said, I like this issue much better.

-1 on adding commit comments retroactively to old/existing issues. It's totally fine and sufficient to have that for new commits only.

If there's going to be a list of commits in a block on the issue, that should be enough. And even without that, the amount of engineering work being required for retroactively processing existing issues does not sound worth the effort for me. I'd recommend to merely leave the option to retroactively process existing issues later (e.g., by storing the exact date of when this was enabled) -- if it turns out that it's required for any reason after all.

Also, as someone who gets and actively uses e-mail notifications for issues I'm following, I'd like to receive a notification for these commit comments. (Actually, I'd also like to get one for testbot test failure follow-ups, so changing or allowing that is probably a separate issue.)

I agree that adding this feature for new commits only would be absolutely fine.

Tagging.

I'm gonna try to take a look at this over the next few weeks and get a patch ready even though it looks like #1092062: Introduce framework for generic repository history synchronization may be stalled :(

A few notes though:

1) This totally doesn't belong in versioncontrol_project.module at all. It doesn't even depend on project_issue, so we should add a sub module to Version Control Project, such as versioncontrol_project_issue.module as a place for this functionality to live.

2) We still need a way to link commits to issues, and the new module would make an excellent place for said schema, and whatever display code is made available a list of commits for each issue (the block for historical commits, for example). I'll start this over in #443000: When viewing an issue, display a list of commits that reference that issue # though.

2a) This module will provide an excellent place for a script that works backward through the d.o database and maps all of our commit history to said schema

3) The best way to handle the question of auto-closing/fixing issues by keyword is to punt that issue to another sub module or d.o specific code. Wherever this code lands, we just need to add a hook, passing the $operations and $changes data before calling project_issue_add_auto_followup().

4) If #1092062: Introduce framework for generic repository history synchronization doesn't land soon (and I'd like it to) it is conceivable that we could tie the creation of the comment to the insertion into the mapping table from #443000. This isn't what I'd call desirable, but some good @TODOs in the code should make it clear how this should be fixed when #1092062 lands.

figured i'd bump this since #1092062: Introduce framework for generic repository history synchronization has done been landed for a while, as have the followup issues which make it more stable.

And another bump. boombatower, mikey_p, marvil07 any chance one of you will be interested to move this issue forward? Is D7 development site needed?

IMHO #443000: When viewing an issue, display a list of commits that reference that issue # is more important, an less intrusive, so I would prioritize it, but as I mentioned on that issue, maybe it worths to generalize the interaction as plugins(see #1796144: Add a "event processor" plugin type for repositories) instead of asking every module wanting to act on hook_versioncontrol_code_arrival to implement it, but sadly that's now postponed to vc-next, meaning not in 6.x-2.x, nor 7.x-1.x

So, if someone wants to implement this soon, I guess it should be through hook_versioncontrol_code_arrival, which is not a bad idea.

boombatower helpfully pointed me here.

I was looking at the change notices
http://drupal.org/list-changes/drupal

and wanting to be able to filter them by those that relate to issues that were/are critical/major (priority) or that were tasks/features (category)

also (really?) what I wanted was to be able to get a list of issues that had been committed (to 8.x) and be able to filter them in a way similar the advanced search for issues.

re 53: this issue will not store issue/commit mappings directly, but #443000: When viewing an issue, display a list of commits that reference that issue # will do. Adding views integration there will make that filtering possible ;-)

I opened #1921430: Add filters to list-changes/drupal to filter change notices by those that relate to *issues* priority and category for #53

Definitely +1 for this. Talk about saving time. Love the mock-up @webchick gives in #13. After reading the rest, I know this is improbable and it makes sense to just do it as a comment like the testbot does, but sill... would be cool.

To stay with the theme of this issue, here's a backlink to the current 2014 roadmap branstorming page: https://groups.drupal.org/node/312898

@tvn: is there is dev site that has git the infra to test a patch for this issue?

The patch in https://drupal.org/node/443000#comment-5973396 looks very usefull to combine with #34 and the use of hook_versioncontrol_code_arrival()...

Adding D.o UX tag:

• Should these look like comments? (I'm leaning toward no.)
• Should they be integrated into the comment list or in the sidebar? (I'm leaning toward in the comment list.)
• What should they look like? Webchick did a mockup in #13.
• Does this establish a pattern that other system messages should follow?

Those sound like excellent questions for the UX lead of the Drupal.org Developer Tools Team! :D Though I can't assign to Bojhan, cos he doesn't have the proper roles or whatever.

I wanted to point out https://github.com/blog/1767-redesigned-conversations, where github just redesigned their automated messages to not look like regular comments, but still be in the flow.

Looking at this tomorrow

Oh, just so there's no ambiguity, I am absolutely not advocating for #13, per se. I was trying to whip up something in Firebug kinda matching something like the Drupal.org style guide. Something much more Github-esque sounds good to me, as long as it doesn't complicate implementation.

Just wondering, do we want all system messages also failed tests etc, to be like this?

That's what Github does, so may be a direction we want to move in. But for the purposes of this issue, I'd say coming up with a suitable design just for commits, and then doing a more holistic look at the design of all of these "non-conversation elements" of the page in a separate issue would probably be easier. People get very grumpy when things all of a sudden change out from under their feet and I wouldn't want to hold up this very nice workflow/productivity improvement over whining about "why does my patch upload now look like X instead of Y I hatessss it" etc. :)

Alright :) Looking at this. I will trow in designs once I have a slightly better understanding

1) Should these look like comments? (I'm leaning toward no.)
- I dont think so. I think it should be very clear when a person explicitly made a change to the issue "a comment/discussion". Where this is more like a reference to a change in the code, something that lives elsewhere and not in this stream.

2) Should they be integrated into the comment list or in the sidebar? (I'm leaning toward in the comment list.)
- I think its part of the discussion flow, it is an "achievement" that you reach once part or the whole issue is resolved. Therefor it should be part of the stream.
- I think the sidebar is easily overlooked, even if you scroll it along side or flash it. Its simply a secondary element on the page. Given that its now at the top, you would also miss it in context with the discussion.

3) What should they look like? Webchick did a mockup in #13.
- I am going to explore this a bit, I am very busy tomorrow so will probably pick this up Friday.

4) Does this establish a pattern that other system messages should follow?
- Yes, I don't think we should introduce too many one-offs.
- Do we need to convert everything now? No, lets convert just this to a pattern and possibly experiment a little with different styles to find the one, people like the most.

I do have a few questions:

5) How is it triggered? Just by issue # in the commit?

6) I see that several commits can be referenced. What is the common use case here, is it 4/5 or more likely to be just one?

7) How interactive is the commit message, just the commit or also the contributors the issue # that is referenced? What are the desires here?

Notice that IIRC testbot already adds one comment when a patch fails testing with no special style, so it sounds like we should follow the same behavior here. Actually I think the idea comes from there ;-)

Re 5: the plan is to make that pluggable, but yes, it will be a regex extracting issue numbers from commit messages some time after the git push(at least after git push synchronization), again the plan is to make a queue to process them when possible, so it's definitely not realtime.

Re 6: For now the idea is that it should be one, but we can change the regex(that's why I want to make it pluggable, notice other projects has different commit message standards).

Re 7: Originally only the issue ID's, other things like author parsing could be possible, but I definitely would say that let's defer that for the future, really, it is a tricky problem. Also the basic issue for this is trying to help make multiple plugins able to do different things #1796144: Add a "event processor" plugin type for repositories.

I'll preface these comments by pointing out that almost every other issue tracker out there except for Drupal.org already has this feature, so it probably makes a lot of sense to look at other issue trackers and see how they do it, and cherry-pick from there.

1) I'm ambivalent. Marco raises a valid point that we already have this kind of automated message and it's just done as a normal comment with a special name / predictable message template.

2) Definitely in the comment stream. It's most definitely part of the conversation. Though you might also want it in both places (similar to how we do files), since there's a valid use case that "I'm a site builder, I just want to know if this was fixed or not in my version of Drupal" without scanning through 300 comments.

3) If we do make them comments, then this suddenly becomes very easy. :) But I think it's worth doing a touch of exploration here and seeing what we can come up with.

4) Sounds good.

5) I defer to Marco, but yes, that was my impression.

6) I'd say it's most often 1, but "stuff happens" and it can be 3-4 with rollbacks/minor follow-ups. In case of backportable issues, there will always be at least 2. And in other client projects I've worked on, we just make one "story" and attach multiple commits (10+) to it rather than spinning off sub-tickets for every little thing.

So basically, I think building in an assumption of a 1 commit:1 issue ratio would be a mistake. We should assume multiple commits:1 issue from the get-go, IMO.

7) https://drupal.org/commitlog could serve as inspiration, maybe. I'd say the information you need/want is:

- Who committed it? (link username)
- When was it committed? (this becomes the date/time of the comment, perhaps)
- What was the commit message? (I don't think we need to be fancy and link usernames here; git log doesn't do that.)
- What was the commit hash? (e.g. "d23ac0ed," linked to the commit viewer on drupalcode.org, like http://drupalcode.org/project/drupal.git/commit/d23ac0edcb5cfa39db546d12...)
- What branch was it committed to (e.g. "7.x", so people can know which version(s) have the fix.)

Some minor clarifications:
1) consistency++ :-)
2) this ticket is about having the information on the comment stream, for the static list see #443000: When viewing an issue, display a list of commits that reference that issue #
6) I meant 1 issue number reference per commit message, not one issue number reference in only one commit. so, yes, multiple commits should definitely be allowed. The only thing I was in doubt was about supporting more than one issue number per commit message ;-)
7) One thing to consider: the information added in the comment is static, as the one provided by the testbot when a test fails, so i.e. if after that a committer does a git push -f, and removes the commit, the comment will not be removed. Again for a dynamic table see the issue mentioned in point 2.

Here's something that I kinda mocked and what I would like to see. I know this probably should really go into another issue (to style comments), but just having the commit comment styled looked odd. So that's why I went a bit further here, to see the potential. I tried to not rip off GH too much though, so no icons... just indented and colorized for Drupal blue (comments) and green for commit message.

A few notes:

1. Comment number count (in template) will need to ignore commit comments so it doesn't mess up the count iteration.
2. I'd like to use https://drupal.org/project/timeago. We're already using timestamps in <time/> tags here.
3. I've given widths to all nodechanges fields (so they're even across comments) and given them red/green colors accordingly (helps explain that table a bit).
4. The actually commit message pertains to a different project, I just injected it on this page, so ignore that. I think this is all the pertinent information needed.
5. I linked certain elements in the commit message, we'll likely need to revisit this bit and see what's actually "linkable".

edit: removing embedded image, see #936304: [META] Style issue comments instead

Better version (moved the timestamp and branch around on commit)

Thanks for mocking this up Mark. It's good to see some ideas in this area and the style for the commit is good.

1. There is a lot of emphasis on the commit message, I think this makes sense for commits because they're a major event in the issue but if we intend to use a similar design for all system messages we should probably have a more subtle version that doesn't draw attention away from the conversation
2. Some of the visual details feel very different to the rest of the styling on d.o. There's a lot of colours close together and with a lot of contrast. It feels like everything is grabbing my attention at once. Looking at the Github design there's actually very little ornamentation and emphasis. That kind of style feels more d.o

Here's the more compact version @sun requested:
edit: removing embedded image, see #936304: [META] Style issue comments instead

Some of the visual details feel very different to the rest of the styling on d.o. There's a lot of colours close together and with a lot of contrast. It feels like everything is grabbing my attention at once. Looking at the Github design there's actually very little ornamentation and emphasis. That kind of style feels more d.o

1. GH doesn't have comment numbers
2. GH doesn't have node changes
3. GH's brand is more monochrome with colors splashed in, Drupal's is blue(-ish). To me, d.o has never really "felt" like it's had a proper brand (just "improvements" to key areas over time (and totally not discounting the HUGE effort undertook in the redesign))... this was an attempt at that. I'm not opposed to going more neutral, but I was trying not to just rip off GH here.

edit: GH also has user pictures to facilitate a lot of context in the streams, we don't.

I commented on comment style at #936304: [META] Style issue comments.

For commit styling - how about a green background, with no border, and square corners? That is more in line with styles we have on Drupal.org.

For implementation, we need to decide:

• Alter comment thread render array to insert commits from the View built by #443000: When viewing an issue, display a list of commits that reference that issue #. Email notifications to issue followers would not happen. The code to alter the render array could get cumbersome.
• Insert comments by the System Message user. Email notifications to issue followers would be possible. The code to track which commits have a comment could get cumbersome.

There hasn't been much discussion of email notification here. Do you expect/want it? Would it be unnecessary since a comment from the committer usually follows?

GH doesn't have node changes

It does have “system actions” though such as commit references and status changes. I don't want to just rip off Github wholesale either, but they have the benefit of constant iteration.

That comment was in relation to my first styling iteration (edit: which has significantly changed since then), which is now over at #936304: [META] Style issue comments. This issue should remain focused for implementation of a git commit back link as a "System Message" comment on the thread.

The attached patch seems to be working ok:

I'm using the project_issue_followup_user variable to retrieve the user to use as the author comment.

Reviews are welcome.

Let's stick to not sending mail on commit right now. It will be easy to add it, if people ask.

Otherwise, this looks great!

Ok, this is now added to 7.x-1.x in two commits:

• e048966 Move general issue/operation related code to an abstract class.
• 305bf70 Issue #493074: Added an event processor git specific plugin to back-link to the commit as a comment on the related issue.

Also changed some comments before adding, see interdiff.

Hurray! :)

Great! Thanks Marco! Should we tag this for D.o deployment?

Probably should be tagged for testing on git7 before deployment.

I did not visually review this, is there a screenshot?

This issue will have simple markup, as in #82, but with our theme. A sub-issue of #936304: [META] Style issue comments will handle styling system messages.

Test on https://git7site.devdrupal.org/comment/8588907#comment-8588907

Looks like it is ready to deploy.

This is now deployed on Drupal.org!

For example, https://drupal.org/node/2129153

wow!

this is really cool!

+1 to everyone on this thread; this rocks! I won't have to post links back to commits in comments by hand anymore, saving me a few minutes every month :)

Great work!
One nice to have follow-up: #2223049: Shorten the path from issue to commit diff

This is really appreciated guys, now we don't have to manually comment with the commit message. Now, can we take it one step further and have the issue Fix'd when we push our commits?

Regards,
K.

Creating another issue.

#2270763: Automatically fix issues when commit message contains "Fixed …" keyword (powerful/smart commit messages) is it.

Anyone knows if there's a follow-up for this about repeatedly spamming issues?

When working with feature branches, for example in sandboxes, a new comment is added if a feature branch is merged and updated, see for example: #2252171: Port Views integration

Sort of related bug. Not at all caused by this feature, but revealed more obviously as a result. :)

+++ b/versioncontrol_project_issue/versioncontrol_project_issue_git/plugins/event_processor/VersioncontrolEventProcessorGitCommitsComment.inc
@@ -0,0 +1,88 @@
+          foreach ($valid_issue_nids as $valid_issue_nid) {
+            $operation_nid_maps[] = array(
+              'nid' => $valid_issue_nid,
+              'vc_op_id' => $operation->vc_op_id

So this foreach looks like it was intended to store a mapping of nids to version control operations, and there is this empty db table on d.o staging:

mysql> describe versioncontrol_project_issue_operations;
+----------+------------------+------+-----+---------+-------+
| Field    | Type             | Null | Key | Default | Extra |
+----------+------------------+------+-----+---------+-------+
| nid      | int(10) unsigned | NO   | PRI | 0       |       |
| vc_op_id | int(10) unsigned | NO   | PRI | 0       |       |
+----------+------------------+------+-----+---------+-------+
2 rows in set (0.01 sec)

However, the $operation_nid_maps is never used outside this line (and, as mentioned, the above table is empty). Was the intention originally to store the commit/issue nid mapping?

Yes, that is something we can turn on, from #443000: When viewing an issue, display a list of commits that reference that issue #. Turning it on should re-parse the entire history of all repositories, so it needs some deployment coordination, to watch for any problems.

I think turning that back on should also remove the "SPAM" from pushing a branch with the same commits as it can then see that it already has this commit associated.

@xjm, I understand the confusion, the mentioned code was indeed added with the patch here, but it was removed on commit 5096f0f.
As drumm mentioned, the code already let do the mapping, but via another versioncontrol event processor(ctools plugin), which is not yet in use in d.o.