| Message ID | 20211113033358.2179376-2-andersk@mit.edu (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | protect branches checked out in all worktrees | expand |
Anders Kaseorg <andersk@mit.edu> writes: > Documentation/CodingGuidelines says “do not end error messages with a > full stop” and “do not capitalize the first word”. Reviewers requested > updating the existing messages to comply with these guidelines prior to > the following patches. Thanks. Whether reviewers requested or you thought of it on your own, separating such a preliminary clean-up into its own patch would be a good idea, especially if the later patches need to update (some of) them. > @@ -1062,13 +1062,13 @@ static void close_fetch_head(struct fetch_head *fetch_head) > } > > static const char warn_show_forced_updates[] = > -N_("Fetch normally indicates which branches had a forced update,\n" > - "but that check has been disabled. To re-enable, use '--show-forced-updates'\n" > - "flag or run 'git config fetch.showForcedUpdates true'."); > +N_("fetch normally indicates which branches had a forced update,\n" > + "but that check has been disabled; to re-enable, use '--show-forced-updates'\n" > + "flag or run 'git config fetch.showForcedUpdates true'"); > static const char warn_time_show_forced_updates[] = > -N_("It took %.2f seconds to check forced updates. You can use\n" > +N_("it took %.2f seconds to check forced updates; you can use\n" > "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n" > - " to avoid this check.\n"); > + " to avoid this check\n"); The two guidelines cited in the proposed log message are primarily to prefer fatal: unrecognized argument: --no-such-option over fatal: Unrecognized argument: --no-such-option. and does not say much what to do to a multi-sentence message. In this part (and other parts) of the patch, I can see that you thought this one through when preparing this patch. I very much appreciate it. The approach chosen (consistently) in this patch is to (1) turn them into a (semi) single sentence, concatenated with ';' (2) as a side effect of not being a free-standing sentence anymore, the second and subsequent sentences in the original, that are now just pieces in a single sentence separated with ';', do not get capitalized, and (3) the sentence as a whole lacks the full-stop, just like a single sentence message. I think we are fine with these rules, especially given that these multi-sentence messages are not the main part of this topic touches and are not the primary focus of this topic anyway. I am highlighting this part of the change, just in case others think of a better set of rules to follow. Existing multi-sentence messages follow different ad-hoc patterns, it seems (e.g. "git show 00000000"). Thanks.
On Tue, 16 Nov 2021, Junio C Hamano wrote: > > Documentation/CodingGuidelines says “do not end error messages with a > > full stop” and “do not capitalize the first word”. Reviewers requested > > updating the existing messages to comply with these guidelines prior to > > the following patches. > > Thanks. Whether reviewers requested or you thought of it on your > own, separating such a preliminary clean-up into its own patch would > be a good idea, especially if the later patches need to update (some > of) them. It was your request; I just mentioned it in case other reviewers wonder why this belongs in this topic. https://public-inbox.org/git/xmqq8rxvwp4b.fsf@gitster.g/ > The approach chosen (consistently) in this patch is to > > (1) turn them into a (semi) single sentence, concatenated with ';' > > (2) as a side effect of not being a free-standing sentence anymore, > the second and subsequent sentences in the original, that are > now just pieces in a single sentence separated with ';', do not > get capitalized, and > > (3) the sentence as a whole lacks the full-stop, just like a single > sentence message. > > I think we are fine with these rules, especially given that these > multi-sentence messages are not the main part of this topic touches > and are not the primary focus of this topic anyway. I guess I should add that there are a few messages I left alone: refuse_unconfigured_deny_msg and refuse_unconfigured_deny_delete_current_msg in builtin/receive-pack.c (patch 2/8). Not sure if they count as “error messages”, but these multi-paragraph messages are too long to comfortably apply this approach. Now I see that I missed a few others. This could be squashed into the first three patches: diff --git a/builtin/fetch.c b/builtin/fetch.c index f373252490..f3c7c057d0 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -1783,7 +1783,7 @@ static int fetch_failed_to_start(struct strbuf *out, void *cb, void *task_cb) struct parallel_fetch_state *state = cb; const char *remote = task_cb; - state->result = error(_("Could not fetch %s"), remote); + state->result = error(_("could not fetch %s"), remote); return 0; } @@ -1838,7 +1838,7 @@ static int fetch_multiple(struct string_list *list, int max_children) if (verbosity >= 0) printf(_("Fetching %s\n"), name); if (run_command_v_opt(argv.v, RUN_GIT_CMD)) { - error(_("Could not fetch %s"), name); + error(_("could not fetch %s"), name); result = 1; } strvec_pop(&argv); diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c index d72058543e..70b4e23a26 100644 --- a/builtin/receive-pack.c +++ b/builtin/receive-pack.c @@ -2495,9 +2495,9 @@ int cmd_receive_pack(int argc, const char **argv, const char *prefix) argc = parse_options(argc, argv, prefix, options, receive_pack_usage, 0); if (argc > 1) - usage_msg_opt(_("Too many arguments."), receive_pack_usage, options); + usage_msg_opt(_("too many arguments"), receive_pack_usage, options); if (argc == 0) - usage_msg_opt(_("You must specify a directory."), receive_pack_usage, options); + usage_msg_opt(_("you must specify a directory"), receive_pack_usage, options); service_dir = argv[0]; diff --git a/branch.c b/branch.c index 3a7d205fa4..0bea1335ae 100644 --- a/branch.c +++ b/branch.c @@ -64,7 +64,7 @@ int install_branch_config(int flag, const char *local, const char *origin, const if (skip_prefix(remote, "refs/heads/", &shortname) && !strcmp(local, shortname) && !origin) { - warning(_("Not setting branch %s as its own upstream."), + warning(_("not setting branch %s as its own upstream"), local); return 0; } @@ -116,7 +116,7 @@ int install_branch_config(int flag, const char *local, const char *origin, const out_err: strbuf_release(&key); - error(_("Unable to write upstream branch configuration")); + error(_("unable to write upstream branch configuration")); advise(_(tracking_advice), origin ? origin : "", Anders
Anders Kaseorg <andersk@mit.edu> writes: > On Tue, 16 Nov 2021, Junio C Hamano wrote: >> > Documentation/CodingGuidelines says “do not end error messages with a >> > full stop” and “do not capitalize the first word”. Reviewers requested >> > updating the existing messages to comply with these guidelines prior to >> > the following patches. >> >> Thanks. Whether reviewers requested or you thought of it on your >> own, separating such a preliminary clean-up into its own patch would >> be a good idea, especially if the later patches need to update (some >> of) them. > > It was your request; I just mentioned it in case other reviewers wonder > why this belongs in this topic. Sorry, let me try again, as I wasn't clear enough. Readers of "git log", for whom we write our log messages, are not interested if reviewers suggested, or you came up on your own. The more relevant thing for them to learn from our log messages is the reason why that solution was chosen (and the fact that the author is now committed to the chosen solution---not "this does not make much sense but I am randomly updating as I was told"). E.g. ... first word". This file has many existing messages that violate these guidelines. Clean them up in preparation for subsequent patches that touch some of these messages. or something like that.
On Sat, Nov 13, 2021 at 11:34 AM Anders Kaseorg <andersk@mit.edu> wrote: > static const char warn_show_forced_updates[] = > -N_("Fetch normally indicates which branches had a forced update,\n" > - "but that check has been disabled. To re-enable, use '--show-forced-updates'\n" > - "flag or run 'git config fetch.showForcedUpdates true'."); > +N_("fetch normally indicates which branches had a forced update,\n" > + "but that check has been disabled; to re-enable, use '--show-forced-updates'\n" > + "flag or run 'git config fetch.showForcedUpdates true'"); > static const char warn_time_show_forced_updates[] = > -N_("It took %.2f seconds to check forced updates. You can use\n" > +N_("it took %.2f seconds to check forced updates; you can use\n" > "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n" > - " to avoid this check.\n"); > + " to avoid this check\n"); The leading space character before "to avoid ..." is not necessary. This will change "po/git.pot" like this: https://github.com/git-l10n/git-po/blob/pot/seen/2021-11-19.diff#L374-L377 It was introduced in commit 182f59daf0 (l10n: reformat some localized strings for v2.23.0, 2019-08-06). -- Jiang Xin
diff --git a/builtin/fetch.c b/builtin/fetch.c index e064687dbd..e5971fa6e5 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c @@ -552,7 +552,7 @@ static struct ref *get_ref_map(struct remote *remote, for (i = 0; i < fetch_refspec->nr; i++) get_fetch_map(ref_map, &fetch_refspec->items[i], &oref_tail, 1); } else if (refmap.nr) { - die("--refmap option is only meaningful with command-line refspec(s)."); + die("--refmap option is only meaningful with command-line refspec(s)"); } else { /* Use the defaults */ struct branch *branch = branch_get(NULL); @@ -583,7 +583,7 @@ static struct ref *get_ref_map(struct remote *remote, } else if (!prefetch) { ref_map = get_remote_ref(remote_refs, "HEAD"); if (!ref_map) - die(_("Couldn't find remote ref HEAD")); + die(_("couldn't find remote ref HEAD")); ref_map->fetch_head_status = FETCH_HEAD_MERGE; tail = &ref_map->next; } @@ -1062,13 +1062,13 @@ static void close_fetch_head(struct fetch_head *fetch_head) } static const char warn_show_forced_updates[] = -N_("Fetch normally indicates which branches had a forced update,\n" - "but that check has been disabled. To re-enable, use '--show-forced-updates'\n" - "flag or run 'git config fetch.showForcedUpdates true'."); +N_("fetch normally indicates which branches had a forced update,\n" + "but that check has been disabled; to re-enable, use '--show-forced-updates'\n" + "flag or run 'git config fetch.showForcedUpdates true'"); static const char warn_time_show_forced_updates[] = -N_("It took %.2f seconds to check forced updates. You can use\n" +N_("it took %.2f seconds to check forced updates; you can use\n" "'--no-show-forced-updates' or run 'git config fetch.showForcedUpdates false'\n" - " to avoid this check.\n"); + " to avoid this check\n"); static int store_updated_refs(const char *raw_url, const char *remote_name, int connectivity_checked, struct ref *ref_map) @@ -1381,8 +1381,9 @@ static void check_not_current_branch(struct ref *ref_map) for (; ref_map; ref_map = ref_map->next) if (ref_map->peer_ref && !strcmp(current_branch->refname, ref_map->peer_ref->name)) - die(_("Refusing to fetch into current branch %s " - "of non-bare repository"), current_branch->refname); + die(_("refusing to fetch into current branch %s " + "of non-bare repository"), + current_branch->refname); } static int truncate_fetch_head(void) @@ -1400,10 +1401,10 @@ static void set_option(struct transport *transport, const char *name, const char { int r = transport_set_option(transport, name, value); if (r < 0) - die(_("Option \"%s\" value \"%s\" is not valid for %s"), + 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\n"), name, transport->url); } @@ -1437,7 +1438,7 @@ static void add_negotiation_tips(struct git_transport_options *smart_options) old_nr = oids->nr; for_each_glob_ref(add_oid, s, oids); if (old_nr == oids->nr) - warning("Ignoring --negotiation-tip=%s because it does not match any refs", + warning("ignoring --negotiation-tip=%s because it does not match any refs", s); } smart_options->negotiation_tips = oids; @@ -1475,7 +1476,7 @@ static struct transport *prepare_transport(struct remote *remote, int deepen) if (transport->smart_options) add_negotiation_tips(transport->smart_options); else - warning("Ignoring --negotiation-tip because the protocol does not support it."); + warning("ignoring --negotiation-tip because the protocol does not support it"); } return transport; } @@ -1638,8 +1639,8 @@ static int do_fetch(struct transport *transport, else warning(_("unknown branch type")); } else { - warning(_("no source branch found.\n" - "you need to specify exactly one branch with the --set-upstream option.")); + warning(_("no source branch found;\n" + "you need to specify exactly one branch with the --set-upstream option")); } } skip: @@ -1893,8 +1894,8 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, int remote_via_config = remote_is_configured(remote, 0); if (!remote) - die(_("No remote repository specified. Please, specify either a URL or a\n" - "remote name from which new revisions should be fetched.")); + die(_("no remote repository specified; please specify either a URL or a\n" + "remote name from which new revisions should be fetched")); gtransport = prepare_transport(remote, 1); @@ -1929,7 +1930,7 @@ static int fetch_one(struct remote *remote, int argc, const char **argv, if (!strcmp(argv[i], "tag")) { i++; if (i >= argc) - die(_("You need to specify a tag name.")); + die(_("you need to specify a tag name")); refspec_appendf(&rs, "refs/tags/%s:refs/tags/%s", argv[i], argv[i]); @@ -1997,7 +1998,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (deepen_relative) { if (deepen_relative < 0) - die(_("Negative depth in --deepen is not supported")); + die(_("negative depth in --deepen is not supported")); if (depth) die(_("--deepen and --depth are mutually exclusive")); depth = xstrfmt("%d", deepen_relative); @@ -2034,14 +2035,15 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) /* All arguments are assumed to be remotes or groups */ for (i = 0; i < argc; i++) if (!add_remote_or_group(argv[i], &list)) - die(_("No such remote or remote group: %s"), argv[i]); + die(_("no such remote or remote group: %s"), + argv[i]); } else { /* Single remote or group */ (void) add_remote_or_group(argv[0], &list); if (list.nr > 1) { /* More than one remote */ if (argc > 1) - die(_("Fetching a group and specifying refspecs does not make sense")); + die(_("fetching a group and specifying refspecs does not make sense")); } else { /* Zero or one remotes */ remote = remote_get(argv[0]); @@ -2062,7 +2064,7 @@ int cmd_fetch(int argc, const char **argv, const char *prefix) if (gtransport->smart_options) { gtransport->smart_options->acked_commits = &acked_commits; } else { - warning(_("Protocol does not support --negotiate-only, exiting.")); + warning(_("protocol does not support --negotiate-only, exiting")); return 1; } if (server_options.nr)
Documentation/CodingGuidelines says “do not end error messages with a full stop” and “do not capitalize the first word”. Reviewers requested updating the existing messages to comply with these guidelines prior to the following patches. Signed-off-by: Anders Kaseorg <andersk@mit.edu> --- builtin/fetch.c | 46 ++++++++++++++++++++++++---------------------- 1 file changed, 24 insertions(+), 22 deletions(-)