git-clang-format: format index not worktree when using --staged

[Maetveis]

Summary

When --staged (or --cached) use the index for formatting as well, not just for the line numbers
to format.
Without this change git-clang-format gets the changed line numbers based on the index, but then
formats these lines on the working tree version of the file.
This is a problem when the working tree and index differ. One common case would be
(and is the motivation behind this patch) when applying the suggested changes git-clang-format --staged,
then forgetting to add the applied changes. When git-clang-format --staged --diff is used in a
pre-commit hook in this scenario, then the hook would allow committing the improperly formatted changes,
as the file is correctly formatted in the work tree.

Fixes https://github.com/llvm/llvm-project/issues/56797.

[owenpan]

Can you open an issue at https://github.com/llvm/llvm-project/issues? See https://github.com/llvm/llvm-project/issues/56736 for an example.

[Maetveis]

In D130108#3685620, @owenpan wrote:

Can you open an issue at https://github.com/llvm/llvm-project/issues? See https://github.com/llvm/llvm-project/issues/56736 for an example.

Of course, here it is: https://github.com/llvm/llvm-project/issues/56797

[Maetveis]

Updated to add missing comma after 'git ls-tree'. It broke normal (non staged) operation.

[Maetveis]

Friendly Ping.

[MyDeveloperDay]

So trying to understand the problem statement, is it:

1. you have some files staged
2. and then you have to change them locally (review comment)
3. but forget to git add them,
4. you run git clang-format --staged
5. and it formats the new file (not added) based on the line numbers that are changing in the already staged file, which could be a different range to the original (is that correct?)
6. You think you've formatted the new file correctly (but likely missed a line or two)
7. You perform git add (as you now notice you've missed adding it)
8. You commit, but a commit hook tells you that you didn't clang format. (or worse still you are able to commit and push but you are pushing a badly formatted file, potentially)

is that correct?

[Maetveis]

In D130108#3706550, @MyDeveloperDay wrote:

So trying to understand the problem statement, is it:

1. you have some files staged
2. and then you have to change them locally (review comment)
3. but forget to git add them,
4. you run git clang-format --staged
5. and it formats the new file (not added) based on the line numbers that are changing in the already staged file, which could be a different range to the original (is that correct?)
6. You think you've formatted the new file correctly (but likely missed a line or two)
7. You perform git add (as you now notice you've missed adding it)
8. You commit, but a commit hook tells you that you didn't clang format. (or worse still you are able to commit and push but you are pushing a badly formatted file, potentially)

is that correct?

Not exactly, in 5 git-clang-format --staged would refuse to change anything, because the same file is modified in both the index and the worktree.
But with --diff, yes it would run clang-format on the new file, but with the line numbers based on the index, then show the diff with this result.

Also at 8 there would be no problem, as there's no two versions of the file anymore. The issue can only appear when there is a file that is different in the index and on disk (Not all changes to it are staged).

I think this captures the original case well:

1. Have a commit hook script that uses git-clang-format --staged --diff to reject commits that would be badly formatted
2. Change a file, add changes with incorrect formatting
3. add the file to the index via git add
4. Try to commit
5. The commit hook rejects the commit, gives the diff that needs to be applied to fix
6. Fix the formatting, but forget to git add
7. Commit
8. Commit succeeds, but it should have failed, the resulting commit has incorrect formatting (because it does not include the formatting changes)

[owenpan]

clang/tools/clang-format/git-clang-format

181–192

To refactor the code a little.

398–399

Nit: keep two empty lines between top-level functions and break the long comment.

404

To break the long line.

419–420

To keep lines within the 80-column limit.

436

To break the long line.

[Maetveis]

Address formatting and stylistic comments.

[owenpan]

Please mark review comments as done if you have addressed them.

clang/tools/clang-format/git-clang-format

401

Nit: end the sentence with a period as previously suggested.

419–420

Nit: extraneous trailing whitespace and lack of a full stop.

[Maetveis]

Sorry, I somehow missed the big obvious checkbox to mark the comments, fixed now.
I don't have commit access could you commit if the last change seems okay to you?

Name: Gergely Meszaros
Email: [e-mail]

clang/tools/clang-format/git-clang-format

if changed_lines:
         print('    %s' % filename)
 
   if not changed_lines:
     if opts.verbose >= 0:
       print('no modified files to format')
     return 0
 
   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)
+    revision = old_tree
+  elif opts.staged:
+    old_tree = create_tree_from_index(changed_lines)
+    revision = ''
   else:
     old_tree = create_tree_from_workdir(changed_lines)
-    new_tree = run_clang_format_and_save_to_tree(changed_lines,
-                                                 binary=opts.binary,
-                                                 style=opts.style)
+    revision = None
+                                               revision,
   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')
     return 0

 
 
 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 create_tree_from_index(filenames):
+  # Copy the environment, because the files have to be read from the original
+  # index.
+  env = os.environ.copy()
+  def index_contents_generator():
+    for filename in filenames:
+      git_ls_files_cmd = ['git', 'ls-files', '--stage', '-z', '--', filename]
+      git_ls_files = subprocess.Popen(git_ls_files_cmd, env=env,
+                                      stdin=subprocess.PIPE,
+                                      stdout=subprocess.PIPE)
+      stdout = git_ls_files.communicate()[0]
+      yield convert_string(stdout.split(b'\0')[0])
+  return create_tree(index_contents_generator(), '--index-info')
+
+
 def run_clang_format_and_save_to_tree(changed_lines, revision=None,
                                       binary='clang-format', style=None):
   """Run clang-format on each file and save the result to a git tree.
 
   Returns the object ID (SHA-1) of the created tree."""
+  # Copy the environment when formatting the files in the index, because the
+  # files have to be read from the original index.
+  env = os.environ.copy() if revision == '' else None
   def iteritems(container):
       try:
           return container.iteritems() # Python 2
       except AttributeError:
           return container.items() # Python 3
   def index_info_generator():
     for filename, line_ranges in iteritems(changed_lines):
-      if revision:
+      if revision is not None:
+        if len(revision) > 0:
           git_metadata_cmd = ['git', 'ls-tree',
                               '%s:%s' % (revision, os.path.dirname(filename)),
                               os.path.basename(filename)]
-        git_metadata = subprocess.Popen(git_metadata_cmd, stdin=subprocess.PIPE,
+        else:
+          git_metadata_cmd = ['git', 'ls-files', '--stage', '--', filename]
+        git_metadata = subprocess.Popen(git_metadata_cmd, env=env,
+                                        stdin=subprocess.PIPE,
                                         stdout=subprocess.PIPE)
         stdout = git_metadata.communicate()[0]
         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)
+                                     style=style,
+                                     env=env)
       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.
 
   If mode is '--stdin', it must be a list of filenames.  If mode is
with temporary_index_file():
     p.stdin.close()
     if p.wait() != 0:
       die('`%s` failed' % ' '.join(cmd))
     tree_id = run('git', 'write-tree')
     return tree_id
 
 
 def clang_format_to_blob(filename, line_ranges, revision=None,
-                         binary='clang-format', style=None):
+                         binary='clang-format', style=None, env=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.
+  directory if `revision` is None. Revision can be set to an empty string to run
+  clang-format on the file in the index.
 
   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 revision is not None:
     clang_format_cmd.extend(['-assume-filename='+filename])
     git_show_cmd = ['git', 'cat-file', 'blob', '%s:%s' % (revision, filename)]
-    git_show = subprocess.Popen(git_show_cmd, stdin=subprocess.PIPE,
+    git_show = subprocess.Popen(git_show_cmd, env=env, 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: