[clang-format] Fix the return code of git-clang-format

[sridhar_gopinath]

Summary

In diff and diffstat modes, the return code is != 0 even when there are no
changes between commits. This issue can be fixed by passing --exit-code to
git-diff command that returns 0 when there are no changes and using that as
the return code for git-clang-format.

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

[owenpan]

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

200–201

200–201

You can delete this line.

539–541

--exit-code is implied?

550–552

[sridhar_gopinath]

Replaced subprocess.check_call with subprocess.call since the former crashes when the return code is not zero.
+ formatting changes

[sridhar_gopinath]

Could someone please review this change? Thanks!

— Sridhar

[owenpan]

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

539–540

Can we use run() instead of call() here and below?

[sridhar_gopinath]

Updated subprocess.call -> subprocess.run

[owenpan]

Please mark as done if you have addressed an inline comment.

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

539–541

Omit --exit-code (if implied) and fix the location of --.

551–553

Ditto.

578

While we are at it, let's also replace this one with run().

[sridhar_gopinath]

Changes to the args order and changing check_call -> run

[sridhar_gopinath]

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

539–541

--exit-code is not implied. From the docs:

--exit-code
Make the program exit with codes similar to diff(1). That is, it exits with 1 if there were differences and 0 means no differences.

[owenpan]

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

539–541

--exit-code is not implied. From the docs:

--exit-code
Make the program exit with codes similar to diff(1). That is, it exits with 1 if there were differences and 0 means no differences.

From https://git-scm.com/docs/git-diff#Documentation/git-diff.txt-emgitdiffemltoptionsgt--no-index--ltpathgtltpathgt:

git diff [<options>] --no-index [--] <path> <path>
This form is to compare the given two paths on the filesystem. You can omit the --no-index option when running the command in a working tree controlled by Git and at least one of the paths points outside the working tree, or when running the command outside a working tree controlled by Git. This form implies --exit-code.

578

Good catch with check=True. Should we add it to the other two instances of run() above?

[sridhar_gopinath]

Removed '--' in the git-diff commands as it only applies to files
and not commits.

[sridhar_gopinath]

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

539–541

This seems to be an incorrect usage of -- in the git-diff command.

-- is used in git-diff to diff between two paths. In such cases, --exit-code is implied. But when diffing two commits, -- is not needed. In this script, git-diff is used only on commits. The old-tree and new-tree variables point to git-tree hashes. Hence, using -- on the git hashes is incorrect as git tries to interpret the hashes as file names. This issue was not seen earlier because it was added at the end of the command and was being omitted.

Now, since the git-diff is not on paths, --exit-code is not implied. For diff of hashes, --exit-code has to be specified explicitely.

578

Not really. We want the above two commands to return non-zero exit code when there is a diff. Adding check=True will crash the process in such cases.

[owenpan]

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

201–210

Actually, doesn't line 175 make sure it will return 0 if there is no diff? Can you open an issue at https://github.com/llvm/llvm-project/issues and give an example where this isn't true?

[sridhar_gopinath]

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

201–210

Created https://github.com/llvm/llvm-project/issues/56736

Line 175 is only checking if there are any relevant files that have been modified. There is an option to only consider a subset of changed files. So this line is checking if there are any changed lines after filtering the relevant files. Then, the tool generates a new git tree for the relevant changes after running clang-format. Line 198 is checking if the old and the new git trees are the same git hashes. When there are code changes, these two hashes won't be the same and we won't hit this case too. Finally, the actual formatting changes are checked by running git diff in line 201. This function call will define the exit code of the program. Currently, that's not being accounted for and the tool always returns 1 after reaching this point.

[owenpan]

Thanks for the explanations! LGTM.

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

539–541

Nit: align --exit-code with git or binpack the arguments.

[sridhar_gopinath]

Addressed nitpick

[sridhar_gopinath]

I do not have commit access. Could you please commit on my behalf? Thank you!

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

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
 
   if opts.diff:
-    print_diff(old_tree, new_tree)
-  elif opts.diffstat:
-    print_diffstat(old_tree, new_tree)
-  else:
-    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)
+    return print_diff(old_tree, new_tree)
+  if opts.diffstat:
+    return print_diffstat(old_tree, new_tree)
+
 
   return 1
 
 
 def load_git_config(non_string_options=None):
   """Return the git configuration as a dictionary.
 
   All options are assumed to be strings unless in `non_string_options`, in which
def print_diff(old_tree, new_tree):
   """Print the diff between the two trees to stdout."""
   # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output
   # is expected to be viewed by the user, and only the former does nice things
   # like color and pagination.
   #
   # We also only print modified files since `new_tree` only contains the files
   # that were modified, so unmodified files would show as deleted without the
   # filter.
-  subprocess.check_call(['git', 'diff', '--diff-filter=M', old_tree, new_tree,
-                         '--'])
+  return subprocess.run(['git', 'diff', '--diff-filter=M',
+                         '--exit-code', old_tree, new_tree]).returncode
 
 def print_diffstat(old_tree, new_tree):
   """Print the diffstat between the two trees to stdout."""
   # We use the porcelain 'diff' and not plumbing 'diff-tree' because the output
   # is expected to be viewed by the user, and only the former does nice things
   # like color and pagination.
   #
   # We also only print modified files since `new_tree` only contains the files
   # that were modified, so unmodified files would show as deleted without the
   # filter.
-  subprocess.check_call(['git', 'diff', '--diff-filter=M', '--stat', old_tree, new_tree,
-                         '--'])
+  return subprocess.run(['git', 'diff', '--diff-filter=M', '--exit-code',
+                         '--stat', old_tree, new_tree]).returncode
 
 def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
   """Apply the changes in `new_tree` to the working directory.
 
   Bails if there are local changes in those files and not `force`.  If
   `patch_mode`, runs `git checkout --patch` to select hunks interactively."""
   changed_files = run('git', 'diff-tree', '--diff-filter=M', '-r', '-z',
                       '--name-only', old_tree,
                       new_tree).rstrip('\0').split('\0')
   if not force:
     unstaged_files = run('git', 'diff-files', '--name-status', *changed_files)
     if unstaged_files:
       print('The following files would be modified but '
                 'have unstaged changes:', file=sys.stderr)
       print(unstaged_files, file=sys.stderr)
       print('Please commit, stage, or stash them first.', file=sys.stderr)
       sys.exit(2)
   if patch_mode:
     # In patch mode, we could just as well create an index from the new tree
     # and checkout from that, but then the user will be presented with a
     # message saying "Discard ... from worktree".  Instead, we use the old
     # tree as the index and checkout from new_tree, which gives the slightly
     # better message, "Apply ... to index and worktree".  This is not quite
     # right, since it won't be applied to the user's index, but oh well.
     with temporary_index_file(old_tree):
-      subprocess.check_call(['git', 'checkout', '--patch', new_tree])
+      subprocess.run(['git', 'checkout', '--patch', new_tree], check=True)
     index_tree = old_tree
   else:
     with temporary_index_file(new_tree):
       run('git', 'checkout-index', '-a', '-f')
   return changed_files
 
 
 def run(*args, **kwargs):