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
