split out cli validation from execution

A common pattern in our subcommands is to verify the arguments &
options before executing things.  For some subcommands, that check
stage is quite long which makes the execution function even bigger.
Lets split that logic out of the execute phase so it's easier to
manage these.

This is most noticeable in the sync subcommand whose Execute func
is quite large, and the option checking makes up ~15% of it.

The manifest command's Execute can be simplified significantly as
the optparse configuration always sets output_file to a string.

Change-Id: I7097847ff040e831345e63de6b467ee17609990e
Reviewed-on: https://gerrit-review.googlesource.com/c/git-repo/+/234834
Reviewed-by: David Pursehouse <dpursehouse@collab.net>
Tested-by: Mike Frysinger <vapier@google.com>

command.py

@@ -98,6 +98,16 @@ class Command(object):
     self.OptionParser.print_usage()
     sys.exit(1)
 
+  def ValidateOptions(self, opt, args):
+    """Validate the user options & arguments before executing.
+
+    This is meant to help break the code up into logical steps.  Some tips:
+    * Use self.OptionParser.error to display CLI related errors.
+    * Adjust opt member defaults as makes sense.
+    * Adjust the args list, but do so inplace so the caller sees updates.
+    * Try to avoid updating self state.  Leave that to Execute.
+    """
+
   def Execute(self, opt, args):
     """Perform the action, after option parsing is complete.
     """

docs/manifest-format.md

@@ -322,13 +322,29 @@ Zero or more copyfile elements may be specified as children of a
 project element. Each element describes a src-dest pair of files;
 the "src" file will be copied to the "dest" place during `repo sync`
 command.
+
 "src" is project relative, "dest" is relative to the top of the tree.
+Copying from paths outside of the project or to paths outside of the repo
+client is not allowed.
+
+"src" and "dest" must be files.  Directories or symlinks are not allowed.
+Intermediate paths must not be symlinks either.
+
+Parent directories of "dest" will be automatically created if missing.
 
 ### Element linkfile
 
 It's just like copyfile and runs at the same time as copyfile but
 instead of copying it creates a symlink.
 
+The symlink is created at "dest" (relative to the top of the tree) and
+points to the path specified by "src".
+
+Parent directories of "dest" will be automatically created if missing.
+
+The symlink target may be a file or directory, but it may not point outside
+of the repo client.
+
 ### Element remove-project
 
 Deletes the named project from the internal manifest table, possibly

docs/python-support.md

@@ -28,5 +28,20 @@ The master branch will require Python 3.6 at a minimum.
 If the system has an older version of Python 3, then users will have to select
 the legacy Python 2 branch instead.
 
+### repo hooks
 
+Projects that use [repo hooks] run on independent schedules.
+They might migrate to Python 3 earlier or later than us.
+To support them, we'll probe the shebang of the hook script and if we find an
+interpreter in there that indicates a different version than repo is currently
+running under, we'll attempt to reexec ourselves under that.
+
+For example, a hook with a header like `#!/usr/bin/python2` will have repo
+execute `/usr/bin/python2` to execute the hook code specifically if repo is
+currently running Python 3.
+
+For more details, consult the [repo hooks] documentation.
+
+
+[repo hooks]: ./repo-hooks.md
 [repo launcher]: ../repo

docs/repo-hooks.md

@@ -83,6 +83,31 @@ then check it directly.  Hooks should not normally modify the active git repo
 the user.  Although user interaction is discouraged in the common case, it can
 be useful when deploying automatic fixes.
 
+### Shebang Handling
+
+*** note
+This is intended as a transitional feature.  Hooks are expected to eventually
+migrate to Python 3 only as Python 2 is EOL & deprecated.
+***
+
+If the hook is written against a specific version of Python (either 2 or 3),
+the script can declare that explicitly.  Repo will then attempt to execute it
+under the right version of Python regardless of the version repo itself might
+be executing under.
+
+Here are the shebangs that are recognized.
+
+* `#!/usr/bin/env python` & `#!/usr/bin/python`: The hook is compatible with
+  Python 2 & Python 3.  For maximum compatibility, these are recommended.
+* `#!/usr/bin/env python2` & `#!/usr/bin/python2`: The hook requires Python 2.
+  Version specific names like `python2.7` are also recognized.
+* `#!/usr/bin/env python3` & `#!/usr/bin/python3`: The hook requires Python 3.
+  Version specific names like `python3.6` are also recognized.
+
+If no shebang is detected, or does not match the forms above, we assume that the
+hook is compatible with both Python 2 & Python 3 as if `#!/usr/bin/python` was
+used.
+
 ## Hooks
 
 Here are all the points available for hooking.

git_command.py

@@ -23,7 +23,7 @@ from signal import SIGTERM
 
 from error import GitError
 import platform_utils
-from trace import REPO_TRACE, IsTrace, Trace
+from repo_trace import REPO_TRACE, IsTrace, Trace
 from wrapper import Wrapper
 
 GIT = 'git'

git_config.py

@@ -44,7 +44,7 @@ else:
 from signal import SIGTERM
 from error import GitError, UploadError
 import platform_utils
-from trace import Trace
+from repo_trace import Trace
 if is_python3():
   from http.client import HTTPException
 else:
@@ -699,7 +699,8 @@ class Remote(object):
     if not rev.startswith(R_HEADS):
       return rev
 
-    raise GitError('remote %s does not have %s' % (self.name, rev))
+    raise GitError('%s: remote %s does not have %s' %
+                   (self.projectname, self.name, rev))
 
   def WritesTo(self, ref):
     """True if the remote stores to the tracking ref.

git_refs.py

@@ -15,7 +15,7 @@
 # limitations under the License.
 
 import os
-from trace import Trace
+from repo_trace import Trace
 import platform_utils
 
 HEAD      = 'HEAD'

main.py

@@ -45,7 +45,7 @@ except ImportError:
 
 from color import SetDefaultColoring
 import event_log
-from trace import SetTrace
+from repo_trace import SetTrace
 from git_command import git, GitCommand
 from git_config import init_ssh, close_ssh
 from command import InteractiveCommand
@@ -84,7 +84,10 @@ global_options.add_option('--color',
                           help='control color usage: auto, always, never')
 global_options.add_option('--trace',
                           dest='trace', action='store_true',
-                          help='trace git command execution')
+                          help='trace git command execution (REPO_TRACE=1)')
+global_options.add_option('--trace-python',
+                          dest='trace_python', action='store_true',
+                          help='trace python command execution')
 global_options.add_option('--time',
                           dest='time', action='store_true',
                           help='time repo command execution')
@@ -102,8 +105,8 @@ class _Repo(object):
     # add 'branch' as an alias for 'branches'
     all_commands['branch'] = all_commands['branches']
 
-  def _Run(self, argv):
-    result = 0
+  def _ParseArgs(self, argv):
+    """Parse the main `repo` command line options."""
     name = None
     glob = []
 
@@ -120,6 +123,12 @@ class _Repo(object):
       argv = []
     gopts, _gargs = global_options.parse_args(glob)
 
+    return (name, gopts, argv)
+
+  def _Run(self, name, gopts, argv):
+    """Execute the requested subcommand."""
+    result = 0
+
     if gopts.trace:
       SetTrace()
     if gopts.show_version:
@@ -188,6 +197,7 @@ class _Repo(object):
     cmd_event = cmd.event_log.Add(name, event_log.TASK_COMMAND, start)
     cmd.event_log.SetParent(cmd_event)
     try:
+      cmd.ValidateOptions(copts, cargs)
       result = cmd.Execute(copts, cargs)
     except (DownloadError, ManifestInvalidRevisionError,
         NoManifestException) as e:
@@ -526,7 +536,15 @@ def _Main(argv):
     try:
       init_ssh()
       init_http()
-      result = repo._Run(argv) or 0
+      name, gopts, argv = repo._ParseArgs(argv)
+      run = lambda: repo._Run(name, gopts, argv) or 0
+      if gopts.trace_python:
+        import trace
+        tracer = trace.Trace(count=False, trace=True, timing=True,
+                             ignoredirs=set(sys.path[1:]))
+        result = tracer.runfunc(run)
+      else:
+        result = run()
     finally:
       close_ssh()
   except KeyboardInterrupt:

progress.py

@@ -17,7 +17,7 @@
 import os
 import sys
 from time import time
-from trace import IsTrace
+from repo_trace import IsTrace
 
 _NOT_TTY = not os.isatty(2)
 

project.py

@@ -18,6 +18,7 @@ from __future__ import print_function
 import errno
 import filecmp
 import glob
+import json
 import os
 import random
 import re
@@ -38,7 +39,7 @@ from error import GitError, HookError, UploadError, DownloadError
 from error import ManifestInvalidRevisionError
 from error import NoManifestException
 import platform_utils
-from trace import IsTrace, Trace
+from repo_trace import IsTrace, Trace
 
 from git_refs import GitRefs, HEAD, R_HEADS, R_TAGS, R_PUB, R_M
 
@@ -544,6 +545,105 @@ class RepoHook(object):
         prompt % (self._GetMustVerb(), self._script_fullpath),
         'Scripts have changed since %s was allowed.' % (self._hook_type,))
 
+  @staticmethod
+  def _ExtractInterpFromShebang(data):
+    """Extract the interpreter used in the shebang.
+
+    Try to locate the interpreter the script is using (ignoring `env`).
+
+    Args:
+      data: The file content of the script.
+
+    Returns:
+      The basename of the main script interpreter, or None if a shebang is not
+      used or could not be parsed out.
+    """
+    firstline = data.splitlines()[:1]
+    if not firstline:
+      return None
+
+    # The format here can be tricky.
+    shebang = firstline[0].strip()
+    m = re.match(r'^#!\s*([^\s]+)(?:\s+([^\s]+))?', shebang)
+    if not m:
+      return None
+
+    # If the using `env`, find the target program.
+    interp = m.group(1)
+    if os.path.basename(interp) == 'env':
+      interp = m.group(2)
+
+    return interp
+
+  def _ExecuteHookViaReexec(self, interp, context, **kwargs):
+    """Execute the hook script through |interp|.
+
+    Note: Support for this feature should be dropped ~Jun 2021.
+
+    Args:
+      interp: The Python program to run.
+      context: Basic Python context to execute the hook inside.
+      kwargs: Arbitrary arguments to pass to the hook script.
+
+    Raises:
+      HookError: When the hooks failed for any reason.
+    """
+    # This logic needs to be kept in sync with _ExecuteHookViaImport below.
+    script = """
+import json, os, sys
+path = '''%(path)s'''
+kwargs = json.loads('''%(kwargs)s''')
+context = json.loads('''%(context)s''')
+sys.path.insert(0, os.path.dirname(path))
+data = open(path).read()
+exec(compile(data, path, 'exec'), context)
+context['main'](**kwargs)
+""" % {
+        'path': self._script_fullpath,
+        'kwargs': json.dumps(kwargs),
+        'context': json.dumps(context),
+    }
+
+    # We pass the script via stdin to avoid OS argv limits.  It also makes
+    # unhandled exception tracebacks less verbose/confusing for users.
+    cmd = [interp, '-c', 'import sys; exec(sys.stdin.read())']
+    proc = subprocess.Popen(cmd, stdin=subprocess.PIPE)
+    proc.communicate(input=script.encode('utf-8'))
+    if proc.returncode:
+      raise HookError('Failed to run %s hook.' % (self._hook_type,))
+
+  def _ExecuteHookViaImport(self, data, context, **kwargs):
+    """Execute the hook code in |data| directly.
+
+    Args:
+      data: The code of the hook to execute.
+      context: Basic Python context to execute the hook inside.
+      kwargs: Arbitrary arguments to pass to the hook script.
+
+    Raises:
+      HookError: When the hooks failed for any reason.
+    """
+    # Exec, storing global context in the context dict.  We catch exceptions
+    # and convert to a HookError w/ just the failing traceback.
+    try:
+      exec(compile(data, self._script_fullpath, 'exec'), context)
+    except Exception:
+      raise HookError('%s\nFailed to import %s hook; see traceback above.' %
+                      (traceback.format_exc(), self._hook_type))
+
+    # Running the script should have defined a main() function.
+    if 'main' not in context:
+      raise HookError('Missing main() in: "%s"' % self._script_fullpath)
+
+    # Call the main function in the hook.  If the hook should cause the
+    # build to fail, it will raise an Exception.  We'll catch that convert
+    # to a HookError w/ just the failing traceback.
+    try:
+      context['main'](**kwargs)
+    except Exception:
+      raise HookError('%s\nFailed to run main() for %s hook; see traceback '
+                      'above.' % (traceback.format_exc(), self._hook_type))
+
   def _ExecuteHook(self, **kwargs):
     """Actually execute the given hook.
 
@@ -568,19 +668,8 @@ class RepoHook(object):
       # hooks can't import repo files.
       sys.path = [os.path.dirname(self._script_fullpath)] + sys.path[1:]
 
-      # Exec, storing global context in the context dict.  We catch exceptions
-      # and  convert to a HookError w/ just the failing traceback.
+      # Initial global context for the hook to run within.
       context = {'__file__': self._script_fullpath}
-      try:
-        exec(compile(open(self._script_fullpath).read(),
-                     self._script_fullpath, 'exec'), context)
-      except Exception:
-        raise HookError('%s\nFailed to import %s hook; see traceback above.' %
-                        (traceback.format_exc(), self._hook_type))
-
-      # Running the script should have defined a main() function.
-      if 'main' not in context:
-        raise HookError('Missing main() in: "%s"' % self._script_fullpath)
 
       # Add 'hook_should_take_kwargs' to the arguments to be passed to main.
       # We don't actually want hooks to define their main with this argument--
@@ -592,15 +681,31 @@ class RepoHook(object):
       kwargs = kwargs.copy()
       kwargs['hook_should_take_kwargs'] = True
 
-      # Call the main function in the hook.  If the hook should cause the
-      # build to fail, it will raise an Exception.  We'll catch that convert
-      # to a HookError w/ just the failing traceback.
-      try:
-        context['main'](**kwargs)
-      except Exception:
-        raise HookError('%s\nFailed to run main() for %s hook; see traceback '
-                        'above.' % (traceback.format_exc(),
-                                    self._hook_type))
+      # See what version of python the hook has been written against.
+      data = open(self._script_fullpath).read()
+      interp = self._ExtractInterpFromShebang(data)
+      reexec = False
+      if interp:
+        prog = os.path.basename(interp)
+        if prog.startswith('python2') and sys.version_info.major != 2:
+          reexec = True
+        elif prog.startswith('python3') and sys.version_info.major == 2:
+          reexec = True
+
+      # Attempt to execute the hooks through the requested version of Python.
+      if reexec:
+        try:
+          self._ExecuteHookViaReexec(interp, context, **kwargs)
+        except OSError as e:
+          if e.errno == errno.ENOENT:
+            # We couldn't find the interpreter, so fallback to importing.
+            reexec = False
+          else:
+            raise
+
+      # Run the hook by importing directly.
+      if not reexec:
+        self._ExecuteHookViaImport(data, context, **kwargs)
     finally:
       # Restore sys.path and CWD.
       sys.path = orig_syspath
@@ -1036,6 +1141,8 @@ class Project(object):
                    capture_stderr=True)
     has_diff = False
     for line in p.process.stdout:
+      if not hasattr(line, 'encode'):
+        line = line.decode()
       if not has_diff:
         out.nl()
         out.project('project %s/' % self.relpath)
@@ -1490,7 +1597,7 @@ class Project(object):
     last_mine = None
     cnt_mine = 0
     for commit in local_changes:
-      commit_id, committer_email = commit.decode('utf-8').split(' ', 1)
+      commit_id, committer_email = commit.split(' ', 1)
       if committer_email == self.UserEmail:
         last_mine = commit_id
         cnt_mine += 1
@@ -2094,6 +2201,8 @@ class Project(object):
     if not current_branch_only:
       # Fetch whole repo
       spec.append(str((u'+refs/heads/*:') + remote.ToLocal('refs/heads/*')))
+      if not (no_tags or depth):
+        spec.append(str((u'+refs/tags/*:') + remote.ToLocal('refs/tags/*')))
     elif tag_name is not None:
       spec.append('tag')
       spec.append(tag_name)
@@ -2301,10 +2410,7 @@ class Project(object):
     cmd = ['ls-remote', self.remote.name, refs]
     p = GitCommand(self, cmd, capture_stdout=True)
     if p.Wait() == 0:
-      if hasattr(p.stdout, 'decode'):
-        return p.stdout.decode('utf-8')
-      else:
-        return p.stdout
+      return p.stdout
     return None
 
   def _Revert(self, rev):
@@ -2715,6 +2821,8 @@ class Project(object):
                      capture_stderr=True)
       try:
         out = p.process.stdout.read()
+        if not hasattr(out, 'encode'):
+          out = out.decode()
         r = {}
         if out:
           out = iter(out[:-1].split('\0'))
@@ -2874,10 +2982,6 @@ class Project(object):
           raise GitError('%s %s: %s' %
                          (self._project.name, name, p.stderr))
         r = p.stdout
-        try:
-          r = r.decode('utf-8')
-        except AttributeError:
-          pass
         if r.endswith('\n') and r.index('\n') == len(r) - 1:
           return r[:-1]
         return r

repo_trace.py

@@ -14,15 +14,19 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Logic for tracing repo interactions.
+
+Activated via `repo --trace ...` or `REPO_TRACE=1 repo ...`.
+"""
+
 from __future__ import print_function
 import sys
 import os
+
+# Env var to implicitly turn on tracing.
 REPO_TRACE = 'REPO_TRACE'
 
-try:
-  _TRACE = os.environ[REPO_TRACE] == '1'
-except KeyError:
-  _TRACE = False
+_TRACE = os.environ.get(REPO_TRACE) == '1'
 
 def IsTrace():
   return _TRACE

subcmds/abandon.py

@@ -37,19 +37,19 @@ It is equivalent to "git branch -D <branchname>".
                  dest='all', action='store_true',
                  help='delete all branches in all projects')
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if not opt.all and not args:
       self.Usage()
 
     if not opt.all:
       nb = args[0]
       if not git.check_ref_format('heads/%s' % nb):
-        print("error: '%s' is not a valid name" % nb, file=sys.stderr)
-        sys.exit(1)
+        self.OptionParser.error("'%s' is not a valid branch name" % nb)
     else:
-      args.insert(0,None)
-      nb = "'All local branches'"
+      args.insert(0, "'All local branches'")
 
+  def Execute(self, opt, args):
+    nb = args[0]
     err = defaultdict(list)
     success = defaultdict(list)
     all_projects = self.GetProjects(args[1:])

subcmds/checkout.py

@@ -34,10 +34,11 @@ The command is equivalent to:
   repo forall [<project>...] -c git checkout <branchname>
 """
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if not args:
       self.Usage()
 
+  def Execute(self, opt, args):
     nb = args[0]
     err = []
     success = []

subcmds/cherry_pick.py

@@ -37,10 +37,11 @@ change id will be added.
   def _Options(self, p):
     pass
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if len(args) != 1:
       self.Usage()
 
+  def Execute(self, opt, args):
     reference = args[0]
 
     p = GitCommand(None,

subcmds/diffmanifests.py

@@ -176,10 +176,11 @@ synced and their revisions won't be found.
             self.printText(log)
             self.out.nl()
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if not args or len(args) > 2:
-      self.Usage()
+      self.OptionParser.error('missing manifests to diff')
 
+  def Execute(self, opt, args):
     self.out = _Coloring(self.manifest.globalConfig)
     self.printText = self.out.nofmt_printer('text')
     if opt.color:

subcmds/forall.py

@@ -177,10 +177,11 @@ without iterating through the remaining projects.
       'worktree': project.worktree,
     }
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if not opt.command:
       self.Usage()
 
+  def Execute(self, opt, args):
     cmd = [opt.command[0]]
 
     shell = True

subcmds/help.py

@@ -88,7 +88,7 @@ Displays detailed usage information about a command.
 "See 'repo help <command>' for more information on a specific command.\n"
 "See 'repo help --all' for a complete list of recognized commands.")
 
-  def _PrintCommandHelp(self, cmd):
+  def _PrintCommandHelp(self, cmd, header_prefix=''):
     class _Out(Coloring):
       def __init__(self, gc):
         Coloring.__init__(self, gc, 'help')
@@ -106,7 +106,7 @@ Displays detailed usage information about a command.
 
         self.nl()
 
-        self.heading('%s', heading)
+        self.heading('%s%s', header_prefix, heading)
         self.nl()
         self.nl()
 
@@ -124,7 +124,7 @@ Displays detailed usage information about a command.
 
           m = asciidoc_hdr.match(para)
           if m:
-            self.heading(m.group(1))
+            self.heading('%s%s', header_prefix, m.group(1))
             self.nl()
             self.nl()
             continue
@@ -138,14 +138,25 @@ Displays detailed usage information about a command.
     cmd.OptionParser.print_help()
     out._PrintSection('Description', 'helpDescription')
 
+  def _PrintAllCommandHelp(self):
+    for name in sorted(self.commands):
+      cmd = self.commands[name]
+      cmd.manifest = self.manifest
+      self._PrintCommandHelp(cmd, header_prefix='[%s] ' % (name,))
+
   def _Options(self, p):
     p.add_option('-a', '--all',
                  dest='show_all', action='store_true',
                  help='show the complete list of commands')
+    p.add_option('--help-all',
+                 dest='show_all_help', action='store_true',
+                 help='show the --help of all commands')
 
   def Execute(self, opt, args):
     if len(args) == 0:
-      if opt.show_all:
+      if opt.show_all_help:
+        self._PrintAllCommandHelp()
+      elif opt.show_all:
         self._PrintAllCommands()
       else:
         self._PrintCommonCommands()

subcmds/init.py

@@ -436,18 +436,17 @@ to update the working directory files.
       print('   rm -r %s/.repo' % self.manifest.topdir)
       print('and try again.')
 
-  def Execute(self, opt, args):
-    git_require(MIN_GIT_VERSION, fail=True)
-
+  def ValidateOptions(self, opt, args):
     if opt.reference:
       opt.reference = os.path.expanduser(opt.reference)
 
     # Check this here, else manifest will be tagged "not new" and init won't be
     # possible anymore without removing the .repo/manifests directory.
     if opt.archive and opt.mirror:
-      print('fatal: --mirror and --archive cannot be used together.',
-            file=sys.stderr)
-      sys.exit(1)
+      self.OptionParser.error('--mirror and --archive cannot be used together.')
+
+  def Execute(self, opt, args):
+    git_require(MIN_GIT_VERSION, fail=True)
 
     self._SyncManifest(opt)
     self._LinkManifest(opt.manifest_name)

subcmds/list.py

@@ -49,6 +49,10 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
                  dest='path_only', action='store_true',
                  help="Display only the path of the repository")
 
+  def ValidateOptions(self, opt, args):
+    if opt.fullpath and opt.name_only:
+      self.OptionParser.error('cannot combine -f and -n')
+
   def Execute(self, opt, args):
     """List all projects and the associated directories.
 
@@ -60,11 +64,6 @@ This is similar to running: repo forall -c 'echo "$REPO_PATH : $REPO_PROJECT"'.
       opt: The options.
       args: Positional args.  Can be a list of projects to list, or empty.
     """
-
-    if opt.fullpath and opt.name_only:
-      print('error: cannot combine -f and -n', file=sys.stderr)
-      sys.exit(1)
-
     if not opt.regex:
       projects = self.GetProjects(args, groups=opt.groups)
     else:

subcmds/manifest.py

@@ -73,14 +73,9 @@ in a Git repository for use during future 'repo init' invocations.
     if opt.output_file != '-':
       print('Saved manifest to %s' % opt.output_file, file=sys.stderr)
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if args:
       self.Usage()
 
-    if opt.output_file is not None:
-      self._Output(opt)
-      return
-
-    print('error: no operation to perform', file=sys.stderr)
-    print('error: see repo help manifest', file=sys.stderr)
-    sys.exit(1)
+  def Execute(self, opt, args):
+    self._Output(opt)

subcmds/rebase.py

@@ -71,7 +71,22 @@ branch but need to incorporate new upstream changes "underneath" them.
       if len(args) == 1:
         print('note: project %s is mapped to more than one path' % (args[0],),
             file=sys.stderr)
-      return -1
+      return 1
+
+    # Setup the common git rebase args that we use for all projects.
+    common_args = ['rebase']
+    if opt.whitespace:
+      common_args.append('--whitespace=%s' % opt.whitespace)
+    if opt.quiet:
+      common_args.append('--quiet')
+    if opt.force_rebase:
+      common_args.append('--force-rebase')
+    if opt.no_ff:
+      common_args.append('--no-ff')
+    if opt.autosquash:
+      common_args.append('--autosquash')
+    if opt.interactive:
+      common_args.append('-i')
 
     for project in all_projects:
       cb = project.CurrentBranch
@@ -79,7 +94,7 @@ branch but need to incorporate new upstream changes "underneath" them.
         if one_project:
           print("error: project %s has a detached HEAD" % project.relpath,
                 file=sys.stderr)
-          return -1
+          return 1
         # ignore branches with detatched HEADs
         continue
 
@@ -88,30 +103,11 @@ branch but need to incorporate new upstream changes "underneath" them.
         if one_project:
           print("error: project %s does not track any remote branches"
                 % project.relpath, file=sys.stderr)
-          return -1
+          return 1
         # ignore branches without remotes
         continue
 
-      args = ["rebase"]
-
-      if opt.whitespace:
-        args.append('--whitespace=%s' % opt.whitespace)
-
-      if opt.quiet:
-        args.append('--quiet')
-
-      if opt.force_rebase:
-        args.append('--force-rebase')
-
-      if opt.no_ff:
-        args.append('--no-ff')
-
-      if opt.autosquash:
-        args.append('--autosquash')
-
-      if opt.interactive:
-        args.append("-i")
-
+      args = common_args[:]
       if opt.onto_manifest:
         args.append('--onto')
         args.append(project.revisionExpr)
@@ -131,13 +127,13 @@ branch but need to incorporate new upstream changes "underneath" them.
           stash_args = ["stash"]
 
           if GitCommand(project, stash_args).Wait() != 0:
-            return -1
+            return 1
 
       if GitCommand(project, args).Wait() != 0:
-        return -1
+        return 1
 
       if needs_stash:
         stash_args.append('pop')
         stash_args.append('--quiet')
         if GitCommand(project, stash_args).Wait() != 0:
-          return -1
+          return 1

subcmds/start.py

@@ -41,15 +41,16 @@ revision specified in the manifest.
                  dest='all', action='store_true',
                  help='begin branch in all projects')
 
-  def Execute(self, opt, args):
+  def ValidateOptions(self, opt, args):
     if not args:
       self.Usage()
 
     nb = args[0]
     if not git.check_ref_format('heads/%s' % nb):
-      print("error: '%s' is not a valid name" % nb, file=sys.stderr)
-      sys.exit(1)
+      self.OptionParser.error("'%s' is not a valid name" % nb)
 
+  def Execute(self, opt, args):
+    nb = args[0]
     err = []
     projects = []
     if not opt.all:

subcmds/sync.py

@@ -132,8 +132,8 @@ from the user's .netrc file.
 if the manifest server specified in the manifest file already includes
 credentials.
 
-The -f/--force-broken option can be used to proceed with syncing
-other projects if a project sync fails.
+By default, all projects will be synced. The --fail-fast option can be used
+to halt syncing as soon as possible when the the first project fails to sync.
 
 The --force-sync option can be used to overwrite existing git
 directories if they have previously been linked to a different
@@ -199,8 +199,10 @@ later is required to fix a server side protocol bug.
       self.jobs = 1
 
     p.add_option('-f', '--force-broken',
-                 dest='force_broken', action='store_true',
-                 help="continue sync even if a project fails to sync")
+                 help='obsolete option (to be deleted in the future)')
+    p.add_option('--fail-fast',
+                 dest='fail_fast', action='store_true',
+                 help='stop syncing after first error is hit')
     p.add_option('--force-sync',
                  dest='force_sync', action='store_true',
                  help="overwrite an existing git directory if it needs to "
@@ -284,7 +286,7 @@ later is required to fix a server side protocol bug.
     try:
         for project in projects:
           success = self._FetchHelper(opt, project, *args, **kwargs)
-          if not success and not opt.force_broken:
+          if not success and opt.fail_fast:
             break
     finally:
         sem.release()
@@ -343,10 +345,7 @@ later is required to fix a server side protocol bug.
           print('error: Cannot fetch %s from %s'
                 % (project.name, project.remote.url),
                 file=sys.stderr)
-          if opt.force_broken:
-            print('warn: --force-broken, continuing to sync',
-                  file=sys.stderr)
-          else:
+          if opt.fail_fast:
             raise _FetchError()
 
         fetched.add(project.gitdir)
@@ -384,7 +383,7 @@ later is required to fix a server side protocol bug.
     for project_list in objdir_project_map.values():
       # Check for any errors before running any more tasks.
       # ...we'll let existing threads finish, though.
-      if err_event.isSet() and not opt.force_broken:
+      if err_event.isSet() and opt.fail_fast:
         break
 
       sem.acquire()
@@ -410,7 +409,7 @@ later is required to fix a server side protocol bug.
       t.join()
 
     # If we saw an error, exit with code 1 so that other scripts can check.
-    if err_event.isSet() and not opt.force_broken:
+    if err_event.isSet() and opt.fail_fast:
       print('\nerror: Exited sync due to fetch errors', file=sys.stderr)
       sys.exit(1)
 
@@ -436,9 +435,7 @@ later is required to fix a server side protocol bug.
           _CheckoutOne docstring for details.
     """
     try:
-      success = self._CheckoutOne(opt, project, *args, **kwargs)
-      if not success:
-        sys.exit(1)
+      return self._CheckoutOne(opt, project, *args, **kwargs)
     finally:
       sem.release()
 
@@ -534,7 +531,7 @@ later is required to fix a server side protocol bug.
     for project in all_projects:
       # Check for any errors before running any more tasks.
       # ...we'll let existing threads finish, though.
-      if err_event.isSet() and not opt.force_broken:
+      if err_event.isSet() and opt.fail_fast:
         break
 
       sem.acquire()
@@ -637,7 +634,7 @@ later is required to fix a server side protocol bug.
       print('Failed to remove %s (%s)' % (os.path.join(path, '.git'), str(e)), file=sys.stderr)
       print('error: Failed to delete obsolete path %s' % path, file=sys.stderr)
       print('       remove manually, then run sync again', file=sys.stderr)
-      return -1
+      return 1
 
     # Delete everything under the worktree, except for directories that contain
     # another git project
@@ -671,7 +668,7 @@ later is required to fix a server side protocol bug.
     if failed:
       print('error: Failed to delete obsolete path %s' % path, file=sys.stderr)
       print('       remove manually, then run sync again', file=sys.stderr)
-      return -1
+      return 1
 
     # Try deleting parent dirs if they are empty
     project_dir = path
@@ -728,9 +725,9 @@ later is required to fix a server side protocol bug.
                     'are present' % project.relpath, file=sys.stderr)
               print('       commit changes, then run sync again',
                     file=sys.stderr)
-              return -1
+              return 1
             elif self._DeleteProject(project.worktree):
-              return -1
+              return 1
 
     new_project_paths.sort()
     fd = open(file_path, 'w')
@@ -741,6 +738,24 @@ later is required to fix a server side protocol bug.
       fd.close()
     return 0
 
+  def ValidateOptions(self, opt, args):
+    if opt.force_broken:
+      print('warning: -f/--force-broken is now the default behavior, and the '
+            'options are deprecated', file=sys.stderr)
+    if opt.network_only and opt.detach_head:
+      self.OptionParser.error('cannot combine -n and -d')
+    if opt.network_only and opt.local_only:
+      self.OptionParser.error('cannot combine -n and -l')
+    if opt.manifest_name and opt.smart_sync:
+      self.OptionParser.error('cannot combine -m and -s')
+    if opt.manifest_name and opt.smart_tag:
+      self.OptionParser.error('cannot combine -m and -t')
+    if opt.manifest_server_username or opt.manifest_server_password:
+      if not (opt.smart_sync or opt.smart_tag):
+        self.OptionParser.error('-u and -p may only be combined with -s or -t')
+      if None in [opt.manifest_server_username, opt.manifest_server_password]:
+        self.OptionParser.error('both -u and -p must be given')
+
   def Execute(self, opt, args):
     if opt.jobs:
       self.jobs = opt.jobs
@@ -748,27 +763,6 @@ later is required to fix a server side protocol bug.
       soft_limit, _ = _rlimit_nofile()
       self.jobs = min(self.jobs, (soft_limit - 5) // 3)
 
-    if opt.network_only and opt.detach_head:
-      print('error: cannot combine -n and -d', file=sys.stderr)
-      sys.exit(1)
-    if opt.network_only and opt.local_only:
-      print('error: cannot combine -n and -l', file=sys.stderr)
-      sys.exit(1)
-    if opt.manifest_name and opt.smart_sync:
-      print('error: cannot combine -m and -s', file=sys.stderr)
-      sys.exit(1)
-    if opt.manifest_name and opt.smart_tag:
-      print('error: cannot combine -m and -t', file=sys.stderr)
-      sys.exit(1)
-    if opt.manifest_server_username or opt.manifest_server_password:
-      if not (opt.smart_sync or opt.smart_tag):
-        print('error: -u and -p may only be combined with -s or -t',
-              file=sys.stderr)
-        sys.exit(1)
-      if None in [opt.manifest_server_username, opt.manifest_server_password]:
-        print('error: both -u and -p must be given', file=sys.stderr)
-        sys.exit(1)
-
     if opt.manifest_name:
       self.manifest.Override(opt.manifest_name)
 

tests/test_git_command.py

@@ -14,6 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Unittests for the git_command.py module."""
+
+from __future__ import print_function
+
 import unittest
 
 import git_command

tests/test_git_config.py

@@ -14,6 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Unittests for the git_config.py module."""
+
+from __future__ import print_function
+
 import os
 import unittest
 

tests/test_project.py

@@ -0,0 +1,62 @@
+# -*- coding:utf-8 -*-
+#
+# Copyright (C) 2019 The Android Open Source Project
+#
+# Licensed under the Apache License, Version 2.0 (the "License");
+# you may not use this file except in compliance with the License.
+# You may obtain a copy of the License at
+#
+#      http://www.apache.org/licenses/LICENSE-2.0
+#
+# Unless required by applicable law or agreed to in writing, software
+# distributed under the License is distributed on an "AS IS" BASIS,
+# WITHOUT WARRANTIES OR CONDITIONS OF ANY KIND, either express or implied.
+# See the License for the specific language governing permissions and
+# limitations under the License.
+
+"""Unittests for the project.py module."""
+
+from __future__ import print_function
+
+import unittest
+
+import project
+
+
+class RepoHookShebang(unittest.TestCase):
+  """Check shebang parsing in RepoHook."""
+
+  def test_no_shebang(self):
+    """Lines w/out shebangs should be rejected."""
+    DATA = (
+        '',
+        '# -*- coding:utf-8 -*-\n',
+        '#\n# foo\n',
+        '# Bad shebang in script\n#!/foo\n'
+    )
+    for data in DATA:
+      self.assertIsNone(project.RepoHook._ExtractInterpFromShebang(data))
+
+  def test_direct_interp(self):
+    """Lines whose shebang points directly to the interpreter."""
+    DATA = (
+        ('#!/foo', '/foo'),
+        ('#! /foo', '/foo'),
+        ('#!/bin/foo ', '/bin/foo'),
+        ('#! /usr/foo ', '/usr/foo'),
+        ('#! /usr/foo -args', '/usr/foo'),
+    )
+    for shebang, interp in DATA:
+      self.assertEqual(project.RepoHook._ExtractInterpFromShebang(shebang),
+                       interp)
+
+  def test_env_interp(self):
+    """Lines whose shebang launches through `env`."""
+    DATA = (
+        ('#!/usr/bin/env foo', 'foo'),
+        ('#!/bin/env foo', 'foo'),
+        ('#! /bin/env /bin/foo ', '/bin/foo'),
+    )
+    for shebang, interp in DATA:
+      self.assertEqual(project.RepoHook._ExtractInterpFromShebang(shebang),
+                       interp)

tests/test_wrapper.py

@@ -14,6 +14,10 @@
 # See the License for the specific language governing permissions and
 # limitations under the License.
 
+"""Unittests for the wrapper.py module."""
+
+from __future__ import print_function
+
 import os
 import unittest
 

wrapper.py

@@ -15,7 +15,12 @@
 # limitations under the License.
 
 from __future__ import print_function
-import imp
+try:
+  from importlib.machinery import SourceFileLoader
+  _loader = lambda *args: SourceFileLoader(*args).load_module()
+except ImportError:
+  import imp
+  _loader = lambda *args: imp.load_source(*args)
 import os
 
 
@@ -26,5 +31,5 @@ _wrapper_module = None
 def Wrapper():
   global _wrapper_module
   if not _wrapper_module:
-    _wrapper_module = imp.load_source('wrapper', WrapperPath())
+    _wrapper_module = _loader('wrapper', WrapperPath())
   return _wrapper_module