[PATCH 1/3] Capitalization and punctuation fixes to some user visible messages

[PATCH 1/3] Capitalization and punctuation fixes to some user visible messages

[Oswald Buddenhagen]

These messages are used in multi-line context, where not sticking to
proper grammar makes things hard to read and has questionable looks.
Therefore, these changes go against the usual rules for error messages.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 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 3be23d7ca2..fda68cd33d 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 ? N_("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

[PATCH 2/3] sequencer: actually translate report in do_exec()

[Oswald Buddenhagen]

N_() is meant to be used on strings that are subsequently _()'d, which
isn't the case here.

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 sequencer.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/sequencer.c b/sequencer.c
index fda68cd33d..21748bbfb0 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

[PATCH 3/3] advice: translate all actions in error_resolve_conflict()

[Oswald Buddenhagen]

action_name() returns a N_()'d string (for good reasons), so we still
need to _() it.

In practice, this affects 'rebase'.

Whether this is actually useful is debatable ...

Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
---
 advice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index d6232439c3..f75f3df582 100644
--- a/advice.c
+++ b/advice.c
@@ -192,7 +192,7 @@ int error_resolve_conflict(const char *me)
 		error(_("Reverting is not possible because you have unmerged files."));
 	else
 		error(_("It is not possible to %s because you have unmerged files."),
-			me);
+			_(me));
 
 	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
 		/*
-- 
2.40.0.152.g15d061e6df

Re: [PATCH 1/3] Capitalization and punctuation fixes to some user visible messages

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

>  			require_clean_work_tree(the_repository,
>  				N_("pull with rebase"),
> -				_("please commit or stash them."), 1, 0);
> +				_("Please commit or stash them."), 1, 0);

The require_clean_work_tree() function uses the message this patch
touches here like so:

	...
	if (err) {
		if (hint)
			error("%s", hint);
		if (!gently)
			exit(128);
	}

and the message given to error() is shown with "error: " prefix.
Our friendly CodingGuidelines is fairly clear on what to do to a
single sentence error message like this one:

| 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.

So, if you want to touch this message, what you want to do is to
leave the capitalization alone, but to drop the full stop at the
end.

> diff --git a/sequencer.c b/sequencer.c
> index 3be23d7ca2..fda68cd33d 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 ? N_("and made changes to the index and/or the "
> -				"working tree\n") : "");
> +				"working tree.\n") : "");

I think this long and multi-sentence warning message should be split
into two and the latter half should become an advice message that
the user can squelch.  Until it happens, which would require us to
rewrite these messages, I wouldn't bother touching this message if I
were you.

>  	} 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"

Likewise.

Re: [PATCH 2/3] sequencer: actually translate report in do_exec()

[Junio C Hamano]

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.

Correct.  But the original does not look particularly a good way to
allow people to translate the messages (namely, it splits a sentence
in the middle and makes a sentence lego).

I wonder if it should prepare two full sentence messages, one for
dirty case and the other for non-dirty case?  In any case, I think
the latter half of this big warning() should be done as an advice
message that the user can squelch, so I wouldn't worry too much
about it before that happens.

Thanks.

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  sequencer.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/sequencer.c b/sequencer.c
> index fda68cd33d..21748bbfb0 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 */

Re: [PATCH 1/3] Capitalization and punctuation fixes to some user visible messages

[Oswald Buddenhagen]

On Thu, Mar 23, 2023 at 01:39:28PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>>  			require_clean_work_tree(the_repository,
>>  				N_("pull with rebase"),
>> -				_("please commit or stash them."), 1, 0);
>> +				_("Please commit or stash them."), 1, 0);
>
>Our friendly CodingGuidelines is fairly clear on what to do to a
>single sentence error message like this one: [...]
>
i know. that's why the commit message makes a point of stating that it's 
making an exception.

the reasoning is basically that it's not *really* an error message, it's 
just abusing the machinery. you obviously noticed that yourself, 
suggesting to turn these strings into advice messages. but that kinda 
feels like overkill to me.

Re: [PATCH 2/3] sequencer: actually translate report in do_exec()

[Oswald Buddenhagen]

On Thu, Mar 23, 2023 at 01:43:03PM -0700, Junio C Hamano wrote:
>But the original does not look particularly a good way to
>allow people to translate the messages (namely, it splits a sentence
>in the middle and makes a sentence lego).
>
it's not that bad, because the "brick" is inserted at a location that is 
specified by another translatable string.

>I wonder if it should prepare two full sentence messages, one for
>dirty case and the other for non-dirty case?
>
... so this seems unnecessary if no translator complained yet.

>In any case, I think
>the latter half of this big warning() should be done as an advice
>message that the user can squelch, so I wouldn't worry too much
>about it before that happens.
>
yeah, but unless this is on someone's shortlist, i don't think it makes 
sense to hold up my fix.

Re: [PATCH 2/3] sequencer: actually translate report in do_exec()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> yeah, but unless this is on someone's shortlist, i don't think it
> makes sense to hold up my fix.

My point was that your "fix" is not really a "fix" that is worth the
code churn.  Or did I mis-read your patches?

Thanks.

Re: [PATCH 2/3] sequencer: actually translate report in do_exec()

[Oswald Buddenhagen]

On Thu, Mar 23, 2023 at 02:25:04PM -0700, Junio C Hamano wrote:
>Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:
>
>> yeah, but unless this is on someone's shortlist, i don't think it
>> makes sense to hold up my fix.
>
>My point was that your "fix" is not really a "fix" that is worth the
>code churn.  Or did I mis-read your patches?
>
they all fix actual (minor) bugs: missing i18n, bad grammar (the missing 
periods between sentences actaully threw me off, which is how i got 
started on the series at all), and arguably bad capitalization (though 
obviously, for my own sake i couldn't care less about *that*).

Re: [PATCH 3/3] advice: translate all actions in error_resolve_conflict()

[Junio C Hamano]

Oswald Buddenhagen <oswald.buddenhagen@gmx.de> writes:

> action_name() returns a N_()'d string (for good reasons), so we still
> need to _() it.
>
> In practice, this affects 'rebase'.
>
> Whether this is actually useful is debatable ...

Yes, it is debatable.  It may be much better to add a new "else if"
that covers the case we _know_ is not covered with the current code.

Having 'me' in _() would be consistent with its source marked with
N_() as you found out, but I agree with you that it is debatable if
this patch is moving things in the right direction.  It would belong
to the part that should never be exercised once we give action names
proper covering by adding missing "else if".

We could even replace the fallback else with a "else BUG()" to ensure
that the action names all callers pass have corresponding message
that can be translated without sentence lego.

Thanks.

> Signed-off-by: Oswald Buddenhagen <oswald.buddenhagen@gmx.de>
> ---
>  advice.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/advice.c b/advice.c
> index d6232439c3..f75f3df582 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -192,7 +192,7 @@ int error_resolve_conflict(const char *me)
>  		error(_("Reverting is not possible because you have unmerged files."));
>  	else
>  		error(_("It is not possible to %s because you have unmerged files."),
> -			me);
> +			_(me));
>  
>  	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
>  		/*

[PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()

[Oswald Buddenhagen]

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.

We now also BUG() out when encountering an unexpected command.

Arguably, it would be cleaner to pass the command as an enum in the
first place ...

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))
 		/*
-- 
2.40.0.152.g15d061e6df

[PATCH v2 2/3] sequencer: actually translate report in do_exec()

[Oswald Buddenhagen]

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

[PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages

[Oswald Buddenhagen]

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

Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages

[Junio C Hamano]

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"

Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()

[Junio C Hamano]

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))
>  		/*

Re: [PATCH v2 2/3] sequencer: actually translate report in do_exec()

[Junio C Hamano]

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 */

Re: [PATCH v2 3/3] Capitalization and punctuation fixes to some user visible messages

[Junio C Hamano]

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.

Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()

[Oswald Buddenhagen]

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

Re: [PATCH v2 1/3] advice: handle "rebase" in error_resolve_conflict()

[Junio C Hamano]

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.

[PATCH v3] advice: handle "rebase" in error_resolve_conflict()

[Oswald Buddenhagen]

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

Re: [PATCH v3] advice: handle "rebase" in error_resolve_conflict()

[Junio C Hamano]

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))
>  		/*