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
_______________________________________________
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
