On Fri, Jun 20, 2025 at 2:20 AM Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Christian Couder <christian.couder@xxxxxxxxx> 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.
