[Release] Make release scripts aware of packed tags

tools/release/common_includes.py

Index: tools/release/common_includes.py
diff --git a/tools/release/common_includes.py b/tools/release/common_includes.py
index 19841a34a66d640ee07ebed50192194ffc2a2bb1..601668edc1159d315804cd7110f345ff73fca5f8 100644
--- a/tools/release/common_includes.py
+++ b/tools/release/common_includes.py
@@ -50,7 +50,8 @@ DAY_IN_SECONDS = 24 * 60 * 60
 PUSH_MSG_GIT_RE = re.compile(r".* \(based on (?P<git_rev>[a-fA-F0-9]+)\)$")
 PUSH_MSG_NEW_RE = re.compile(r"^Version \d+\.\d+\.\d+$")
 VERSION_FILE = os.path.join("include", "v8-version.h")
-VERSION_RE = re.compile(r"^\d+\.\d+\.\d+(?:\.\d+)?$")
+VERSION_RE_RAW = re.compile(r"^\d+\.\d+\.\d+(?:\.\d+)?$")
+VERSION_RE_TAGS = re.compile(r"^tags\/\d+\.\d+\.\d+(?:\.\d+)?$")
 
 # V8 base directory.
 V8_BASE = os.path.dirname(
@@ -607,10 +608,7 @@ class Step(GitRecipesMixin):
 
   def GetVersionTag(self, revision):
     tag = self.Git("describe --tags %s" % revision).strip()
-    if VERSION_RE.match(tag):
-      return tag
-    else:
-      return None
+    return self._SanitizeVersionTag(tag)
 
   def GetRecentReleases(self, max_age):
     # Make sure tags are fetched.
@@ -633,7 +631,17 @@ class Step(GitRecipesMixin):
 
     # Make sure tags are fetched.
     self.Git("fetch origin +refs/tags/*:refs/tags/*")
-    version = sorted(filter(VERSION_RE.match, self.vc.GetTags()),
+
+    all_tags = self.vc.GetTags()
+    only_version_tags = []
+
+    # Remove tags/ prefix because of packed refs.
+    for current_tag in all_tags:
+      version_tag = self._SanitizeVersionTag(current_tag)
+      if version_tag != None:
+        only_version_tags.append(version_tag)
+
+    version = sorted(only_version_tags,
                      key=SortingKey, reverse=True)[0]
     self["latest_version"] = version
     return version
@@ -712,6 +720,14 @@ class Step(GitRecipesMixin):
       output += "%s\n" % line
     TextToFile(output, version_file)
 
+  def _SanitizeVersionTag(self, tag):

[Michael Achenbach, 2015/11/05 13:08:38]

I'd prefer to just do this:

if tag.startswith('tags/'):
  return tag[len('tags/'):]
else:
  return tag

Then you can leave most of the other code as it is. Just prefix every use of
tags with _SanitizeVersionTag. And lists with map(_SanitizeVersionTag, tags)
WDYT?

Maybe make the method static to not get trouble with self.

[Michael Hablich, 2015/11/05 13:31:54]

I still need to check and filter for valid version tags => two loops instead of
one. With the current solution the validation does not need to be done in every
method.

I prefer leaving this as a private method because that is the current scope the
method is handling. If more is needed later on this can simply be refactored.
What do you mean with 'trouble with self'?

[Michael Achenbach, 2015/11/05 14:07:55]

I generally find functional things easier to read. E.g.:

map(_SanitizeVersionTag, tags)
-> a list of sanitized tags

filter(VERSION_RE.match, map(_SanitizeVersionTag, tags))
-> a list of sanitized tags that are versions

Code that has a local list and appends things to it under certain conditions is
harder to read IMO as you need to keep your eyes on different things.

Trouble with self: You might not be able to map a member function like above.
Better have it static, i.e. without self parameter. Or toplevel. The method
doesn't need to access self anyways.

[Michael Hablich, 2015/11/05 14:38:21]

Well, I converted the method to a function so it can be properly tested.

I disagree with 'map(_SanitizeVersionTag, tags)'. For me this is not easier to
read than 'only_version_tags = NormalizeVersionTags(all_tags)'. And it still
needs to loop two times over the list. See latest patchset.

+    if VERSION_RE_RAW.match(tag):
+      return tag
+    elif VERSION_RE_TAGS.match(tag):
+        return tag[5:]

[Michael Achenbach, 2015/11/05 13:08:38]

nit tag[len('tags/'):] might make the intention clearer here.

[Michael Hablich, 2015/11/05 13:31:54]

Acknowledged.

+    else:
+      return None
+
 
 class BootstrapStep(Step):
   MESSAGE = "Bootstapping v8 checkout."