Issue 7004 - Actually return git's result

packager.py

 # This Source Code Form is subject to the terms of the Mozilla Public
 # License, v. 2.0. If a copy of the MPL was not distributed with this
 # file, You can obtain one at http://mozilla.org/MPL/2.0/.
 
 # Note: These are the base functions common to all packagers, the actual
 # packagers are implemented in packagerChrome and packagerEdge.
 
 import sys
 import os
 import re
@@ skipping 58 matching lines @@
     try:
         from buildtools.ensure_dependencies import Mercurial, Git
         if Mercurial().istype(baseDir):
             result = subprocess.check_output(['hg', 'id', '-R', baseDir, '-n'])
             return re.sub(r'\D', '', result)
         elif Git().istype(baseDir):
             result = subprocess.check_output(
                 ['git', 'rev-list', '--count', '--branches', '--tags'],
                 cwd=baseDir,
             )
-            return re.sub(r'\D', '', result)

[Sebastian Noack, 2018/10/08 13:28:25]

Wouldn't strip() do?

[tlucas, 2018/10/08 13:35:28]

It does :) Done.

+            return result.strip()

[Sebastian Noack, 2018/10/08 14:01:04]

Perhaps we can unify the code paths here:

  if Mercurial().istype(baseDir):
      output = subprocess.check_output(['hg', 'id', '-R', baseDir, '-n'])
  else:
      output = subprocess.check_output(
          ['git', 'rev-list', '--count', '--branches', '--tags'],
          cwd=baseDir,
      )
  return re.sub(r'\D', '', output)

1. While strip() would be sufficient for Git it's not for Mercurial (as it might
add a plus character at the end). So if the code path is unified we have to go
back using the regexp for both cases.
2. Explicitly checking for Git is redundant since if it's neither a Mercurial
nor a Git repository CalledProcess Error is raised which is handled below.

[tlucas, 2018/10/08 14:19:42]

I see how this would improve the code, but since we already agreed on eventually
moving the buildtools to Node.js, i'd rather not introduce any cosmetic changes.

What do you think?

[Sebastian Noack, 2018/10/08 14:26:06]

I think the requested change is trivial, and makes this code easier to maintain
for the the time being (and who knows how long that will be in the end). If we'd
be talking about a larger refactoring, I'd agree of course.

But it's probably not worth to argue about it. LGTM, either with or without my
suggested change.

     except subprocess.CalledProcessError:
         pass
 
     return '0'
 
 
 def getBuildVersion(baseDir, metadata, releaseBuild, buildNum=None):
     version = metadata.get('general', 'version')
     if not releaseBuild:
         if buildNum == None:
@@ skipping 77 matching lines @@
 
     def zip(self, outFile, sortKey=None, compression=zipfile.ZIP_DEFLATED):
         with zipfile.ZipFile(outFile, 'w', compression) as zf:
             for name in sorted(self, key=sortKey):
                 zf.writestr(name, self[name])
 
     def zipToString(self, sortKey=None):
         buffer = StringIO()
         self.zip(buffer, sortKey=sortKey)
         return buffer.getvalue()