Create project maintainers block and committers page

Seems like this needs to be provided by versioncontrol_project to be done right.

Upping version

I'm not sure that views enabling this block is the right answer, it'd need data from quite a few tables in order to work properly. It would need data from the vc_operations tables that are currently views enabled, but then it'd need to join that against the project via {versioncontrol_project_projects} which isn't too touch, but then it'd need to also join against the auth tables that are provided by the auth_handler plugins in vc_api which are pluggable, and we'd like to keep them relatively plugable. (without joining against the table this could work with the maintainers system directly on the properties that are loaded in $node->project['maintainers']).

I'm definitely open to suggestions though.

Yeah, given how complicated the logic for this block actually is, given that we're doing custom caching on it since core's usual block caching was insufficient for d.o's needs, I don't think it makes sense to try to do this in views. The queries are hard enough to write as it is -- I don't think it'd be wise to have to add a whole bunch of views relationship handlers and other goodies to try to get views to construct a potentially even more insane query for this. And, in fact, the way the block currently works, there's one query for all committers with their stats, and another query to find the current maintainers, and then we use some PHP code to reconcile those instead of trying to JOIN everything together into one extremely nasty query.

Anyway, point being, I doubt making this a view is going to be a win, at least not the block itself. The "View all committers" page (e.g.
http://drupal.org/node/3281/committers) probably could be a view. (Do we have a separate issue for that page, or are we just going to handle that in here?) It's possible that once we have a view for *that* page, the block won't be that much harder to do as a view.

Honestly, I can't really meaningfully comment on the implementation details until I either started seeing patches for the various approaches, or was trying to implement it myself. If I were to do so, I'd probably first see how terrible it is to get the "View all committers" page working via views. Based on that experience, I'd have a better idea of what to do with the block itself.

p.s. To clarify: core (or views) block caching is insufficient since we want this cache invalidated IFF there's a commit/push.

Upon some discussion with mikey_p and dww I agree. I think it would be nice to offer a simple 'commits' field to a user that gives you a configurable summary, but this particular block is really not views friendly.

Lets just hard code it.

Righto. Meanwhile mikey_p opened #1005698: Create pages listing committers/authors for a project to handle that page via a separate issue so this issue can remain focused on the block. It's possible #1005698 could be done via views, but let's talk about that over there. ;)

I started on this and ended up going the non-views route, and it seems to be working well. Since this shares a common query function and data structure with the committers page, i've closed #1005698: Create pages listing committers/authors for a project and rolled both patches into one for now.

My battery is dying right now but I'll try to post a detailed idea of what this patch is doing, and specific points that I'd like to have reviewed.

Here's an updated patch that changes the query up a bit if the repo is a git repo, and also clears the block cache when updating the maintainers.

Some points of note:

A) Since we can have authors of commits that aren't Drupal users, we do a CASE statement in the SQL to determine where to pull their name from, we could do this in PHP just as easily, but this seemed simpler when I wrote it.

B) Since we can have committers not in the Drupal users table, then it's a given that we could have multiple authors with the uid == 0. To overcome this I used their name as a key instead of UID, and made the function that return the maintainers work the same way. This felt kinda fragile with the possibility that someone could try to manipulate the name for their commits to 'Dries' or something like that, and then if they could get someone to push them, then they would collide with everyone's favorite BDFL. To get around this, I made the key a combination of "$uid:$name" which still seems like it could be fragile, but 0:Dries is not 1:Dries so this seems to work okay for now.

Other than those two items this is pretty much a 100% port from the CVS.module way of handling these things. I would love to switch these items over to views, but the complexity of the MIN, MAX and COUNT queries in addition to the multiple grouping seem to indicate that this is pretty much impossible with Views 2.

Yeah, I'm not at all surprised doing this in views isn't going to be easy. ;) I'm totally fine with this approach. Overall, it's looking great. I like how you've mostly kept the approach from cvs.module but taken the opportunity to clean up some things and make it a little more sane while you're at it. ;)

Haven't tested, but looking at the patch itself, here's what I saw:

A) I think we *need* to do the name via CASE in the SQL so that the tablesort SQL clause for ORDER BY actually works. Have you tested that?

B) Sure, the uid:username thing seems like the best solution given the circumstances. However, this PHPDoc is therefore no longer true:

+ *   Array containing objects of committer information, keyed by uid.

C) else if

Core coding standard is elseif, although...

D) Wouldn't a switch($op) be more sane for versioncontrol_project_block()?

E) Totally minor, but I'd rather the $query was only initialized once here:

+  $query = "SELECT o.author_uid as uid, MIN(o.date) AS first_commit, MAX(o.date) AS last_commit, COUNT(o.vc_op_id) AS commits, CASE WHEN uid = 0 THEN o.author WHEN uid != 0 THEN u.name END as name FROM {versioncontrol_operations} o INNER JOIN {users} u ON o.author_uid = u.uid WHERE o.repo_id = %d GROUP BY o.repo_id, o.author $order";
+  if ($repo->vcs == 'git') {
+    $query = "SELECT o.author_uid as uid, MIN(o.date) AS first_commit, MAX(o.date) AS last_commit, COUNT(o.vc_op_id) AS commits, CASE WHEN uid = 0 THEN go.author_name WHEN uid != 0 THEN u.name END as name FROM {versioncontrol_operations} o INNER JOIN {users} u ON o.author_uid = u.uid INNER JOIN {versioncontrol_git_operations} go ON o.vc_op_id = go.vc_op_id WHERE o.repo_id = %d GROUP BY o.repo_id, o.author $order";
+  }

I'd do this as:

  if ($repo->vcs == 'git') {
   $query = $something;
  }
  else {
   $query = $something_else;
  }

F) Probably the patch is perfect, but since I don't know the ins-and-outs of VCAPI's schema, I figured I should ask. ;) If it's okay to GROUP BY {versioncontrol_operations}.author why are we messing with UIDs and {versioncontrol_git_operations}.author_name? Rather, given all you've said in B about why we can't rely on the operation's author, why can we safely use that in GROUP BY?

G) Doesn't this do special things we don't want when UID == 0?

+      theme('username', $committer),

H) Instead of calling time() over and over again (twice per row in the table), let's just call it once, stash it in a variable, and use that everywhere in versioncontrol_project_committers_page(). In D7 there's a constant for this, in fact, b/c it was deemed enough of a performance hit to call that frequently on a single page load.

I) Is this block and page actually showing commits or pushes? I assume it's showing all commits (regardless of author) that were pushed into the canonical project repo, right? Do we want to clarify that in the UI somehow? All the text says "commit". Probably that's fine, and people are more used to understanding "commit" than "push" (or "comush" ;)). But, it'd be nice to clarify WTF we're actually displaying here for people who really understand. ;) Not sure exactly how that should look, but I thought I'd raise it for consideration. Probably there's nothing really to do here and we should just leave it.

J) In most of the code, we assume {versioncontrol_project_cache_block} could be for multiple blocks, and we use the block delta as a cid. However, this spot doesn't:

+      db_query("DELETE FROM {versioncontrol_project_cache_block}");

Seems like it should. ;) Or at least it needs a comment. Or we should call this table {versioncontrol_project_maintainers_block_cache} or something so we know it's specifically for that one block. If we're going to keep the code to just blast the whole table, I believe TRUNCATE is faster and better than DELETE FROM...

In fact, reading that comment and looking closely at versioncontrol_project_block_view() I see we're not caching the block itself in any way. We're really caching the committer array for the project. So maybe it should just be called {versioncontrol_project_committer_cache}.

AND, given that, maybe versioncontrol_project_get_project_committers() itself should be responsible for caching, instead of making callers do it. ;)

K) Utterly pedantic nitpick, but is "Versioncontrol project" the standard way we're referring to it in the rest of the codebase, or would "Version control project" or "VersionControl Project" or something be better?

L) Is there a good reason theme_versioncontrol_project_maintainer_list() is doing the <ul> and <li> markup itself instead of just using theme('item_list')?

M) versioncontrol_project_get_project_maintainers() is generating a lot of separate queries to {users}. Seems better to load all the UIDs we care about into an array, then query {users} once via IN to keep the total query count down. Especially in an API call like this.

That's all I could find. ;)

Great work on this!

Thanks,
-Derek

Just went over a decision in IRC that will greatly simplify the query and item B) along with the keying off uid and name by only including users that have accounts on d.o. We will not display stats for any committers that cannot be mapped to a Drupal user account.

A) This is no longer needed since we are only pulling in users that have a d.o account.

B) I had fixed this after rolling the patch, but this doesn't apply anymore so it's back to original.

C) & D) switched this to a switch statement.

E) & F) No longer need a separate query for git, no longer have grouping issues, see A)

G) worked fine anyway, it just marked unknown users as '(not verified)' which made a lot of sense, but this is now no longer an issues due to A)

H) Fixed all the extra calls to time().

I) This is mapping actually commits, not pushes, since a push can contain multiple commits, and as far as I know we aren't storing pushers anywhere in the db. This is using the author field which according to sdboyer is the more accurate field from the git metadata for each commit. Git allows the person performing a commit to give credit to another through the author field, although that is typically defaulted to the person doing the commit, so we use the author field regardless of who actually performed the commit. I'll leave the semantics of this up to the git team to determine if we need any messages here. I suspect that chance of committer and author being different are rather rare.

J) After reviewing the code and comment related to this, I don't think we actually need to clear any cache when the block settings are updated since they are only using in the theming, not fetching the data.

Further, I don't think caching in versioncontrol_project_get_project_committers() is the best way of handling the cache, since we'd have to key off the sort order in addition to whether or not only maintainers is requested.

Also renamed the table to {versioncontrol_project_maintainers_block_cache}.

K) Updated to 'Version Control Project' for now. We should probably circle back on this through all the VC* family at some point.

L) Now using theme('item_list') for this block.

M) No need to fetch name anymore. See A). We are doing a similar query in versioncontrol_project_project_maintainer_project_load() but I can fix that at a later time.

============= New item ====================

N) I suppose we should change the wording of the 'all committers' or wording on the committers page to indicate that anonymous committers aren't shown, but I'm not really sure how to word that and keep the link in the maintainers block short.

Thought I posted this, but I went ahead and committed the patch from #14 so that we could show this at the sprint demo.