Junio C HamanoCc-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 styleI 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.
Show 15 quoted lines
> 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.
Show 6 quoted lines
> 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.
Show 10 quoted lines
> @@ -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.Show 8 quoted lines
> 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.
Show 15 quoted lines
> 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.
Show 5 quoted lines
> 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.
Show 7 quoted lines
> 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?
Show 8 quoted lines
> @@ -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 ;-)
Show 8 quoted lines
> @@ -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,
Show 6 quoted lines
> } > > 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".
Show 12 quoted lines
> 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".
