Re: Change in output as a result of patch

[Kaartic Sivaraam]

On Mon, 2017-07-24 at 14:25 -0700, Junio C Hamano wrote:
> 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"
I guess it's not possible to merge the two parameters into one as the
following code path shouldn't be taken when 'attr_only' is set,

if (!attr_only) {
const char *head;
struct object_id oid;

head = resolve_ref_unsafe("HEAD", 0, oid.hash, NULL);
if (!is_bare_repository() && head && !strcmp(head, ref->buf))
die(_("Cannot force update the current branch."));
}

and I guess this means the 'attr_only' can't merged with 'force'.

Further, I saw this in 'branch.h'

>  NEEDSWORK: This needs to be split into two separate functions in the
>  longer run for sanity.

Any ways in which I could help with this?

-- 
Kaartic

Re: Change in output as a result of patch

[Junio C Hamano]

Kaartic Sivaraam  writes:

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

I suspect that it would not make the refactoring that much less
work, but you are right---it is about time we started looking into
removing the --set-upstream optin whose 5th anniversary after
deprecation is only one month away.

Re: Change in output as a result of patch

[Kaartic Sivaraam]

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

Re: Change in output as a result of patch

[Junio C Hamano]

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

Change in output as a result of patch

[Kaartic Sivaraam]

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.

-- 
Kaartic