Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[clang-format] Add "three dot" diff option to git-clang-format #74230

Merged

Conversation

boomanaiden154
Copy link
Contributor

This patch adds in the ability to do a "three dot" git-clang-format between two commits. This looks at the diff between the second commit and the common merge base rather than comparing at the point of the specified commits. This is needed to improve the reliability of the LLVM code formatting CI action which currently breaks in some cases where files have been modified in the upstream tree and when the person created their branch, leaving phantom formatting diffs that weren't touched by the PR author.

Part of a fix for #73873

@llvmbot
Copy link
Collaborator

llvmbot commented Dec 3, 2023

@llvm/pr-subscribers-clang-format

Author: Aiden Grossman (boomanaiden154)

Changes

This patch adds in the ability to do a "three dot" git-clang-format between two commits. This looks at the diff between the second commit and the common merge base rather than comparing at the point of the specified commits. This is needed to improve the reliability of the LLVM code formatting CI action which currently breaks in some cases where files have been modified in the upstream tree and when the person created their branch, leaving phantom formatting diffs that weren't touched by the PR author.

Part of a fix for #73873


Full diff: https://github.com/llvm/llvm-project/pull/74230.diff

1 Files Affected:

  • (modified) clang/tools/clang-format/git-clang-format (+15-4)
diff --git a/clang/tools/clang-format/git-clang-format b/clang/tools/clang-format/git-clang-format
index 0f33b5339ec14..1f5d9a9bbf676 100755
--- a/clang/tools/clang-format/git-clang-format
+++ b/clang/tools/clang-format/git-clang-format
@@ -132,6 +132,10 @@ def main():
                  help='passed to clang-format'),
   p.add_argument('-v', '--verbose', action='count', default=0,
                  help='print extra information')
+  p.add_argument('--diff_from_common_commit', action='store_true',
+                 help=('diff from the last common commit for commits in '
+                      'separate branches rather than the exact point of the '
+                      'commits'))
   # We gather all the remaining positional arguments into 'args' since we need
   # to use some heuristics to determine whether or not <commit> was present.
   # However, to print pretty messages, we make use of metavar and help.
@@ -153,7 +157,10 @@ def main():
   else:
     if len(commits) > 2:
       die('at most two commits allowed; %d given' % len(commits))
-  changed_lines = compute_diff_and_extract_lines(commits, files, opts.staged)
+  changed_lines = compute_diff_and_extract_lines(commits,
+                                                 files,
+                                                 opts.staged,
+                                                 opts.diff_from_common_commit)
   if opts.verbose >= 1:
     ignored_files = set(changed_lines)
   filter_by_extension(changed_lines, opts.extensions.lower().split(','))
@@ -301,9 +308,9 @@ def get_object_type(value):
   return convert_string(stdout.strip())
 
 
-def compute_diff_and_extract_lines(commits, files, staged):
+def compute_diff_and_extract_lines(commits, files, staged, diff_common_commit):
   """Calls compute_diff() followed by extract_lines()."""
-  diff_process = compute_diff(commits, files, staged)
+  diff_process = compute_diff(commits, files, staged, diff_common_commit)
   changed_lines = extract_lines(diff_process.stdout)
   diff_process.stdout.close()
   diff_process.wait()
@@ -313,7 +320,7 @@ def compute_diff_and_extract_lines(commits, files, staged):
   return changed_lines
 
 
-def compute_diff(commits, files, staged):
+def compute_diff(commits, files, staged, diff_common_commit):
   """Return a subprocess object producing the diff from `commits`.
 
   The return value's `stdin` file object will produce a patch with the
@@ -327,6 +334,10 @@ def compute_diff(commits, files, staged):
     git_tool = 'diff-tree'
   elif staged:
     extra_args += ['--cached']
+
+  if len(commits) > 1 and diff_common_commit:
+    commits = [f'{commits[0]}...{commits[1]}']
+
   cmd = ['git', git_tool, '-p', '-U0'] + extra_args + commits + ['--']
   cmd.extend(files)
   p = subprocess.Popen(cmd, stdin=subprocess.PIPE, stdout=subprocess.PIPE)

This patch adds in the ability to do a "three dot" git-clang-format
between two commits. This looks at the diff between the second commit
and the common merge base rather than comparing at the point of the
specified commits. This is needed to improve the reliability of the LLVM
code formatting CI action which currently breaks in some cases where
files have been modified in the upstream tree and when the person
created their branch, leaving phantom formatting diffs that weren't
touched by the PR author.

Part of a fix for llvm#73873
Copy link
Contributor

@cor3ntin cor3ntin left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

LGTM.
Please give some time for clang-format folks to review the change as it is not my area of expertise.

clang/tools/clang-format/git-clang-format Outdated Show resolved Hide resolved
clang/tools/clang-format/git-clang-format Outdated Show resolved Hide resolved
@boomanaiden154 boomanaiden154 merged commit aeaae53 into llvm:main Dec 6, 2023
3 of 4 checks passed
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Projects
None yet
Development

Successfully merging this pull request may close these issues.

None yet

4 participants