[PATCH] D129311: [clang-format] Update return code

[Sridhar Gopinath via Phabricator via cfe-commits]

sridhar_gopinath marked 2 inline comments as done.
sridhar_gopinath added inline comments.


================
Comment at: clang/tools/clang-format/git-clang-format:579
     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
----------------
owenpan wrote:
> Good catch with `check=True`. Should we add it to the other two instances of `run()` above?
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.


================
Comment at: clang/tools/clang-format/git-clang-format:539-540
   # filter.
-  subprocess.check_call(['git', 'diff', '--diff-filter=M', old_tree, new_tree,
-                         '--'])
+  subprocess.check_call(['git', 'diff', '--diff-filter=M',
+                        old_tree, new_tree, '--exit-code', '--'])
 
----------------
owenpan wrote:
> sridhar_gopinath wrote:
> > owenpan wrote:
> > > `--exit-code` is implied?
> > `--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.
> > ```
> > `--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.
> ```
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.


Repository:
  rG LLVM Github Monorepo

CHANGES SINCE LAST ACTION
  https://reviews.llvm.org/D129311/new/

https://reviews.llvm.org/D129311