Re: Change in output as a result of patch

Kaartic Sivaraam [off-list ref] 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.