[PATCH] Fixning WTF porcelain messages

[PATCH] Fixning WTF porcelain messages

[Alexander Shopov]

Some user-facing porcelain messages in Git are hard to understand and
hard to translate. They confuse users winthout providing proper
information and course of action. Here is the list of these messages
(starting with "-") and my suggestions (starting with "+"). The patch
follows in the next letter.

-Huh (%s)?
+Wrong choice (%s). Choose again.

-Clever... amending the last one with dirty index.
+You are amending the last commit only. There are additional changes in the index.

-confusion beyond insanity in parse_pack_objects()
+fatal error in function "parse_pack_objects". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org.

-confusion beyond insanity
+fatal error in function "conclude_pack". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org.

-insane in-reply-to: %s
+wrong format for the "in-reply-to" header: %s

-Two output directories?
+Maximum one output directory is allowed.

-Wonderful.\n
+The first part of the trivial merge finished successfully.\n

-Nope.\n
+Merge was not successful.\n

- ???
+ unknown state

-no tag message?
+missing tag message

-?? what are you talking about?
+unknown command. Only "start", "good", "bad" and "skip" are possible.

-Unprocessed path??? %s
+The path "%s" was not processed but it had to be. This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org.



Alexander Shopov (1):
  Fixing unclear messages

 builtin/clean.c      | 2 +-
 builtin/commit.c     | 2 +-
 builtin/index-pack.c | 4 ++--
 builtin/log.c        | 4 ++--
 builtin/merge.c      | 5 +++--
 builtin/remote.c     | 2 +-
 builtin/tag.c        | 2 +-
 git-bisect.sh        | 2 +-
 merge-recursive.c    | 2 +-
 9 files changed, 13 insertions(+), 12 deletions(-)

-- 
1.9.3

[PATCH] Fixing unclear messages

[Alexander Shopov]

Signed-off-by: Alexander Shopov <ash@kambanaria.org>
---
 builtin/clean.c      | 2 +-
 builtin/commit.c     | 2 +-
 builtin/index-pack.c | 4 ++--
 builtin/log.c        | 4 ++--
 builtin/merge.c      | 5 +++--
 builtin/remote.c     | 2 +-
 builtin/tag.c        | 2 +-
 git-bisect.sh        | 2 +-
 merge-recursive.c    | 2 +-
 9 files changed, 13 insertions(+), 12 deletions(-)

diff --git a/builtin/clean.c b/builtin/clean.c
index 1032563..9f38068 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -514,7 +514,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
 		if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||
 		    (is_single && bottom != top)) {
 			clean_print_color(CLEAN_COLOR_ERROR);
-			printf_ln(_("Huh (%s)?"), (*ptr)->buf);
+			printf_ln(_("Wrong choice (%s). Choose again."), (*ptr)->buf);
 			clean_print_color(CLEAN_COLOR_RESET);
 			continue;
 		}
diff --git a/builtin/commit.c b/builtin/commit.c
index 5ed6036..cdc8a4e 100644
--- a/builtin/commit.c
+++ b/builtin/commit.c
@@ -1198,7 +1198,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
 	if (argc == 0 && (also || (only && !amend)))
 		die(_("No paths with --include/--only does not make sense."));
 	if (argc == 0 && only && amend)
-		only_include_assumed = _("Clever... amending the last one with dirty index.");
+		only_include_assumed = _("You are amending the last commit only. There are additional changes in the index.");
 	if (argc > 0 && !also && !only)
 		only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");
 	if (!cleanup_arg || !strcmp(cleanup_arg, "default"))
diff --git a/builtin/index-pack.c b/builtin/index-pack.c
index 5568a5b..d9c5911 100644
--- a/builtin/index-pack.c
+++ b/builtin/index-pack.c
@@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
 		nr_delays--;
 	}
 	if (nr_delays)
-		die(_("confusion beyond insanity in parse_pack_objects()"));
+		die(_("fatal error in function \"parse_pack_objects\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));
 }
 
 /*
@@ -1139,7 +1139,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
 		int nr_unresolved = nr_deltas - nr_resolved_deltas;
 		int nr_objects_initial = nr_objects;
 		if (nr_unresolved <= 0)
-			die(_("confusion beyond insanity"));
+			die(_("fatal error in function \"conclude_pack\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));
 		objects = xrealloc(objects,
 				   (nr_objects + nr_unresolved + 1)
 				   * sizeof(*objects));
diff --git a/builtin/log.c b/builtin/log.c
index 4389722..d614a20 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -992,7 +992,7 @@ static const char *clean_message_id(const char *msg_id)
 		m++;
 	}
 	if (!z)
-		die(_("insane in-reply-to: %s"), msg_id);
+		die(_("wrong format for the \"in-reply-to\" header: %s"), msg_id);
 	if (++z == m)
 		return a;
 	return xmemdupz(a, z - a);
@@ -1065,7 +1065,7 @@ static int output_directory_callback(const struct option *opt, const char *arg,
 {
 	const char **dir = (const char **)opt->value;
 	if (*dir)
-		die(_("Two output directories?"));
+		die(_("Maximum one output directory is allowed."));
 	*dir = arg;
 	return 0;
 }
diff --git a/builtin/merge.c b/builtin/merge.c
index ce82eb2..e92a74a 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -842,7 +842,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
 	struct commit_list *parents, **pptr = &parents;
 
 	write_tree_trivial(result_tree);
-	printf(_("Wonderful.\n"));
+	printf(_("The first part of the trivial merge finished successfully
+.\n"));
 	pptr = commit_list_append(head, pptr);
 	pptr = commit_list_append(remoteheads->item, pptr);
 	prepare_to_commit(remoteheads);
@@ -1400,7 +1401,7 @@ int cmd_merge(int argc, const char **argv, const char *prefix)
 				ret = merge_trivial(head_commit, remoteheads);
 				goto done;
 			}
-			printf(_("Nope.\n"));
+			printf(_("Merge was not successful.\n"));
 		}
 	} else {
 		/*
diff --git a/builtin/remote.c b/builtin/remote.c
index 9a4640d..3f480b1 100644
--- a/builtin/remote.c
+++ b/builtin/remote.c
@@ -951,7 +951,7 @@ static int show_remote_info_item(struct string_list_item *item, void *cb_data)
 		else if (string_list_has_string(&states->stale, name))
 			arg = _(" stale (use 'git remote prune' to remove)");
 		else
-			arg = _(" ???");
+			arg = _(" unknown state");
 		printf("    %-*s", info->width, name);
 		printf(fmt, arg);
 		printf("\n");
diff --git a/builtin/tag.c b/builtin/tag.c
index 19eb747..8a565fc 100644
--- a/builtin/tag.c
+++ b/builtin/tag.c
@@ -501,7 +501,7 @@ static void create_tag(const unsigned char *object, const char *tag,
 		stripspace(buf, opt->cleanup_mode == CLEANUP_ALL);
 
 	if (!opt->message_given && !buf->len)
-		die(_("no tag message?"));
+		die(_("missing tag message"));
 
 	strbuf_insert(buf, 0, header_buf, header_len);
 
diff --git a/git-bisect.sh b/git-bisect.sh
index 1e0d602..c7fb095 100755
--- a/git-bisect.sh
+++ b/git-bisect.sh
@@ -421,7 +421,7 @@ bisect_replay () {
 		good|bad|skip)
 			bisect_write "$command" "$rev" ;;
 		*)
-			die "$(gettext "?? what are you talking about?")" ;;
+			die "$(gettext "unknown command. Only \"start\", \"good\", \"bad\" and \"skip\" are possible.")" ;;
 		esac
 	done <"$file"
 	bisect_auto_next
diff --git a/merge-recursive.c b/merge-recursive.c
index 1d332b8..bb71cb6 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -1858,7 +1858,7 @@ int merge_trees(struct merge_options *o,
 		for (i = 0; i < entries->nr; i++) {
 			struct stage_data *e = entries->items[i].util;
 			if (!e->processed)
-				die(_("Unprocessed path??? %s"),
+				die(_("The path \"%s\" was not processed but it had to be. This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."),
 				    entries->items[i].string);
 		}
 
-- 
1.9.3

Re: [PATCH] Fixing unclear messages

[Jeff King]

On Sun, Aug 10, 2014 at 06:13:27PM +0300, Alexander Shopov wrote:

> Signed-off-by: Alexander Shopov <ash@kambanaria.org>

It would probably make sense to put the discussion from your cover
letter into the commit message.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 5568a5b..d9c5911 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
>  		nr_delays--;
>  	}
>  	if (nr_delays)
> -		die(_("confusion beyond insanity in parse_pack_objects()"));
> +		die(_("fatal error in function \"parse_pack_objects\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));
>  }

We usually just say:

  die("BUG: ...");

here (and hopefully the "..." actually describes the situation a bit
better). I have wondered if we should actually introduce a

  BUG("...");

function. Then it would make it simple to be more verbose (e.g.,
pointing the user to the mailing list as you do here) without having to
repeat the text in each place.

-Peff

Re: [PATCH] Fixing unclear messages

[Junio C Hamano]

Alexander Shopov <ash@kambanaria.org> writes:

> diff --git a/builtin/clean.c b/builtin/clean.c
> index 1032563..9f38068 100644
> --- a/builtin/clean.c
> +++ b/builtin/clean.c
> @@ -514,7 +514,7 @@ static int parse_choice(struct menu_stuff *menu_stuff,
>  		if (top <= 0 || bottom <= 0 || top > menu_stuff->nr || bottom > top ||
>  		    (is_single && bottom != top)) {
>  			clean_print_color(CLEAN_COLOR_ERROR);
> -			printf_ln(_("Huh (%s)?"), (*ptr)->buf);
> +			printf_ln(_("Wrong choice (%s). Choose again."), (*ptr)->buf);

Why is this an improvement?

> diff --git a/builtin/commit.c b/builtin/commit.c
> index 5ed6036..cdc8a4e 100644
> --- a/builtin/commit.c
> +++ b/builtin/commit.c
> @@ -1198,7 +1198,7 @@ static int parse_and_validate_options(int argc, const char *argv[],
>  	if (argc == 0 && (also || (only && !amend)))
>  		die(_("No paths with --include/--only does not make sense."));
>  	if (argc == 0 && only && amend)
> -		only_include_assumed = _("Clever... amending the last one with dirty index.");
> +		only_include_assumed = _("You are amending the last commit only. There are additional changes in the index.");

Why is this an improvement?

It would be very good to give a way for people to discover that
"commit --amend -o" (no other arguments) is a good way to amend only
the commit log message without changing the tree even when the user
has already added changes to the index.  But this message only
rewards those who already knew that trick and exercised it---when
they see it, they already know what they are doing.  In other words,
this is really a rare "expert-only" message.  I am not sure if
rewording would add much value to it, especially because it is very
unlikely for anybody to run "commit --amend -o" (no other arguments)
by mistake, expecting something else to happen, which warrant a
warning.

Besides, "amending the last commit only."  implies as if there is a
way to amend more than that (there isn't), and "additional changes
in the index" does not convey any extra information as to what would
happen to them---would they be recorded in the resulting tree, or
would they be left out?

>  	if (argc > 0 && !also && !only)
>  		only_include_assumed = _("Explicit paths specified without -i or -o; assuming --only paths...");

It may be time to remove these messages, by the way.  This one, and
the previous one, were remnants from the days we transitioned the
default behaviour of "git commit <path>" from "Record changes that
have already been added to the index plus the changes in the given
path" (aka "include/also") to "Record only changes in the given
path, temporarily disregarding the changes already added to the
index" (aka "only").  Giving this warning had some value to remind
people who were used to the then-old default, as the result would be
different with the then-new world order.  But these days, I do not
think people even remember that "include" used to be the default.

> diff --git a/builtin/index-pack.c b/builtin/index-pack.c
> index 5568a5b..d9c5911 100644
> --- a/builtin/index-pack.c
> +++ b/builtin/index-pack.c
> @@ -1064,7 +1064,7 @@ static void parse_pack_objects(unsigned char *sha1)
>  		nr_delays--;
>  	}
>  	if (nr_delays)
> -		die(_("confusion beyond insanity in parse_pack_objects()"));
> +		die(_("fatal error in function \"parse_pack_objects\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));

It probably should be spelled die("BUG: ...").

> @@ -1139,7 +1139,7 @@ static void conclude_pack(int fix_thin_pack, const char *curr_pack, unsigned cha
>  		int nr_unresolved = nr_deltas - nr_resolved_deltas;
>  		int nr_objects_initial = nr_objects;
>  		if (nr_unresolved <= 0)
> -			die(_("confusion beyond insanity"));
> +			die(_("fatal error in function \"conclude_pack\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));

Likewise.

> diff --git a/builtin/log.c b/builtin/log.c
> index 4389722..d614a20 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -992,7 +992,7 @@ static const char *clean_message_id(const char *msg_id)
>  		m++;
>  	}
>  	if (!z)
> -		die(_("insane in-reply-to: %s"), msg_id);
> +		die(_("wrong format for the \"in-reply-to\" header: %s"), msg_id);

Why is s/insane/wrong format/ an improvement?

> @@ -1065,7 +1065,7 @@ static int output_directory_callback(const struct option *opt, const char *arg,
>  {
>  	const char **dir = (const char **)opt->value;
>  	if (*dir)
> -		die(_("Two output directories?"));
> +		die(_("Maximum one output directory is allowed."));

Why is it an improvement?

> diff --git a/builtin/merge.c b/builtin/merge.c
> index ce82eb2..e92a74a 100644
> --- a/builtin/merge.c
> +++ b/builtin/merge.c
> @@ -842,7 +842,8 @@ static int merge_trivial(struct commit *head, struct commit_list *remoteheads)
>  	struct commit_list *parents, **pptr = &parents;
>  
>  	write_tree_trivial(result_tree);
> -	printf(_("Wonderful.\n"));
> +	printf(_("The first part of the trivial merge finished successfully
> +.\n"));

Huh?

I would buy s/something/BUG: &/;, but I do not think we want to
apply most of the others.

Thanks.

Re: [PATCH] Fixing unclear messages

[Alexander Shopov]

>> -                     printf_ln(_("Huh (%s)?"), (*ptr)->buf);
>> +                     printf_ln(_("Wrong choice (%s). Choose again."), (*ptr)->buf);
> Why is this an improvement?
Because 'Huh?' without intonation, gesture or a frown provided by a
human being is hard to understand. It does not state that it is the
choice the user provided is wrong and does not prompt the user for the
next action.


>> -             only_include_assumed = _("Clever... amending the last one with dirty index.");
>> +             only_include_assumed = _("You are amending the last commit only. There are additional changes in the index.");
> Why is this an improvement?
...
> Besides, "amending the last commit only."  implies ...
> ... does not convey any extra information ...
> ...It may be time to remove these messages, by the way. ...
You say that my change does not tangibly improve on an initially
unclear and already obsolete message, am I right?
I prefer the messages to be removed then.

>> +             die(_("fatal error in function \"parse_pack_objects\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));
> It probably should be spelled die("BUG: ...").
>> +                     die(_("fatal error in function \"conclude_pack\". This is a bug in Git. Please report it to the developers with an e-mail to git@vger.kernel.org."));
> Likewise.
I have no stand on the issue "fatal error" vs "BUG", if you prefer
"BUG" I can reword.
There was a suggestion to have a separate function that is meant to
echo messages when there are genuine bugs in Git (impossible cases
happening, invariants breaking, etc.) This will allow factoring out
the clause "This is a bug in Git. Please report it to the developers
with an e-mail to git@vger.kernel.org." as a single message and I
prefer that for ease of maintenance.

>> +             die(_("wrong format for the \"in-reply-to\" header: %s"), msg_id);
> Why is s/insane/wrong format/ an improvement?
Because it is more factual and narrows down what is wrong. "Insane"
could mean many different things.

>> -             die(_("Two output directories?"));
>> +             die(_("Maximum one output directory is allowed."));
> Why is it an improvement?
Because the question only implies that the problem is the number of
directories but says nothing how many directories there should be (0,
1, 3... 100?)

>> -     printf(_("Wonderful.\n"));
>> +     printf(_("The first part of the trivial merge finished successfully
> Huh?
I am not sure what you mean or your objection would be, perhaps I am
misreading the source of Git.
The message appears as a part of sequence during merge when the first
stage passes successfully but there still could be a case that makes
the whole merge impossible.
What does "Wonderful" mean in a sequence of steps git is doing for you.

You say "I would buy s/something/BUG: &/;, but I do not think we want
to apply most of the others."
Does this mean the following changes are totally unwelcome or you
(plural, as corresponds to "we) want me to resubmit them and
substantiate changes on a message by message base?

-Nope.\n
+Merge was not successful.\n

- ???
+ unknown state

-no tag message?
+missing tag message

-?? what are you talking about?
+unknown command. Only "start", "good", "bad" and "skip" are possible.

-Unprocessed path??? %s
+The path "%s" was not processed but it had to be. This is a bug in
Git. Please report it to the developers with an e-mail to
git@vger.kernel.org.

Kind regards:
al_shopov

Re: [PATCH] Fixing unclear messages

[Junio C Hamano]

Alexander Shopov <ash@kambanaria.org> writes:

>>> -                     printf_ln(_("Huh (%s)?"), (*ptr)->buf);
>>> +                     printf_ln(_("Wrong choice (%s). Choose again."), (*ptr)->buf);
>> Why is this an improvement?
> Because 'Huh?' without intonation, gesture or a frown provided by a
> human being is hard to understand. It does not state that it is the
> choice the user provided is wrong and does not prompt the user for the
> next action.

This is shown in a human-interactive exchange where a menu has
already given by list_and_choose().  If there were something else
"Huh?" could mean after you give a response to that prompt, but I do
not think there is.

> You say that my change does not tangibly improve on an initially
> unclear and already obsolete message, am I right?

Not really.  This is not obsolete (it is a good trick even in
today's world order), but is not teaching anything new to the user
because it has to be issued too late (and there is no way to give it
before the user realizes it is necessary), so it is not "unclear"
per-se, but it does not help much.  If I were asked to say what it
is then, I would say "it reassures".

I do not strongly oppose its removal, but if we were to remove it,
we shold add a comment to the condition of the previous "if"
statement to remind us why no argument with "only" is allowed if
"amend" is set.

> There was a suggestion to have a separate function that is meant to
> echo messages when there are genuine bugs in Git (impossible cases
> happening, invariants breaking, etc.) This will allow factoring out
> the clause "This is a bug in Git. Please report it to the developers
> with an e-mail to git@vger.kernel.org." as a single message and I
> prefer that for ease of maintenance.

Yes, I see the primary value of this thread was to trigger that
suggestion to classify which die()s are BUG()s.

>>> -             die(_("Two output directories?"));
>>> +             die(_("Maximum one output directory is allowed."));
>> Why is it an improvement?
> Because the question only implies that the problem is the number of
> directories but says nothing how many directories there should be (0,
> 1, 3... 100?)

Because I've never imagined anybody would sensibly expect "mv a1
a2... dst1 dst2 dst3..." to work (what does that even mean?  Make N
copies of a$m and drop them into each of dst$n?), I never thought of
such a possiblity.  Trying to help more people is good, unless it
needs to be done by bending backwards, and your rewrite here is
definitely a good one in that sense.

> I am not sure what you mean or your objection would be, perhaps I am
> misreading the source of Git.
> ...
> Does this mean the following changes are totally unwelcome or you

FWIW, I see it as a feature to have small number of messages phrased
in colourful ways, especially the ones that do not require reaction
by the users (e.g. "trivial merge succeeded" does not trigger "oops,
I have to ^C and recover immediately") or the required reaction is
obvious (e.g. "you gave me no X and expect me to work?" can only
mean "ah, I have to give X")

We obviously do not want to overdo it, but the ones we have are all
old ones.

Re: [PATCH] Fixing unclear messages

[Alexander Shopov]

> If there were something else "Huh?" could mean after you
> give a response to that prompt, but I do not think there is.
OK, you love your "Huh"s. Good for you. I cannot find a convincing
argument then.

> If I were asked to say what it is then, I would say "it reassures".
It is like a jewel you find in a quest? On the other hand you say the
message is rare enough and shown too late to be useful so there is
little gain to change it. OK, fair enough.

> Yes, I see the primary value of this thread was to trigger that
> suggestion to classify which die()s are BUG()s.
Wonderful.

> Because I've never imagined anybody would sensibly expect "mv a1...
>... your rewrite here is definitely a good one in that sense.
My experience shows that messages need to be as helpful as possible
even at the cost of some repetition.

> FWIW, I see it as a feature to have small number of messages phrased
> in colourful ways, especially the ones that do not require reaction
I really do not know what to say. People can be color-blind even for
messages plus in-jokes frequently do not travel well across languages.
Sharing my experience: the messages were hard to translate because
they were hard to understand.
I had to follow the code in order to understand their meaning and
usage. Hopefully other users of git will be more clever than me.
I did my best at improving the messages but as you do not perceive it
the same way there would be no sense in continuing the discussion much
longer.

Will you reconsider:
- ???
+ unknown state
Recoding problems with translations, settings of console sometimes
lead to missing or wrongly encoded characters to show as '?'. Three
'?' can be confusing when shown in translation.

> We obviously do not want to overdo it, but the ones we have are all old ones.
You overdid it for me. On the positive side I hope I have listed all
oldies but goldies and next changes will be less touchy.

Do you want me redoing this patch or not at all?

Kind regards:
al_shopov