From 2a6d071e41bab89c29c587a12dcc3654ee289e9b Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 25 Jun 2025 09:50:06 +0200 Subject: [PATCH 1/4] clang-format: modify rules to reduce false-positives This series is in response to an email thread [1] around the usage of '.clang-format' and its effectiveness. The goal of the series is to improve the usage of 'clang-format' in the repository. To do this we: 1. Reduce the number of false positives. Majority of which is due to line-wrapping. We remove the 'ColumnLimit' from 'clang-format'. This removes the responsibility of line-wrapping from 'clang-format' and puts it into the hands of the user. 2. Add a rule to 'meson' to run 'git clang-format' by running 'meson compile style'. 3. Make the 'RemoveBracesLLVM' permanent by moving it to '.clang-format'. With this, running `git clang-format` for the last 25 commits in master, seems to produce much less false positives. diff --git a/builtin/diff.c b/builtin/diff.c index c6231edce4..246b81caa2 100644 --- a/builtin/diff.c +++ b/builtin/diff.c @@ -30,14 +30,13 @@ #define DIFF_NO_INDEX_IMPLICIT 2 static const char builtin_diff_usage[] = -"git diff [] [] [--] [...]\n" -" or: git diff [] --cached [--merge-base] [] [--] [...]\n" -" or: git diff [] [--merge-base] [...] [--] [...]\n" -" or: git diff [] ... [--] [...]\n" -" or: git diff [] \n" -" or: git diff [] --no-index [--] [...]" -"\n" -COMMON_DIFF_OPTIONS_HELP; + "git diff [] [] [--] [...]\n" + " or: git diff [] --cached [--merge-base] [] [--] [...]\n" + " or: git diff [] [--merge-base] [...] [--] [...]\n" + " or: git diff [] ... [--] [...]\n" + " or: git diff [] \n" + " or: git diff [] --no-index [--] [...]" + "\n" COMMON_DIFF_OPTIONS_HELP; static const char *blob_path(struct object_array_entry *entry) { diff --git a/builtin/stash.c b/builtin/stash.c index 7cd3ad8aa4..90e441a6e5 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -1802,7 +1802,8 @@ static int push_stash(int argc, const char **argv, const char *prefix, argc = parse_options(argc, argv, prefix, options, push_assumed ? git_stash_usage : - git_stash_push_usage, flags); + git_stash_push_usage, + flags); force_assume |= patch_mode; } diff --git a/bundle-uri.c b/bundle-uri.c index c9d65aa0ce..89f59aafe8 100644 --- a/bundle-uri.c +++ b/bundle-uri.c @@ -123,7 +123,7 @@ void print_bundle_list(FILE *fp, struct bundle_list *list) for (i = 0; i < BUNDLE_HEURISTIC__COUNT; i++) { if (heuristics[i].heuristic == list->heuristic) { fprintf(fp, "\theuristic = %s\n", - heuristics[list->heuristic].name); + heuristics[list->heuristic].name); break; } } diff --git a/diff-no-index.c b/diff-no-index.c index 4aeeb98cfa..a3892a9ccc 100644 --- a/diff-no-index.c +++ b/diff-no-index.c @@ -325,7 +325,7 @@ static int fixup_paths(const char **path, struct strbuf *replacement) return 0; } -static const char * const diff_no_index_usage[] = { +static const char *const diff_no_index_usage[] = { N_("git diff --no-index [] [...]"), NULL }; diff --git a/pathspec.h b/pathspec.h index 5e3a6f1fe7..601b9ca201 100644 --- a/pathspec.h +++ b/pathspec.h @@ -80,7 +80,7 @@ struct pathspec { * For git diff --no-index, indicate that we are operating without * a repository or index. */ -#define PATHSPEC_NO_REPOSITORY (1<<7) +#define PATHSPEC_NO_REPOSITORY (1 << 7) /** * Given command line arguments and a prefix, convert the input to While now I'm tempted to mark the 'check-style' CI job as required. I think we should do that in the future. [1]: https://lore.kernel.org/git/xmqqmsa3adpw.fsf@gitster.g/ --- Changes in v4: - EDITME: describe what is new in this series revision. - EDITME: use bulletpoints and terse descriptions. - Link to v3: https://lore.kernel.org/r/20250702-525-make-clang-format-more-robust-v3-0-705344f30580@gmail.com Changes in v3: - In meson.build, set 'native: true' and use the variable obtained from 'find_program()' directly in 'run_target()'. - Link to v2: https://lore.kernel.org/r/20250630-525-make-clang-format-more-robust-v2-0-05cbcdbf7817@gmail.com Changes in v2: - Drop the patch to add 120 column length to editorconfig. This way, we will continue to use the default of 80 columns. Adding a higher column length makes editorconfig combine smaller lines during block formatting. This is not desirable. - Ensure that meson specifically checks for 'git-clang-format' and not just 'clang-format'. - Link to v1: https://lore.kernel.org/r/20250625-525-make-clang-format-more-robust-v1-0-67a49ecc2fd5@gmail.com --- b4-submit-tracking --- # This section is used internally by b4 prep for tracking purposes. { "series": { "revision": 4, "change-id": "20250625-525-make-clang-format-more-robust-968126c991b9", "prefixes": [], "history": { "v1": [ "20250625-525-make-clang-format-more-robust-v1-0-67a49ecc2fd5@gmail.com" ], "v2": [ "20250630-525-make-clang-format-more-robust-v2-0-05cbcdbf7817@gmail.com" ], "v3": [ "20250702-525-make-clang-format-more-robust-v3-0-705344f30580@gmail.com" ] } } } -- GitLab From a38179c525e129f0f986d88193c142f7a93c25e5 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 25 Jun 2025 11:38:07 +0200 Subject: [PATCH 2/4] clang-format: set 'ColumnLimit' to 0 When clang-format was introduced to the Git project in 6134de6ac1 (clang-format: outline the git project's coding style, 2017-08-14), the 'ColumnLimit' was set to 80. This is inline with our recommendation in 'Documentation/CodingGuidelines', which states: We try to keep to at most 80 characters per line. However while this is recommended limit, this is not the enforced limit. In some cases in we do overflow this limit to prioritize readability. Setting the 'ColumnLimit' also means that shorter lines are concatenated to simply as the result would still be below 80 characters, which is undesirable. In the past, we tried to adjust the penalties around line wrapping, once in 42efde4c29 (clang-format: adjust line break penalties, 2017-09-29) and another time in 5e9fa0f9fa (clang-format: re-adjust line break penalties, 2024-10-18). While these settings help tweak the line break penalties to be more in-line with the requirements of the Git project, using 'clang-format' still produces a lot of false positives. So to make 'clang-format' more usable, set the 'ColumnLimit' to 0. This means that line-wrapping is no-longer a concern of the formatter and something that the user needs to take care of. The previous commit also added a more flexible guideline to the '.editorconfig' setting a 'max_line_length' of 120 characters. This should provide some guidance to users. In the future, it would be nice to re-instate this limit with adequate penalties which would follow our guidelines, but currently, it makes more sense to have a working formatter which we can rely on and which doesn't create too many false positives. Signed-off-by: Karthik Nayak --- .clang-format | 21 +++++++++------------ 1 file changed, 9 insertions(+), 12 deletions(-) diff --git a/.clang-format b/.clang-format index 9547fe1b77c..19d6cf4200f 100644 --- a/.clang-format +++ b/.clang-format @@ -12,7 +12,15 @@ UseTab: Always TabWidth: 8 IndentWidth: 8 ContinuationIndentWidth: 8 -ColumnLimit: 80 + +# While we do want to enforce a character limit of 80 characters, we often +# allow lines to overflow that limit to prioritize readability. Setting a +# character limit here with penalties has been finicky and creates too many +# false positives. +# +# NEEDSWORK: It would be nice if we can find optimal settings to ensure we +# can re-enable the limit here. +ColumnLimit: 0 # C Language specifics Language: Cpp @@ -210,16 +218,5 @@ MaxEmptyLinesToKeep: 1 # No empty line at the start of a block. KeepEmptyLinesAtTheStartOfBlocks: false -# Penalties -# This decides what order things should be done if a line is too long -PenaltyBreakAssignment: 5 -PenaltyBreakBeforeFirstCallParameter: 5 -PenaltyBreakComment: 5 -PenaltyBreakFirstLessLess: 0 -PenaltyBreakOpenParenthesis: 300 -PenaltyBreakString: 5 -PenaltyExcessCharacter: 10 -PenaltyReturnTypeOnItsOwnLine: 300 - # Don't sort #include's SortIncludes: false -- GitLab From aab28a108fed6f17b4d9be11ccd1fb93b661f677 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 25 Jun 2025 11:54:23 +0200 Subject: [PATCH 3/4] clang-format: add 'RemoveBracesLLVM' to the main config In 1b8f306612 (ci/style-check: add `RemoveBracesLLVM` in CI job, 2024-07-23) we added 'RemoveBracesLLVM' to the CI job of running the clang formatter. This rule checks and warns against using braces on simple single-statement bodies of statements. Since we haven't had any issues regarding this rule, we can now move it into the main clang-format config and remove it from being CI exclusive. Signed-off-by: Karthik Nayak --- .clang-format | 6 ++++++ ci/run-style-check.sh | 18 +----------------- 2 files changed, 7 insertions(+), 17 deletions(-) diff --git a/.clang-format b/.clang-format index 19d6cf4200f..dcfd0aad60c 100644 --- a/.clang-format +++ b/.clang-format @@ -220,3 +220,9 @@ KeepEmptyLinesAtTheStartOfBlocks: false # Don't sort #include's SortIncludes: false + +# Remove optional braces of control statements (if, else, for, and while) +# according to the LLVM coding style. This avoids braces on simple +# single-statement bodies of statements but keeps braces if one side of +# if/else if/.../else cascade has multi-statement body. +RemoveBracesLLVM: true diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh index 6cd4b1d934a..0832c19df07 100755 --- a/ci/run-style-check.sh +++ b/ci/run-style-check.sh @@ -5,21 +5,5 @@ baseCommit=$1 -# Remove optional braces of control statements (if, else, for, and while) -# according to the LLVM coding style. This avoids braces on simple -# single-statement bodies of statements but keeps braces if one side of -# if/else if/.../else cascade has multi-statement body. -# -# As this rule comes with a warning [1], we want to experiment with it -# before adding it in-tree. since the CI job for the style check is allowed -# to fail, appending the rule here allows us to validate its efficacy. -# While also ensuring that end-users are not affected directly. -# -# [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm -{ - cat .clang-format - echo "RemoveBracesLLVM: true" -} >/tmp/clang-format-rules - -git clang-format --style=file:/tmp/clang-format-rules \ +git clang-format --style=file:.clang-format \ --diff --extensions c,h "$baseCommit" -- GitLab From 797ac61934826022bd9960ad5f11c657e4842d2f Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Wed, 25 Jun 2025 12:29:03 +0200 Subject: [PATCH 4/4] meson: add rule to run 'git clang-format' The Makefile has a 'style' rule to run 'git clang-format'. While Meson intrinsically supports a 'clang-format' target, which can be run when using the ninja backend by running 'ninja clang-format', this runs the formatting on all existing files. Our Meson build doesn't yet support a way to run 'git clang-format', which runs the formatter between the working directory and commit provided. Add a new 'style' target to Meson to mimic the target in the Makefile. Signed-off-by: Karthik Nayak --- meson.build | 12 ++++++++++++ 1 file changed, 12 insertions(+) diff --git a/meson.build b/meson.build index 7fea4a34d68..48dff818e0a 100644 --- a/meson.build +++ b/meson.build @@ -2144,6 +2144,18 @@ if headers_to_check.length() != 0 and compiler.get_argument_syntax() == 'gcc' alias_target('check-headers', hdr_check) endif +git_clang_format = find_program('git-clang-format', required: false, native: true) +if git_clang_format.found() + run_target('style', + command: [ + git_clang_format, + '--style', 'file', + '--diff', + '--extensions', 'c,h' + ] + ) +endif + foreach key, value : { 'DIFF': diff.full_path(), 'GIT_SOURCE_DIR': meson.project_source_root(), -- GitLab