Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming
From: Eric Sunshine <hidden>
Date: 2017-11-02 14:21:21
Possibly related (same subject, not in this thread)
- 2017-11-12 · Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming · Kaartic Sivaraam <hidden>
- 2017-11-06 · Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming · Junio C Hamano <hidden>
- 2017-11-02 · [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming · Kaartic Sivaraam <hidden>
On Thu, Nov 2, 2017 at 2:54 AM, Kaartic Sivaraam [off-list ref] wrote:
From: Kaartic Sivaraam <redacted>
When trying to rename an inexistent branch to with a name of a branch
that already exists the rename failed specifying the new branch name
exists rather than specifying that the branch trying to be renamed
doesn't exist.
$ git branch -m tset master
fatal: A branch named 'master' already exists.
It's conventional to report that 'tset' doesn't exist rather than
reporting that 'master' exists, the same way the 'mv' command does.
(hypothetical)
$ git branch -m tset master
fatal: branch 'tset' doesn't exist.
That has the problem that the error about an existing branch is shown
only after the user corrects the error about inexistent branch.
$ git branch -m test master
fatal: A branch named 'master' already exists.
This isn't useful either because the user would have corrected this error in
a single go if he had been told this alongside the first error. So, give
more useful error messages by giving errors about old branch name and new
branch name at the same time. This is possible as the branch name validation
functions now return the reason they were about to die, when requested.
$ git branch -m tset master
fatal: branch 'tset' doesn't exist, and branch 'master' already existsNicely explained; easily understood.
Note: Thanks to the strbuf API that made it possible to easily construct the composite error message strings!
This may be a problem. See below...
quoted hunk ↗ jump to hunk
Signed-off-by: Kaartic Sivaraam <redacted> ---diff --git a/builtin/branch.c b/builtin/branch.c +static void get_error_msg(struct strbuf* error_msg, const char* oldname, unsigned old_branch_exists, + const char* newname, enum branch_validation_result res) +{ + const char* connector_string = _(", and "); + + if (!old_branch_exists) { + strbuf_addf(error_msg, _("branch '%s' doesn't exist"), oldname); + } + + switch (res) { + case BRANCH_EXISTS_NO_FORCE: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addf(error_msg,_("branch '%s' already exists"), newname); + break; + case CANNOT_FORCE_UPDATE_CURRENT_BRANCH: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addstr(error_msg, _("cannot force update the current branch")); + break; + case INVALID_BRANCH_NAME: + strbuf_addf(error_msg, "%s", (!old_branch_exists) ? connector_string : ""); + strbuf_addf(error_msg, _("branch name '%s' is invalid"), newname); + break; + /* not necessary to handle success cases */ + case BRANCH_EXISTS: + case BRANCH_DOESNT_EXIST: + break; + } +}
Translators can correct me, but this smells like "sentence lego"[1],
which we'd like to avoid. Translators lack full context when presented
with bits and pieces of a sentence like this, thus the translation may
be of poor quality; it may even be entirely incorrect since they don't
necessarily know how you will be composing the various pieces.
You _might_ be able to able to resolve this by dropping "and" from:
"foo is moo, and bar is boo"
to turn the error messages into independent clauses:
"foo is moo; bar is boo"
but I'm no translator, and even that may fail the lego sniff test.
A sure-fire way to avoid lego construction would be simply to emit
each messages on its own line:
fatal: branch 'tset' doesn't exist
fatal: branch 'master' already exists
(though, you might consider that too ugly).
[1]: http://www.gnu.org/software/gettext/manual/gettext.html#Preparing-Strings