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!

Support from Acquia helps fund testing for Drupal Acquia logo

Comments

aclight’s picture

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.

What 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.

aclight’s picture

Also, 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.

grendzy’s picture

Title: When viewing an issue, display a list of commits that reference that issue # » [r:NNN] filter for CVS commits
Project: CVS integration » Project issue tracking
Component: User interface » Issues

would be a way for an issue to automatically have links somewhere to all commits in which that issue was mentioned.

The 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?

grendzy’s picture

Title: [r:NNN] filter for CVS commits » when issue # is mentioned in CVS commit, display commit message on issue page
dww’s picture

Title: when issue # is mentioned in CVS commit, display commit message on issue page » When viewing an issue, display a list of commits that reference that issue #
Project: Project issue tracking » CVS integration
Component: Issues » User interface

Yeah, 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.

mikey_p’s picture

Title: [r:NNN] filter for CVS commits » When viewing an issue, display a list of commits that reference that issue #
Project: Project issue tracking » CVS integration
Component: Issues » User interface

If 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 :(

boombatower’s picture

sub, working on issue in #6.

marvil07’s picture

Project: CVS integration » Version Control / Project* integration
Version: 6.x-1.x-dev » 6.x-2.x-dev
Component: User interface » Code

Moving 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)

marvil07’s picture

Status: Active » Needs work
FileSize
5.67 KB

I 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.

marvil07’s picture

After some minor fixes, this is working locally :-).

Still needed:

  • initialization of values(maybe a drush command? e.g. project repositories with lots of operations can take a while).
  • Change the mappings when a project issue is changed of project.
  • UI to show the operations(now it simply load the operation objects on $node->project_issue['operations']).

To decide:

  • Do we want to make message parsing a ctools plugin? (sounds like a good idea instead of hardcoding a regex).
marvil07’s picture

Issue tags: +git phase 3

tagging

marvil07’s picture

This 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)

YesCT’s picture

I 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:

This will probably can be implemented following #1796144:

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?

marvil07’s picture

Version: 6.x-2.x-dev » 7.x-1.x-dev
Status: Needs work » Postponed

Sorry 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.

helmo’s picture

#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.

helmo’s picture

Issue summary: View changes

Updated issue summary: Major update creating the issue summary.

YesCT’s picture

Maybe this can be unpostponed now?

dww’s picture

Issue summary: View changes
Status: Postponed » Active

Sure -- 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...

marvil07’s picture

Just 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.

marvil07’s picture

Same tags than sibling issue.

marvil07’s picture

Wim Leers’s picture

Woot, thanks, marvil07! :)

marvil07’s picture

Assigned: Unassigned » marvil07
Status: Active » Needs work
Issue tags: +Needs issue summary update
FileSize
11.91 KB
9.46 KB

What is new in this patch:

In the middle of this, I found some problems:

  • Event processors deal with VersioncontrolEvent objects, so we do not have the needed data(vc_op_id's) there to do this in a straight-forward way. And we cannot assume we have access to git repository from a webhead, therefore we cannot do revwalks to i.e figure out commit hashes in a commit interval received on the event. FTR a database revwalk is just a bad idea, and cannot be done completely accurate(i.e. multiple parent comimts).
  • Event processors are executed after the reposync, so i.e. in forced updates from there we cannot figure out what operations were removed.

Those will probably needs to be solved on #1796144: Add a "event processor" plugin type for repositories first.

drumm’s picture

Event processors are executed after the reposync, so i.e. in forced updates from there we cannot figure out what operations were removed.

Can 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.

marvil07’s picture

FileSize
10.44 KB
4.59 KB

Event processors deal with VersioncontrolEvent objects, so we do not have the needed data(vc_op_id's) there to do this in a straight-forward way. And we cannot assume we have access to git repository from a webhead, therefore we cannot do revwalks to i.e figure out commit hashes in a commit interval received on the event. FTR a database revwalk is just a bad idea, and cannot be done completely accurate(i.e. multiple parent comimts).

After 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.

(about non-ff pushes)Can fixing that be done in a followup issue?

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.

drumm’s picture

Needs t():

      '#title' => 'Extract pattern',

In versioncontrol_project_issue_node_load() and versioncontrol_project_issue_node_delete(), the node type should not be hardcoded 'project_issue'. Check if nodes are issues with project_issue_node_is_issue() (project_issue_node_type_is_issue() and project_issue_issue_node_types() are available too.)

marvil07’s picture

FileSize
13.3 KB
5.7 KB

@drumm, thanks for the quick review.

What's new in this patch:

  • drumm suggested changes
  • minor conversion of a query to dbtng
  • Remove one issue mapings when the issue project is changed. Notice I'm not trying to re-add it on the new project, since it will need to analyze all operations, potentially a lot.
  • Start implementation of hook_versioncontrol_repository_pre_resync() to do a best effort to preserve existing mappings.
marvil07’s picture

FileSize
5.8 KB
15.31 KB

What's new in this patch:

  • Implemented hook_versioncontrol_repository_post_resync().
  • Implemented hook_versioncontrol_repository_bypassing_purge().

Pending:

  • Move git specific code to a submodule?
  • Review fixme's and todo's.
  • Add initialization of values(maybe a drush command? e.g. project repositories with lots of operations can take a while).
  • Design UI to show the operations(now it simply load the operation objects on $node->project_issue['operations']).
drumm’s picture

Looks 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..

marvil07’s picture

FileSize
22.58 KB
17.13 KB

What's new in this patch:

  • Moved git specific code to a submodule.
  • Do mass removal of operation/issue associations in 500 items chunks.
  • Try to deal with big pushes loading operations in chunks of 500 items.

I have reviewed todos/fixmes and after solving some of them, I think it's ok to leave there the other ones.

marvil07’s picture

Status: Needs work » Needs review
FileSize
10.75 KB
24.62 KB

What is new in this patch:

  • Separate a function for removing issue/operation maps based on a repository.
  • Makes valid issue node check an independent method.
  • Added a function to check is a repository has a project associated.
  • Added drush command to sync issue/operation maps based on repository ids or a project ids.

This seems to be good enough to be added to upstream now.

marvil07’s picture

Issue summary: View changes
Status: Needs review » Active

Added the last patch to 7.x-1.x, in three commits:

  • b767976 Added a function to check is a repository has a project associated.
  • f9eed82 Issue #443000: Added a new module to store the relation between versioncontrol operations and project issues.
  • 658556c Issue #443000: Added a git specific module extending versioncontrol_project_issue.

Also, updated the issue summary.

Moving to active again, since the code in patches is already added.

helmo’s picture

Is there a dev site where we can see this in action?

marvil07’s picture

@helmo: Not yet, I will add this to git7site soon.

In the meantime, added one extra fix:

  • e57b09d Issue #443000 follow-up: Support pushing the same commit to other branch on git_issue_mapper plugin.
marvil07’s picture

I 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.

marvil07’s picture

Status: Active » Needs review
FileSize
9.22 KB
25.44 KB

Here what I ended up with to make it work:

  • A new versioncontrol views_set plugin, project_issue_operations_view, which refer to a versioncontrol operations block view with a vc_op_id contextual filter accepting multiple values.
  • A new versioncontrol_project_issue block, vcpi_operations, which based on the current node, renders the new view_set.
  • A new default view, vcpi_related_operations, for the project_issue_operations_view views_set.

So, what is is show can be changed via views.

marvil07’s picture

Issue summary: View changes
Status: Needs review » Active

Added the last patch to 7.x-1.x.

New pending:

  • 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.

Back to active for the new change.

drumm’s picture

This 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.

marvil07’s picture

Status: Active » Needs review
FileSize
2.71 KB

Neil, thanks for the suggestion!

Patch here pending to be tested with a big(many issues) project.

marvil07’s picture

I 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!

$ time drush vcpi-git-resync-maps repositories 2

real    9m12.566s
user    1m42.220s
sys     0m18.840s

  • Commit bd272e4 on 7.x-1.x by marvil07:
    Issue #443000 follow-up: Improves vcpi-git-resync-maps drush command...
marvil07’s picture

Status: Needs review » Fixed
helmo’s picture

@marvil07: Do you have a git7site link? The few random(fixed) issues I viewed there don't show a commit list.

marvil07’s picture

@helmo git7site is re-initialized everyday, that's why you cannot see them today.

drumm’s picture

Issue tags: +needs drupal.org deployment
drumm’s picture

Issue tags: -needs drupal.org deployment

The 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?

Status: Fixed » Closed (fixed)

Automatically closed - issue fixed for 2 weeks with no activity.

xjm’s picture

So 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?

drumm’s picture

Issue tags: +needs drupal.org deployment

I don't remember if there was a better issue for tracking deployment. Please tag if found.

marvil07’s picture

FileSize
316.01 KB

@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.

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?

@drumm Here the list you asked(missing d.o deploy steps):
* Configure the new plugins(settings*php):

// Since, "commits as comment" plugin is already configured, I'm including its configuration here too, so it's clear how it looks like at the end:
$conf['versioncontrol_repository_plugin_defaults_git_event_processors'] = array(
  'git_issue_mapper',
  'git_commits_as_comment',
);
$conf['versioncontrol_repository_plugin_defaults_git_event_processors_data'] = array(
  'git_issue_mapper' => array(
    'message_pattern' => '/#(\\d+)\\b/',
  ),
  'git_commits_as_comment' => array(
    'message_pattern' => '/#(\\d+)\\b/',
  ),
);

* 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):

drush vc-project-issue-git-resync-all-projects-maps --chunk=10

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.

drumm’s picture

This 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.

drumm’s picture

Issue tags: -needs drupal.org deployment

Calling this deployed. We have over 3/4 of projects back-filled, including core. The script I landed on is:

$query = new EntityFieldQuery();
$query->entityCondition('entity_type', 'versioncontrol_repo')
->propertyCondition('repo_id', [2, 4364], 'NOT IN') // Already done
->propertyOrderBy('repo_id');
$result = $query->execute();
foreach (array_keys($result['versioncontrol_repo']) as $repo_id) {
  $repository = versioncontrol_repository_load($repo_id);
  print 'repo ' . $repository->repo_id . ':' . $repository->name . "\n";
  $processor = $repository->getEventProcessors()['git_issue_mapper'];
  $query = new EntityFieldQuery();
  $query->entityCondition('entity_type', 'versioncontrol_event')
  ->propertyCondition('repo_id', $repository->repo_id)
  ->propertyCondition('elid', 1571612, '<') // Launched at this elid
  ->propertyOrderBy('elid');
  $result = $query->execute();
  if (isset($result['versioncontrol_event'])) {
    print count($result['versioncontrol_event']) . " events\n";
    foreach (array_chunk(array_keys($result['versioncontrol_event']), 50) as $elids) {
      foreach ($repository->loadEvents($elids) as $event) {
        print 'elid ' . $event->elid . "\n";
        foreach ($event as $ref) {
          if ($ref->eventCreatedMe()) {
            print "skipping\n";
            break 2;
          }
        }
        $processor->process($event);
      }
      drupal_static_reset();
    }
  }
}