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

[Sridhar Gopinath via Phabricator via cfe-commits]

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 via Phabricator via cfe-commits]

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 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan accepted this revision.
owenpan added a comment.
This revision is now accepted and ready to land.

Thanks for the explanations! LGTM.




Comment at: clang/tools/clang-format/git-clang-format:539-540
   # 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
 

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


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 via Phabricator via cfe-commits]

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

[Owen Pan via Phabricator via cfe-commits]

owenpan 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:

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?


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 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 [] --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

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

[Sridhar Gopinath via Phabricator via cfe-commits]

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

[Owen Pan via Phabricator via cfe-commits]

owenpan 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

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



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', '--'])
 

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.
```


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 via Phabricator via cfe-commits]

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 via Phabricator via cfe-commits]

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, 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added a comment.

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




Comment at: clang/tools/clang-format/git-clang-format:539-540
   # 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
 

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



Comment at: clang/tools/clang-format/git-clang-format:551-558
+  return subprocess.run(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',

Ditto.



Comment at: clang/tools/clang-format/git-clang-format:584
 with temporary_index_file(old_tree):
   subprocess.check_call(['git', 'checkout', '--patch', new_tree])
 index_tree = old_tree

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


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 via Phabricator via cfe-commits]

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',
+ 

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/tools/clang-format/git-clang-format:539
   # 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', '--'])

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


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

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

[Sridhar Gopinath via cfe-commits]

Could someone please review this change? Thanks!

— Sridhar

> On Jul 11, 2022, at 12:55 PM, Sridhar Gopinath via Phabricator 
>  wrote:
> 
> 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.
> 
> 
> 

___
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 via Phabricator via cfe-commits]

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

[Owen Pan via Phabricator via cfe-commits]

owenpan added inline comments.



Comment at: clang/tools/clang-format/git-clang-format:202
+return print_diff(old_tree, new_tree)
   elif opts.diffstat:
+return print_diffstat(old_tree, new_tree)





Comment at: clang/tools/clang-format/git-clang-format:204
+return print_diffstat(old_tree, new_tree)
   else:
 changed_files = apply_changes(old_tree, new_tree, force=opts.force,

You can delete this line.



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', '--'])
 

`--exit-code` is implied?



Comment at: clang/tools/clang-format/git-clang-format:551-558
+  subprocess.check_call(['git',
+ 'diff',
+ '--diff-filter=M',
+ '--stat',
+ old_tree,
+ new_tree,
+ '--exit-code',




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 via Phabricator via cfe-commits]

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