.clang-format: how useful, how often used, and how well maintained?

.clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

Cc-list chosen from "git shortlog --since=12.months --no-merges .clang-format".

I am wondering how often our developers use "make style" aka

    git clang-format --style file --diff --extensions c,h

and also wondering if the suggested style fixes are really
"improvements".  For example, taking randomly the latest patch I
just injested into my tree, i.e.

    $ git am a-single-patch-file.txt
    $ git reset --soft HEAD^
    $ make style

I got the output attached at the end of the message.  The result is
a mixed bag (I commented on the "patch" as if it were a patch
submission).

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

Even the output from the tool is of mixed quality, there are good
pieces that can be used to improve your patches.  So we may prefer
to see the tool used more often, but not in a way to suggest its
output is always better than what the human developer has written.

For that, there are a few things we'd probably need to do:

 - Improve our tooling so that the develper can check a range of
   commits they made before running format-patch, and other
   situations.

 - Improve .clang-format rules to reduce false positives.

> 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.

>  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.

> @@ -735,19 +734,20 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
>  		 * The searches must start from the same position.
>  		 */
>  		sig_sha1 = find_commit_multiline_header(sig_cursor + 1,
> -							"gpgsig",
> -							&after_sha1);
> +							"gpgsig", &after_sha1);
>  		sig_sha256 = find_commit_multiline_header(sig_cursor + 1,
>  							  "gpgsig-sha256",
>  							  &after_sha256);

This is a suggestion that is clearly worse than the original.  These
two statements should look similar as they are doing similar things.
Line wrapping the former only because it uses tokens slightly
shorter than the ones used by the latter inevitably makes them look
more different.

This is why I am dubious of any automated tools that have to make
their decision mechanically.

Is there a way to express:

    We want lines that are longer than the 80-column limit to be
    wrapped at 80-column, but do not coalesce shorter lines only
    to make them into a smaller number of longer lines.

If we can say "wrap overly long lines, whose definition is longer
than 100-column, at 80-column" in the earlier half of the sentence,
it would be even better.

> -		/* Warn on any additional signatures, as they will be ignored. */
> +		/* Warn on any additional signatures, as they will be ignored.
> +		 */

Looks significantly worse.

Is there a way to express:

    Our multi-line comments begin and end with slash-asterisk and
    asterisk-slash on their own line without anything else.

>  		if (sig_sha1)
>  			warn_on_extra_sig(&after_sha1, commit, 1);
>  		if (sig_sha256)
>  			warn_on_extra_sig(&after_sha256, commit, 0);
>  
> -		commit_buffer_cursor = (after_sha1 > after_sha256) ? after_sha1 : after_sha256;
> +		commit_buffer_cursor =
> +			(after_sha1 > after_sha256) ? after_sha1 : after_sha256;

Good.

> diff --git a/builtin/fast-import.c b/builtin/fast-import.c
> index 48ce8ebb77..5da80e69f3 100644
> --- a/builtin/fast-import.c
> +++ b/builtin/fast-import.c
> @@ -2720,19 +2720,21 @@ static struct hash_list *parse_merge(unsigned int *count)
>  }
>  
>  struct signature_data {
> -	char *hash_algo;      /* "sha1" or "sha256" */
> -	char *sig_format;     /* "openpgp", "x509", "ssh", "unknown" */
> -	struct strbuf data;   /* The actual signature data */
> +	char *hash_algo; /* "sha1" or "sha256" */
> +	char *sig_format; /* "openpgp", "x509", "ssh", "unknown" */
> +	struct strbuf data; /* The actual signature data */
>  };

This is not better or worse, where "once the code is written"
comment would apply.

>  static void parse_one_signature(struct signature_data *sig, const char *v)
>  {
> -	char *args = xstrdup(v); /* Will be freed when sig->hash_algo is freed */
> +	char *args = xstrdup(v); /* Will be freed when sig->hash_algo is freed
> +				  */

Looks significantly worse.

>  	char *space = strchr(args, ' ');
>  
>  	if (!space)
>  		die("Expected gpgsig format: 'gpgsig <hash-algo> <signature-format>', "
> -		    "got 'gpgsig %s'", args);
> +		    "got 'gpgsig %s'",
> +		    args);

What was the tool thinking when it made this suggestion?  IOW, is
there a stupid rule in .clang-format kicking in?

> @@ -2744,8 +2746,7 @@ static void parse_one_signature(struct signature_data *sig, const char *v)
>  		*space = '\0';
>  
>  	/* Validate hash algorithm */
> -	if (strcmp(sig->hash_algo, "sha1") &&
> -	    strcmp(sig->hash_algo, "sha256"))
> +	if (strcmp(sig->hash_algo, "sha1") && strcmp(sig->hash_algo, "sha256"))
>  		die("Unknown git hash algorithm in gpgsig: '%s'", sig->hash_algo);

This is probably slightly worse from extensibility's pov, which a
mechanical tool cannot make a good judgement, but the author of the
original did ;-)

> @@ -2759,8 +2760,7 @@ static void parse_one_signature(struct signature_data *sig, const char *v)
>  	parse_data(&sig->data, 0, NULL);
>  }
>  
> -static void add_gpgsig_to_commit(struct strbuf *commit_data,
> -				 const char *header,
> +static void add_gpgsig_to_commit(struct strbuf *commit_data, const char *header,
>  				 struct signature_data *sig)

"once the code is written".

> @@ make: *** [Makefile:3346: style] Error 1
-2778,8 +2778,7 @@ static void add_gpgsig_to_commit(struct strbuf *commit_data,
>  }
>  
>  static void store_signature(struct signature_data *stored_sig,
> -			    struct signature_data *new_sig,
> -			    const char *hash_type)
> +			    struct signature_data *new_sig, const char *hash_type)

"once the code is written".

> diff --git a/gpg-interface.c b/gpg-interface.c
> index 6f2d87475f..3e17f69cdc 100644
> --- a/gpg-interface.c
> +++ b/gpg-interface.c
> @@ -152,8 +152,7 @@ const char *get_signature_format(const char *buf)
>  
>  int valid_signature_format(const char *format)
>  {
> -       return (!!get_format_by_name(format) ||
> -	       !strcmp(format, "unknown"));
> +	return (!!get_format_by_name(format) || !strcmp(format, "unknown"));
>  }

"once the code is written".

Re: .clang-format: how useful, how often used, and how well maintained?

[Christian Couder]

On Thu, Jun 19, 2025 at 6:39 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Cc-list chosen from "git shortlog --since=12.months --no-merges .clang-format".
>
> I am wondering how often our developers use "make style" aka
>
>     git clang-format --style file --diff --extensions c,h
>
> and also wondering if the suggested style fixes are really
> "improvements".

Karthik and I discussed this today in the context of the "check-style"
GitLab CI job which often fails even if it doesn't make the whole CI
job fail. We agreed that it might be a good idea to either improve or
just disable some dubious style checks.

> For example, taking randomly the latest patch I
> just injested into my tree, i.e.
>
>     $ git am a-single-patch-file.txt
>     $ git reset --soft HEAD^
>     $ make style
>
> I got the output attached at the end of the message.  The result is
> a mixed bag (I commented on the "patch" as if it were a patch
> submission).
>
> 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?

> Even the output from the tool is of mixed quality, there are good
> pieces that can be used to improve your patches.  So we may prefer
> to see the tool used more often, but not in a way to suggest its
> output is always better than what the human developer has written.
>
> For that, there are a few things we'd probably need to do:
>
>  - Improve our tooling so that the develper can check a range of
>    commits they made before running format-patch, and other
>    situations.
>
>  - Improve .clang-format rules to reduce false positives.

I agree that both of these would be nice.

> > 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.

> >  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.

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,

  - suggest wrapping lines only when they are longer than
"max(existing lines around the hunk, our maximum line length
default)".

> > @@ -735,19 +734,20 @@ static void handle_commit(struct commit *commit, struct rev_info *rev,
> >                * The searches must start from the same position.
> >                */
> >               sig_sha1 = find_commit_multiline_header(sig_cursor + 1,
> > -                                                     "gpgsig",
> > -                                                     &after_sha1);
> > +                                                     "gpgsig", &after_sha1);
> >               sig_sha256 = find_commit_multiline_header(sig_cursor + 1,
> >                                                         "gpgsig-sha256",
> >                                                         &after_sha256);
>
> This is a suggestion that is clearly worse than the original.  These
> two statements should look similar as they are doing similar things.

I agree.

> Line wrapping the former only because it uses tokens slightly
> shorter than the ones used by the latter inevitably makes them look
> more different.
>
> This is why I am dubious of any automated tools that have to make
> their decision mechanically.

When such a tool suggests wrapping too long lines, it's more likely to
be useful than when it suggests unwrapping short lines. So I think
that if we could configure the tool so that it stops suggesting
unwrapping lines, we could likely reduce the number of false positives
without much drawback.

> Is there a way to express:
>
>     We want lines that are longer than the 80-column limit to be
>     wrapped at 80-column, but do not coalesce shorter lines only
>     to make them into a smaller number of longer lines.
>
> If we can say "wrap overly long lines, whose definition is longer
> than 100-column, at 80-column" in the earlier half of the sentence,
> it would be even better.

Yeah, if it could look at lines around the current hunk that could be
even better.

> > -             /* Warn on any additional signatures, as they will be ignored. */
> > +             /* Warn on any additional signatures, as they will be ignored.
> > +              */
>
> Looks significantly worse.

I agree.

> Is there a way to express:
>
>     Our multi-line comments begin and end with slash-asterisk and
>     asterisk-slash on their own line without anything else.

If there is no way to express this, then it might be better to disable
wrapping any comment if the tool has a knob for that.

[...]

> >       char *space = strchr(args, ' ');
> >
> >       if (!space)
> >               die("Expected gpgsig format: 'gpgsig <hash-algo> <signature-format>', "
> > -                 "got 'gpgsig %s'", args);
> > +                 "got 'gpgsig %s'",
> > +                 args);
>
> What was the tool thinking when it made this suggestion?  IOW, is
> there a stupid rule in .clang-format kicking in?

Yeah, this is kind of strange because it looks like the opposite of
what the tool suggested in some cases above.

> > @@ -2744,8 +2746,7 @@ static void parse_one_signature(struct signature_data *sig, const char *v)
> >               *space = '\0';
> >
> >       /* Validate hash algorithm */
> > -     if (strcmp(sig->hash_algo, "sha1") &&
> > -         strcmp(sig->hash_algo, "sha256"))
> > +     if (strcmp(sig->hash_algo, "sha1") && strcmp(sig->hash_algo, "sha256"))
> >               die("Unknown git hash algorithm in gpgsig: '%s'", sig->hash_algo);
>
> This is probably slightly worse from extensibility's pov, which a
> mechanical tool cannot make a good judgement, but the author of the
> original did ;-)

Thanks ;-)

That makes me wonder if we could use an AI tool with a fixed prompt
(that we could improve over time) and provide it our CodingGuideline
for these kinds of things rather than a mechanical tool.

[...]

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

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
;-)

Re: .clang-format: how useful, how often used, and how well maintained?

[Christian Couder]

On Fri, Jun 20, 2025 at 2:20 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > 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's up to each one to decide if they prefer post-commit or pre-commit
hooks or other ways to trigger the style check. So yeah, we could both
suggest using hooks and add a format-patch option to make it easier
for those who don't want a hook.

>  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.

I agree that we shouldn't recommend doing that.

> Or just write that command invocation into "MyFirstContribution" etc.?

Yeah, we could do that too.

> >> 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.

In your earlier message, you said "the updated one is not worse", now
you say it's still an improvement. My opinion is that it's not clear
that it's an improvement, especially because the updated one doesn't
group "(unsigned)strlen(signature)" and "signature" together on the
same line. So I would say it's more bikeshedding territory than a
clear improvement.

> 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.

If we encourage a style checker to make suggestions that are often in
bikeshedding territory, then I think we take the risks of:

  - annoying some users who disagree with some suggestions,
  - having bikeshedding discussions on the list (like this one) about
things that are just not worth it,
  - the style checker being actually wrong (because of the context for example).

In my opinion the possible small gains are not worth taking those
risks. In other words from a style checker I'd rather have fewer
suggestions that are more likely to be right, than more suggestions
that are more likely to be dubious.

> >> >  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.

I agree that in the example above the suggested change might be good.
But if there is for example the following code:

my_foo_variable = my_function_with_a_very_long_name(foo1, foo2, foo3);
my_bar_variable = my_function_with_a_very_long_name(bar1, bar2, bar3);

and a patch wants to replace "bar" with "baz", I don't think you would
like the patch to look like:

 my_foo_variable = my_function_with_a_very_long_name(foo1, foo2, foo3);
-my_bar_variable = my_function_with_a_very_long_name(bar1, bar2, bar3);
+my_baz_variable = my_function_with_a_very_long_name(baz1,

                        baz2,

                        baz3);

So my opinion is that a style checker that doesn't take into account
the length of the lines around where it makes suggestions is likely to
make a number of wrong line wrapping suggestions.

> > 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
> ;-)

I didn't mean that users should send the patch ouput as-is ;-) The
idea is about encouraging users to send preparatory patches to improve
formatting on whole files, and this way get source code files that are
more coherent. Then it would be less likely that suggestions from the
tool about a subsequent patch are dubious or wrong.

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

Christian Couder <christian.couder@gmail.com> writes:

> It's up to each one to decide if they prefer post-commit or pre-commit
> hooks or other ways to trigger the style check. So yeah, we could both
> suggest using hooks and add a format-patch option to make it easier
> for those who don't want a hook.

The thing is, I have no idea what the "option" to format-patch you
talk about should look like.  This topic is about improving Git
codebase by helping Git developers, and I do not want to add a
project-specific option to the tool that is meant for the general
public.

>> Or just write that command invocation into "MyFirstContribution" etc.?
>
> Yeah, we could do that too.

I think that is a good first step.  Give an example that shows that
the tool can give suboptimal suggestions as well as good ones, to
stress the point that the output from the tool should not be taken
blindly, but should still be looked at.

>> 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.
>
> If we encourage a style checker to make suggestions that are often in
> bikeshedding territory, then I think we take the risks of:

I do not think I am advocating to encourage a style checker to make
bikeshedding suggestions at all.  We are not discussing any change
to .clang-format file in this discussion (a side thread we discussed
about concluding a year-old experiment, which would result in a small
change to the file, though).

>   - annoying some users who disagree with some suggestions,

That I do not think we care too much.  This is not about "users".

>   - having bikeshedding discussions on the list (like this one) about
> things that are just not worth it,

But now we have a good thing to point at.  "You want to bikeshed?
Let's see what clang-format with our recommended setting says."  And
the discussion should end there.

>   - the style checker being actually wrong (because of the context for example).

If you are worried about "bikeshedding territory", there is no
additional risks for the tool to be actually wrong.  Whether you
blindly take its stylistic changes or not, you'd need to be careful
about tool changing the meanings of the code it is touching.

> In my opinion the possible small gains are not worth taking those
> risks.

So, from my point of view, I am not sure if there is _any_ risks,
but again I do not know who you think is advocating for more
bikeshedding via the style checker.

> In other words from a style checker I'd rather have fewer
> suggestions that are more likely to be right, than more suggestions
> that are more likely to be dubious.

No question about that, but I see no logical linkage between what
you said above and this goal, sorry.

> I agree that in the example above the suggested change might be good.

> But if there is ...

Don't move the goalpost.  It is not the case we are discussing.

> So my opinion is that a style checker that doesn't take into account
> the length of the lines around where it makes suggestions is likely to
> make a number of wrong line wrapping suggestions.

Your example is not even about "around", but the exact line that is
being changed, isn't it?  Even if the line being changed were the
only overly long line, and all lines around it were below the length
that is comfortably read with minimal horizontal eye movement, we
still want to wrap it down, don't we?

Re: .clang-format: how useful, how often used, and how well maintained?

[Jeff King]

On Fri, Jun 20, 2025 at 04:08:32PM +0200, Christian Couder wrote:

> > 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's up to each one to decide if they prefer post-commit or pre-commit
> hooks or other ways to trigger the style check. So yeah, we could both
> suggest using hooks and add a format-patch option to make it easier
> for those who don't want a hook.

I'm not sure I would want to make changes at the format-patch stage. If
I am adjusting commits, I'd want those changes in my local commits, too.
Then they'd be there if I re-roll, etc (plus they'd actually be tested
when I run "make test"!). Ditto for folks using GGG, where they need to
push the fully-realized commits.

My ideal workflow would probably be taking a pass with:

  git rebase -x 'git clang-format --style=file -p HEAD^ || git commit --no-edit --amend -a'

is a better match. That command is a bit of a mouthful, but we could
perhaps roll it into a script or a Makefile target. The current "make
style" only looks at uncommitted changes in the working tree (and of
course isn't interactive).

The big pain I see in this (or any other workflow) is getting bugged
about suggestions you've rejected. In an ideal world we'd tune
.clang-format so that all of its suggestions are good, but I don't think
we're there yet. ;)

-Peff

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> My ideal workflow would probably be taking a pass with:
>
>   git rebase -x 'git clang-format --style=file -p HEAD^ || git commit --no-edit --amend -a'
>
> is a better match.  That command is a bit of a mouthful, but we
> could perhaps roll it into a script or a Makefile target.

The "-p" option does sound like a good thing.  Taking the tool's
suggestion wholesale without human judgement would be better than
taking what a human developer typed as-is only for somebody totally
new to the language and to the project.  For more experienced
developers, I would trust human judgement a lot more than I trust
the tool's suggestion, and "-p" may be a good escape hatch.

> The current "make
> style" only looks at uncommitted changes in the working tree (and of
> course isn't interactive).

Yes, that makes it much less useful than it could be.

> The big pain I see in this (or any other workflow) is getting bugged
> about suggestions you've rejected. In an ideal world we'd tune
> .clang-format so that all of its suggestions are good, but I don't think
> we're there yet. ;)

Me neither.

Re: .clang-format: how useful, how often used, and how well maintained?

[brian m. carlson]

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

On 2025-06-19 at 16:38:35, Junio C Hamano wrote:
> Cc-list chosen from "git shortlog --since=12.months --no-merges .clang-format".
> 
> I am wondering how often our developers use "make style" aka
> 
>     git clang-format --style file --diff --extensions c,h
> 
> and also wondering if the suggested style fixes are really
> "improvements".  For example, taking randomly the latest patch I
> just injested into my tree, i.e.
> 
>     $ git am a-single-patch-file.txt
>     $ git reset --soft HEAD^
>     $ make style

I don't at the moment.  In my view, the main utility of tidy tools is
that the project has picked a style, whatever everyone may think of
it[0], and we apply the tool consistently on every change and enforce
it. Then, the application of the tidy tool becomes a rote keystroke in
one's editor and one does not need to think about it.  This is how
things work in most Rust and Go projects, for instance, since they have
well-defined tidy tools.

That is not how things work here, however.

> I got the output attached at the end of the message.  The result is
> a mixed bag (I commented on the "patch" as if it were a patch
> submission).
> 
> 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

I agree most people probably do not use it, probably for the reasons I
don't.  I don't know if clang-format produces stable output: that is,
using a newer version of clang-format with the same config does not
result in diff changes.  If it does, then we can simply pick a set of
style configs and a minimum version and tell people to apply it.

We can then check it in CI and if CI fails, we can output a
base64-encoded diff (since it's going to have lots of whitespace, base64
encoding will be practically useful) that the author can apply.  Then
people using esoteric systems without clang-format can simply apply the
diff from CI.

If clang-format does not produce stable output, we're going to have a
bunch of practical problems.  I use Debian unstable at home and I know
Peff does as well, but I also use Ubuntu 24.04 at work.  Some
contributors use Fedora or Cygwin, and we're all going to have a giant
problem picking a consistent version of clang-format to use such that
people don't have to compile their own or use external packages. Perhaps
we can create a small script that does the tidying in a Linux
Docker/Podman container in that case.

> Even the output from the tool is of mixed quality, there are good
> pieces that can be used to improve your patches.  So we may prefer
> to see the tool used more often, but not in a way to suggest its
> output is always better than what the human developer has written.

I really would prefer us to pick a set of standards that is good enough
and just apply them.  I agree clang-format may not produce ideal output,
but I really do not want to think about formatting and style and whether
my lines exceed 80 characters.  Fixing those style issues is annoying
and I can say that it often delays me getting to re-rolls.

> For that, there are a few things we'd probably need to do:
> 
>  - Improve our tooling so that the develper can check a range of
>    commits they made before running format-patch, and other
>    situations.

I agree better tooling would be valuable.

>  - Improve .clang-format rules to reduce false positives.

I think we should iterate on the rules a bit to get them to good enough
and then commit to the style.

[0] Go's tool, gofmt, even acknowledges that the style it uses is
nobody's favourite, but having it is better than bikeshedding arguments
over style.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: .clang-format: how useful, how often used, and how well maintained?

[Collin Funk]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I agree most people probably do not use it, probably for the reasons I
> don't.  I don't know if clang-format produces stable output: that is,
> using a newer version of clang-format with the same config does not
> result in diff changes.  If it does, then we can simply pick a set of
> style configs and a minimum version and tell people to apply it.
>
> We can then check it in CI and if CI fails, we can output a
> base64-encoded diff (since it's going to have lots of whitespace, base64
> encoding will be practically useful) that the author can apply.  Then
> people using esoteric systems without clang-format can simply apply the
> diff from CI.
>
> If clang-format does not produce stable output, we're going to have a
> bunch of practical problems.  I use Debian unstable at home and I know
> Peff does as well, but I also use Ubuntu 24.04 at work.  Some
> contributors use Fedora or Cygwin, and we're all going to have a giant
> problem picking a consistent version of clang-format to use such that
> people don't have to compile their own or use external packages. Perhaps
> we can create a small script that does the tidying in a Linux
> Docker/Podman container in that case.

From what I remember, clang-format is not at all stable between
releases. Newer versions will produce different output than old 
ones (usually better, but that does not matter).

For the reasons that you already mention, it ends up being a chore, in
my opinion. I don't think we should expect everyone to build/install a
clang-format version that is newer or older than what their distro ships
with, just to align the output with the project.

If you wanted to be help avoid badly formatted patches adding a .vimrc
and .dir-locals.el file would cover most people, I think. For Emacs, the
.dir-locals.el would be something simple like:

    (c-mode . ((c-file-style . "linux")
               (fill-column . 80)
               ((indent-tabs-mode . t))))

At least with Emacs it is easy to type things that break these rules. So
one can avoid diffs like this, which clang-format would produce:

> -		/* Warn on any additional signatures, as they will be ignored. */
> +		/* Warn on any additional signatures, as they will be ignored.
> +		 */

I assume this is similar for vim, but I do not use it enough.

Collin

Re: .clang-format: how useful, how often used, and how well maintained?

[brian m. carlson]

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

On 2025-06-19 at 21:31:03, Collin Funk wrote:
> From what I remember, clang-format is not at all stable between
> releases. Newer versions will produce different output than old
> ones (usually better, but that does not matter).
> 
> For the reasons that you already mention, it ends up being a chore, in
> my opinion. I don't think we should expect everyone to build/install a
> clang-format version that is newer or older than what their distro ships
> with, just to align the output with the project.

Yeah, then in that case, we probably want to ship some sort of container
and script that can do that.  Our default Rust target is Debian stable,
so that seems like a decent target if we need to pick a distro.  It's
also a very common distro used in containers, so it's widely available
to contributors using container-based development environments.

I still think that if we're going to have this functionality and expect
it to be used, we need to make it the default, build appropriate
tooling, and check it in CI.  If it's not fire-and-forget, people won't
use it.

> If you wanted to be help avoid badly formatted patches adding a .vimrc
> and .dir-locals.el file would cover most people, I think. For Emacs, the
> .dir-locals.el would be something simple like:
> 
>     (c-mode . ((c-file-style . "linux")
>                (fill-column . 80)
>                ((indent-tabs-mode . t))))
> 
> At least with Emacs it is easy to type things that break these rules. So
> one can avoid diffs like this, which clang-format would produce:
> 
> > -		/* Warn on any additional signatures, as they will be ignored. */
> > +		/* Warn on any additional signatures, as they will be ignored.
> > +		 */
> 
> I assume this is similar for vim, but I do not use it enough.

Certainly there are a lot of Vim and Emacs users in this project, but
there are also many people who are not.  I use Neovim myself and still
have to deal with wrapping lines at 80 characters.

I also don't think Vim actually honours per-directory configuration like
`.dir-locals.el` by default without turning on the `exrc` option, which
is rightly documented as a security risk.
-- 
brian m. carlson (they/them)
Toronto, Ontario, CA

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

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

"brian m. carlson" <sandals@crustytoothpaste.net> writes:

> I still think that if we're going to have this functionality and expect
> it to be used, we need to make it the default, build appropriate
> tooling, and check it in CI.  If it's not fire-and-forget, people won't
> use it.

There probably needs some balancing act, as I already pointed out,
what clang-format gives often do not make sense, and the point is
that they are not about styles (where we can safely say "no style is
liked by everybody") but about how readable the result is (which
sometimes is subjective but more often it is not).  Until the tool
and its configuration is polished enough, blindly applying the
result with fire-and-forget mentality will degrade the quality of
our codebase.

Re: .clang-format: how useful, how often used, and how well maintained?

[Toon Claes]

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

> "brian m. carlson" <sandals@crustytoothpaste.net> writes:
>
>> Yeah, then in that case, we probably want to ship some sort of container
>> and script that can do that.  Our default Rust target is Debian stable,
>> so that seems like a decent target if we need to pick a distro.  It's
>> also a very common distro used in containers, so it's widely available
>> to contributors using container-based development environments.

That sounds like the ideal solution.

>> I still think that if we're going to have this functionality and expect
>> it to be used, we need to make it the default, build appropriate
>> tooling, and check it in CI.  If it's not fire-and-forget, people won't
>> use it.
>
> There probably needs some balancing act, as I already pointed out,
> what clang-format gives often do not make sense, and the point is
> that they are not about styles (where we can safely say "no style is
> liked by everybody") but about how readable the result is (which
> sometimes is subjective but more often it is not).  Until the tool
> and its configuration is polished enough, blindly applying the
> result with fire-and-forget mentality will degrade the quality of
> our codebase.

Allow me to share an unpopular opinion. I think you either fully commit
to a formatter, or you don't care about formatting at all. I realize
that's probably overly strict for most people, but I've been working
mostly in Golang for several years, and having a tool that formats code
and it's output is unarguably the standard is a bliss.

I think the only way we can stop bikeshedding about formatting, is by
adopting clang-format and make it's output the golden standard. We might
not like it's output (similar to many people do not like `gofmt`s
output), but it's a standard. If we have to wait for clang-format to
support all the configuration options we prefer, we will be having this
conversation over and over again over time. I don't think that's worth
it.

Code formatting should be the job of an automated tool, not a person.
It's annoying to have this back-and-forth in reviews because it's not
following the standard _the Git project_ has set, while it would be a
lot less friction to follow a standard that's set by _the formatting
tool_.

-- 
Cheers,
Toon

Re: .clang-format: how useful, how often used, and how well maintained?

[Johannes Schindelin]

Hi Toon,

On Tue, 1 Jul 2025, Toon Claes wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > "brian m. carlson" <sandals@crustytoothpaste.net> writes:
> >
> >> I still think that if we're going to have this functionality and
> >> expect it to be used, we need to make it the default, build
> >> appropriate tooling, and check it in CI.  If it's not
> >> fire-and-forget, people won't use it.
> >
> > There probably needs some balancing act, as I already pointed out,
> > what clang-format gives often do not make sense, and the point is that
> > they are not about styles (where we can safely say "no style is liked
> > by everybody") but about how readable the result is (which sometimes
> > is subjective but more often it is not).  Until the tool and its
> > configuration is polished enough, blindly applying the result with
> > fire-and-forget mentality will degrade the quality of our codebase.
> 
> Allow me to share an unpopular opinion. I think you either fully commit
> to a formatter, or you don't care about formatting at all. I realize
> that's probably overly strict for most people, but I've been working
> mostly in Golang for several years, and having a tool that formats code
> and it's output is unarguably the standard is a bliss.
> 
> I think the only way we can stop bikeshedding about formatting, is by
> adopting clang-format and make it's output the golden standard. We might
> not like it's output (similar to many people do not like `gofmt`s
> output), but it's a standard. If we have to wait for clang-format to
> support all the configuration options we prefer, we will be having this
> conversation over and over again over time. I don't think that's worth
> it.
> 
> Code formatting should be the job of an automated tool, not a person.
> It's annoying to have this back-and-forth in reviews because it's not
> following the standard _the Git project_ has set, while it would be a
> lot less friction to follow a standard that's set by _the formatting
> tool_.

For the record, I share this opinion, and I don't even think that it is
unpopular. There is a reason why there's ESLint, gofmt, rustfmt, RuboCop,
etc. Even cURL comes with a helpful `make checksrc` with little room for
distracting nitpicks about code style, making more room for a focus on
correct code.

Ciao,
Johannes

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

Toon Claes <toon@iotcl.com> writes:

> I think the only way we can stop bikeshedding about formatting, is by
> adopting clang-format and make it's output the golden standard. We might
> not like it's output (similar to many people do not like `gofmt`s
> output), but it's a standard.

Having other people to blame when the tool leaves unreadable code
that is not easy to read is certainly handy.  But I care more about
the tool giving a reasonably readable result.  It is not about
people not "liking" it.

Here is what clang-format suggests on top of your "last-modified"
series.  For this particular example, the tool gets the resulting
format mostly right, except for an extra space after "foreach"
before the opening parenthesis.  I presume that this is some setting
in the .clang-format file can tweak?

There is one instance of 80-column line wrapping making the result
less easy to view.  If you need to wrap, keep related things
together, i.e. when you rewrite this line ...

-int last_modified_run(struct last_modified *lm, last_modified_callback cb, void *cbdata)

... we should not wrap after "cb," only because it is still shorter
than 80 columns.

+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata)

In fact, slightly busting the 80-column limit like the original had
would be easier to understand while coasting your eyes over the line.
If we need to wrap, we should do this instead ...

+int last_modified_run(struct last_modified *lm,
+		      last_modified_callback cb, void *cbdata)

... so that related things (i.e., the callback function and the
data fed to it) are kept together.

In this particular patch, there was only one such instance that the
tools output was noticeably more irritating than the original.  With
more excercise like this with enough positive experience (I do count
this one as positive, if we fix the space after "foreach"), I might
change my mind.

Anyway, here is the patch.

 builtin.h               |  3 ++-
 builtin/last-modified.c |  6 ++----
 git.c                   | 20 +++++++++++---------
 last-modified.c         | 26 ++++++++++++--------------
 last-modified.h         | 14 +++++---------
 5 files changed, 32 insertions(+), 37 deletions(-)

diff --git c/builtin.h i/builtin.h
index 6ed6759ec4..5f89e51c61 100644
--- c/builtin.h
+++ i/builtin.h
@@ -176,7 +176,8 @@ int cmd_hook(int argc, const char **argv, const char *prefix, struct repository
 int cmd_index_pack(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_init_db(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_interpret_trailers(int argc, const char **argv, const char *prefix, struct repository *repo);
-int cmd_last_modified(int argc, const char **argv, const char *prefix, struct repository *repo);
+int cmd_last_modified(int argc, const char **argv, const char *prefix,
+		      struct repository *repo);
 int cmd_log_reflog(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_log(int argc, const char **argv, const char *prefix, struct repository *repo);
 int cmd_ls_files(int argc, const char **argv, const char *prefix, struct repository *repo);
diff --git c/builtin/last-modified.c i/builtin/last-modified.c
index 4ff058c302..86b98e7a46 100644
--- c/builtin/last-modified.c
+++ i/builtin/last-modified.c
@@ -23,10 +23,8 @@ static void show_entry(const char *path, const struct commit *commit, void *d)
 	fflush(stdout);
 }
 
-int cmd_last_modified(int argc,
-		   const char **argv,
-		   const char *prefix,
-		   struct repository *repo)
+int cmd_last_modified(int argc, const char **argv, const char *prefix,
+		      struct repository *repo)
 {
 	struct last_modified lm;
 
diff --git c/git.c i/git.c
index 65afc0d0e7..918d83762a 100644
--- c/git.c
+++ i/git.c
@@ -516,12 +516,10 @@ static struct cmd_struct commands[] = {
 	{ "check-attr", cmd_check_attr, RUN_SETUP },
 	{ "check-ignore", cmd_check_ignore, RUN_SETUP | NEED_WORK_TREE },
 	{ "check-mailmap", cmd_check_mailmap, RUN_SETUP },
-	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT  },
+	{ "check-ref-format", cmd_check_ref_format, NO_PARSEOPT },
 	{ "checkout", cmd_checkout, RUN_SETUP | NEED_WORK_TREE },
-	{ "checkout--worker", cmd_checkout__worker,
-		RUN_SETUP | NEED_WORK_TREE },
-	{ "checkout-index", cmd_checkout_index,
-		RUN_SETUP | NEED_WORK_TREE},
+	{ "checkout--worker", cmd_checkout__worker, RUN_SETUP | NEED_WORK_TREE },
+	{ "checkout-index", cmd_checkout_index, RUN_SETUP | NEED_WORK_TREE },
 	{ "cherry", cmd_cherry, RUN_SETUP },
 	{ "cherry-pick", cmd_cherry_pick, RUN_SETUP | NEED_WORK_TREE },
 	{ "clean", cmd_clean, RUN_SETUP | NEED_WORK_TREE },
@@ -578,10 +576,14 @@ static struct cmd_struct commands[] = {
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
-	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-ours", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-theirs", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-subtree", cmd_merge_recursive,
+	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 	{ "mktag", cmd_mktag, RUN_SETUP },
 	{ "mktree", cmd_mktree, RUN_SETUP },
diff --git c/last-modified.c i/last-modified.c
index 2097894c6e..f7f6a67d3b 100644
--- c/last-modified.c
+++ i/last-modified.c
@@ -19,8 +19,7 @@ struct last_modified_entry {
 };
 
 static void add_path_from_diff(struct diff_queue_struct *q,
-			       struct diff_options *opt UNUSED,
-			       void *data)
+			       struct diff_options *opt UNUSED, void *data)
 {
 	struct last_modified *lm = data;
 
@@ -72,9 +71,9 @@ static int populate_paths_from_revs(struct last_modified *lm)
 }
 
 static int last_modified_entry_hashcmp(const void *unused UNUSED,
-				    const struct hashmap_entry *hent1,
-				    const struct hashmap_entry *hent2,
-				    const void *path)
+				       const struct hashmap_entry *hent1,
+				       const struct hashmap_entry *hent2,
+				       const void *path)
 {
 	const struct last_modified_entry *ent1 =
 		container_of(hent1, const struct last_modified_entry, hashent);
@@ -83,10 +82,8 @@ static int last_modified_entry_hashcmp(const void *unused UNUSED,
 	return strcmp(ent1->path, path ? path : ent2->path);
 }
 
-int last_modified_init(struct last_modified *lm,
-		     struct repository *r,
-		     const char *prefix,
-		     int argc, const char **argv)
+int last_modified_init(struct last_modified *lm, struct repository *r,
+		       const char *prefix, int argc, const char **argv)
 {
 	memset(lm, 0, sizeof(*lm));
 	hashmap_init(&lm->paths, last_modified_entry_hashcmp, NULL, 0);
@@ -119,7 +116,7 @@ void last_modified_release(struct last_modified *lm)
 	struct hashmap_iter iter;
 	struct last_modified_entry *ent;
 
-	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent)
+	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent)
 		clear_bloom_key(&ent->key);
 
 	hashmap_clear_and_free(&lm->paths, struct last_modified_entry, hashent);
@@ -213,7 +210,7 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	if (!filter)
 		return 1;
 
-	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent) {
+	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent) {
 		if (bloom_filter_contains(filter, &ent->key,
 					  lm->rev.bloom_filter_settings))
 			return 1;
@@ -221,7 +218,8 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	return 0;
 }
 
-int last_modified_run(struct last_modified *lm, last_modified_callback cb, void *cbdata)
+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata)
 {
 	struct last_modified_callback_data data;
 
@@ -245,8 +243,8 @@ int last_modified_run(struct last_modified *lm, last_modified_callback cb, void
 
 		if (data.commit->object.flags & BOUNDARY) {
 			diff_tree_oid(lm->rev.repo->hash_algo->empty_tree,
-				       &data.commit->object.oid,
-				       "", &lm->rev.diffopt);
+				      &data.commit->object.oid, "",
+				      &lm->rev.diffopt);
 			diff_flush(&lm->rev.diffopt);
 		} else {
 			log_tree_commit(&lm->rev, data.commit);
diff --git c/last-modified.h i/last-modified.h
index 04d5a1a5b6..3e83094d77 100644
--- c/last-modified.h
+++ i/last-modified.h
@@ -13,23 +13,19 @@ struct last_modified {
 /*
  * Initialize the last-modified machinery using command line arguments.
  */
-int last_modified_init(struct last_modified *lm,
-		     struct repository *r,
-		     const char *prefix,
-		     int argc, const char **argv);
+int last_modified_init(struct last_modified *lm, struct repository *r,
+		       const char *prefix, int argc, const char **argv);
 
 void last_modified_release(struct last_modified *);
 
 typedef void (*last_modified_callback)(const char *path,
-				    const struct commit *commit,
-				    void *data);
+				       const struct commit *commit, void *data);
 
 /*
  * Run the last-modified traversal. For each path found the callback is called
  * passing the path, the commit, and the cbdata.
  */
-int last_modified_run(struct last_modified *lm,
-		   last_modified_callback cb,
-		   void *cbdata);
+int last_modified_run(struct last_modified *lm, last_modified_callback cb,
+		      void *cbdata);
 
 #endif /* LAST_MODIFIED_H */

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

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

> In this particular patch, there was only one such instance that the
> tools output was noticeably more irritating than the original.  With
> more excercise like this with enough positive experience (I do count
> this one as positive, if we fix the space after "foreach"), I might
> change my mind.

And for completeness, if you choose to take the tool-generated style
changes, here is what I want you to also place on top.

And then, I want those who advocate blind application of tool's
output, only because then we would have somebody else to blame for
poorly formatted code, to think if they can improve the rule to
consider these points as well.

Thanks.


--- >8 ---
Subject: [PATCH] (style) fix bad changes suggested by clang-format

 * The commands array in git.c is traditionally one line per
   command, a possibly long line is not wrapped for grep-ability.

 * No SP between "foreach" and "(".

 * When wrapping a line, keep related things together.
---
 git.c           | 12 ++++--------
 last-modified.c |  8 ++++----
 2 files changed, 8 insertions(+), 12 deletions(-)

diff --git a/git.c b/git.c
index 918d83762a..8ad1ce8dea 100644
--- a/git.c
+++ b/git.c
@@ -576,14 +576,10 @@ static struct cmd_struct commands[] = {
 	{ "merge-file", cmd_merge_file, RUN_SETUP_GENTLY },
 	{ "merge-index", cmd_merge_index, RUN_SETUP | NO_PARSEOPT },
 	{ "merge-ours", cmd_merge_ours, RUN_SETUP | NO_PARSEOPT },
-	{ "merge-recursive", cmd_merge_recursive,
-	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-ours", cmd_merge_recursive,
-	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-recursive-theirs", cmd_merge_recursive,
-	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
-	{ "merge-subtree", cmd_merge_recursive,
-	  RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-ours", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-recursive-theirs", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
+	{ "merge-subtree", cmd_merge_recursive, RUN_SETUP | NEED_WORK_TREE | NO_PARSEOPT },
 	{ "merge-tree", cmd_merge_tree, RUN_SETUP },
 	{ "mktag", cmd_mktag, RUN_SETUP },
 	{ "mktree", cmd_mktree, RUN_SETUP },
diff --git a/last-modified.c b/last-modified.c
index f7f6a67d3b..dfa3b2d324 100644
--- a/last-modified.c
+++ b/last-modified.c
@@ -116,7 +116,7 @@ void last_modified_release(struct last_modified *lm)
 	struct hashmap_iter iter;
 	struct last_modified_entry *ent;
 
-	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent)
+	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent)
 		clear_bloom_key(&ent->key);
 
 	hashmap_clear_and_free(&lm->paths, struct last_modified_entry, hashent);
@@ -210,7 +210,7 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	if (!filter)
 		return 1;
 
-	hashmap_for_each_entry (&lm->paths, &iter, ent, hashent) {
+	hashmap_for_each_entry(&lm->paths, &iter, ent, hashent) {
 		if (bloom_filter_contains(filter, &ent->key,
 					  lm->rev.bloom_filter_settings))
 			return 1;
@@ -218,8 +218,8 @@ static int maybe_changed_path(struct last_modified *lm, struct commit *origin)
 	return 0;
 }
 
-int last_modified_run(struct last_modified *lm, last_modified_callback cb,
-		      void *cbdata)
+int last_modified_run(struct last_modified *lm,
+		      last_modified_callback cb, void *cbdata)
 {
 	struct last_modified_callback_data data;
 
-- 
2.50.0-278-g6118fc98aa

Re: .clang-format: how useful, how often used, and how well maintained?

[Karthik Nayak]

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

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

> Cc-list chosen from "git shortlog --since=12.months --no-merges .clang-format".
>
> I am wondering how often our developers use "make style" aka
>
>     git clang-format --style file --diff --extensions c,h
>
> and also wondering if the suggested style fixes are really
> "improvements".  For example, taking randomly the latest patch I
> just injested into my tree, i.e.
>
>     $ git am a-single-patch-file.txt
>     $ git reset --soft HEAD^
>     $ make style
>
> I got the output attached at the end of the message.  The result is
> a mixed bag (I commented on the "patch" as if it were a patch
> submission).
>
> 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
>
> Even the output from the tool is of mixed quality, there are good
> pieces that can be used to improve your patches.  So we may prefer
> to see the tool used more often, but not in a way to suggest its
> output is always better than what the human developer has written.
>
> For that, there are a few things we'd probably need to do:
>
>  - Improve our tooling so that the develper can check a range of
>    commits they made before running format-patch, and other
>    situations.
>
>  - Improve .clang-format rules to reduce false positives.
>

I think the biggest issue for this is around line wrapping, I'm
considering just removing it from the '.clang-format'. Perhaps we could
add it to our '.editorconfig'?

The issue itself is that we don't always wrap to 80 characters. We try
to wrap to 80 characters, but prioritize readability over this limit.

This is hard to do in '.clang-format' since it requires tuning the
penalties, which have arbitrary values. I did try to do that in
5e9fa0f9fa (clang-format: re-adjust line break penalties, 2024-10-18),
but as we see, that wasn't very successfull.

If that sounds okay, I can send in a patch to do that and also making
'RemoveBracesLLVM: true' a permanent rule.

[snip]

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

Re: .clang-format: how useful, how often used, and how well maintained?

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

>> For that, there are a few things we'd probably need to do:
>>
>>  - Improve our tooling so that the develper can check a range of
>>    commits they made before running format-patch, and other
>>    situations.
>>
>>  - Improve .clang-format rules to reduce false positives.
>>
>
> I think the biggest issue for this is around line wrapping, I'm
> considering just removing it from the '.clang-format'. Perhaps we could
> add it to our '.editorconfig'?

I would not stop others from trying to improve the rules in such a
way that only an overly long lines (like >120 columns) are folded to
reasonable length (line ~72 columns) without doing anything else
(like not concatenating adjacent lines only because the result would
be shorter than 80 columns), but if it is more involved than we can
manage, removing it from .clang-format so that "make style" would
not use would be the best (or "the least bad") approach.

Re: .clang-format: how useful, how often used, and how well maintained?

[Karthik Nayak]

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

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

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>>> For that, there are a few things we'd probably need to do:
>>>
>>>  - Improve our tooling so that the develper can check a range of
>>>    commits they made before running format-patch, and other
>>>    situations.
>>>
>>>  - Improve .clang-format rules to reduce false positives.
>>>
>>
>> I think the biggest issue for this is around line wrapping, I'm
>> considering just removing it from the '.clang-format'. Perhaps we could
>> add it to our '.editorconfig'?
>
> I would not stop others from trying to improve the rules in such a
> way that only an overly long lines (like >120 columns) are folded to
> reasonable length (line ~72 columns) without doing anything else
> (like not concatenating adjacent lines only because the result would
> be shorter than 80 columns), but if it is more involved than we can
> manage, removing it from .clang-format so that "make style" would
> not use would be the best (or "the least bad") approach.

Agreed. I would definitely like to see us solve this issue eventually.
For now, perhaps removing false positives is just more beneficial.

Let me send in some patches and we can see what everyone thinks.

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