now that everything from http://drupal.org/node/84706 is committed to DRUPAL-4-7--2 branch, and in the process, i added the cvs_local.inc file as cvs_local.example.inc (so it's disabled by default), i noticed that we currently assume cvs_get_version_from_tag() exists, even though it lives in cvs_local.inc which is supposed to be optional.

so, we should probably rename the version in cvs_local.inc to be cvs_local_get_version_from_tag() and add a version of cvs_get_version_from_tag() directly in cvs.module that conditionally checks if cvs_local.inc is being used. if so, we use it, and if not, we return some kind of dummy value (e.g. put the whole tag name into version_extra by default or something).

CommentFileSizeAuthor
#3 91195_cvs.patch_2.txt2.74 KBdww
#1 91195_cvs.patch.txt2.71 KBdww

Comments

dww’s picture

dww
we/he/they
commented
Version: 4.7.x-1.x-dev » 4.7.x-2.x-dev
Assigned: Unassigned » dww
Status: Active » Needs review
StatusFileSize
new 91195_cvs.patch.txt2.71 KB
drewish’s picture

drewish commented
Status: Needs review » Needs work

not sure about:

+function cvs_get_version_from_tag($tag, $project) {
+  if (function_exists('cvs_local_get_version_from_tag')) {
+    return cvs_local_get_version_from_tag($tag, $project);
+  }
+  $version->version_extra = $tag->tag;
+  return $version;
+}

where is $version defined? can you just assign properties to an undefined object like that? even if it's legal i think it'd be better to explicitly define $version = new stdClass();

other than that everything *looks* okay. i don't have cvs setup on this machine so i didn't apply it.

dww’s picture

dww
we/he/they
commented
Status: Needs work » Needs review
StatusFileSize
new 91195_cvs.patch_2.txt2.74 KB

Yeah, it works, but it's probably generating warnings, and might not work on PHP5. ;) Good catch.

hunmonk’s picture

hunmonk commented
Status: Needs review » Reviewed & tested by the community
dww’s picture

dww
we/he/they
commented
Status: Reviewed & tested by the community » Fixed

Fixed typo in comment and committed to HEAD and DRUPAL-4-7--2.

Anonymous’s picture

(not verified) commented
Status: Fixed » Closed (fixed)