recipe engine: add remote_run command

recipe_engine/fetch.py

+# Copyright 2016 The Chromium Authors. All rights reserved.

[nodir, 2016/05/20 23:03:49]

is this file about git? it seems so. Maybe rename to git.py? Especially if you
agree to rename fetch_from_git to ensure_checkout

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

This won't be necessarily exclusive to git. For example, we're already planning
to add fetch from gitiles. It'll be a tarball, so nothing really git-specific,
and only very loosely related (because of gitiles).

[nodir, 2016/05/24 17:46:31]

Acknowledged.

+# Use of this source code is governed by a BSD-style license that can be
+# found in the LICENSE file.
+
+import logging
+import os
+import sys
+
+from .third_party import subprocess42
+
+
+class FetchError(Exception):
+  pass
+
+
+class UncleanFilesystemError(FetchError):
+  pass
+
+
+class FetchNotAllowedError(FetchError):
+  pass
+
+
+def _run_git(checkout_dir, *args):
+  if sys.platform.startswith(('win', 'cygwin')):
+    cmd = ['git.bat']
+  else:
+    cmd = ['git']
+
+  if checkout_dir is not None:
+    cmd += ['-C', checkout_dir]
+  cmd += list(args)
+
+  logging.info('Running: %s', cmd)
+  return subprocess42.check_output(cmd)
+
+
+def _cleanup_pyc(path):

[nodir, 2016/05/20 23:03:49]

why cleanup_pyc is in a file related to fetching?

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Good point. Moved back.

+  """Removes any .pyc files from |path|'s directory tree.
+  This ensures we always use the fresh code.

[nodir, 2016/05/20 23:03:49]

there should be a blank line between first line of docstirng and the rest

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Done.

+  """
+  for root, dirs, files in os.walk(path):
+    for f in files:
+      if f.endswith('.pyc'):
+        os.unlink(os.path.join(root, f))
+
+
+def fetch_from_git(repo, revision, checkout_dir, allow_fetch):

[nodir, 2016/05/20 23:03:49]

it is strange to see "allow_fetch" parameter in a function that starts with
"fetch_". It smells. I'd rename it to ensure_git_checkout. Note that the
function was called "checkout" in the past, probably because it does not always
fetch

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Yeah, I was also thinking it's somewhat ugly but meh. Renamed to
ensure_git_checkout.

FWIW, as for the autoroller we'll need support for returning latest updates, we
may well get objects here (e.g. Git and Gitiles) and this would likely be named
checkout again.

+  """Fetches given |repo| at |revision| to |checkout_dir| using git.
+  Network operations are performed only if |allow_fetch| is True.

[nodir, 2016/05/20 23:03:49]

here too

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Done.

+  """
+  logging.info('Freshening repository %s in %s', repo, checkout_dir)
+
+  if not os.path.isdir(checkout_dir):
+    if allow_fetch:

[nodir, 2016/05/20 23:03:49]

inverse condition and unindent the _run_git call

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Done.

+      _run_git(None, 'clone', '-q', repo, checkout_dir)
+    else:
+      raise FetchNotAllowedError(
+          'need to clone %s but fetch not allowed' % repo)
+  elif not os.path.isdir(os.path.join(checkout_dir, '.git')):
+    raise UncleanFilesystemError(
+        '%s exists but is not a git repo' % checkout_dir)
+

[nodir, 2016/05/20 23:03:49]

what if checkout dir exists but points to a different repo url?

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Good point. Out of scope for this CL (this is just existing code moved here),
but added a TODO.

FWIW, if it's different repo it's very unlikely that below rev-parse would
succeed and point to different contents than expected. Of course it'd most
likely fail, and fixing the TODO would mean recovering from that gracefully.

[nodir, 2016/05/24 17:46:31]

It is likely to succeed because default value of revision is FETCH_HEAD and git
rev-parse --verify will verify that FETCH_HEAD points to an existing commit,
which is very likely to be the case. As a result, this step will succeed, and a
trooper will be very unhappy about this code.

[Paweł Hajdan Jr., 2016/05/25 00:15:41]

Yeah, fair enough. I added code to verify it in this CL + a test.

+  try:
+    _run_git(checkout_dir, 'rev-parse', '-q', '--verify',
+             '%s^{commit}' % revision)
+  except subprocess42.CalledProcessError:
+    if allow_fetch:

[nodir, 2016/05/20 23:03:49]

same here

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Done.

+      _run_git(checkout_dir, 'fetch')
+    else:
+      raise FetchNotAllowedError(
+          'need to fetch %s but fetch not allowed' % repo)
+  _run_git(checkout_dir, 'reset', '-q', '--hard', revision)
+  _cleanup_pyc(checkout_dir)

[nodir, 2016/05/20 23:03:49]

pyc files have nothing to do with git. This function should be called by callers
of fetch_from_git

[Paweł Hajdan Jr., 2016/05/24 16:58:06]

Yup, done.