Skip to content

Clang format action picking up diffs from other commits #73873

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.

Sign up for GitHub

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

Jump to bottom
Closed
boomanaiden154 opened this issue Nov 30, 2023 · 2 comments
Closed

Clang format action picking up diffs from other commits #73873

boomanaiden154 opened this issue Nov 30, 2023 · 2 comments
Assignees
@boomanaiden154
Labels
github:workflow

Comments

@boomanaiden154
Copy link
Contributor

There is at least one instance where the clang format action picked up the diff from a different commit in the same file that wasn't included in the current PR.

This occurred in #73506, with the job also trying to format 7c93452. I'm guessing this has to do with an out of date merge base, but this should work regardless.

I've saved the branch to my fork at https://github.com/boomanaiden154/llvm-project/tree/indvars-dead-code so that the issue can be reproduced.

Will do some more investigation.
@boomanaiden154 boomanaiden154 self-assigned this Nov 30, 2023
@boomanaiden154 boomanaiden154 mentioned this issue Nov 30, 2023
Merged
@boomanaiden154
Copy link
Contributor Author

Looks like this is due to the default diff strategy in git-clang-format being the git two dot diff form which will include also include all changes made in the main branch between the merge base and the head of the tree. This seems to show up exceedingly rarely as usually we're able to find a recent merge base that doesn't have the same files touched.

The best way to fix this seems to be to add a new option to git-clang-format enabling the use of the three dot diff. Will post a PR for that. Then there's the matter of getting a development version of clang-format into the action, but that should be doable.

@boomanaiden154 boomanaiden154 mentioned this issue Dec 3, 2023
Merged
boomanaiden154 added a commit to boomanaiden154/llvm-project that referenced this issue Dec 3, 2023
@boomanaiden154
[clang-format] Add "three dot" diff option to git-clang-format
ae4097b 
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
boomanaiden154 added a commit that referenced this issue Dec 6, 2023
@boomanaiden154
[clang-format] Add "three dot" diff option to git-clang-format (#74230)
aeaae53 
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
@boomanaiden154 boomanaiden154 mentioned this issue Dec 7, 2023
Merged
boomanaiden154 added a commit that referenced this issue Dec 7, 2023
@boomanaiden154
[Github] Use three dot diff for darker in code format action (#74704)
2b1c76c 
Using a two dot diff allows changes made in main after the merge base to
show up in the formatting diff. Using a three dot diff fixes this and
ensures that only changes made in the source branch (branch from the PR
author) will get passed along to the formatter.

Without this, issues like #73873 occur.
@boomanaiden154
Copy link
Contributor Author

This has been fixed in #75132 by only checking the diff through the merge base. A todo has been added to convert this to the option in clang-format, but as a stop-gap, this works well.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Assignees

@boomanaiden154 boomanaiden154

Labels
github:workflow
Projects
None yet
Milestone
No milestone
Development

No branches or pull requests
2 participants
@EugeneZelenko @boomanaiden154