Re: [PATCH v6 6/6] ci/style-check: add `RemoveBracesLLVM` in CI job

[Karthik Nayak <karthik.188@gmail.com>]

[-- Attachment #1: Type: text/plain, Size: 4012 bytes --]

Junio C Hamano <gitster@pobox.com> writes:

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> For 'clang-format', setting 'RemoveBracesLLVM' to 'true', adds a check
>> to ensure we avoid curly braces for single-statement bodies in
>> conditional blocks.
>>
>> However, the option does come with two warnings [1]:
>>
>>     This option will be renamed and expanded to support other styles.
>>
>> and
>>
>>     Setting this option to true could lead to incorrect code formatting
>>     due to clang-format’s lack of complete semantic information. As
>>     such, extra care should be taken to review code changes made by
>>     this option.
>>
>> The latter seems to be of concern. While we want to experiment with the
>> rule, adding it to the in-tree '.clang-format' could affect end-users.
>> Let's only add it to the CI jobs for now. With time, we can evaluate
>> its efficacy and decide if we want to add it to '.clang-format' or
>> retract it entirely. We do so, by adding the existing rules in
>> '.clang-format' and this rule to a temp file outside the working tree,
>> which is then used by 'git clang-format'. This ensures we don't murk
>> with files in-tree.
>>
>> [1]: https://clang.llvm.org/docs/ClangFormatStyleOptions.html#removebracesllvm
>>
>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  ci/run-style-check.sh | 19 ++++++++++++++++++-
>>  1 file changed, 18 insertions(+), 1 deletion(-)
>
> Have we done enough experiment by now and don't we want to move it
> to the set of rules in our tree?
>

Christian and I were talking about clang-format today. So great to see
that you too were thinking about it. I think this particular rule should
be okay to move our tree. For this particular rule, I found one spot it
fails:

  $ ./ci/run-style-check.sh @~1
  diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c
  index 1809539201..341d9eac0d 100644
  --- a/builtin/receive-pack.c
  +++ b/builtin/receive-pack.c
  @@ -1929,12 +1929,11 @@ static void
execute_commands_non_atomic(struct command *commands,
   			goto cleanup;

   	failure:
  -		for (cmd = commands; cmd; cmd = cmd->next) {
  +		for (cmd = commands; cmd; cmd = cmd->next)
   			if (reported_error)
   				cmd->error_string = reported_error;
   			else if (strmap_contains(&failed_refs, cmd->ref_name))
   				cmd->error_string = strmap_get(&failed_refs, cmd->ref_name);
  -		}

   	cleanup:
   		ref_transaction_free(transaction);

We generally add brackets in such scenarios, but the style-check
suggests to use them instead.

> How often do people use "git clang-format" before they submit their
> series these days, by the way?
>

To be honest, I don't think anyone does. As far as I've seen there is
too many false positives currently.

>> diff --git a/ci/run-style-check.sh b/ci/run-style-check.sh
>> index 76dd37d22b..6cd4b1d934 100755
>> --- a/ci/run-style-check.sh
>> +++ b/ci/run-style-check.sh
>> @@ -5,4 +5,21 @@
>>
>>  baseCommit=$1
>>
>> -git clang-format --style file --diff --extensions c,h "$baseCommit"
>> +# 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 \
>> +	--diff --extensions c,h "$baseCommit"

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]