[D6] Gitolite

Nice to see this! Not sure how it all works yet, but looks promising :)

> 2. The Content Type where Gitolite repositories will be managed must be called
"group". Otherwise the module will fail to add the field that manages repositories
in a group (this is configured by default if you are using Open Atrium).

Is that enforced by OG too? IIRC any node type can be declared a group. Might be an idea to follow that.

> 3. Enable the module, clear the cache and go to admin/settings/gitolite. In the first
field, set your gitolite-admin url. In order to test it, run the following drush task:
[path to drush] gitolite-load-config
If the task completed with no errors, it will load the contents of your gitolite.conf
file into the field "Main configuration for gitolite.conf" at admin/settings/gitolite.

Can the testing be done in the GUI too?

Also, I think you can say

$ drush foo

instead of

[path to drush] foo

And also:

> set your gitolite-admin url

What is that? can you give an example?

> [path to drush] gitolite
Which does:
1. Clone gitolite-admin to a temporary directory.

Can this be done by pointing to an existing clone of the gitolite admin?

> crontab -e # opens your crontab console. Then type:
*/1 * * * * /path/to/php /path/to/drush gitolite --root=/root/directory/of/your/site

Euw... why can't this run through hook_cron()?

// $Id$
/**
 * @file
 */

The $Id is no longer needed. The @file block should say something!

    case 'admin/modules#description':

IIRC this is obsolete; set it in the info file instead.

> 'title' => 'Gitolite Administration',

Should be sentence case.

> 'type' => MENU_NORMAL_ITEM,

Superfluous.

@juampy: you may want to run the code through the coder module: http://drupal.org/project/coder

There are still some minor things to fix:

Coder found 1 projects, 4 files, 1 critical warnings, 31 minor warnings, 0 warnings were flagged to be ignored

The attached patch fixes some of them:

• Trailing spaces and indentation
• A typo: s/Implemenation/Implementation/
• drupal_set_message() want all the placeholder filtered, for the installation message I use something like that:
  

    drupal_set_message(st('Your module settings are available under <a href="@link">Administer > Site configuration > @module_name</a>.',
      array(
        '@link' => url('admin/settings/gitolite'),
        '@module_name' => 'Gitolite',
      )
    ));
  

Regards,
Antonio

I have set this up in openatrium and can confirm it works. Looking over the code I can't see any major issues or problems with the code. I do have a few feature ideas but will post them in the issue queue for the module.

You want http://api.drupal.org/api/drupal/includes--common.inc/function/drupal_ge... rather than a clunky file_exists().

+  if (!variable_get('gitolite_content_type_name', FALSE)) {
+    $options = node_get_types('names');
+    $form['gitolite_node_types'] = array(
+      '#type' => 'radios',
+      '#title' => t('Select a content type to add the field "Git repositories" to'),
+      '#options' => $options,
+      '#default_value' => variable_get('gitolite_node_types', ''),
+      '#description' => t('This setting is mandatory for the module to work correcly. There  must be a content type acting as "group node".'),
+      '#required' => TRUE,
+    );
+  }
+  else {
+    $form['gitolite_repository'] = array(
+      '#type' => 'textfield',
+      '#title' => t('Full URL of gitolite-admin repository'),
+      '#default_value' => variable_get('gitolite_repository', ''),
+      '#description' => t("The url of the gitolite-admin repository. For example 
+        'git@domain.to.my.repo:gitolite-admin.git'."),
+      '#required' => TRUE,
+    );

You seem to be showing different form elements depending on the situation. That seems a bit confusing. It's better to always show the content type selector but lock it once set, and explain in the description text 'this may not be changed'. Same as parts of CCK.

@juampy - I'm not completely familiar with the current review process, so not really in a position to confirm everything has been done to satisfy the review requirements. But for other reviewers I can confirm that the module works as specified.

The one issue I do have, is with the modules design. Which I think is relevant to the review process since I prefer to see drupal modules trying to solve problems in generic ways which other developers can then build off. So I would prefer to see the gitolite module as its own module that focuses just on the gitolite configurations files and not integrating with openatrium at all.

There is the possiblity of a lot of interesting permission based configurations that could be incorporated into the modules functionality. Giving users access to just read or write specific repositories or branches. With my own use of gitolite I have created sandbox areas where a user can create their own repositories, that functionality would be really cool to be configurable trough a drupal project management site.

Then it would be great to have a submodule that is a feature which is spaces aware so will work with openatrium and allows you to enable git repositories just on specific groups. I would probably also make each repo a new node so you could add a title, description and any other field to it. Then you could use the node permissions as the basis of permissions which are set in gitolite.

I'm not sure this is a big enough reason to not accept the module in its current form though.

@juampy: It's great to see your interest on this topic. Actually I really wanted to have a open atrium integration, since some users there wanted some related features.

From the Version Control API perspective, you can re-use the abstraction there to integrate with gitosis/gitolite through Repository manager (ctools) plugins, actually there is an extension of the interface used for it at Version Control Git backend module. For a reference implementation, take a look to the default plugin provided by versioncontrol_git and the one used on d.o.

For the access control side you can integrate using vcs_auth (ctools) plugins. For reference implementation take a look to the two plugins provided by versioncontrol.

@sdboyer: Please correct me if I failed in guide somewhere here ;-)

@juampy - Yeah, let me second marvil07's comment that this is a great effort, I'm very excited to see it. And I'll also second the notion that it could (bordering on should) be built on top of the vc* suite. We've designed that whole system to be *the* way to keep track of repositories that you want to do something with. The visible functionality it provides does appear to be more oriented towards generating lists of commits, but tbh, that's the messiest stuff we have. Treating git repositories as entities, hooking into their events...that's the kind of thing the API is really great for.

What you're doing might require us to make some additions to the API to really be done comfortably, but we'd be super, super open to that. I'd even go so far as to say that we might want to roll the cck integration into vcapi's core. If you are interested The best place to really talk it over is probably irc, so please feel free to ping marvil07 or I in #drupal-vcs.

Integrating with vc* does pertain to @univate's comment in #11, in that it'd really be ideal if providing gitolite functionality was separate from integration with OA. Integration with OA is great, and something I've been hoping would happen for a while, but gitolite integration is a segment of functionality that's really useful outside of OA. So it'd be good to build in layers, gitolite first then OA on (even if it's contained in the same project package).

To be clear, your being granted an account is in no way predicated on your choice to integrate with vc* or not. I've done a very cursory review of the code, and it at least looks consistent with coding standards. I share some concerns over the logic & structure choices you've made - using crontab instead of hook_cron(), directly attaching to a node type instead of simply offering a CCK field + widget, no hook_requirements() verification that gitolite is installed/has been configured properly - but the approaches you've taken are very reasonable for first attempts. More importantly, though, it's not the policy to reject applications on the basis of how you've chosen to solve a problem - only that you have chosen an effective way to solve it.

I haven't actually tested the module out, otherwise I'd approve the application right now. Can someone verify that the module at least more or less works? If so, I'll wrap this up.

I can confirm the module works as advertised - I have tested it.

I do agree with everything @sdboyer has stated in regard to the choice of how it has been implemented and would certainly like to also see it re-factored into separate layer. But would also support this application as is and look forward to continual development in this area.

So, between #14 and #15, I think we can safely say that this is RTBC.

@juampy, when you are ready to do a Drupal 7 port, please ping me. At some point, I will be developing an alternative to Open Atrium with Drupal 7. It will probably more closely resemble Redmine than OA. This will be a critical piece of functionality, so I'd be very interested in helping with the port.

Forgot to set status >.<

If, for any reason, this application was to be blocked, then please re-open #1163170: [D6] Gitosis on behalf of the applicant.

juampy,

Saw your question in #drupal-codereview ... the next step is for a Git administrator to actually grant you the full project permission, after which they will mark the application 'fixed' (and you will see the 'Promote to full project' link in your sandbox).

wfm. @juampy, you've been granted full project permissions :) do find me in irc, please please!