tools: Refactor python clang-format wrapping #2981
Conversation
This merges the clang-format and line-length hooks into a single one. Semantically, this makes way more sense. Also, this prevent multiple attempted commit that are rejected by pre-commit (pre-commit might skip the line-length hook if old clang-format hook already failed). Also, this removes any dependency on git-clang-format and implements the whole mechanism itself. It retrieves all added lines from the unified diff instead and is now capable of taking PRE_COMMIT_FROM_REF into account. By moving the git-related code into a separate githelper library, it's way easier to write additional python wrappers for new hooks, e.g. for clang-tidy.
tools/githelper.py
Outdated
| start_added, _, length_added = match_lineno.group(2).partition(",") | ||
| lineno_removed = int(start_removed) | ||
| lineno_added = int(start_added) | ||
| lines_left = int(length_removed) if length_removed else 1 |
There was a problem hiding this comment.
well, i decrement this in the code below, so it's the remaing lines to parse. I renamed it to hunk_lines_left.
tools/githelper.py
Outdated
| continue | ||
|
|
||
| if lines_left and line: | ||
| lines_left -= 1 |
There was a problem hiding this comment.
I use this for counting if we still need to parse lines of the hunk (to ignore lines that are not interesting for us)
tools/githelper.py
Outdated
|
|
||
| logger = logging.getLogger(__name__) | ||
|
|
||
| cmd = ["git", "diff", "--unified=0", from_ref if from_ref else "HEAD"] |
There was a problem hiding this comment.
We cannot only use git diff, because in case of merge commits, the automatic merged lines are also reformatted.
In merge commits, we only want to check the resolution of merge conflicts.
This can be done with "git show"
Lines with conflict resolutions are marked with ++ or --
I think we can only do it with a single revision.
There was a problem hiding this comment.
I think this was original done with the
"git status --porcelain" call
There was a problem hiding this comment.
I don't think this is worth the hassle, for various reasons:
- This is what the original git-clang-format does, too and wasn't aware of any issue with that.
git showwill only work for a single commit, so implementing--from-ref/PRE_COMMIT_FROM_REFwill be a mess (and we adding support for that is the whole point of our custom script)- For FF-only merges, this won't be an issue
- For merges this would re-run clang-format on the changed lines, but tbh all those lines should be pre-commit-clean anyway.
There was a problem hiding this comment.
-
We already had strong issues with it in the PR of @hacksdump.
Due to a merge with master many unrelated files have been reformatted. This must not happen, because it makes a PR hard to review. -
We have no use case for this, so it is fine to use it fro single commits only. At the users machine the user will commit only a single merge commit, so it is fine. In CI check it will check the PR against the master, so intermediate master merges are not considered.
-
right
-
No, If we merge master we merge legacy lines which are not clean. We must only check lines with solved conflicts.
There was a problem hiding this comment.
We have no use case for this, so it is fine to use it fro single commits only. At the users machine the user will commit only a single merge commit, so it is fine. In CI check it will check the PR against the master, so intermediate master merges are not considered.
Using git show is not possible. You can only use it to show a commit's diff, but this runs before committing, so we need to compare the working tree to a commit. I looked into the git documentation, but I see no real alternative.
No, If we merge master we merge legacy lines which are not clean. We must only check lines with solved conflicts.
This is only true for very old branches that didn't merge master since we introduced the clang-format pre-commit hook in January. Any code committed since then needs to be pre-commit clean anyway. I'm not sure that we even have such branches that are likely to be merged, and if so you can just SKIP=clang-format git pull upstream master for the merge.
We already had strong issues with it in the PR of @hacksdump.
Due to a merge with master many unrelated files have been reformatted. This must not happen, because it makes a PR hard to review.
In this case it's probably because the branch was old, so the merge included commits that were not formatted properly. I think this is unfortunate, but not critical and the clang-format hook can be skipped in that case.
tools/githelper.py
Outdated
| "--patch", | ||
| "--unified=0", | ||
| from_ref if from_ref else "HEAD", | ||
| ] |
There was a problem hiding this comment.
Follow up of: #2981 (comment)
In case of merge commits we need to use here "git show" and use only ++lines containing resolved conflicts.
In case of the Travis check, we need to use:
git diff PRE_COMMIT_FROM_REF...PRE_COMMIT_TO_REF
to not include merge commits into the diff.
This merges the clang-format and line-length hooks into a single one.
Semantically, this makes way more sense. Also, this prevent multiple
attempted commit that are rejected by pre-commit (pre-commit might skip
the line-length hook if old clang-format hook already failed).
Also, this removes any dependency on git-clang-format and implements the
whole mechanism itself. It retrieves all added lines from the unified diff
instead and is now capable of taking PRE_COMMIT_FROM_REF into account.
By moving the git-related code into a separate githelper library, it's
way easier to write additional python wrappers for new hooks, e.g. for
clang-tidy.