git-clang-format: refactor to support upcoming --staged flag

[lodato]

Summary

The only user-visible change is rewriting of the --help message to make the different modes more clear.

Internally, this is a significant refactoring to make the source and destination of the diff to be more clear by using subclasses rather than conditionals. The purpose is to support the next patch in this series, which adds support for a --staged flag. But even if that does not go through, the intent is for this code to be a readability improvement.

Note: In run_clang_format_and_save_to_tree(), we now generate the entire index up front rather than feeding it in line-by-line via a generator. The reason we must do this is because, in the future --staged implementation, we need to read from the index *before* calling crate_tree() since create_tree() uses a temporary index file. In practice, this will have no performance impact because the previous commit (D41130) switched to loading it all into memory in create_tree() anyway.

[klimek]

lg

google3/third_party/llvm/llvm/tools/clang/tools/clang-format/git-clang-format

252

Don't we want to die if len(commits) > 2?

[lodato]

Re-add check for len(commits) > 2.

[lodato]

Oops! Fixed. Thanks for catching this!

[shafik]

Is this still relevant or can we close this?

google3/third_party/llvm/llvm/tools/clang/tools/clang-format/git-clang-format

 import os
 import re
 import subprocess
 import sys
 
 usage = 'git clang-format [OPTIONS] [<commit>] [<commit>] [--] [<file>...]'
 
 desc = '''
-If zero or one commits are given, run clang-format on all lines that differ
-between the working directory and <commit>, which defaults to HEAD.  Changes are
-only applied to the working directory.
+Run clang-format on all modified lines in the working directory or in a given
+commit.
 
-If two commits are given (requires --diff), run clang-format on all lines in the
-second <commit> that differ from the first <commit>.
+Forms:
+
+    git clang-format [<commit>]
+        Run clang-format on all lines in the working directory that differ from
+        <commit>, which defaults to HEAD. Changes are written in-place.
+
+    git clang-format [<commit>] --diff
+        Same as first form, but print the diff instead of writing changes
+        in-place.
+
+    git clang-format [<commit>] --patch
+        Same as first form, but interactively choose hunks to apply, a la `git
+        add -p`.
+
+    git clang-format <commit1> <commit2> --diff
+        Run clang-format on all lines in <commit2> that differ from <commit1>.
+        Requires --diff.
+
+In all of the forms above, <file>... can be used to limit what files are
+affected, using the same syntax as `git diff`. Use `--` to disambiguate between
+files and commits.
 
 The following git-config settings set the default of the corresponding option:
   clangFormat.binary
   clangFormat.commit
   clangFormat.extension
   clangFormat.style
 '''
 
p.add_argument('args', nargs='*', metavar='<commit>',
                  help='revision from which to compute the diff')
   p.add_argument('ignored', nargs='*', metavar='<file>...',
                  help='if specified, only consider differences in these files')
   opts = p.parse_args(argv)
 
   opts.verbose -= opts.quiet
   del opts.quiet
 
-  commits, files = interpret_args(opts.args, dash_dash, opts.commit)
-  if len(commits) > 1:
-    if not opts.diff:
-      die('--diff is required when two commits are given')
-  else:
-    if len(commits) > 2:
-      die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files)
+  source, dest, files = interpret_args(opts.args, dash_dash,
+                                       default_commit=opts.commit)
+  if isinstance(dest, Revision) and not opts.diff:
+  changed_lines = dest.compute_diff_from(source, files)
   if opts.verbose >= 1:
     ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
   if opts.verbose >= 1:
     ignored_files.difference_update(changed_lines)
     if ignored_files:
       print('Ignoring changes in the following files (wrong extension):')
       for filename in ignored_files:
         print('    %s' % filename)
     if changed_lines:
       print('Running clang-format on the following files:')
       for filename in changed_lines:
         print('    %s' % filename)
   if not changed_lines:
     print('no modified files to format')
     return
   # The computed diff outputs absolute paths, so we must cd before accessing
   # those files.
   cd_to_toplevel()
-  if len(commits) > 1:
-    old_tree = commits[1]
-    new_tree = run_clang_format_and_save_to_tree(changed_lines,
-                                                 revision=commits[1],
-                                                 binary=opts.binary,
-                                                 style=opts.style)
-  else:
-    old_tree = create_tree_from_workdir(changed_lines)
-    new_tree = run_clang_format_and_save_to_tree(changed_lines,
+  old_tree = dest.create_tree(changed_lines)
+  new_tree = dest.run_clang_format_and_save_to_tree(changed_lines,
                                                     binary=opts.binary,
                                                     style=opts.style)
   if opts.verbose >= 1:
     print('old tree: %s' % old_tree)
     print('new tree: %s' % new_tree)
   if old_tree == new_tree:
     if opts.verbose >= 0:
       print('clang-format did not modify any files')
   elif opts.diff:
     print_diff(old_tree, new_tree)
   else:
+    assert isinstance(dest, Workdir)
     changed_files = apply_changes(old_tree, new_tree, force=opts.force,
                                   patch_mode=opts.patch)
     if (opts.verbose >= 0 and not opts.patch) or opts.verbose >= 1:
       print('changed files:')
       for filename in changed_files:
         print('    %s' % filename)
 
 
if entry:
       name, value = entry.split('\n', 1)
       if name in non_string_options:
         value = run('git', 'config', non_string_options[name], name)
       out[name] = value
   return out
 
 
 def interpret_args(args, dash_dash, default_commit):
-  """Interpret `args` as "[commits] [--] [files]" and return (commits, files).
+  """Interpret `args` as "[commits] [--] [files]" and return (src, dest, files).
 
   It is assumed that "--" and everything that follows has been removed from
   args and placed in `dash_dash`.
 
   If "--" is present (i.e., `dash_dash` is non-empty), the arguments to its
   left (if present) are taken as commits.  Otherwise, the arguments are checked
   from left to right if they are commits or files.  If commits are not given,
-  a list with `default_commit` is used."""
+  a list with `default_commit` is used.
+
+  Return value is `(src, dest, files)`, where `src` and `dest` are TreeLocation
+  objects and `files` is a (possibly empty) list of filenames.
+  """
+  # First, get the list of commits.
   if dash_dash:
     if len(args) == 0:
       commits = [default_commit]
     else:
       commits = args
     for commit in commits:
       object_type = get_object_type(commit)
       if object_type not in ('commit', 'tag'):
while args:
         break
       commits.append(args.pop(0))
     if not commits:
       commits = [default_commit]
     files = args
   else:
     commits = [default_commit]
     files = []
-  return commits, files
+
+  assert len(commits) != 0
+  if len(commits) > 2:
+    die('at most two commits allowed; %d given' % len(commits))
+  elif len(commits) == 2:
+    return Revision(commits[0]), Revision(commits[1]), files
+  else:
+    return Revision(commits[0]), Workdir(), files
 
 
 def disambiguate_revision(value):
   """Returns True if `value` is a revision, False if it is a file, or dies."""
   # If `value` is ambiguous (neither a commit nor a file), the following
   # command will die with an appropriate error message.
   run('git', 'rev-parse', value, verbose=False)
   object_type = get_object_type(value)
def get_object_type(value):
   cmd = ['git', 'cat-file', '-t', value]
   p = subprocess.Popen(cmd, stdout=subprocess.PIPE, stderr=subprocess.PIPE)
   stdout, stderr = p.communicate()
   if p.returncode != 0:
     return None
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files):
-  """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files)
-  changed_lines = extract_lines(diff_process.stdout)
+class TreeLocation (object):
+  """Represents either a commit or the working directory.
+
+  Do not use this directly. Instead, use one of the subclasses."""
+
+  def compute_diff_from(self, source, files):
+    """Return diff from `source` to `self`.
+
+    Return value is dictionary mapping filename to list of (start_line,
+    line_count) pairs."""
+    diff_process = self._compute_diff_process_from(source, files)
+    changed_lines = self._extract_lines(diff_process.stdout)
     diff_process.stdout.close()
     diff_process.wait()
     if diff_process.returncode != 0:
       # Assume error was already printed to stderr.
       sys.exit(2)
     return changed_lines
 
-
-def compute_diff(commits, files):
-  """Return a subprocess object producing the diff from `commits`.
+  def _compute_diff_process_from(self, source, files):
+    """Return a subprocess object producing the diff from `source` to `self`.
 
     The return value's `stdin` file object will produce a patch with the
-  differences between the working directory and the first commit if a single
-  one was specified, or the difference between both specified commits, filtered
-  on `files` (if non-empty).  Zero context lines are used in the patch."""
-  git_tool = 'diff-index'
-  if len(commits) > 1:
-    git_tool = 'diff-tree'
-  cmd = ['git', git_tool, '-p', '-U0'] + commits + ['--']
+    differences between `source` and `self`, limiting to only paths matching the
+    patterns in `files` if non-empty. Zero context lines are used in the
+    patch."""
+    assert isinstance(source, Revision)
+    cmd = self._compute_diff_from_base_command(source)
+    cmd.append('--')
     cmd.extend(files)
     p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)
     p.stdin.close()
     return p
 
-
-def extract_lines(patch_file):
+  @staticmethod
+  def _extract_lines(patch_file):
     """Extract the changed lines in `patch_file`.
 
-  The return value is a dictionary mapping filename to a list of (start_line,
-  line_count) pairs.
-
     The input must have been produced with ``-U0``, meaning unidiff format with
     zero lines of context.  The return value is a dict mapping filename to a
     list of line `Range`s."""
     matches = {}
     for line in patch_file:
       line = convert_string(line)
       match = re.search(r'^\+\+\+\ [^/]+/(.*)', line)
       if match:
         filename = match.group(1).rstrip('\r\n')
       match = re.search(r'^@@ -[0-9,]+ \+(\d+)(,(\d+))?', line)
       if match:
         start_line = int(match.group(1))
         line_count = 1
         if match.group(3):
           line_count = int(match.group(3))
         if line_count > 0:
           matches.setdefault(filename, []).append(Range(start_line, line_count))
     return matches
 
-
-def filter_by_extension(dictionary, allowed_extensions):
-  """Delete every key in `dictionary` that doesn't have an allowed extension.
-
-  `allowed_extensions` must be a collection of lowercase file extensions,
-  excluding the period."""
-  allowed_extensions = frozenset(allowed_extensions)
-  for filename in list(dictionary.keys()):
-    base_ext = filename.rsplit('.', 1)
-    if len(base_ext) == 1 and '' in allowed_extensions:
-        continue
-    if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions:
-      del dictionary[filename]
-
-
-def cd_to_toplevel():
-  """Change to the top level of the git repository."""
-  toplevel = run('git', 'rev-parse', '--show-toplevel')
-  os.chdir(toplevel)
-
-
-def create_tree_from_workdir(filenames):
-  """Create a new git tree with the given files from the working directory.
-
-  Returns the object ID (SHA-1) of the created tree."""
-  return create_tree(filenames, '--stdin')
-
-
-def run_clang_format_and_save_to_tree(changed_lines, revision=None,
+  def run_clang_format_and_save_to_tree(self, changed_lines,
                                         binary='clang-format', style=None):
     """Run clang-format on each file and save the result to a git tree.
 
+    `changed_lines` is return value from compute_diff_from().
+
     Returns the object ID (SHA-1) of the created tree."""
     def iteritems(container):
         try:
             return container.iteritems() # Python 2
         except AttributeError:
             return container.items() # Python 3
-  def index_info_generator():
+    create_tree_input = []
     for filename, line_ranges in iteritems(changed_lines):
-      if revision:
-        stdout = run('git', 'ls-tree',
-                     '%s:%s' % (revision, os.path.dirname(filename)),
-                     os.path.basename(filename))
-        mode = oct(int(stdout.split()[0], 8))
-      else:
-        mode = oct(os.stat(filename).st_mode)
-      # Adjust python3 octal format so that it matches what git expects
-      if mode.startswith('0o'):
-          mode = '0' + mode[2:]
-      blob_id = clang_format_to_blob(filename, line_ranges,
-                                     revision=revision,
-                                     binary=binary,
-                                     style=style)
-      yield '%s %s\t%s' % (mode, blob_id, filename)
-  return create_tree(index_info_generator(), '--index-info')
-
-
-def create_tree(input_lines, mode):
-  """Create a tree object from the given input.
+      mode = '0%o' % self._file_mode(filename)
+      blob_id = self._clang_format_to_blob(filename, line_ranges,
+                                           binary=binary, style=style)
+      create_tree_input.append('%s %s\t%s' % (mode, blob_id, filename))
+    return create_tree(create_tree_input, '--index-info')
 
-  If mode is '--stdin', it must be a list of filenames.  If mode is
-  '--index-info' is must be a list of values suitable for "git update-index
-  --index-info", such as "<mode> <SP> <sha1> <TAB> <filename>".  Any other mode
-  is invalid."""
-  assert mode in ('--stdin', '--index-info')
-  with temporary_index_file():
-    run('git', 'update-index', '--add', '-z', mode,
-        stdin=to_bytes(''.join('%s\0' % line for line in input_lines)))
-    tree_id = run('git', 'write-tree')
-    return tree_id
-
-
-def clang_format_to_blob(filename, line_ranges, revision=None,
-                         binary='clang-format', style=None):
+  def _clang_format_to_blob(self, filename, line_ranges, binary='clang-format',
+                           style=None):
     """Run clang-format on the given file and save the result to a git blob.
 
-  Runs on the file in `revision` if not None, or on the file in the working
-  directory if `revision` is None.
-
     Returns the object ID (SHA-1) of the created blob."""
     clang_format_cmd = [binary]
     if style:
       clang_format_cmd.extend(['-style='+style])
     clang_format_cmd.extend([
         '-lines=%s:%s' % (start_line, start_line+line_count-1)
         for start_line, line_count in line_ranges])
-  if revision:
+    if not self.is_workdir:
       clang_format_cmd.extend(['-assume-filename='+filename])
-    git_show_cmd = ['git', 'cat-file', 'blob', '%s:%s' % (revision, filename)]
+      git_show_cmd = ['git', 'cat-file', 'blob', self._blob_name(filename)]
       git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE,
                                   stdout=subprocess.PIPE)
       git_show.stdin.close()
       clang_format_stdin = git_show.stdout
     else:
       clang_format_cmd.extend([filename])
       git_show = None
       clang_format_stdin = subprocess.PIPE
     try:
       clang_format = subprocess.Popen(clang_format_cmd, stdin=clang_format_stdin,
                                       stdout=subprocess.PIPE)
       if clang_format_stdin == subprocess.PIPE:
         clang_format_stdin = clang_format.stdin
     except OSError as e:
       if e.errno == errno.ENOENT:
         die('cannot find executable "%s"' % binary)
       else:
         raise
     clang_format_stdin.close()
     hash_object_cmd = ['git', 'hash-object', '-w', '--path='+filename, '--stdin']
     hash_object = subprocess.Popen(hash_object_cmd, stdin=clang_format.stdout,
                                    stdout=subprocess.PIPE)
     clang_format.stdout.close()
     stdout = hash_object.communicate()[0]
     if hash_object.returncode != 0:
       die('`%s` failed' % ' '.join(hash_object_cmd))
     if clang_format.wait() != 0:
       die('`%s` failed' % ' '.join(clang_format_cmd))
     if git_show and git_show.wait() != 0:
       die('`%s` failed' % ' '.join(git_show_cmd))
     return convert_string(stdout).rstrip('\r\n')
 
+  # --- Things for subclasses to override ---
+
+  def create_tree(self, filenames):
+    """Create a new git tree with the given files and return its SHA-1."""
+    raise NotImplementedError()
+
+  def _file_mode(self, filename):
+    """Return the file mode integer of the given file."""
+    raise NotImplementedError()
+
+  def _compute_diff_from_base_command(self, source):
+    raise NotImplementedError()
+
+  def _blob_name(self, filename):
+    raise NotImplementedError()
+
+  is_workdir = False
+
+
+class Workdir (TreeLocation):
+  """Represents the working directory."""
+
+  def create_tree(self, filenames):
+    return create_tree(filenames, '--stdin')
+
+  def _file_mode(self, filename):
+    return os.stat(filename).st_mode
+
+  def _compute_diff_from_base_command(self, source):
+    return ['git', 'diff-index', '-p', '-U0', source.revision]
+
+  is_workdir = True
+
+
+class Revision (TreeLocation):
+  """Represents a specific revision, a.k.a. a commit."""
+
+  def __init__(self, revision):
+    self.revision = revision
+
+  def create_tree(self, unused_filenames):
+    return self.revision
+
+  def _file_mode(self, filename):
+    stdout = run('git', 'ls-tree',
+                 '%s:%s' % (self.revision, os.path.dirname(filename)),
+                 os.path.basename(filename))
+    return int(stdout.split()[0], 8)
+
+  def _compute_diff_from_base_command(self, source):
+    return ['git', 'diff-tree', '-p', '-U0', source.revision, self.revision]
+
+  def _blob_name(self, filename):
+    return '%s:%s' % (self.revision, filename)
+
+
+def filter_by_extension(dictionary, allowed_extensions):
+  """Delete every key in `dictionary` that doesn't have an allowed extension.
+
+  `allowed_extensions` must be a collection of lowercase file extensions,
+  excluding the period."""
+  allowed_extensions = frozenset(allowed_extensions)
+  for filename in list(dictionary.keys()):
+    base_ext = filename.rsplit('.', 1)
+    if len(base_ext) == 1 and '' in allowed_extensions:
+        continue
+    if len(base_ext) == 1 or base_ext[1].lower() not in allowed_extensions:
+      del dictionary[filename]
+
+
+def cd_to_toplevel():
+  """Change to the top level of the git repository."""
+  toplevel = run('git', 'rev-parse', '--show-toplevel')
+  os.chdir(toplevel)
+
+
+def create_tree(input_lines, mode):
+  """Create a tree object from the given input.
+
+  If mode is '--stdin', it must be a list of filenames.  If mode is
+  '--index-info' is must be a list of values suitable for "git update-index
+  --index-info", such as "<mode> <SP> <sha1> <TAB> <filename>".  Any other mode
+  is invalid."""
+  assert mode in ('--stdin', '--index-info')
+  with temporary_index_file():
+    run('git', 'update-index', '--add', '-z', mode,
+        stdin=to_bytes(''.join('%s\0' % line for line in input_lines)))
+    tree_id = run('git', 'write-tree')
+    return tree_id
 
 @contextlib.contextmanager
 def temporary_index_file(tree=None):
   """Context manager for setting GIT_INDEX_FILE to a temporary file and deleting
   the file afterward."""
   index_path = create_temporary_index(tree)
   old_index_path = os.environ.get('GIT_INDEX_FILE')
   os.environ['GIT_INDEX_FILE'] = index_path