[Release] Make release scripts aware of packed tags

[Release] Make release scripts aware of packed tags

We are creating a lot of tags. The result is that git 'packs'
some of them. Our parsers in the release script cannot handle
this.

R=machenbach@chromium.org
LONG=N
NOTRY=true

Committed: https://crrev.com/760eb0a9f4e5d51a763f0eea0b0be494feb98c23
Cr-Commit-Position: refs/heads/master@{#31833}

[Michael Hablich, 2015-11-05 12:48:17]

Description was changed from

==========
[Release] Fix the handling of tags in the release scripts

We are creating a lot of tags. The result is that git 'packs'
some of them. Our parsers in the release script cannot handle
this.

R=machenbach@chromium.org
LONG=N
NOTRY=true
==========

to

==========
[Release] Make release scripts aware of packed tags

We are creating a lot of tags. The result is that git 'packs'
some of them. Our parsers in the release script cannot handle
this.

R=machenbach@chromium.org
LONG=N
NOTRY=true
==========

[Michael Hablich, 2015-11-05 12:48:56]

PTAL

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

A simplification suggestion.

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
File tools/release/common_includes.py (right):

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:723: def _SanitizeVersionTag(self, tag):
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.

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:727: return tag[5:]
nit tag[len('tags/'):] might make the intention clearer here.

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

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
File tools/release/common_includes.py (right):

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:723: def _SanitizeVersionTag(self, tag):
On 2015/11/05 13:08:38, Michael Achenbach wrote:
> 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.

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'?

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:727: return tag[5:]
On 2015/11/05 13:08:38, Michael Achenbach wrote:
> nit tag[len('tags/'):] might make the intention clearer here.

Acknowledged.

[Michael Achenbach, 2015-11-05 14:07:56]

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
File tools/release/common_includes.py (right):

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:723: def _SanitizeVersionTag(self, tag):
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]

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
File tools/release/common_includes.py (right):

https://codereview.chromium.org/1403293009/diff/1/tools/release/common_includ...
tools/release/common_includes.py:723: def _SanitizeVersionTag(self, tag):
On 2015/11/05 14:07:55, Michael Achenbach wrote:
> 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.

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.

[Michael Achenbach, 2015-11-05 14:58:39]

ok - lgtm then

[Michael Hablich, 2015-11-05 15:30:45]

The CQ bit was checked by hablich@chromium.org

[commit-bot: I haz the power, 2015-11-05 15:30:57]

CQ is trying da patch. Follow status at
 https://chromium-cq-status.appspot.com/patch-status/1403293009/60001
View timeline at
 https://chromium-cq-status.appspot.com/patch-timeline/1403293009/60001

[commit-bot: I haz the power, 2015-11-05 17:26:46]

Committed patchset #4 (id:60001)

[commit-bot: I haz the power, 2015-11-05 17:27:30]

Patchset 4 (id:??) landed as
https://crrev.com/760eb0a9f4e5d51a763f0eea0b0be494feb98c23
Cr-Commit-Position: refs/heads/master@{#31833}