Christian Couder <christian.couder@xxxxxxxxx> 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
;-)
