* [PATCH v2 2/3] sequencer: actually translate report in do_exec()
2023-04-28 12:56 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Oswald Buddenhagen
@ 2023-04-28 12:56 ` Oswald Buddenhagen
2023-04-28 19:02 ` Junio C Hamano
2023-04-28 12:56 ` [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages Oswald Buddenhagen
` (2 subsequent siblings)
3 siblings, 1 reply; 21+ messages in thread
From: Oswald Buddenhagen @ 2023-04-28 12:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
N_() is meant to be used on strings that are subsequently _()'d, which
isn't the case here.
The affected construct is a bit questionable from an i18n perspective,
as it pieces together a sentence from separate strings. However, it
doesn't appear to be that bad, as the "assembly instructions" are in a
translatable message as well. Lacking specific complaints from
translators, it doesn't seem worth changing this.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>
---
v2:
- mention the word puzzle in the commit message
---
sequencer.c | 2 +-
1 file changed, 1 insertion(+), 1 deletion(-)
diff --git a/sequencer.c b/sequencer.c
index 3be23d7ca2..0677c9ab09 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)
" git rebase --continue\n"
"\n"),
command_line,
- dirty ? N_("and made changes to the index and/or the "
+ dirty ? _("and made changes to the index and/or the "
"working tree\n") : "");
if (status == 127)
/* command not found */
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 2/3] sequencer: actually translate report in do_exec()
2023-04-28 12:56 ` [PATCH v2 2/3] sequencer: actually translate report in do_exec() Oswald Buddenhagen
@ 2023-04-28 19:02 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-04-28 19:02 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> N_() is meant to be used on strings that are subsequently _()'d, which
> isn't the case here.
Good eyes.
>
> The affected construct is a bit questionable from an i18n perspective,
> as it pieces together a sentence from separate strings. However, it
> doesn't appear to be that bad, as the "assembly instructions" are in a
> translatable message as well. Lacking specific complaints from
> translators, it doesn't seem worth changing this.
True that we frown upon sentence Legos like this. At least the
original message in C locale does not break the flow too badly, so
hpoefully all the supported languages are happy with the existing
composition.
Will queue.
Thanks.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> Cc: Junio C Hamano <gitster@pobox.com>
>
> ---
> v2:
> - mention the word puzzle in the commit message
> ---
> sequencer.c | 2 +-
> 1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..0677c9ab09 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -3628,7 +3628,7 @@ static int do_exec(struct repository *r, const char *command_line)
> " git rebase --continue\n"
> "\n"),
> command_line,
> - dirty ? N_("and made changes to the index and/or the "
> + dirty ? _("and made changes to the index and/or the "
> "working tree\n") : "");
> if (status == 127)
> /* command not found */
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages
2023-04-28 12:56 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Oswald Buddenhagen
2023-04-28 12:56 ` [PATCH v2 2/3] sequencer: actually translate report in do_exec() Oswald Buddenhagen
@ 2023-04-28 12:56 ` Oswald Buddenhagen
2023-04-28 18:57 ` Junio C Hamano
2023-04-28 19:01 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Junio C Hamano
2023-08-07 17:09 ` [PATCH v3] " Oswald Buddenhagen
3 siblings, 1 reply; 21+ messages in thread
From: Oswald Buddenhagen @ 2023-04-28 12:56 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
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(-)
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"
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages
2023-04-28 12:56 ` [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages Oswald Buddenhagen
@ 2023-04-28 18:57 ` Junio C Hamano
2023-04-28 19:09 ` Junio C Hamano
0 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2023-04-28 18:57 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
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"
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages
2023-04-28 18:57 ` Junio C Hamano
@ 2023-04-28 19:09 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-04-28 19:09 UTC (permalink / raw)
To: git; +Cc: Oswald Buddenhagen
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.
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()
2023-04-28 12:56 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Oswald Buddenhagen
2023-04-28 12:56 ` [PATCH v2 2/3] sequencer: actually translate report in do_exec() Oswald Buddenhagen
2023-04-28 12:56 ` [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages Oswald Buddenhagen
@ 2023-04-28 19:01 ` Junio C Hamano
2023-04-29 7:18 ` Oswald Buddenhagen
2023-08-07 17:09 ` [PATCH v3] " Oswald Buddenhagen
3 siblings, 1 reply; 21+ messages in thread
From: Junio C Hamano @ 2023-04-28 19:01 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> This makes sure that we get a properly translated message rather than
> inserting the command (which we failed to translate) into a generic
> fallback message.
Hmph, can this be accompanied with a change to add a test to an
existing test script to demonstrate that the function can be called
with me set to "rebase" and results in a generic message?
> We now also BUG() out when encountering an unexpected command.
This needs to be reviewed by somebody who is more familiar with the
rebase/chrry-pick/revert/sequencer codepaths so that they can give a
definitive "good--I know that we never call this function with any
other value in 'me'" and that person would not be me.
> Arguably, it would be cleaner to pass the command as an enum in the
> first place ...
True, but that can be left to a different topic, I would think.
Thanks.
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
> advice.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index d6232439c3..c35ae82e7d 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -190,9 +190,10 @@ int error_resolve_conflict(const char *me)
> error(_("Pulling is not possible because you have unmerged files."));
> else if (!strcmp(me, "revert"))
> error(_("Reverting is not possible because you have unmerged files."));
> + else if (!strcmp(me, "rebase"))
> + error(_("Rebasing is not possible because you have unmerged files."));
> else
> - error(_("It is not possible to %s because you have unmerged files."),
> - me);
> + BUG("Unhandled conflict reason '%s'", me);
>
> if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
> /*
^ permalink raw reply [flat|nested] 21+ messages in thread* Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()
2023-04-28 19:01 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Junio C Hamano
@ 2023-04-29 7:18 ` Oswald Buddenhagen
2023-04-30 5:06 ` Junio C Hamano
0 siblings, 1 reply; 21+ messages in thread
From: Oswald Buddenhagen @ 2023-04-29 7:18 UTC (permalink / raw)
To: Junio C Hamano; +Cc: git
On Fri, Apr 28, 2023 at 12:01:02PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> This makes sure that we get a properly translated message rather than
>> inserting the command (which we failed to translate) into a generic
>> fallback message.
>
>Hmph, can this be accompanied with a change to add a test to an
>existing test script to demonstrate that the function can be called
>with me set to "rebase" and results in a generic message?
>
i suppose it could, but see next paragraph.
>> We now also BUG() out when encountering an unexpected command.
>
>This needs to be reviewed by somebody who is more familiar with the
>rebase/chrry-pick/revert/sequencer codepaths so that they can give a
>definitive "good--I know that we never call this function with any
>other value in 'me'" and that person would not be me.
>
assuming we care only about in-tree code, i'm just about as confident
about this as one can reasonably be - because i grepped through the
code, recursively looking for entry points. there are several calls via
die_resolve_conflict() which have hard-coded `me`s (none of which is
rebase), and two from the sequencer, where `me` comes from
action_name(), which in turn returns one of three hard-coded strings
(one of which is rebase). the latter is also kinda the test case,
because it is obvious that this will be actually invoked when the
situation occurs. it's probably also how i actually ran into the problem
in the first place (i surely wasn't *looking* for it ...).
>> Arguably, it would be cleaner to pass the command as an enum in the
>> first place ...
>
>True, but that can be left to a different topic, I would think.
>
yes, otherwise i'd have already done it. ^^
i can make it more explicit if you prefer that.
if you agree with the reasoning, i'll prepare an update to the commit
message and leave the patch as-is.
-- ossi
^ permalink raw reply [flat|nested] 21+ messages in thread
* Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()
2023-04-29 7:18 ` Oswald Buddenhagen
@ 2023-04-30 5:06 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-04-30 5:06 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> On Fri, Apr 28, 2023 at 12:01:02PM -0700, Junio C Hamano wrote:
>>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>>
>>> This makes sure that we get a properly translated message rather than
>>> inserting the command (which we failed to translate) into a generic
>>> fallback message.
>>
>>Hmph, can this be accompanied with a change to add a test to an
>>existing test script to demonstrate that the function can be called
>>with me set to "rebase" and results in a generic message?
>>
> i suppose it could, but see next paragraph.
>
>>> We now also BUG() out when encountering an unexpected command.
>>
>>This needs to be reviewed by somebody who is more familiar with the
>>rebase/chrry-pick/revert/sequencer codepaths so that they can give a
>>definitive "good--I know that we never call this function with any
>>other value in 'me'" and that person would not be me.
>>
> assuming we care only about in-tree code, i'm just about as confident
> about this as one can reasonably be - because i grepped through the
> code, recursively looking for entry points. there are several calls
> via die_resolve_conflict() which have hard-coded `me`s (none of which
> is rebase), and two from the sequencer, where `me` comes from
> action_name(), which in turn returns one of three hard-coded strings
> (one of which is rebase). the latter is also kinda the test case,
> because it is obvious that this will be actually invoked when the
> situation occurs. it's probably also how i actually ran into the
> problem in the first place (i surely wasn't *looking* for it ...).
>
>>> Arguably, it would be cleaner to pass the command as an enum in the
>>> first place ...
>>
>>True, but that can be left to a different topic, I would think.
>>
> yes, otherwise i'd have already done it. ^^
> i can make it more explicit if you prefer that.
>
> if you agree with the reasoning, i'll prepare an update to the commit
> message and leave the patch as-is.
Yeah, how you arrived the conclusion that just covering "rebase" in
addition to what the code already covers would make the if/elseif
cascade complete is a very helpful addition to the log message.
^ permalink raw reply [flat|nested] 21+ messages in thread
* [PATCH v3] advice: handle "rebase" in error_resolve_conflict()
2023-04-28 12:56 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Oswald Buddenhagen
` (2 preceding siblings ...)
2023-04-28 19:01 ` [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict() Junio C Hamano
@ 2023-08-07 17:09 ` Oswald Buddenhagen
2023-08-07 20:20 ` Junio C Hamano
3 siblings, 1 reply; 21+ messages in thread
From: Oswald Buddenhagen @ 2023-08-07 17:09 UTC (permalink / raw)
To: git; +Cc: Junio C Hamano
This makes sure that we get a properly translated message rather than
inserting the command (which we failed to translate) into a generic
fallback message.
The function is called indirectly via die_resolve_conflict() with fixed
strings, and directly with the string obtained via action_name(), which
in turn returns a string from a fixed set. Hence we know that the now
covered set of strings is exhausitive, and will therefore BUG() out when
encountering an unexpected string. We also know that all covered strings
are actually used.
Arguably, the above suggests that it would be cleaner to pass the
command as an enum in the first place, but that's left for another time.
Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
v2:
- more verbose description
Cc: Junio C Hamano <gitster@pobox.com>
---
advice.c | 5 +++--
1 file changed, 3 insertions(+), 2 deletions(-)
diff --git a/advice.c b/advice.c
index e5a9bb9b44..50c79443ba 100644
--- a/advice.c
+++ b/advice.c
@@ -191,9 +191,10 @@ int error_resolve_conflict(const char *me)
error(_("Pulling is not possible because you have unmerged files."));
else if (!strcmp(me, "revert"))
error(_("Reverting is not possible because you have unmerged files."));
+ else if (!strcmp(me, "rebase"))
+ error(_("Rebasing is not possible because you have unmerged files."));
else
- error(_("It is not possible to %s because you have unmerged files."),
- me);
+ BUG("Unhandled conflict reason '%s'", me);
if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
/*
--
2.40.0.152.g15d061e6df
^ permalink raw reply related [flat|nested] 21+ messages in thread* Re: [PATCH v3] advice: handle "rebase" in error_resolve_conflict()
2023-08-07 17:09 ` [PATCH v3] " Oswald Buddenhagen
@ 2023-08-07 20:20 ` Junio C Hamano
0 siblings, 0 replies; 21+ messages in thread
From: Junio C Hamano @ 2023-08-07 20:20 UTC (permalink / raw)
To: Oswald Buddenhagen; +Cc: git
Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
> This makes sure that we get a properly translated message rather than
> inserting the command (which we failed to translate) into a generic
> fallback message.
Makes sense.
>
> The function is called indirectly via die_resolve_conflict() with fixed
> strings, and directly with the string obtained via action_name(), which
> in turn returns a string from a fixed set. Hence we know that the now
> covered set of strings is exhausitive, and will therefore BUG() out when
> encountering an unexpected string. We also know that all covered strings
> are actually used.
>
> Arguably, the above suggests that it would be cleaner to pass the
> command as an enum in the first place, but that's left for another time.
>
> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
> v2:
> - more verbose description
>
> Cc: Junio C Hamano <gitster@pobox.com>
> ---
> advice.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
>
> diff --git a/advice.c b/advice.c
> index e5a9bb9b44..50c79443ba 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -191,9 +191,10 @@ int error_resolve_conflict(const char *me)
> error(_("Pulling is not possible because you have unmerged files."));
> else if (!strcmp(me, "revert"))
> error(_("Reverting is not possible because you have unmerged files."));
> + else if (!strcmp(me, "rebase"))
> + error(_("Rebasing is not possible because you have unmerged files."));
> else
> - error(_("It is not possible to %s because you have unmerged files."),
> - me);
> + BUG("Unhandled conflict reason '%s'", me);
>
> if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
> /*
^ permalink raw reply [flat|nested] 21+ messages in thread