use per-project logic to determine handling of certain things (cvslog vs. manual uploads)

i'm seeing quite a pattern here. ;) you've been very helpful at finding and patching these bugs, and i can't tell you how thrilled i am to finally be getting serious patch help with project*.

however, this is the Nth time (and almost certainly not the last) you've added this rather long line of code:

+  if (module_exists('cvs') && isset($node->project->cvs_repository) && $node->project->cvs_repository != 0) {

a) this makes the code longer and slightly harder to maintain (if we ever decide we want another or a different case in that if).

b) i fear this only creates more work for the versioncontrol abstraction efforts jpesto is doing.

to solve (a), and perhaps make (b) easier down the road, let's just add a helper method like project_use_cvs($project) or something (much like the existing project_use_taxonomy()), that does these tests. then, all the call sites just have to be:

// $node is a release node, so we pass $node->project... in other cases, it might just be $node
if (project_use_cvs($node->project)) {
  ...
}

and then the helper is just (for now):

function project_use_cvs($project) {
  if (module_exists('cvs')) {
    return isset($project->cvs_repository) && $project->cvs_repository != 0;
  }
}

if we were going to be really slick, we'd make this a hook, or at least a more generic helper function:

function project_use_versioncontrol($project) {
  if (module_exists('cvs') && isset($project->cvs_repository) && $project->cvs_repository != 0) {
    return TRUE;
  }
  if (module_exists('svn') && isset($project->svn_repository) && $project->svn_repository != 0) {
    return TRUE;
  }
  ...
}

i'll leave the making of it into a hook up to jpetso. ;) but, i'd like to see the simple project_user_cvs() helper for now, to at least consolidate all this logic in one place.

thanks!
-derek

Thanks for the patch...

Well, you already committed one patch using the logic in the others, so I didn't figure it would be a problem (see http://drupal.org/node/151490)

Right, that's what I meant by my comment: i'm seeing quite a pattern here. ;) ... however, this is the Nth time (and almost certainly not the last) you've added this rather long line of code

Whatever the impressions or mythology about my grip on every aspect of the project* codebase may be, I don't actually have it all in my working set all the time. ;) It was the fact you recently fixed another bug by the same means, then looking at your original patch here, that made me notice a pattern that justifies a different approach to these bugs... hope that makes sense.

And, you're gonna kick me for this, but I don't think I was clear with my previous comment. :( project_use_versioncontrol() doesn't make much sense as a generic function if we then turn around at the call-sites and do utterly CVS-specific things. Making it truly general-purpose requires adding hooks and other things that jpesto will be doing for his SoC effort. Therefore, given that all the call sites are still doing CVS-specific stuff, for now, it makes more sense to just fix this bug with a CVS-specific function, since that's all we actually support, anyway. Therefore, i'd run s/project_use_versioncontrol/project_use_cvs/ on your patch before I would commit it...

I gotta run right now, but I'll take care of this before committing, don't feel a need to re-roll this.

thanks, again!
-derek

Reviewed, tested, and committed to HEAD. Backported to DRUPAL-4-7--2 and committed, there, too. Too much of a pain to backport all the way to DRUPAL-4-7, and I don't think I care. ;) Thanks!

FYI: this patch was the source of a critical bug on drupal.org: http://drupal.org/node/160585. See the patch there for details... Slipped by both of us -- alas. ;)

Geeze, I guess we really needed more careful testing and review... this patch also caused http://drupal.org/node/161552 ;)