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

Jeff King [off-list ref] 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 [off-list ref]
Signed-off-by: Junio C Hamano <redacted>
---
 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)

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

Jeff King [off-list ref] writes:

quoted

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

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

-Peff