Re: Change in output as a result of patch

From: Junio C Hamano <gitster@pobox.com>
Date: Jul 24, 2017, 21:25 UTC
Message-ID: <xmqqd18pcysa.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1500923812.20078.8.camel@gmail.com>

Kaartic Sivaraam <kaarticsivaraam91196@gmail.com> writes:

Show 24 quoted lines

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