Re: Change in output as a result of patch

[Kaartic Sivaraam <kaarticsivaraam91196@gmail.com>]

Let me see if I got everything correctly. Correct me if any of the
below observations are wrong.

On Mon, 2017-07-24 at 14:25 -0700, Junio C Hamano wrote:
> 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".
> 
So what's expected is an error message that details all possible errors
found in a command in a single message. Now that would be better than
what 'mv' does.

> 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.
> 
Ok, now that seems to require little work. I'll see what I could come
up with.

Before I get into this I noticed that "--set-upstream" has been
deprecated since,

b347d06bf09 (branch: deprecate --set-upstream and show help if we detect possible mistaken use,
             Thu Aug 30 19:23:13 2012)

Is there any possibility for it to be removed some time in the near
future?

I'm asking this because IIRC, the 'attr_only' parameter of
"validate_new_branchname" was introduced to fix a regression
(fa7993767560) caused by the "--set-upstream" option. In case it has
been planned to be removed some time soon, it could make the word
easier (?), not sure though.

> In any case, the illustrations of interaction before and after the
> change is a very good thing to have when discussing a patch,
I would like to credit that to "Ævar Arnfjörð Bjarmason", who suggested
that to me in one of the other threads.

-- 
Kaartic