Re: [PATCH] Fixing unclear messages
From: Junio C Hamano <hidden>
Date: 2016-06-15 23:02:14
Alexander Shopov [off-list ref] writes:
quoted hunk
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?
quoted hunk
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.
quoted hunk
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: ...").
quoted hunk
@@ -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.
quoted hunk
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?
quoted hunk
@@ -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?
quoted hunk
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.