[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath created this revision.
sridhar_gopinath added reviewers: curdeius, owenpan.
Herald added a project: All.
sridhar_gopinath requested review of this revision.
Herald added a project: clang.
Herald added a subscriber: cfe-commits.
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 clang-format.
Repository:
rG LLVM Github Monorepo
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,9 +198,9 @@
return 0
if opts.diff:
-print_diff(old_tree, new_tree)
+return print_diff(old_tree, new_tree)
elif opts.diffstat:
-print_diffstat(old_tree, new_tree)
+return print_diffstat(old_tree, new_tree)
else:
changed_files = apply_changes(old_tree, new_tree, force=opts.force,
patch_mode=opts.patch)
@@ -209,7 +209,7 @@
for filename in changed_files:
print('%s' % filename)
- return 1
+return 1
def load_git_config(non_string_options=None):
@@ -536,8 +536,8 @@
# 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,
- '--'])
+ subprocess.check_call(['git', 'diff', '--diff-filter=M',
+old_tree, new_tree, '--exit-code', '--'])
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,7 +548,13 @@
# 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,
+ subprocess.check_call(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',
'--'])
def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,9 +198,9 @@
return 0
if opts.diff:
-print_diff(old_tree, new_tree)
+return print_diff(old_tree, new_tree)
elif opts.diffstat:
-print_diffstat(old_tree, new_tree)
+return print_diffstat(old_tree, new_tree)
else:
changed_files = apply_changes(old_tree, new_tree, force=opts.force,
patch_mode=opts.patch)
@@ -209,7 +209,7 @@
for filename in changed_files:
print('%s' % filename)
- return 1
+return 1
def load_git_config(non_string_options=None):
@@ -536,8 +536,8 @@
# 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,
- '--'])
+ subprocess.check_call(['git', 'diff', '--diff-filter=M',
+old_tree, new_tree, '--exit-code', '--'])
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,7 +548,13 @@
# 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,
+ subprocess.check_call(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',
'--'])
def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
___
cfe-commits mailing list
cfe-commits@lists.llvm.org
https://lists.llvm.org/cgi-bin/mailman/listinfo/cfe-commits
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath marked 3 inline comments as done.
sridhar_gopinath added inline comments.
Comment at: clang/tools/clang-format/git-clang-format:201
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:
owenpan wrote:
> 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?
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.
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
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath updated this revision to Diff 448099.
sridhar_gopinath marked an inline comment as done.
sridhar_gopinath added a comment.
Addressed nitpick
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129311/new/
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,8 @@
# 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.
@@ -575,7 +575,7 @@
# 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):
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,8 @@
# 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 su
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath added a comment. I do not have commit access. Could you please commit on my behalf? Thank you! 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
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath updated this revision to Diff 443726.
sridhar_gopinath added a comment.
Replaced subprocess.check_call with subprocess.call since the former crashes
when the return code is not zero.
+ formatting changes
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129311/new/
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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.call(['git', 'diff', '--diff-filter=M',
+ old_tree, new_tree, '--exit-code', '--'])
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,8 +548,14 @@
# 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.call(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',
+ '--'])
def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
"""Apply the changes in `new_tree` to the working directory.
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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.call(['git', 'diff', '--diff-filter=M',
+ old_tree, new_tree, '--exit-code', '--'])
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,8 +548,14 @@
# 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.call(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath updated this revision to Diff 445652.
sridhar_gopinath added a comment.
Updated subprocess.call -> subprocess.run
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129311/new/
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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',
+ old_tree, new_tree, '--exit-code', '--']).returncode
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,8 +548,14 @@
# 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',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',
+ '--']).returncode
def apply_changes(old_tree, new_tree, force=False, patch_mode=False):
"""Apply the changes in `new_tree` to the working directory.
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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',
+ old_tree, new_tree, '--exit-code', '--']).returncode
def print_diffstat(old_tree, new_tree):
"""Print the diffstat between the two trees to stdout."""
@@ -548,8 +548,14 @@
# 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',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',
+ '--']).returnc
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath updated this revision to Diff 445954.
sridhar_gopinath marked 4 inline comments as done.
sridhar_gopinath added a comment.
Changes to the args order and changing check_call -> run
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129311/new/
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,9 @@
# 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.
@@ -575,7 +576,7 @@
# 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):
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,9 @@
# 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
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath marked 4 inline comments as done. sridhar_gopinath added inline comments. 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: > `--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. ``` 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
[PATCH] D129311: [clang-format] Update return code
sridhar_gopinath updated this revision to Diff 446581.
sridhar_gopinath added a comment.
Removed '--' in the git-diff commands as it only applies to files
and not commits.
Repository:
rG LLVM Github Monorepo
CHANGES SINCE LAST ACTION
https://reviews.llvm.org/D129311/new/
https://reviews.llvm.org/D129311
Files:
clang/tools/clang-format/git-clang-format
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,8 @@
# 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.
@@ -575,7 +575,7 @@
# 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):
Index: clang/tools/clang-format/git-clang-format
===
--- clang/tools/clang-format/git-clang-format
+++ clang/tools/clang-format/git-clang-format
@@ -198,16 +198,16 @@
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)
+
+ 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 1
@@ -536,8 +536,8 @@
# 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."""
@@ -548,8 +548,8 @@
# 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,
- '--'])
[PATCH] D129311: [clang-format] Update return code
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 [] --no-index [--] > 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
