Re: Change in output as a result of patch

[Junio C Hamano <gitster@pobox.com>]

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

> The patch in the previous mail results in a change in output as
> specified below.
>
>     $ git branch
>     * master
>       foo
>       bar
>
> Before patch,
>
>     $ git branch -m hypothet master
>     fatal: A branch named 'master' already exists.
>
>     $ git branch -m hypothet real
>     error: refname refs/heads/hypothet not found
>     fatal: Branch rename failed
>
> After patch,
>
>     $ git branch -m hypothet master
>     fatal: Branch 'hypothet' does not exist.
>
>     $ git branch -m hypothet real
>     fatal: Branch 'hypothet' does not exist.

Imagine this scenario instead, which I think is more realistic
example of making a typo.  The set of existing branches are like
this:

     $ git branch
       devel
     * test

And then you get these with your patch:

     $ git branch -m tets devel
     fatal: Branch 'tets' does not exist.

     $ git branch -m test devel
     fatal: A branch named 'devel' already exists.

My reaction to the above exchange would be a moderately strong
annoyance.  If I were told that I am using 'devel' for something
else already, my "corrected" attempt to turn the 'test' branch to a
real development branch after getting the first error would have
been:

     $ git branch -m test devel2

and I didn't have to get the second error.

I think your patch points in the right direction---if an operation
can fail due to two reasons, reordering the two checks and still
fail with a report for just the first one you happened to check
first does not give us a real improvement.  If it is easy to check
the other one after you noticed one of the condition is violated,
then you could give a more useful diagnosis, namely, "There is
branch 'tets' to rename, and there already is 'devel' branch".

I suspect that with a moderately-sized refactoring around
validate_new_branchname() function, this should be doable.  Instead
of passing two "int" parameters force and attr_only, make them into
a single "unsigned flag" and allow the caller to pass another option
to tell the function "do not die, do not issue a message, but tell
the caller what is wrong with a return value".  And by using that
updated API, rename_branch() could tell four cases apart and fail
the three bad cases in a more sensible way.

In any case, the illustrations of interaction before and after the
change is a very good thing to have when discussing a patch, and you
are encouraged to put them in your proposed log message.