postgres support

i don't think you need to bother capturing the results of db_query() in cvs_install().

bunch of tabs in cvs_update_7().

can't you quote 'user' to make it pgsql safe? username would be better than t_user...

@drewish: in that case, please set the status appropriately. ;)

@hunmonk: very cool, thanks for the start. haven't tested yet, but visual inspection of the patch revealed 1 other minor readability personal-preference thing, aside from the issues that drewish mentioned. in cvs_uninstall(), for the huge block of variables to delete, i'd rather just populate an array of variable names and then foreach() it, much like we do with the tables to drop. so, instead of:

  variable_del('cvs_email_address');
  variable_del('cvs_use_file');
  variable_del('cvs_default_repo');
  ...

I'd rather see:

  $variables = array(
    'cvs_email_address',
    'cvs_use_file',
    'cvs_default_repo',
    ...
  );
  foreach ($variables as $variable) {
    variable_del($variable);
  }

That's just a personal preference, but I find it's nicer to just add something to an array than copy-paste a whole other variable_del() line. But, certainly all those tabs gotta go, if nothing else. ;)

Will test/review more closely once there's a new patch.

Thanks!
-Derek

{cvs_messages}.user is the account name of the CVS user who committed a given change. In general, this is the same as the drupal username, but not always. Furthermore, it's entirely possible that someone commits to a repo who doesn't have a drupal account at all, in which case the uid field is 0, but the user field is filled in.

Therefore, if we're going to rename the field anyway, maybe it should be called "cvs_user" to make it clear this is the username of the CVS account. While we're at it, perhaps we should rename {cvs_accounts}.name to also be "cvs_user" to match.

Thanks for the array-driven implementation for deleting variables in cvs_uninstall(). However, variable_del() does some other stuff, besides *just* removing the entry from the {variables} table. It also clears the entire 'variables' entry in the cache, and unsets the $conf entry for the particular variable. Not sure we need to do about clearing the $conf entry, since we're going to just reload the page after we hit the uninstall hook, so we'll bootstrap again and make a new $conf array. However, if you put the manual pruning of {variables} *before* any of the calls to variable_del() (like you had originally), then the cache will be appropriately cleared by the time we're rebuilding $conf.

Thanks for working on this!
-Derek

p.s. If you care about pgsql support for this module, I encourage you to take a look at http://drupal.org/node/136866.

visual inspection reveals no problems. updating status accordingly. ;) agreed, this requires a more careful review and testing before we can commit, but it's certainly close.

A) does UNIQUE (name) automatically create an index in pgsql?

B) tabs in cvs_uninstall()

otherwise, i tested cvs_update_7() on MySQL and it's working fine. altering the field for the primary key works, and the primary key is then on cvs_user, which is great. a clean install also works, once i patched the d.o testing profile to use the new field names. ;)

Attached patch includes the (trivial) changes for the test profile, and fixes the tabs in cvs_uninstall(). So, if you're happy with (A), I think this is RTBC.

so it turns out that the generate-cvs-passwd.php script wasn't updated for this change. nnewton showed up in IRC and helped up figure out what the problem is.

yeah the script isn't in CVS, but this change affected it so i made a note on the issue. i'd forgotten that dww wasn't going to be back around to take care of it. i looks like nnewton got it fixed though.