[PATCH] checkout: indicate when a detached head is checked out for a branch

[PATCH] checkout: indicate when a detached head is checked out for a branch

[Michael J Gruber]

In a setup with more than 1 workdir, it can happen that a user requests
to checkout a branch that is already checked out somewhere else. The
current "hint" about this gives the impression that it is merely warning
about this fact.

Actually, "checkout" changes its behavior in such a situation from
checking out the requested branch to checking out its head as a detached
head. The only indication is the usual checkout message which appears
before the hint.

Change the hint to make it clearer that checkout changed its behavior to
something that is possible to do in that situation, but may or may not
be what the user intended.

Signed-off-by: Michael J Gruber <git@drmicha.warpmail.net>
---
I really like the new --to feature and will convert all my "new workdir"
checkouts to that. But that detached checkout is so easy to miss - in fact
I noticed it only when I compared "new-workdir" to "checkout --to" for a
test repo with one branch, to see what a converter would need to do.

I'm even wondering whether we should do this DWIMmery at all, given how
"dangerous" a detached head is for those who are not aware of it
before gc kicks in.

(Sorry if that dupes something on the list, can't keep up these days;
so this is coming from a "mere user" ;-)

 builtin/checkout.c | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index cfc6db7..38a5670 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -645,9 +645,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 				detach_advice(new->name);
 			describe_detached_head(_("HEAD is now at"), new->commit);
 			if (new->checkout && !*new->checkout)
-				fprintf(stderr, _("hint: the main checkout is holding this branch\n"));
+				fprintf(stderr, _("hint: the main checkout is holding this branch; detaching branch head instead.\n"));
 			else if (new->checkout)
-				fprintf(stderr, _("hint: the linked checkout %s is holding this branch\n"),
+				fprintf(stderr, _("hint: the linked checkout %s is holding this branch; detaching branch head instead.\n"),
 					new->checkout);
 		}
 	} else if (new->path) {	/* Switch branches. */
-- 
2.0.1.583.gbe96573

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Duy Nguyen]

On Fri, Jul 18, 2014 at 4:50 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> I really like the new --to feature and will convert all my "new workdir"
> checkouts to that. But that detached checkout is so easy to miss - in fact
> I noticed it only when I compared "new-workdir" to "checkout --to" for a
> test repo with one branch, to see what a converter would need to do.
>
> I'm even wondering whether we should do this DWIMmery at all,

This is what this series needs, user's opinions (bad or good). The
other option is abort the checkout immediately. I think I made detach
behavior default is because it's more work (and needs to be proven
feasible). How about a config key that lets user decide what to do
here, abort or detach. We may change the default behavior too if
people think the current one is not good.

> given how "dangerous" a detached head is for those who are not aware of it
> before gc kicks in.

Wait, what danger are we talking about? I thought gc pays attention to
detached heads as well..
-- 
Duy

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Michael J Gruber]

Duy Nguyen venit, vidit, dixit 18.07.2014 12:58:
> On Fri, Jul 18, 2014 at 4:50 PM, Michael J Gruber
> <git@drmicha.warpmail.net> wrote:
>> I really like the new --to feature and will convert all my "new workdir"
>> checkouts to that. But that detached checkout is so easy to miss - in fact
>> I noticed it only when I compared "new-workdir" to "checkout --to" for a
>> test repo with one branch, to see what a converter would need to do.
>>
>> I'm even wondering whether we should do this DWIMmery at all,
> 
> This is what this series needs, user's opinions (bad or good). The
> other option is abort the checkout immediately. I think I made detach
> behavior default is because it's more work (and needs to be proven
> feasible). How about a config key that lets user decide what to do
> here, abort or detach. We may change the default behavior too if
> people think the current one is not good.

Uh, config bloat :)

I think DWIMmery is OK if it's made clear to the user what happened, and
it is somewhat expected/probably intended behavior.

Do we have a precedent where a detached head is produced when a branch
checkout is requested, or something similar? I think checking out remote
tracking branches is somehow in that same boat.

>> given how "dangerous" a detached head is for those who are not aware of it
>> before gc kicks in.
> 
> Wait, what danger are we talking about? I thought gc pays attention to
> detached heads as well..

As long as HEAD points to it, of course.

I think detached head is one of the killer features of git, in both
senses of the meaning...

Don't we DWIM (or suggest) "git checkout origin/master" to "git checkout
--track origin/master" which creates master with upstream origin/master?

Maybe I'm mixing things up, but I think we try to produce detached heads
only on special requests. New users get confused by them, some don't
understand the (well crafted) message you get when you switch away from
them, and while you can recover them from HEAD's reflog, they are gone
with the next gc unless they remain checked out (or get referenced).

I think I've just convinced myself that we shouldn't DWIm to a detached
head, and rather tell the user how to produce one if she really intended
to: "git checkout --detach..." That one seems to be broken by multiple
workdir setup (in the sense of producing an unnecessary hint).

Michael

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Max Kirillov]

Hi.

On Fri, Jul 18, 2014 at 4:27 PM, Michael J Gruber
<git@drmicha.warpmail.net> wrote:
> Duy Nguyen venit, vidit, dixit 18.07.2014 12:58:
>> This is what this series needs, user's opinions (bad or good).

Actually, if options "-b branch" works with the "--to" (does it?), then user
probably shouldn't need to create detached checkouts (I need them
only for scripts), so this action is probably a mistake. And when user
does want to create detached checkout he can use the "--detach" option.

So I would say checkout of already checked out branch should fail, suggesting
using "-b" or "--detach" options.

-- 
Max

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Junio C Hamano]

Michael J Gruber <git@drmicha.warpmail.net> writes:

> I really like the new --to feature and will convert all my "new workdir"
> checkouts to that. But that detached checkout is so easy to miss - in fact
> I noticed it only when I compared "new-workdir" to "checkout --to" for a
> test repo with one branch, to see what a converter would need to do.
>
> I'm even wondering whether we should do this DWIMmery at all, given how
> "dangerous" a detached head is for those who are not aware of it
> before gc kicks in.

As long as the amount of warning about 'detached HEAD' is about the
same between this case and a "git checkout v1.2.3" in a normal
repository, I do not think there is no additional "danger" you need
to be worried about.

But I do agree that there should not be any DWIM here.

The reason to introduce this new set of rather intrusive changes is
so that working trees can be aware of branches other working trees
have checked out.  And the whole point of wanting to have that
mutual awareness is to enable us to forbid users from mucking with
the same branch from two different places.

But Git is not in the position to dictate which alternative action
the user would want to take, when her "git checkout foo" is
prevented by this mechanism.  In one scenario, she may say "I only
wanted to take a peek" and run "git checkout foo^0" instead.  In
another, she may say "Ah, I forgot I already was doing this change
in the other one" and run "cd ../foo".  There may be other classes
of alternative actions.

Don't make it easier for the first class of scenario and make it
less useful and more dangerous for the second class, especially the
second class involve forgetful users who are likely to forget seeing
the "we've warned you that we detached without being asked" message.

Please fix it to always just error out.

> (Sorry if that dupes something on the list, can't keep up these days;
> so this is coming from a "mere user" ;-)
>
>  builtin/checkout.c | 4 ++--
>  1 file changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index cfc6db7..38a5670 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -645,9 +645,9 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  				detach_advice(new->name);
>  			describe_detached_head(_("HEAD is now at"), new->commit);
>  			if (new->checkout && !*new->checkout)
> -				fprintf(stderr, _("hint: the main checkout is holding this branch\n"));
> +				fprintf(stderr, _("hint: the main checkout is holding this branch; detaching branch head instead.\n"));
>  			else if (new->checkout)
> -				fprintf(stderr, _("hint: the linked checkout %s is holding this branch\n"),
> +				fprintf(stderr, _("hint: the linked checkout %s is holding this branch; detaching branch head instead.\n"),
>  					new->checkout);
>  		}
>  	} else if (new->path) {	/* Switch branches. */

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Dennis Kaarsemaker]

On vr, 2014-07-18 at 10:36 -0700, Junio C Hamano wrote:
> Michael J Gruber <git@drmicha.warpmail.net> writes:
> 
> > I really like the new --to feature and will convert all my "new workdir"
> > checkouts to that. But that detached checkout is so easy to miss - in fact
> > I noticed it only when I compared "new-workdir" to "checkout --to" for a
> > test repo with one branch, to see what a converter would need to do.
> >
> > I'm even wondering whether we should do this DWIMmery at all, given how
> > "dangerous" a detached head is for those who are not aware of it
> > before gc kicks in.
> 
> As long as the amount of warning about 'detached HEAD' is about the
> same between this case and a "git checkout v1.2.3" in a normal
> repository, I do not think there is no additional "danger" you need
> to be worried about.
> 
> But I do agree that there should not be any DWIM here.
> 
> The reason to introduce this new set of rather intrusive changes is
> so that working trees can be aware of branches other working trees
> have checked out.  And the whole point of wanting to have that
> mutual awareness is to enable us to forbid users from mucking with
> the same branch from two different places.
> 
> But Git is not in the position to dictate which alternative action
> the user would want to take, when her "git checkout foo" is
> prevented by this mechanism.  In one scenario, she may say "I only
> wanted to take a peek" and run "git checkout foo^0" instead.  In
> another, she may say "Ah, I forgot I already was doing this change
> in the other one" and run "cd ../foo".  There may be other classes
> of alternative actions.
> 
> Don't make it easier for the first class of scenario and make it
> less useful and more dangerous for the second class, especially the
> second class involve forgetful users who are likely to forget seeing
> the "we've warned you that we detached without being asked" message.
> 
> Please fix it to always just error out.

I really would appreciate it if it wouldn't always error out. Erroring
out by default is fine, but please keep it overridable. 

My use case for this is checking out the same branch (or commit, so
already on a detached HEAD) in multiple different places to run
independent actions (e.g. make test with different compiler options, or
creating several different packages) and I would really appreciate it if
that would keep working.

-- 
Dennis Kaarsemaker
www.kaarsemaker.net

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Junio C Hamano]

Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:

> My use case for this is checking out the same branch (or commit, so
> already on a detached HEAD) in multiple different places to run
> independent actions (e.g. make test with different compiler options, or
> creating several different packages) and I would really appreciate it if
> that would keep working.

I do not have any problem if multiple working trees have the same
commit checked out on their own detached HEADs at all.  The "should
error out" was solely for the case where the user asked not to detach
but to obtain a state where a named branch is checked out.  In such
a case, the command should not turn it into a detached HEAD, with or
without a warning.

Re: [PATCH] checkout: indicate when a detached head is checked out for a branch

[Michael J Gruber]

Junio C Hamano venit, vidit, dixit 19.07.2014 00:18:
> Dennis Kaarsemaker <dennis@kaarsemaker.net> writes:
> 
>> My use case for this is checking out the same branch (or commit, so
>> already on a detached HEAD) in multiple different places to run
>> independent actions (e.g. make test with different compiler options, or
>> creating several different packages) and I would really appreciate it if
>> that would keep working.
> 
> I do not have any problem if multiple working trees have the same
> commit checked out on their own detached HEADs at all.  The "should
> error out" was solely for the case where the user asked not to detach
> but to obtain a state where a named branch is checked out.  In such
> a case, the command should not turn it into a detached HEAD, with or
> without a warning.

Exactly, all of that, plus:

* "git checkout --detach --to foo bar" could/should be a way to spell out
"git checkout --to foo bar^0".

* In the case of erroring out, "git checkout" could suggest one of the
above two commands.

Michael