Support for Drupal 7 is ending on 5 January 2025—it’s time to migrate to Drupal 10! Learn about the many benefits of Drupal 10 and find migration tools in our resource center.
As far as I'm aware, there's no direct link between a changeset and a bug/ticket. A critical feature IMO is providing such a link. I realise this could be raised against the versioncontrol_svn module as well but I thought I would start here.
This feature would facilitate implementing code reviews e.g.
Comment | File | Size | Author |
---|---|---|---|
#92 | backlink.png | 59.9 KB | YesCT |
#84 | interdiff.txt | 2.35 KB | marvil07 |
#82 | back-link-commit-as-comment-20130312_0057.patch | 17.03 KB | marvil07 |
Comments
Comment #1
jpetso CreditAttribution: jpetso commentedHow 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.
Comment #2
stodge CreditAttribution: stodge commentedI just started reading through the version control code so I'm not familiar with it yet; I'm just thinking at a higher level to incorporate existing functionality we have at work with Team Foundation Server and Trac.
So as a high level example, say we have a content type ChangesetOperation in the database that contains details of a single commit operation (update/add etc). The ChangesetOperation would have a field called ticket_id, which is extracted from the commit message. E.g. the commit message is:
refs #100
The changeset operation's ticket_id would be set to 100. Then when you view ticket 100, you can see a list of all changesets (basic details to save space). Exactly as you do in ClearQuest/ClearCase and Team Foundation Server.
Hopefully that makes sense. I'm only working at a high abstract level as I'm not familiar enough with version control modules to relate it to their data. I'm trying to find a screenshot to help but I haven't found anything useful yet.
Thanks
Comment #3
stodge CreditAttribution: stodge commentedThe nearest I could find in a hurry is this Redmine page:
http://www.redmine.org/issues/3279
Note that the "Associated revisions" are shown when viewing the ticket.
Comment #4
jpetso CreditAttribution: jpetso commentedRight, 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.
Comment #5
stodge CreditAttribution: stodge commentedI don't claim to yet understand most of what you wrote, but I think it makes sense. :)
Comment #6
marvil07 CreditAttribution: marvil07 commentednew features on the 2.x branch
Comment #7
marvil07 CreditAttribution: marvil07 commentedI have marked #782588: Add support for issue nid hyperlinking based on commit as a duplicate of this issue
Comment #8
eliza411 CreditAttribution: eliza411 commentedtagging with views integration
Comment #10
mikey_p CreditAttribution: mikey_p commentedI 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().
Comment #11
dwwNote: 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. ;)
Comment #12
rfayI would dearly love to see this.
Comment #13
webchickSubscribe! :D
I really really really want this:
dww says this is the place to be. :)
Comment #14
mikey_p CreditAttribution: mikey_p commented@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?
Comment #15
webchickWell, 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. :)
Comment #16
boombatower CreditAttribution: boombatower commentedAfter 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.
Comment #17
boombatower CreditAttribution: boombatower commentedShould 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.
Comment #18
catchThis looks just lovely.
Comment #19
chx CreditAttribution: chx commentedI love it, for sure
Comment #20
dwwCool, 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
Comment #21
boombatower CreditAttribution: boombatower commentedA) 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?
Comment #22
boombatower CreditAttribution: boombatower commentedFixed comments, removed auto-fixed, and moved t() parameters to $comment.
Comment #23
marvil07 CreditAttribution: marvil07 commentedSorry 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!
Comment #24
dwwDefinitely 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
Comment #25
mikey_p CreditAttribution: mikey_p commentedJust 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.
Comment #26
catchAgreed 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.
Comment #27
dwwI'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:
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.
Comment #28
boombatower CreditAttribution: boombatower commented$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.
Comment #29
boombatower CreditAttribution: boombatower commentedLeft some @todos which marvil07 sounds like he is willing to fix.
EDIT: missed the $operation['message'] in preg_match_all()
Comment #30
dwwThis is a stretch to exploit, but technically this is a XSS vulnerability:
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. ;)
Comment #31
marvil07 CreditAttribution: marvil07 commentedWhat's new on the patch:
Also adding a diff against last patch state.
I also did not really tried this patch on a proper environment :-(
Comment #32
Lars Toomre CreditAttribution: Lars Toomre commentedIn the line $changes['comment'] = t('Commit !revision on @labels by !committer:
"', $placeholders);, there is an unmatched and unneeded double quote.
Comment #33
boombatower CreditAttribution: boombatower commentedAssume you wanted $label_names[] = $label->name;
Have to see how this works out after groups.drupal.org/node/161659, but probably fine for now.
I believe this is where dww wanted check_plain which makes sense.
Powered by Dreditor.
Comment #34
marvil07 CreditAttribution: marvil07 commentedThanks for the corrections, we really need to test this on a proper environment ;-)
Comment #35
boombatower CreditAttribution: boombatower commentedSeems good and agree we need to test somewhere. Not sure how we proceed.
Comment #36
dwwGood call on using theme('username'). However, you're using it wrong. ;)
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
Comment #37
sdboyer CreditAttribution: sdboyer commentedReally 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).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.Comment #38
sdboyer CreditAttribution: sdboyer commentedAnd 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.
Comment #39
sdboyer CreditAttribution: sdboyer commentedAnd, 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.
Comment #40
boombatower CreditAttribution: boombatower commentedSummary:
- 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.
Comment #41
marvil07 CreditAttribution: marvil07 commented@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:
In conclusion, I guess we need to postpone this :-/
Comment #42
sdboyer CreditAttribution: sdboyer commented@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.
Comment #43
xjmAlright, 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?
Comment #44
dww@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
Comment #45
IceCreamYou CreditAttribution: IceCreamYou commentedMight 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.
Comment #46
sun-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.)
Comment #47
rfayI agree that adding this feature for new commits only would be absolutely fine.
Comment #48
webchickTagging.
Comment #49
mikey_p CreditAttribution: mikey_p commentedI'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.
Comment #50
sdboyer CreditAttribution: sdboyer commentedfigured 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.
Comment #51
tvn CreditAttribution: tvn commentedAnd another bump. boombatower, mikey_p, marvil07 any chance one of you will be interested to move this issue forward? Is D7 development site needed?
Comment #52
marvil07 CreditAttribution: marvil07 commentedIMHO #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.
Comment #53
YesCT CreditAttribution: YesCT commentedboombatower 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.
Comment #54
marvil07 CreditAttribution: marvil07 commentedre 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 ;-)
Comment #55
YesCT CreditAttribution: YesCT commentedI opened #1921430: Add filters to list-changes/drupal to filter change notices by those that relate to *issues* priority and category for #53
Comment #56
markhalliwellDefinitely +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.
Comment #57
helmo CreditAttribution: helmo commentedTo 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()...
Comment #58
xjmComment #59
marvil07 CreditAttribution: marvil07 commentedComment #60
drummAdding D.o UX tag:
Comment #61
webchickThose 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.
Comment #62
tim.plunkettI 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.
Comment #63
Bojhan CreditAttribution: Bojhan commentedLooking at this tomorrow
Comment #64
webchickOh, 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.
Comment #65
Bojhan CreditAttribution: Bojhan commentedJust wondering, do we want all system messages also failed tests etc, to be like this?
Comment #66
webchickThat'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. :)
Comment #67
Bojhan CreditAttribution: Bojhan commentedAlright :) 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?
Comment #68
marvil07 CreditAttribution: marvil07 commentedNotice 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.
Comment #69
webchickI'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.)
Comment #70
marvil07 CreditAttribution: marvil07 commentedSome 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.
Comment #71
markhalliwellHere'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:
<time/>
tags here.edit: removing embedded image, see #936304: [META] Style issue comments instead
Comment #72
markhalliwellBetter version (moved the timestamp and branch around on commit)
Comment #73
markhalliwellComment #74
LewisNymanThanks for mocking this up Mark. It's good to see some ideas in this area and the style for the commit is good.
Comment #75
markhalliwellHere's the more compact version @sun requested:
edit: removing embedded image, see #936304: [META] Style issue comments instead
Comment #76
markhalliwelledit: GH also has user pictures to facilitate a lot of context in the streams, we don't.
Comment #77
drummI 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:
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?
Comment #78
markhalliwellComment #79
LewisNymanIt 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.
Comment #80
markhalliwellThat 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.
Comment #81
drummComment #82
marvil07 CreditAttribution: marvil07 commentedThe 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.
Comment #83
drummLet's stick to not sending mail on commit right now. It will be easy to add it, if people ask.
Otherwise, this looks great!
Comment #84
marvil07 CreditAttribution: marvil07 commentedOk, this is now added to 7.x-1.x in two commits:
Also changed some comments before adding, see interdiff.
Comment #85
Wim LeersHurray! :)
Comment #86
tvn CreditAttribution: tvn commentedGreat! Thanks Marco! Should we tag this for D.o deployment?
Comment #87
eliza411 CreditAttribution: eliza411 commentedProbably should be tagged for testing on git7 before deployment.
Comment #88
Bojhan CreditAttribution: Bojhan commentedI did not visually review this, is there a screenshot?
Comment #89
drummThis 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.
Comment #90
drummTest on https://git7site.devdrupal.org/comment/8588907#comment-8588907
Looks like it is ready to deploy.
Comment #91
drummThis is now deployed on Drupal.org!
For example, https://drupal.org/node/2129153
Comment #92
YesCT CreditAttribution: YesCT commentedwow!
this is really cool!
Comment #93
geerlingguy CreditAttribution: geerlingguy commented+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 :)
Comment #94
helmo CreditAttribution: helmo commentedGreat work!
One nice to have follow-up: #2223049: Shorten the path from issue to commit diff
Comment #95
YesCT CreditAttribution: YesCT commentedComment #97
KartagisThis 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.
Comment #98
KartagisCreating another issue.
Comment #99
YesCT CreditAttribution: YesCT commented#2270763: Automatically fix issues when commit message contains "Fixed …" keyword (powerful/smart commit messages) is it.
Comment #100
markhalliwellComment #101
BerdirAnyone 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
Comment #102
markhalliwellComment #103
dwwSort of related bug. Not at all caused by this feature, but revealed more obviously as a result. :)
Comment #105
xjmSo 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:
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?Comment #106
drummYes, 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.
Comment #107
Fabianx CreditAttribution: Fabianx commentedI 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.
Comment #108
marvil07 CreditAttribution: marvil07 commented@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.