| Message ID | 20240905085149.GA2340826@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | drop trailing newline from warning/error/die messages | expand |
On Thu, Sep 05, 2024 at 04:51:49AM -0400, Jeff King wrote: > Our error reporting routines append a trailing newline, and the strings > we pass to them should not include them (otherwise we get an extra blank > line after the message). > > These cases were all found by looking at the results of: > > git grep -P '[^_](error|error_errno|warning|die|die_errno)\(.*\\n"[,)]' '*.c' > > Note that we _do_ sometimes include a newline in the middle of such > messages, to create multiline output (hence our grep matching "," or ")" > after we see the newline, so we know we're at the end of the string). > > It's possible that one or more of these cases could intentionally be > including a blank line at the end, but having looked at them all > manually, I think these are all just mistakes. > > Signed-off-by: Jeff King <peff@peff.net> > --- > I just happened to notice one of these, so I grepped for more. Do we maybe want to add a script that detects these via a Coccinelle script? As it turns out, Coccinelle has an embedded Python interpreter that allow us to extend it with arbitrary checks. So the following script would check for trailing newlines in `die()`, `die_errno()`, `error()` and `warning()`: @r@ expression fmt; position p; @@ ( die(_(fmt), ...)@p | die(fmt, ...)@p | die_errno(_(fmt), ...)@p | die_errno(fmt, ...)@p | error(_(fmt), ...)@p | error(fmt, ...)@p | warning(_(fmt), ...)@p | warning(fmt, ...)@p ) @script:python@ fmt << r.fmt; p << r.p; @@ if fmt.endswith("\\n\""): print("{}:{}:{}".format(p[0].file, p[0].line, fmt)) It doesn't auto-generate the patch for us. But at least we'd notice new instances of these via CI. Output on top of 2e7b89e038 (The twelfth batch, 2024-09-03): builtin/bisect.c:586: trailing newline in "revision walk setup failed\n" builtin/bisect.c:1111: trailing newline in "revision walk setup failed\n" builtin/push.c:639: trailing newline in "No configured push destination.\n" "Either specify the URL from the command-line or configure a remote repository using\n" "\n" " git remote add <name> <url>\n" "\n" "and then push using the remote name\n" "\n" " git push <name>\n" builtin/rebase.c:1410: trailing newline in "It seems that there is already a %s directory, and\n" "I wonder if you are in the middle of another rebase. " "If that is the\n" "case, please try\n\t%s\n" "If that is not the case, please\n\t%s\n" "and run me again. I am stopping in case you still " "have something\n" "valuable there.\n" compat/precompose_utf8.c:158: trailing newline in "iconv_open(%s,%s) failed, but needed:\n" " precomposed unicode is not supported.\n" " If you want to use decomposed unicode, run\n" " \"git config core.precomposeunicode false\"\n" sequencer.c:3824: trailing newline in "execution failed: %s\n%s" "You can fix the problem, and then run\n" "\n" " git rebase --continue\n" "\n" sequencer.c:3834: trailing newline in "execution succeeded: %s\nbut " "left changes to the index and/or the working tree.\n" "Commit or stash your changes, and then run\n" "\n" " git rebase --continue\n" "\n" unpack-trees.c:408: trailing newline in "the following paths have collided (e.g. case-sensitive paths\n" "on a case-insensitive filesystem) and only one from the same\n" "colliding group is in the working tree:\n" The last three lines are a bit of a false positive, I think. All of these calls are `warning()` messages though, so we could potentially just drop those conversions. Patrick
Jeff King <peff@peff.net> writes: > Our error reporting routines append a trailing newline, and the strings > we pass to them should not include them (otherwise we get an extra blank > line after the message). > > These cases were all found by looking at the results of: > > git grep -P '[^_](error|error_errno|warning|die|die_errno)\(.*\\n"[,)]' '*.c' > > Note that we _do_ sometimes include a newline in the middle of such > messages, to create multiline output (hence our grep matching "," or ")" > after we see the newline, so we know we're at the end of the string). > > It's possible that one or more of these cases could intentionally be > including a blank line at the end, but having looked at them all > manually, I think these are all just mistakes. > > Signed-off-by: Jeff King <peff@peff.net> > --- > I just happened to notice one of these, so I grepped for more. Just like what a good developer would do ;-) Thanks. I am a bit surprised (and feel somewhat relieved) that no tests try to match the messages exactly (with test_cmp, not with test_grep).
On Thu, Sep 05, 2024 at 12:57:54PM +0200, Patrick Steinhardt wrote: > > I just happened to notice one of these, so I grepped for more. > > Do we maybe want to add a script that detects these via a Coccinelle > script? As it turns out, Coccinelle has an embedded Python interpreter > that allow us to extend it with arbitrary checks. So the following > script would check for trailing newlines in `die()`, `die_errno()`, > `error()` and `warning()`: If you want to pursue this, I certainly have no objection, but personally I have burned enough time trying to make Coccinelle work that I don't want to fall down that particular rabbit hole again. ;) I think long ago we tried to avoid using the embedded python, because not all builds had it (notably Debian's). But I think that may not be the case anymore. If you did want to do automated quality checks of error messages, there are probably a few other common pitfalls we could detect (like ending with a full stop ".", and starting with a capital). > unpack-trees.c:408: trailing newline in "the following paths have collided (e.g. case-sensitive paths\n" "on a case-insensitive filesystem) and only one from the same\n" "colliding group is in the working tree:\n" > > The last three lines are a bit of a false positive, I think. All of > these calls are `warning()` messages though, so we could potentially > just drop those conversions. Yeah, mine didn't find those because it looked for "warning(" on the same source code line as the trailing newline. Which is sort of sloppy, but also kind of works because any message long enough to require multiple lines probably meant the author knew what they were doing. :) -Peff
diff --git a/builtin/bisect.c b/builtin/bisect.c index 453a6cccd7..c8aa92b19d 100644 --- a/builtin/bisect.c +++ b/builtin/bisect.c @@ -583,7 +583,7 @@ static int prepare_revs(struct bisect_terms *terms, struct rev_info *revs) refs_for_each_glob_ref_in(get_main_ref_store(the_repository), add_bisect_ref, good, "refs/bisect/", &cb); if (prepare_revision_walk(revs)) - res = error(_("revision walk setup failed\n")); + res = error(_("revision walk setup failed")); free(good); free(bad); @@ -1108,7 +1108,7 @@ static enum bisect_error bisect_skip(struct bisect_terms *terms, int argc, setup_revisions(2, argv + i - 1, &revs, NULL); if (prepare_revision_walk(&revs)) - die(_("revision walk setup failed\n")); + die(_("revision walk setup failed")); while ((commit = get_revision(&revs)) != NULL) strvec_push(&argv_state, oid_to_hex(&commit->object.oid)); diff --git a/builtin/fetch.c b/builtin/fetch.c index b2b5aee5bf..55f97134aa 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1161,7 +1161,7 @@ static int store_updated_refs(struct display_state *display_state, opt.exclude_hidden_refs_section = "fetch"; rm = ref_map; if (check_connected(iterate_ref_map, &rm, &opt)) { - rc = error(_("%s did not send all necessary objects\n"), + rc = error(_("%s did not send all necessary objects"), display_state->url); goto abort; } @@ -1458,7 +1458,7 @@ static void set_option(struct transport *transport, const char *name, const char die(_("option \"%s\" value \"%s\" is not valid for %s"), name, value, transport->url); if (r > 0) - warning(_("option \"%s\" is ignored for %s\n"), + warning(_("option \"%s\" is ignored for %s"), name, transport->url); } diff --git a/builtin/stash.c b/builtin/stash.c index fcfd97972a..be842258d0 100644 --- a/builtin/stash.c +++ b/builtin/stash.c @@ -484,7 +484,7 @@ static void unstage_changes_unless_new(struct object_id *orig_tree) " to make room.\n"), ce->name, new_path.buf); if (rename(ce->name, new_path.buf)) - die("Failed to move %s to %s\n", + die("Failed to move %s to %s", ce->name, new_path.buf); strbuf_release(&new_path); } diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 85fb23dee8..a46ffd49b3 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -917,7 +917,7 @@ static void generate_submodule_summary(struct summary_cb *info, } else { /* for a submodule removal (mode:0000000), don't warn */ if (p->mod_dst) - warning(_("unexpected mode %o\n"), p->mod_dst); + warning(_("unexpected mode %o"), p->mod_dst); } } diff --git a/fetch-pack.c b/fetch-pack.c index 58b4581ad8..983c560785 100644 --- a/fetch-pack.c +++ b/fetch-pack.c @@ -1614,7 +1614,7 @@ static void receive_packfile_uris(struct packet_reader *reader, while (packet_reader_read(reader) == PACKET_READ_NORMAL) { if (reader->pktlen < the_hash_algo->hexsz || reader->line[the_hash_algo->hexsz] != ' ') - die("expected '<hash> <uri>', got: %s\n", reader->line); + die("expected '<hash> <uri>', got: %s", reader->line); string_list_append(uris, reader->line); } diff --git a/loose.c b/loose.c index a8bf772172..6d6a41b7e5 100644 --- a/loose.c +++ b/loose.c @@ -162,7 +162,7 @@ int repo_write_loose_object_map(struct repository *repo) errout: rollback_lock_file(&lock); strbuf_release(&buf); - error_errno(_("failed to write loose object index %s\n"), path.buf); + error_errno(_("failed to write loose object index %s"), path.buf); strbuf_release(&path); return -1; } @@ -197,7 +197,7 @@ static int write_one_object(struct repository *repo, const struct object_id *oid strbuf_release(&path); return 0; errout: - error_errno(_("failed to write loose object index %s\n"), path.buf); + error_errno(_("failed to write loose object index %s"), path.buf); close(fd); rollback_lock_file(&lock); strbuf_release(&buf); diff --git a/negotiator/skipping.c b/negotiator/skipping.c index f65d47858b..6e61b3c5f1 100644 --- a/negotiator/skipping.c +++ b/negotiator/skipping.c @@ -239,7 +239,7 @@ static int ack(struct fetch_negotiator *n, struct commit *c) { int known_to_be_common = !!(c->object.flags & COMMON); if (!(c->object.flags & SEEN)) - die("received ack for commit %s not sent as 'have'\n", + die("received ack for commit %s not sent as 'have'", oid_to_hex(&c->object.oid)); mark_common(n->data, c); return known_to_be_common; diff --git a/send-pack.c b/send-pack.c index 9666b2c995..5d0c23772a 100644 --- a/send-pack.c +++ b/send-pack.c @@ -626,7 +626,7 @@ int send_pack(struct send_pack_args *args, strbuf_release(&req_buf); strbuf_release(&cap_buf); reject_atomic_push(remote_refs, args->send_mirror); - error("atomic push failed for ref %s. status: %d\n", + error("atomic push failed for ref %s. status: %d", ref->name, ref->status); return args->porcelain ? 0 : -1; } diff --git a/serve.c b/serve.c index 884cd84ca8..d674764a25 100644 --- a/serve.c +++ b/serve.c @@ -323,7 +323,7 @@ static int process_request(void) die("no command requested"); if (client_hash_algo != hash_algo_by_ptr(the_repository->hash_algo)) - die("mismatched object format: server %s; client %s\n", + die("mismatched object format: server %s; client %s", the_repository->hash_algo->name, hash_algos[client_hash_algo].name); diff --git a/t/helper/test-path-utils.c b/t/helper/test-path-utils.c index bf0e23ed50..fd6e6cc4a5 100644 --- a/t/helper/test-path-utils.c +++ b/t/helper/test-path-utils.c @@ -38,7 +38,7 @@ static void normalize_argv_string(const char **var, const char *input) *var = input; if (*var && (**var == '<' || **var == '(')) - die("Bad value: %s\n", input); + die("Bad value: %s", input); } struct test_data { @@ -78,12 +78,12 @@ static int test_function(struct test_data *data, char *(*func)(char *input), if (!strcmp(to, data[i].to)) continue; if (!data[i].alternative) - error("FAIL: %s(%s) => '%s' != '%s'\n", + error("FAIL: %s(%s) => '%s' != '%s'", funcname, data[i].from, to, data[i].to); else if (!strcmp(to, data[i].alternative)) continue; else - error("FAIL: %s(%s) => '%s' != '%s', '%s'\n", + error("FAIL: %s(%s) => '%s' != '%s', '%s'", funcname, data[i].from, to, data[i].to, data[i].alternative); failed = 1; diff --git a/t/helper/test-progress.c b/t/helper/test-progress.c index 66acb6a06c..44be2645e9 100644 --- a/t/helper/test-progress.c +++ b/t/helper/test-progress.c @@ -62,32 +62,32 @@ int cmd__progress(int argc, const char **argv) else if (*end == ' ') title = string_list_insert(&titles, end + 1)->string; else - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); progress = start_progress(title, total); } else if (skip_prefix(line.buf, "progress ", (const char **) &end)) { uint64_t item_count = strtoull(end, &end, 10); if (*end != '\0') - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); display_progress(progress, item_count); } else if (skip_prefix(line.buf, "throughput ", (const char **) &end)) { uint64_t byte_count, test_ms; byte_count = strtoull(end, &end, 10); if (*end != ' ') - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); test_ms = strtoull(end + 1, &end, 10); if (*end != '\0') - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); progress_test_ns = test_ms * 1000 * 1000; display_throughput(progress, byte_count); } else if (!strcmp(line.buf, "update")) { progress_test_force_update(); } else if (!strcmp(line.buf, "stop")) { stop_progress(&progress); } else { - die("invalid input: '%s'\n", line.buf); + die("invalid input: '%s'", line.buf); } } strbuf_release(&line); diff --git a/t/helper/test-reach.c b/t/helper/test-reach.c index 7314f6c0d8..995e382863 100644 --- a/t/helper/test-reach.c +++ b/t/helper/test-reach.c @@ -67,13 +67,13 @@ int cmd__reach(int ac, const char **av) peeled = deref_tag_noverify(the_repository, orig); if (!peeled) - die("failed to load commit for input %s resulting in oid %s\n", + die("failed to load commit for input %s resulting in oid %s", buf.buf, oid_to_hex(&oid)); c = object_as_type(peeled, OBJ_COMMIT, 0); if (!c) - die("failed to load commit for input %s resulting in oid %s\n", + die("failed to load commit for input %s resulting in oid %s", buf.buf, oid_to_hex(&oid)); switch (buf.buf[0]) {
Our error reporting routines append a trailing newline, and the strings we pass to them should not include them (otherwise we get an extra blank line after the message). These cases were all found by looking at the results of: git grep -P '[^_](error|error_errno|warning|die|die_errno)\(.*\\n"[,)]' '*.c' Note that we _do_ sometimes include a newline in the middle of such messages, to create multiline output (hence our grep matching "," or ")" after we see the newline, so we know we're at the end of the string). It's possible that one or more of these cases could intentionally be including a blank line at the end, but having looked at them all manually, I think these are all just mistakes. Signed-off-by: Jeff King <peff@peff.net> --- I just happened to notice one of these, so I grepped for more. builtin/bisect.c | 4 ++-- builtin/fetch.c | 4 ++-- builtin/stash.c | 2 +- builtin/submodule--helper.c | 2 +- fetch-pack.c | 2 +- loose.c | 4 ++-- negotiator/skipping.c | 2 +- send-pack.c | 2 +- serve.c | 2 +- t/helper/test-path-utils.c | 6 +++--- t/helper/test-progress.c | 10 +++++----- t/helper/test-reach.c | 4 ++-- 12 files changed, 22 insertions(+), 22 deletions(-)