From: Junio C Hamano <gitster@pobox•com>
To: Christian Couder <christian.couder@gmail•com>
Cc: git@vger•kernel.org, "Karthik Nayak" <karthik.188@gmail•com>,
"Patrick Steinhardt" <ps@pks•im>, "René Scharfe" <l.s.r@web•de>
Subject: Re: .clang-format: how useful, how often used, and how well maintained?
Date: Thu, 19 Jun 2025 17:20:48 -0700 [thread overview]
Message-ID: <xmqqfrfv8dr3.fsf@gitster.g> (raw)
In-Reply-To: <CAP8UFD0YEgh4Oy8MDpT0DfZJgo++NHf3mF6VsYxAG1CjhrKGLQ@mail.gmail.com> (Christian Couder's message of "Thu, 19 Jun 2025 22:30:01 +0200")
Christian Couder <christian.couder@gmail•com> writes:
>> I have this suspicion that nobody complained these sub-par
>> suggestions the tool makes based on what we have in .clang-format
>> because not many folks run "make style", and "make style" is not
>> very easy to use after you record your changes into a commit. IOW,
>> there is nothing packaged to help "I have four commits on top of the
>> upstream, I want to run style checks before running format-patch",
>> i.e.
>>
>> git clang-format --diff HEAD~4
>
> Maybe a format-patch option (perhaps called '--format-check') could be
> added to run such a command before format-patch actually outputs the
> patches?
A post-commit hook that does *not* prevent your changes that do not
pass the "style-check" from getting committed, but does give you a
feedback that let you consider before moving forward? It could be
pre-commit hook that stops you, but then the people may bend their
code to please the "style-check" and commit a sub-par code, which is
not what we want.
Or just write that command invocation into "MyFirstContribution" etc.?
>> > git clang-format --style file --diff --extensions c,h diff --git
>> > a/builtin/fast-export.c b/builtin/fast-export.c index
>> > 332c036ee4..d89e5ba6d5 100644 --- a/builtin/fast-export.c +++
>> > b/builtin/fast-export.c @@ -658,17 +658,16 @@ static void
>> > print_signature(const char *signature, const char *object_hash) if
>> > (!signature) return;
>> >
>> > - printf("gpgsig %s %s\ndata %u\n%s",
>> > - object_hash,
>> > - get_signature_format(signature),
>> > - (unsigned)strlen(signature),
>> > + printf("gpgsig %s %s\ndata %u\n%s", object_hash,
>> > + get_signature_format(signature), (unsigned)strlen(signature),
>> > signature);
>> > }
>>
>> I do not mind the original but the updated one is not worse. IOW, I
>> would reject if a human sent this patch to fix the original that is
>> already in-tree with "once the code is written in an acceptable way,
>> it is not worth the patch noise to replace it with the updated one
>> that is not significantly better".
>>
>> I'll call this kind "once the code is written" in the rest of the
>> message.
>
> Yeah, I think those should be considered false positives. They are not
> worth failing the "check-style" CI job or even having a human look at
> them.
We disagree here. I notice that at least GitHub CI suite does not
use clang-format task for "new 'seen' was pushed, so let's check"
set of jobs. The style checks are done for pull requests, and I
think that is a more appropriate place.
And I do not consider it false positive IF they are pointed out on
the changes that are *not* in tree yet. On the other hand, if the
preimage and the postimage of the style checker's suggestions were
iterations of the same series, neither of which has hit 'next', then
I would consider a change like the above not "false positive". It
is still an improvement; it is not improvement enough to warrant a
churn by piling new commits on top, once the preimage hits the
public tree.
What I called "once the code is written" is something I would refuse
to accept as a "style fix" patch, but they are still improvements
and it would be great if contributors followed these style checker's
suggestion _before_ sending the patch to the list.
>> > static void warn_on_extra_sig(const char **pos, struct commit *commit, int is_sha1)
>> > {
>> > const char *header = is_sha1 ? "gpgsig" : "gpgsig-sha256";
>> > - const char *extra_sig = find_commit_multiline_header(*pos + 1, header, pos);
>> > + const char *extra_sig =
>> > + find_commit_multiline_header(*pos + 1, header, pos);
>>
>> OK.
>
> I don't think those are OK. If the existing code already has longer
> lines, like perhaps here the `static void warn_on_extra_sig(...)` line
> above, then it's not worth suggesting wrapping lines like this. It
> could result in a code with both long lines and wrapped short ones
> which could be puzzling and harder to read than if the code had only
> long lines.
Existing mistakes are not excuses for piling new ones on top.
I do not think the code with suggested change here is making the
code so uneven to make it hard to read. Quite the contrary, the
body being easier to read is a good thing. There is one
contributing factor that clang-format may not be able to understand
(or perhaps there is a way to do so that we are not taking advantage
of). There also is a reason to special case a line that has return
type + function name + parameter list and allow it to go over the
usual limit, which is grep-ability.
> Ideally our tools should be able to:
>
> - provide full patch (including the commit message) which correctly
> wraps all the long lines in a file, so that such a patch can easily be
> created and added as a preparatory patch to a patch series,
Ah, I wasn't talking about the proposed log message part.
Especially in genAIs era, I would not want to go there, just not yet
;-)
next prev parent reply other threads:[~2025-06-20 0:20 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
2025-06-19 16:38 .clang-format: how useful, how often used, and how well maintained? Junio C Hamano
2025-06-19 20:30 ` Christian Couder
2025-06-20 0:20 ` Junio C Hamano [this message]
2025-06-20 14:08 ` Christian Couder
2025-06-20 16:36 ` Junio C Hamano
2025-06-21 5:07 ` Jeff King
2025-06-22 4:11 ` Junio C Hamano
2025-06-19 21:17 ` brian m. carlson
2025-06-19 21:31 ` Collin Funk
2025-06-19 22:56 ` brian m. carlson
2025-06-20 15:19 ` Junio C Hamano
2025-07-01 14:08 ` Toon Claes
2025-07-01 16:59 ` Johannes Schindelin
2025-07-01 20:42 ` Junio C Hamano
2025-07-01 21:12 ` Junio C Hamano
2025-06-23 8:46 ` Karthik Nayak
2025-06-23 16:26 ` Junio C Hamano
2025-06-24 23:27 ` Karthik Nayak
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqfrfv8dr3.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=christian.couder@gmail$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=karthik.188@gmail$(echo .)com \
--cc=l.s.r@web$(echo .)de \
--cc=ps@pks$(echo .)im \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox