Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Monday 13 November 2017 05:00 PM, Kevin Daudt wrote:

On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote:

That was a little attribution I wanted make to the strbuf API as this was
the first time I leveraged it to this extent and I was surprised by the way
it made string manipulation easier in C. Just documented my excitation. In
case it seems to be noise (?) which should removed, let me know.


I guess that would fit better below the the ---



That's a nice point. Thanks. (Why didn't I think of it before)


---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Mon, Nov 13, 2017 at 08:01:12AM +0530, Kaartic Sivaraam wrote:
> On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:
> > On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> > > From: Kaartic Sivaraam 
> > > 
> > > When trying to rename an inexistent branch to with a name of a branch
> > 
> > This sentence does not read well. Probably s/with a/the/ helps.
> > 
> 
> Thanks. Seems I missed it somehow. Will fix it.
> 
> > > 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.
> > > 
> > > [..]
> > > 
> > > Note: Thanks to the strbuf API that made it possible to easily construct
> > > the composite error message strings!
> > 
> > I'm not sure this note adds a lot, since the strbuf API is not that new.
> > 
> 
> That was a little attribution I wanted make to the strbuf API as this was
> the first time I leveraged it to this extent and I was surprised by the way
> it made string manipulation easier in C. Just documented my excitation. In
> case it seems to be noise (?) which should removed, let me know.

I guess that would fit better below the the ---

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Sunday 12 November 2017 11:53 PM, Kevin Daudt wrote:

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:

From: Kaartic Sivaraam 

When trying to rename an inexistent branch to with a name of a branch


This sentence does not read well. Probably s/with a/the/ helps.



Thanks. Seems I missed it somehow. Will fix it.


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.

[..]

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!


I'm not sure this note adds a lot, since the strbuf API is not that new.



That was a little attribution I wanted make to the strbuf API as this 
was the first time I leveraged it to this extent and I was surprised by 
the way it made string manipulation easier in C. Just documented my 
excitation. In case it seems to be noise (?) which should removed, let 
me know.


---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kevin Daudt]

On Thu, Nov 02, 2017 at 12:24:07PM +0530, Kaartic Sivaraam wrote:
> From: Kaartic Sivaraam 
> 
> When trying to rename an inexistent branch to with a name of a branch

This sentence does not read well. Probably s/with a/the/ helps.

> 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.
> 
> [..]
> 
> Note: Thanks to the strbuf API that made it possible to easily construct
> the composite error message strings!

I'm not sure this note adds a lot, since the strbuf API is not that new.

Kevin

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Mon, 2017-11-06 at 11:30 +0900, Junio C Hamano wrote:
> Kaartic Sivaraam  writes:
> 
> No {} around a single statement block of "if", especially when there
> is no "else" that has multi-statement block that needs {}.
> 

The code has changed a little since v3 so this if has been replaced
with a switch case.


> > +   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;
> 
> The case arms and their statements are indented by one level too much.
> The lines are getting overlong.  Find a good place to split, e.g.
> 
>   strbuf_addf(error_msg, "%s",
>   !old_branch_exists ? connector_string : "");
> 
> Leave a single SP after each "," in an arguments list.
> 

Fixed these.


> As Eric pointed out, this certainly smells like a sentence lego that
> we would be better off without.
> 

As stated in that mail,  I've replaced the connector " and " with "; "
as it seemed to be the most simple way to overcome the issue, at least
in my opinion. In case there's any better way to fix this let me know.


> >  static void copy_or_rename_branch(const char *oldname, const char 
> > *newname, int copy, int force)
> >  {
> > struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
> > STRBUF_INIT;
> > struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
> > int recovery = 0;
> > +   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> > +   enum branch_validation_result res;
> >  
> > if (!oldname) {
> > if (copy)
> > @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char 
> > *oldname, const char *newname, int
> > die(_("cannot rename the current branch while not on 
> > any."));
> > }
> >  
> > -   if (strbuf_check_branch_ref(, oldname)) {
> > +   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
> > +   {
> 
> Opening brace { that begins a block comes at the end of the line
> that closes the condition of "if"; if you found that your line is
> overlong, perhaps do it like so instead:
> 
>   if (strbuf_check_branch_ref(, oldname) &&
>   ref_exists(oldref.buf)) {

This part changed too. Anyways thanks for the detailed review :-)

-- 
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Junio C Hamano]

Kaartic Sivaraam  writes:

> diff --git a/builtin/branch.c b/builtin/branch.c
> index 7018e5d75..c2bbf8c3d 100644
> --- a/builtin/branch.c
> +++ b/builtin/branch.c
> @@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
> *target)
>   free_worktrees(worktrees);
>  }
>  
> +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);
> + }

No {} around a single statement block of "if", especially when there
is no "else" that has multi-statement block that needs {}.

> + 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;

The case arms and their statements are indented by one level too much.
The lines are getting overlong.  Find a good place to split, e.g.

strbuf_addf(error_msg, "%s",
!old_branch_exists ? connector_string : "");

Leave a single SP after each "," in an arguments list.

As Eric pointed out, this certainly smells like a sentence lego that
we would be better off without.

>  static void copy_or_rename_branch(const char *oldname, const char *newname, 
> int copy, int force)
>  {
>   struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
> STRBUF_INIT;
>   struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
>   int recovery = 0;
> + struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
> + enum branch_validation_result res;
>  
>   if (!oldname) {
>   if (copy)
> @@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
> const char *newname, int
>   die(_("cannot rename the current branch while not on 
> any."));
>   }
>  
> - if (strbuf_check_branch_ref(, oldname)) {
> + if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
> + {

Opening brace { that begins a block comes at the end of the line
that closes the condition of "if"; if you found that your line is
overlong, perhaps do it like so instead:

if (strbuf_check_branch_ref(, oldname) &&
ref_exists(oldref.buf)) {

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

On Thursday 02 November 2017 07:51 PM, Eric Sunshine wrote:


Nicely explained; easily understood.



Good to hear that.




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.

Though I can't be very sure that the one you suggested will pass the 
"lego sniff test", its better than the "and" I used. Further, my 
instinct says it wouldn't qualify as sentence lego (it's just a ";").




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



Though it might not look that ugly, I don't know how you could make 
'git' die() twice (or am I missing something)! Of course we could use 
'error()' to report the errors and then 'die()' with a message like 
"Branch rename failed" but I find that to be a little too verbose and 
further using the connector ";" instead of "and" does seem to reduce the 
possibilities for the above program fragment to pass the "lego sniff test".


---
Kaartic

Re: [RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Eric Sunshine]

On Thu, Nov 2, 2017 at 2:54 AM, Kaartic Sivaraam
 wrote:
> From: Kaartic Sivaraam 
>
> 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 exists

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

> Signed-off-by: Kaartic Sivaraam 
> ---
> 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

[RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

From: Kaartic Sivaraam 

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 exists

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!

Signed-off-by: Kaartic Sivaraam 
---
 builtin/branch.c | 49 ++---
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7018e5d75..c2bbf8c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
+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;
+   }
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
+   enum branch_validation_result res;
 
if (!oldname) {
if (copy)
@@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
die(_("cannot rename the current branch while not on 
any."));
}
 
-   if (strbuf_check_branch_ref(, oldname)) {
+   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
+   {
/*
 * Bad name --- this could be an attempt to rename a
 * ref that we used to allow to be created by accident.
 */
-   if (ref_exists(oldref.buf))
-   recovery = 1;
-   else
-   die(_("Invalid branch name: '%s'"), oldname);
+   recovery = 1;
}
 
/*
@@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 * cause the worktree to become inconsistent with HEAD, so allow it.
 */
if (!strcmp(oldname, newname))
-   validate_branchname(newname, , 0);
+   res = validate_branchname(newname, , 1);
else
-   validate_new_branchname(newname, , force, 0);
+   res = validate_new_branchname(newname, , force, 1);
+
+   get_error_msg(_msg, oldname, ref_exists(oldref.buf), newname, 

[RFC PATCH v3 4/4] builtin/branch: give more useful error messages when renaming

[Kaartic Sivaraam]

From: Kaartic Sivaraam 

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 exists

Note: Thanks to the strbuf API that made it possible to easily construct
the composite error message strings!

Signed-off-by: Kaartic Sivaraam 
---
 builtin/branch.c | 49 ++---
 1 file changed, 42 insertions(+), 7 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 7018e5d75..c2bbf8c3d 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -458,11 +458,42 @@ static void reject_rebase_or_bisect_branch(const char 
*target)
free_worktrees(worktrees);
 }
 
+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;
+   }
+}
+
 static void copy_or_rename_branch(const char *oldname, const char *newname, 
int copy, int force)
 {
struct strbuf oldref = STRBUF_INIT, newref = STRBUF_INIT, logmsg = 
STRBUF_INIT;
struct strbuf oldsection = STRBUF_INIT, newsection = STRBUF_INIT;
int recovery = 0;
+   struct strbuf error_msg = STRBUF_INIT, empty = STRBUF_INIT;
+   enum branch_validation_result res;
 
if (!oldname) {
if (copy)
@@ -471,15 +502,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
die(_("cannot rename the current branch while not on 
any."));
}
 
-   if (strbuf_check_branch_ref(, oldname)) {
+   if (strbuf_check_branch_ref(, oldname) && ref_exists(oldref.buf))
+   {
/*
 * Bad name --- this could be an attempt to rename a
 * ref that we used to allow to be created by accident.
 */
-   if (ref_exists(oldref.buf))
-   recovery = 1;
-   else
-   die(_("Invalid branch name: '%s'"), oldname);
+   recovery = 1;
}
 
/*
@@ -487,9 +516,13 @@ static void copy_or_rename_branch(const char *oldname, 
const char *newname, int
 * cause the worktree to become inconsistent with HEAD, so allow it.
 */
if (!strcmp(oldname, newname))
-   validate_branchname(newname, , 0);
+   res = validate_branchname(newname, , 1);
else
-   validate_new_branchname(newname, , force, 0);
+   res = validate_new_branchname(newname, , force, 1);
+
+   get_error_msg(_msg, oldname, ref_exists(oldref.buf), newname,