Updated: Comment #35
Problem/Motivation
Many issues don't have patches in the comments if they are fixed by the author, so it's sometimes inconvenient to find the actual code change.
Some issues have follow-ups or authors just think it make sense to add iterative changes over one issue. This means more than one commit per issue. Notice some issue can have a lot of patches, so it could be tricky to find the ones applied to upstream.
Proposed resolution
Show a list of commits(versioncontrol operations) related with the issue.
Implement it as a new submodule.
Remaining tasks
- Make the vc-project-issue-git-resync-maps drush command use batch api. i.e. on projects with lots of commits and lots of issues, say core, memory consumption can be really high.
User interface changes
- Project issues will have one new element showing the list of its related commits.
API changes
Original report by grendzy
It would be cool if, in addition to the node id filter [#NNN], we could also link to a CVS commit message the same way.
Many issues don't have patches in the comments if they are fixed by the author, so it's sometimes inconvenient to find the actual code change.
I also keep seeing module author's fix and close an issue in CVS, and the the issues gets a deluge of comments like "it still doesn't work! How come there's no new release today?" Making it easier for module authors to say "fixed in [r:NNN]" instead of just "fixed" might help clarify things for beginners.
Thanks!
Comment | File | Size | Author |
---|---|---|---|
#49 | example001.png | 316.01 KB | marvil07 |
#39 | interdiff.txt | 2.97 KB | marvil07 |
#39 | 0001-Issue-443000-follow-up-Improves-vcpi-git-resync-maps.patch | 4.63 KB | marvil07 |
#35 | 0001-Issue-443000-Shows-project-issue-related-operations-.patch | 9.22 KB | marvil07 |
#34 | versioncontrol_project_issue.views_.inc_.txt | 1.64 KB | marvil07 |
Comments
Comment #1
aclight CreditAttribution: aclight commentedWhat you propose won't necessarily help with this problem, because the maintainer would still need to post the commit id in a comment when setting the status to fixed.
What you probably were thinking of (which should IMO be a separate issue) would be a way for an issue to automatically have links somewhere to all commits in which that issue was mentioned. That'd be a nice feature to have, though would be more complicated to write. Maybe someone has already suggested that, but I don't remember hearing such a suggestion.
Comment #2
aclight CreditAttribution: aclight commentedAlso, I'm almost certain that this issue is a duplicate (I think moshe was the one who originally requested this), but I can't find the original issue in the cvslog or project_issue module queue.
Another issue that is slightly related to this one is #132694: add a UI to filter the views of commit messages. From the last link in that original issue, I see what I said in #1 about linking to all commit messages which refer to an issue is already possible. We'd just need to automatically add a link somewhere on the issue node itself.
Comment #3
grendzy CreditAttribution: grendzy commentedThe way you described it, yes that would probably be more useful. Though I've used systems that allow links to be created in either side, putting the issue number in the commit message and then automatically displaying it is probably ideal.
Should this suggestion be in a different queue?
Comment #4
grendzy CreditAttribution: grendzy commentedComment #5
dwwYeah, I probably submitted an issue about this ages ago. ;) It's been on my wish-list for a long time. I was just talking about this again over at #433050-2: Add code to parse and aggregate CVS contributor info in fact...
I think this code should live in the CVS Integration module, not project_issue... I mean, ideally, it should live in versioncontrol_project or something. ;) But, until that's ready for prime time, let's call this a CVS integration issue.
Comment #6
mikey_p CreditAttribution: mikey_p commentedIf you want to see this on d.o, you should followup over at #493074: Back-link to the commit as a comment on the related issue.*
*Not technically a duplicate of this issue :(
Comment #7
boombatower CreditAttribution: boombatower commentedsub, working on issue in #6.
Comment #8
marvil07 CreditAttribution: marvil07 commentedMoving to versioncontrol_project, hoping nobody here still wants this for CVS intergration, if so please move it back and I will open another one for it. But I guess this started as a drupal.org feature, so I guess it's OK to move it.
Some complementary comments can be found on the related issue #493074: Back-link to the commit as a comment on the related issue. (see comments #25, #26)
Comment #9
marvil07 CreditAttribution: marvil07 commentedI have been taking a look at this. It lacks of auto-initialization of values on hook_enable, and it really needs a test(I just wrote what I was thinking), but it's a start.
Comment #10
marvil07 CreditAttribution: marvil07 commentedAfter some minor fixes, this is working locally :-).
Still needed:
$node->project_issue['operations']
).To decide:
Comment #11
marvil07 CreditAttribution: marvil07 commentedtagging
Comment #12
marvil07 CreditAttribution: marvil07 commentedThis will probably can be implemented following #1796144: Add a "event processor" plugin type for repositories, but it can also move to that way in future. (a.k.a. link to relevant issue just in case)
Comment #13
YesCT CreditAttribution: YesCT commentedI came here following a trail of trying to find a way I could search issues, looking for ones say that had been committed (had something committed is fine) and were, for example, critical. (See: #493074-53: Back-link to the commit as a comment on the related issue.)
Reading #12 makes me think that this issue is postponed since:
That issue though is postponed.. is there a cycle of postponement? :)
Should this be postponed on #1796144: Add a "event processor" plugin type for repositories?
Or, can we work around that to accomplish the list of things still needed in #10?
Comment #14
marvil07 CreditAttribution: marvil07 commentedSorry for the long response time here.
The approach in my last patch here was wrong, I was not using event hooks, so that's need to be changed (it's possible to re-parse versioncontrol repositories changing its entities ids, so versioncontrol commit entity hooks is a bad idea).
In the other side, I guess it is just too late to actually add a feature for the D6 version.
@YesCT: About your question, the original idea was to postpone this until we have a new ctools plugin type implemented, so it's more flexible (different sites has different standards or want to make separate things on "code added to upstream"), which is proposed in the issue mentioned. We can do it without depending on that issue.
Let's mark this for D7, and postpone until the next major release, hopefully in D7.
Comment #15
helmo CreditAttribution: helmo commented#493074: Back-link to the commit as a comment on the related issue. mentions hook_versioncontrol_code_arrival() as a good place to insert new mappings.
Comment #15.0
helmo CreditAttribution: helmo commentedUpdated issue summary: Major update creating the issue summary.
Comment #16
YesCT CreditAttribution: YesCT commentedMaybe this can be unpostponed now?
Comment #17
dwwSure -- if someone wants to work on it, they definitely should. I don't fully understand the ctools plugin argument for postponing in the first place...
Comment #18
marvil07 CreditAttribution: marvil07 commentedJust to mention that #1796144: Add a "event processor" plugin type for repositories is now unpostponed.
I will definitely like to have this feature after that issue is already in on vc.
Comment #19
marvil07 CreditAttribution: marvil07 commentedSame tags than sibling issue.
Comment #20
marvil07 CreditAttribution: marvil07 commentedComment #21
Wim LeersWoot, thanks, marvil07! :)
Comment #22
marvil07 CreditAttribution: marvil07 commentedWhat is new in this patch:
In the middle of this, I found some problems:
Those will probably needs to be solved on #1796144: Add a "event processor" plugin type for repositories first.
Comment #23
drummCan fixing that be done in a followup issue? It doesn't seem too important for #493074: Back-link to the commit as a comment on the related issue., but I could be mistaken.
Comment #24
marvil07 CreditAttribution: marvil07 commentedAfter a little chat with Sam, we agree that it is ok to interact with git from the plugin. For now I'm avoiding as much as possible to do that directly, so i'm relying on the synchronization plugin for it.
I guess so.
Here some code working. This patch depends also on #2210595: Make VersioncontrolGitRepositoryHistorySynchronizerDefault::getCommitInterval() public to avoid duplicating the code there.
There are still some pending improvements, but the basic case should be working fine.
Comment #25
drummNeeds
t()
:In
versioncontrol_project_issue_node_load()
andversioncontrol_project_issue_node_delete()
, the node type should not be hardcoded'project_issue'
. Check if nodes are issues withproject_issue_node_is_issue()
(project_issue_node_type_is_issue()
andproject_issue_issue_node_types()
are available too.)Comment #26
marvil07 CreditAttribution: marvil07 commented@drumm, thanks for the quick review.
What's new in this patch:
Comment #27
marvil07 CreditAttribution: marvil07 commentedWhat's new in this patch:
Pending:
Comment #28
drummLooks like this is on track.
If needed, we can always put some fixmes and todos in followup issues, if any of it is not required for adding comments for #493074: Back-link to the commit as a comment on the related issue..
Comment #29
marvil07 CreditAttribution: marvil07 commentedWhat's new in this patch:
I have reviewed todos/fixmes and after solving some of them, I think it's ok to leave there the other ones.
Comment #30
marvil07 CreditAttribution: marvil07 commentedWhat is new in this patch:
This seems to be good enough to be added to upstream now.
Comment #31
marvil07 CreditAttribution: marvil07 commentedAdded the last patch to 7.x-1.x, in three commits:
Also, updated the issue summary.
Moving to active again, since the code in patches is already added.
Comment #32
helmo CreditAttribution: helmo commentedIs there a dev site where we can see this in action?
Comment #33
marvil07 CreditAttribution: marvil07 commented@helmo: Not yet, I will add this to git7site soon.
In the meantime, added one extra fix:
Comment #34
marvil07 CreditAttribution: marvil07 commentedI have been trying to add the views integration directly, but it seems to be a little tricky, if at all possible.
There is already a join from operations table to node table based on the project(via versioncontrol_project_projects), so if I add one more join, will it know what to choose?
AFAIK views does not support correctly tables with two primary keys, so in order to work-around it a serial field will need to be added.
Attached some of the code I have trying.
Comment #35
marvil07 CreditAttribution: marvil07 commentedHere what I ended up with to make it work:
So, what is is show can be changed via views.
Comment #36
marvil07 CreditAttribution: marvil07 commentedAdded the last patch to 7.x-1.x.
New pending:
Back to active for the new change.
Comment #37
drummThis may not need Batch API. Depending on where the memory is being used, using
$reset
parameters, and other ways of keeping static caches from leaking memory, could work. Long-running processes using a stable amount of memory are okay, and can be more simple.Comment #38
marvil07 CreditAttribution: marvil07 commentedNeil, thanks for the suggestion!
Patch here pending to be tested with a big(many issues) project.
Comment #39
marvil07 CreditAttribution: marvil07 commentedI have tested this on git7site, for drupal core(I think our worst case scenario) and it failed again.
But, after applying the attached patch I could make it work!
Comment #41
marvil07 CreditAttribution: marvil07 commentedComment #42
helmo CreditAttribution: helmo commented@marvil07: Do you have a git7site link? The few random(fixed) issues I viewed there don't show a commit list.
Comment #43
marvil07 CreditAttribution: marvil07 commented@helmo git7site is re-initialized everyday, that's why you cannot see them today.
Comment #44
drummComment #45
drummThe code is now deployed.
Either this issue, or a deployment issue, needs deployment instructions. What commands are run? Do they need to run with Git repos accessible on disk (util) or a web server?
Comment #47
xjmSo per #493074-106: Back-link to the commit as a comment on the related issue., there's still another deployment task that needs to be done for this issue, to enable the functionality and populate the data?
Comment #48
drummI don't remember if there was a better issue for tracking deployment. Please tag if found.
Comment #49
marvil07 CreditAttribution: marvil07 commented@xjm Yes, the code to do this is already on d.o, but there is some missing configuration, and was deferred initially because it needed lots of time to actually process all repositories.
@drumm Here the list you asked(missing d.o deploy steps):
* Configure the new plugins(settings*php):
* Enable the block "Version Control Project Issue: Related operations"
* Initialization from old data from all repositories(notice this process is using only databse, so no direct access to git repositories is required):
I deployed this to git7site and run the mapping only for vote_up_down.
I'm attaching a screenshot for reference, since it will be reinitialized at the start of tomorrow.
Sadly I found a bug I do not yet fix, I see the view is duplicating the (operation) values in some cases, but the stored data seems to be always OK, so it should need some views love, anyone interested to fix it, please go ahead ;-)
Update: upps, missing paragraph.
Comment #50
drummThis plugin is now enabled and recording data. And I am back-filling the data.
I don’t plan on enabling the block since we have commit comments. This of course can always change as we evolve issue pages.
Comment #51
drummCalling this deployed. We have over 3/4 of projects back-filled, including core. The script I landed on is: