[PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Junio C Hamano]

When switching away from a detached HEAD with "git checkout", we give a
final warning to tell how to resurrect the commits being left behind since
8e2dc6a (commit: give final warning when reattaching HEAD to leave commits
behind, 2011-02-18) rather loudly.

This is a good safety measure for people who are not comfortable with the
detached HEAD state, but the warning was given even to those who set the
advice.detachedHead to false to decline the warning given when detaching,
resulting in an asymmetric experience.  Silent when going detached, and
very loud when coming back.

Make the call to orphan check and warning conditional to the advice
setting to correct this.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * Admittedly, going into the detached HEAD state is much less warn-worthy
   event than coming out of it, especially if you made any commit that
   will become unreachable, so there is an inherent asymmetry and some
   people may want to have a silent entry and loud exit (i.e. the current
   behaviour).  I do not mind a separate "advice.abouttolosecommit", but
   with this weatherbaloon I am trying to see if we can get away without
   adding yet another knob that the user has to tweak.

 builtin/checkout.c |    9 +++++++--
 1 files changed, 7 insertions(+), 2 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index eece5d6..eb92250 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -723,8 +723,13 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	if (ret)
 		return ret;
 
-	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
-		orphaned_commit_warning(old.commit);
+	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit) {
+		if (advice_detached_head)
+			orphaned_commit_warning(old.commit);
+		else
+			describe_detached_head(_("Previous HEAD position was"),
+					       old.commit);
+	}
 
 	update_refs_for_switch(opts, &old, new);
 

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Jeff King]

On Fri, May 06, 2011 at 01:35:37PM -0700, Junio C Hamano wrote:

> When switching away from a detached HEAD with "git checkout", we give a
> final warning to tell how to resurrect the commits being left behind since
> 8e2dc6a (commit: give final warning when reattaching HEAD to leave commits
> behind, 2011-02-18) rather loudly.
> 
> This is a good safety measure for people who are not comfortable with the
> detached HEAD state, but the warning was given even to those who set the
> advice.detachedHead to false to decline the warning given when detaching,
> resulting in an asymmetric experience.  Silent when going detached, and
> very loud when coming back.

I'm somewhat negative on this. I think there are actually 5 distinct
pieces of information that git currently gives in going to and from a
detached HEAD, and the motivations for suppressing them may be
different:

  1. On detaching, we indicate briefly that the HEAD has been detached
     by saying "HEAD is now at ..." instead of "Switched to branch ...".

  2. On detaching, we give a large warning on what the detached HEAD
     state means, and advise on how to get out of it.

  3. On leaving, if there are no orphaned commits, we indicate briefly
     where the previous HEAD position was with "Previous HEAD position
     was...".

  4. On leaving, if there are orphaned commits, we list them.

  5. On leaving, if there are orphaned commits, we give advice on how to
     make branches out of them.

Right now, advice.detachedhead suppresses (2); that is, we leave the
short indicator that provides distinct per-use information to the user
(1), but suppress the lengthy advice that is not helpful to advanced
user.

So if you wanted symmetry, I think that would mean suppressing (5), but
leaving (4), which contains per-use information, intact.

I can also see somebody wanting to suppress (4), either because it takes
too much time to compute, or because even though there is distinct
information in the message, it is lengthy. But I think that should be a
separate knob.

Personally, I want to see (1) and (4). Since the introduction of the
orphaned-commit check, I can think of at least 2 or 3 times when I have
been happy it reminded me, and immediately cherry-picked or performed
some other action on commits in the list it showed.

I tend to think (3) is now just useless. Before the orphaned-commit
check, it was perhaps a reminder that you might have orphaned something.
But now it is only useful as a reminder of some point you were at that
you might want to go back to (which is slightly more useful than when
you leave a ref, since there are a much smaller number of refs than
there are arbitrary commits that could appear in a detached HEAD). But
for going back to an arbitrary point, I think the reflog is a much more
useful tool.

>  * Admittedly, going into the detached HEAD state is much less warn-worthy
>    event than coming out of it, especially if you made any commit that
>    will become unreachable, so there is an inherent asymmetry and some
>    people may want to have a silent entry and loud exit (i.e. the current
>    behaviour).  I do not mind a separate "advice.abouttolosecommit", but
>    with this weatherbaloon I am trying to see if we can get away without
>    adding yet another knob that the user has to tweak.

Hopefully the above made sense, but to be clear, what I think we should
do is:

  1. Suppress the "If you want to keep it..." advice on exit with the
     existing advice.detachedhead.

  2. Optionally, if anybody cares (and I don't), introduce
     advice.detachedOrphanCheck to suppress the check entirely.

  3. Optionally remove "Previous HEAD position" in the non-orphan case.
     I think it's superfluous, but it's so short that I don't really
     care either way.

-Peff

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, May 06, 2011 at 01:35:37PM -0700, Junio C Hamano wrote:
>
>> When switching away from a detached HEAD with "git checkout", we give a
>> final warning to tell how to resurrect the commits being left behind since
>> 8e2dc6a (commit: give final warning when reattaching HEAD to leave commits
>> behind, 2011-02-18) rather loudly.
>> 
>> This is a good safety measure for people who are not comfortable with the
>> detached HEAD state, but the warning was given even to those who set the
>> advice.detachedHead to false to decline the warning given when detaching,
>> resulting in an asymmetric experience.  Silent when going detached, and
>> very loud when coming back.
>
> I'm somewhat negative on this. I think there are actually 5 distinct
> pieces of information that git currently gives in going to and from a
> detached HEAD, and the motivations for suppressing them may be
> different:
>
>   1. On detaching, we indicate briefly that the HEAD has been detached
>      by saying "HEAD is now at ..." instead of "Switched to branch ...".
>
>   2. On detaching, we give a large warning on what the detached HEAD
>      state means, and advise on how to get out of it.
>
>   3. On leaving, if there are no orphaned commits, we indicate briefly
>      where the previous HEAD position was with "Previous HEAD position
>      was...".
>
>   4. On leaving, if there are orphaned commits, we list them.
>
>   5. On leaving, if there are orphaned commits, we give advice on how to
>      make branches out of them.
>
> Right now, advice.detachedhead suppresses (2); that is, we leave the
> short indicator that provides distinct per-use information to the user
> (1), but suppress the lengthy advice that is not helpful to advanced
> user.
>
> So if you wanted symmetry, I think that would mean suppressing (5), but
> leaving (4), which contains per-use information, intact.

The patch does leave per-use information by giving 3. "HEAD was" as you
noted above, and that is more than sufficient (you can also look at
HEAD@{0}).  If and only if the list is needed (i.e. the user wants to
recover), the user can run "git log $that_commit".

> I can also see somebody wanting to suppress (4), either because it takes
> too much time to compute, or because even though there is distinct
> information in the message, it is lengthy. But I think that should be a
> separate knob.

Ok, then a separate configuration that is.

> I tend to think (3) is now just useless.

Quite the contrary. If you do not want to pay the price of (4) that is
useless most of the time, (3) is a cheap, space efficient and useful
information that is essential to allow you to get rid of (4) without
having to look at reflog.

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Jeff King]

On Fri, May 06, 2011 at 03:59:20PM -0700, Junio C Hamano wrote:

> > I tend to think (3) is now just useless.
> 
> Quite the contrary. If you do not want to pay the price of (4) that is
> useless most of the time, (3) is a cheap, space efficient and useful
> information that is essential to allow you to get rid of (4) without
> having to look at reflog.

Sorry, I should have been more clear. If you turn off orphan-checking,
(3) is just as useful as it always was. But _if_ you have
orphan-checking turned on, and it comes up negative, showing (3) is not
particularly useful. You already know you are not losing commits.

-Peff

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Jeff King]

On Fri, May 06, 2011 at 03:59:20PM -0700, Junio C Hamano wrote:

> > I'm somewhat negative on this. I think there are actually 5 distinct
> > pieces of information that git currently gives in going to and from a
> > detached HEAD, and the motivations for suppressing them may be
> > different:
> >
> >   1. On detaching, we indicate briefly that the HEAD has been detached
> >      by saying "HEAD is now at ..." instead of "Switched to branch ...".
> >
> >   2. On detaching, we give a large warning on what the detached HEAD
> >      state means, and advise on how to get out of it.
> >
> >   3. On leaving, if there are no orphaned commits, we indicate briefly
> >      where the previous HEAD position was with "Previous HEAD position
> >      was...".
> >
> >   4. On leaving, if there are orphaned commits, we list them.
> >
> >   5. On leaving, if there are orphaned commits, we give advice on how to
> >      make branches out of them.
> >
> > Right now, advice.detachedhead suppresses (2); that is, we leave the
> > short indicator that provides distinct per-use information to the user
> > (1), but suppress the lengthy advice that is not helpful to advanced
> > user.
> >
> > So if you wanted symmetry, I think that would mean suppressing (5), but
> > leaving (4), which contains per-use information, intact.
> 
> The patch does leave per-use information by giving 3. "HEAD was" as you
> noted above, and that is more than sufficient (you can also look at
> HEAD@{0}).  If and only if the list is needed (i.e. the user wants to
> recover), the user can run "git log $that_commit".

I think we're probably in agreement on how to go forward, but I wanted
to note one final thing. It is actually not the list itself which is
that valuable to me. It's merely a convenience; if I know there is
something worth looking at, I am perfectly capable of inspecting the
history graph myself.

The real value in the orphan-check to me is whether it says "hey, there
are orphaned commits" or not. Before we had that check, leaving the
detached state _always_ said "Previous HEAD was...", and I quickly
learned to ignore it as uninteresting noise in 99% of the cases. Whereas
in the orphan warning case, I find that I want to do something about it
in at least 50% of the cases. Thus I actually heed the warning, and it
is effective.

It's possible that some people would find it useful to print only the
"warning: you are leaving orphaned commits" message, but not show the
list of them. I don't think it is worth it, though. Leaving orphaned
commits is the uncommon case, and doing so without wanting to
investigate is probably even less common. So the user is not too likely
to get annoyed by a little extra verbosity in the uncommon false
positive case.

-Peff

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> Hopefully the above made sense, but to be clear, what I think we should
> do is:
>
>   1. Suppress the "If you want to keep it..." advice on exit with the
>      existing advice.detachedhead.
>
>   2. Optionally, if anybody cares (and I don't), introduce
>      advice.detachedOrphanCheck to suppress the check entirely.
>
>   3. Optionally remove "Previous HEAD position" in the non-orphan case.
>      I think it's superfluous, but it's so short that I don't really
>      care either way.

I think the above makes sense (sorry for replying a three-week old thread,
but I am trying to rid as many topics as I can from the Stalled category),
except that #3. might be useful after manually bisecting the existing
history.  You may not be losing any commit (all are connected), but you
would be losing the point you have spent efforts to locate by switching
out.

I also think #2 would not be necessary; scripts that know what they are
doing should be using -q to suppress output from checkout anyway, and the
check is disabled in that case.

So on top of 8e2dc6a (commit: give final warning when reattaching HEAD to
leave commits behind, 2011-02-18), here is a re-roll.

-- >8 --
Subject: checkout: make advice when reattaching the HEAD less loud

When switching away from a detached HEAD with "git checkout", we give a
listing of the commits about to be lost, and then tell how to resurrect
them since 8e2dc6a (commit: give final warning when reattaching HEAD to
leave commits behind, 2011-02-18).

This is a good safety measure for people who are not comfortable with the
detached HEAD state, but the advice on how to keep the state you just left
was given even to those who set advice.detachedHead to false.

Keep the warning and informational commit listing, but honor the setting
of advice.detachedHead to squelch the advice.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/checkout.c |   17 +++++++++--------
 1 files changed, 9 insertions(+), 8 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index e44364c..feaf29f 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -634,14 +634,17 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)
 		"Warning: you are leaving %d commit%s behind, "
 		"not connected to\n"
 		"any of your branches:\n\n"
-		"%s\n"
-		"If you want to keep them by creating a new branch, "
-		"this may be a good time\nto do so with:\n\n"
-		" git branch new_branch_name %s\n\n",
+		"%s\n",
 		lost, ((1 < lost) ? "s" : ""),
-		sb.buf,
-		sha1_to_hex(commit->object.sha1));
+		sb.buf);
 	strbuf_release(&sb);
+
+	if (advice_detached_head)
+		fprintf(stderr,
+			"If you want to keep them by creating a new branch, "
+			"this may be a good time\nto do so with:\n\n"
+			" git branch new_branch_name %s\n\n",
+			sha1_to_hex(commit->object.sha1));
 }
 
 /*
@@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)
 		die("internal error in revision walk");
 	if (!(commit->object.flags & UNINTERESTING))
 		suggest_reattach(commit, &revs);
-	else
-		describe_detached_head("Previous HEAD position was", commit);
 }
 
 static int switch_branches(struct checkout_opts *opts, struct branch_info *new)

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Jeff King]

On Tue, May 24, 2011 at 10:11:43AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > Hopefully the above made sense, but to be clear, what I think we should
> > do is:
> >
> >   1. Suppress the "If you want to keep it..." advice on exit with the
> >      existing advice.detachedhead.
> >
> >   2. Optionally, if anybody cares (and I don't), introduce
> >      advice.detachedOrphanCheck to suppress the check entirely.
> >
> >   3. Optionally remove "Previous HEAD position" in the non-orphan case.
> >      I think it's superfluous, but it's so short that I don't really
> >      care either way.
> 
> I think the above makes sense (sorry for replying a three-week old thread,
> but I am trying to rid as many topics as I can from the Stalled category),

No problem. I have an embarrassing number of stalled topics myself.

> except that #3. might be useful after manually bisecting the existing
> history.  You may not be losing any commit (all are connected), but you
> would be losing the point you have spent efforts to locate by switching
> out.

Yeah, I thought of the "you have found this point via some work" as
something valuable but dismissed it because I couldn't think of a good
example.  But bisection is one such example.

> I also think #2 would not be necessary; scripts that know what they are
> doing should be using -q to suppress output from checkout anyway, and the
> check is disabled in that case.

Ah, I didn't realize that "-q" would suppress it, but yes, that makes
perfect sense.

> So on top of 8e2dc6a (commit: give final warning when reattaching HEAD to
> leave commits behind, 2011-02-18), here is a re-roll.

The output looks good to me. I have an almost-complaint, though. I
applied on top of 8e2dc6a and did a quick test. It is actually quite bad
there, because you get:

  Warning: you are leaving 1 commit behind, not connected to
  any of your branches:

    - some subject

and nothing actually tells you the sha1 of the thing you are losing. :)

However, once it is merged into master, you will get:

     abcd1234 some subject

which is more helpful. Not a big deal, as that merge should happen
before release. But you may want to just apply on top of my 0be240c
(checkout: tweak detached-orphan warning format, 2011-03-20).

> @@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)
>  		die("internal error in revision walk");
>  	if (!(commit->object.flags & UNINTERESTING))
>  		suggest_reattach(commit, &revs);
> -	else
> -		describe_detached_head("Previous HEAD position was", commit);
>  }

Wait, I thought we were keeping this, per your argument above?

-Peff

Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>> @@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)
>>  		die("internal error in revision walk");
>>  	if (!(commit->object.flags & UNINTERESTING))
>>  		suggest_reattach(commit, &revs);
>> -	else
>> -		describe_detached_head("Previous HEAD position was", commit);
>>  }
>
> Wait, I thought we were keeping this, per your argument above?

Well, I do not have very strong preference either way. I didn't really
argue for or against. I just explained it might be useful.

Will resurrect.