| Message ID | 20230428125649.1719796-3-oswald.buddenhagen@gmx.de (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b734fe49fddb4bbd02f471fa3b11d3605bd6921d |
| Headers | show |
| Series | [v2,1/3] advice: handle "rebase" in error_resolve_conflict() | expand |
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > These are conscious violations of the usual rules for error messages, > based on this reasoning: > - If an error message is directly followed by another sentence, it needs > to be properly terminated with a period, lest the grammar looks broken > and becomes hard to read. > - That second sentence isn't actually an error message any more, so it > should abide to conventional language rules for good looks and > legibility. Arguably, these should be converted to advice messages > (which the user can squelch, too), but that's a much bigger effort to > get right. I think both of the above are good guidelines to follow, with a hint for a longer-term plan. Good description. > - Neither of these apply to the first hunk in do_exec(), but this > two-line message looks just too much like a real sentence to not > terminate it. Also, leaving it alone would make it asymmetrical to > the other hunk. I do not have a strong opinion on this one, in the sense that if somebody writes a patch to terminate the sentence with the above justification, I do not think I would object to it, and if somebody omits this change from a patch like this, because the above two guidelines do not apply to it, I would not object to it, either. > Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> > Cc: Junio C Hamano <gitster@pobox.com> We generally do not want Cc: in the trailers. Can you move them after the three-dash lines (which I think is still read by send-email) next time you create more patches and throw them at the list? Will queue. > --- > v2: > - tried to make commit message more convincing > - put it last in the series, to make the less controversial changes > easier to apply > --- > builtin/pull.c | 2 +- > sequencer.c | 4 ++-- > 2 files changed, 3 insertions(+), 3 deletions(-) > > diff --git a/builtin/pull.c b/builtin/pull.c > index 56f679d94a..bb2ddc93ab 100644 > --- a/builtin/pull.c > +++ b/builtin/pull.c > @@ -1044,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) > if (!opt_autostash) > require_clean_work_tree(the_repository, > N_("pull with rebase"), > - _("please commit or stash them."), 1, 0); > + _("Please commit or stash them."), 1, 0); > > if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs)) > oidclr(&rebase_fork_point); > diff --git a/sequencer.c b/sequencer.c > index 0677c9ab09..21748bbfb0 100644 > --- a/sequencer.c > +++ b/sequencer.c > @@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line) > "\n"), > command_line, > dirty ? _("and made changes to the index and/or the " > - "working tree\n") : ""); > + "working tree.\n") : ""); > if (status == 127) > /* command not found */ > status = 1; > } else if (dirty) { > warning(_("execution succeeded: %s\nbut " > - "left changes to the index and/or the working tree\n" > + "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"
Junio C Hamano <gitster@pobox.com> writes: > Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes: > >> These are conscious violations of the usual rules for error messages, >> based on this reasoning: >> - If an error message is directly followed by another sentence, it needs >> to be properly terminated with a period, lest the grammar looks broken >> and becomes hard to read. >> - That second sentence isn't actually an error message any more, so it >> should abide to conventional language rules for good looks and >> legibility. Arguably, these should be converted to advice messages >> (which the user can squelch, too), but that's a much bigger effort to >> get right. > > I think both of the above are good guidelines to follow, with a hint > for a longer-term plan. Good description. I think the above two deserves to be added (in some rephrased form to fit better) to the Documentation/CodingGuidelines, somewhere in the following section: --- >8 --- Error Messages - Do not end error messages with a full stop. - Do not capitalize the first word, only because it is the first word in the message ("unable to open %s", not "Unable to open %s"). But "SHA-3 not supported" is fine, because the reason the first word is capitalized is not because it is at the beginning of the sentence, but because the word would be spelled in capital letters even when it appeared in the middle of the sentence. - Say what the error is first ("cannot open %s", not "%s: cannot open") --- 8< --- Volunteers? Thanks.
diff --git a/builtin/pull.c b/builtin/pull.c index 56f679d94a..bb2ddc93ab 100644 --- a/builtin/pull.c +++ b/builtin/pull.c @@ -1044,7 +1044,7 @@ int cmd_pull(int argc, const char **argv, const char *prefix) if (!opt_autostash) require_clean_work_tree(the_repository, N_("pull with rebase"), - _("please commit or stash them."), 1, 0); + _("Please commit or stash them."), 1, 0); if (get_rebase_fork_point(&rebase_fork_point, repo, *refspecs)) oidclr(&rebase_fork_point); diff --git a/sequencer.c b/sequencer.c index 0677c9ab09..21748bbfb0 100644 --- a/sequencer.c +++ b/sequencer.c @@ -3629,13 +3629,13 @@ static int do_exec(struct repository *r, const char *command_line) "\n"), command_line, dirty ? _("and made changes to the index and/or the " - "working tree\n") : ""); + "working tree.\n") : ""); if (status == 127) /* command not found */ status = 1; } else if (dirty) { warning(_("execution succeeded: %s\nbut " - "left changes to the index and/or the working tree\n" + "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"
These are conscious violations of the usual rules for error messages, based on this reasoning: - If an error message is directly followed by another sentence, it needs to be properly terminated with a period, lest the grammar looks broken and becomes hard to read. - That second sentence isn't actually an error message any more, so it should abide to conventional language rules for good looks and legibility. Arguably, these should be converted to advice messages (which the user can squelch, too), but that's a much bigger effort to get right. - Neither of these apply to the first hunk in do_exec(), but this two-line message looks just too much like a real sentence to not terminate it. Also, leaving it alone would make it asymmetrical to the other hunk. Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de> Cc: Junio C Hamano <gitster@pobox.com> --- v2: - tried to make commit message more convincing - put it last in the series, to make the less controversial changes easier to apply --- builtin/pull.c | 2 +- sequencer.c | 4 ++-- 2 files changed, 3 insertions(+), 3 deletions(-)