{"thread":{"id":"27285","subject":"[PATCH] checkout: honor advice.detachedHead when reattaching to a branch","startedAt":"2011-05-06T20:35:37Z","lastAt":"2011-05-24T21:24:37Z","messageCount":8,"participants":["Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"167280","messageId":"7vei4bzhwm.fsf@alter.siamese.dyndns.org","threadId":"27285","inReplyTo":null,"subject":"[PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T20:35:37Z","receivedAt":"2011-05-06T20:35:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"When switching away from a detached HEAD with \"git checkout\", we give a\nfinal warning to tell how to resurrect the commits being left behind since\n8e2dc6a (commit: give final warning when reattaching HEAD to leave commits\nbehind, 2011-02-18) rather loudly.\n\nThis is a good safety measure for people who are not comfortable with the\ndetached HEAD state, but the warning was given even to those who set the\nadvice.detachedHead to false to decline the warning given when detaching,\nresulting in an asymmetric experience. Silent when going detached, and\nvery loud when coming back.\n\nMake the call to orphan check and warning conditional to the advice\nsetting to correct this.\n\nSigned-off-by: Junio C Hamano \n---\n\n * Admittedly, going into the detached HEAD state is much less warn-worthy\n event than coming out of it, especially if you made any commit that\n will become unreachable, so there is an inherent asymmetry and some\n people may want to have a silent entry and loud exit (i.e. the current\n behaviour). I do not mind a separate \"advice.abouttolosecommit\", but\n with this weatherbaloon I am trying to see if we can get away without\n adding yet another knob that the user has to tweak.\n\n builtin/checkout.c | 9 +++++++--\n 1 files changed, 7 insertions(+), 2 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex eece5d6..eb92250 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -723,8 +723,13 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \tif (ret)\n \t\treturn ret;\n \n-\tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n-\t\torphaned_commit_warning(old.commit);\n+\tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit) {\n+\t\tif (advice_detached_head)\n+\t\t\torphaned_commit_warning(old.commit);\n+\t\telse\n+\t\t\tdescribe_detached_head(_(\"Previous HEAD position was\"),\n+\t\t\t\t\t old.commit);\n+\t}\n \n \tupdate_refs_for_switch(opts, &old, new);\n \n"},{"id":"167298","messageId":"20110506223847.GC17848@sigill.intra.peff.net","threadId":"27285","inReplyTo":"7vei4bzhwm.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-06T22:38:47Z","receivedAt":"2011-05-06T22:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 01:35:37PM -0700, Junio C Hamano wrote:\n\n> When switching away from a detached HEAD with \"git checkout\", we give a\n> final warning to tell how to resurrect the commits being left behind since\n> 8e2dc6a (commit: give final warning when reattaching HEAD to leave commits\n> behind, 2011-02-18) rather loudly.\n> \n> This is a good safety measure for people who are not comfortable with the\n> detached HEAD state, but the warning was given even to those who set the\n> advice.detachedHead to false to decline the warning given when detaching,\n> resulting in an asymmetric experience. Silent when going detached, and\n> very loud when coming back.\n\nI'm somewhat negative on this. I think there are actually 5 distinct\npieces of information that git currently gives in going to and from a\ndetached HEAD, and the motivations for suppressing them may be\ndifferent:\n\n 1. On detaching, we indicate briefly that the HEAD has been detached\n by saying \"HEAD is now at ...\" instead of \"Switched to branch ...\".\n\n 2. On detaching, we give a large warning on what the detached HEAD\n state means, and advise on how to get out of it.\n\n 3. On leaving, if there are no orphaned commits, we indicate briefly\n where the previous HEAD position was with \"Previous HEAD position\n was...\".\n\n 4. On leaving, if there are orphaned commits, we list them.\n\n 5. On leaving, if there are orphaned commits, we give advice on how to\n make branches out of them.\n\nRight now, advice.detachedhead suppresses (2); that is, we leave the\nshort indicator that provides distinct per-use information to the user\n(1), but suppress the lengthy advice that is not helpful to advanced\nuser.\n\nSo if you wanted symmetry, I think that would mean suppressing (5), but\nleaving (4), which contains per-use information, intact.\n\nI can also see somebody wanting to suppress (4), either because it takes\ntoo much time to compute, or because even though there is distinct\ninformation in the message, it is lengthy. But I think that should be a\nseparate knob.\n\nPersonally, I want to see (1) and (4). Since the introduction of the\norphaned-commit check, I can think of at least 2 or 3 times when I have\nbeen happy it reminded me, and immediately cherry-picked or performed\nsome other action on commits in the list it showed.\n\nI tend to think (3) is now just useless. Before the orphaned-commit\ncheck, it was perhaps a reminder that you might have orphaned something.\nBut now it is only useful as a reminder of some point you were at that\nyou might want to go back to (which is slightly more useful than when\nyou leave a ref, since there are a much smaller number of refs than\nthere are arbitrary commits that could appear in a detached HEAD). But\nfor going back to an arbitrary point, I think the reflog is a much more\nuseful tool.\n\n> * Admittedly, going into the detached HEAD state is much less warn-worthy\n> event than coming out of it, especially if you made any commit that\n> will become unreachable, so there is an inherent asymmetry and some\n> people may want to have a silent entry and loud exit (i.e. the current\n> behaviour). I do not mind a separate \"advice.abouttolosecommit\", but\n> with this weatherbaloon I am trying to see if we can get away without\n> adding yet another knob that the user has to tweak.\n\nHopefully the above made sense, but to be clear, what I think we should\ndo is:\n\n 1. Suppress the \"If you want to keep it...\" advice on exit with the\n existing advice.detachedhead.\n\n 2. Optionally, if anybody cares (and I don't), introduce\n advice.detachedOrphanCheck to suppress the check entirely.\n\n 3. Optionally remove \"Previous HEAD position\" in the non-orphan case.\n I think it's superfluous, but it's so short that I don't really\n care either way.\n\n-Peff\n"},{"id":"167300","messageId":"7vbozfxwon.fsf@alter.siamese.dyndns.org","threadId":"27285","inReplyTo":"20110506223847.GC17848@sigill.intra.peff.net","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-06T22:59:20Z","receivedAt":"2011-05-06T22:59:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Fri, May 06, 2011 at 01:35:37PM -0700, Junio C Hamano wrote:\n>\n>> When switching away from a detached HEAD with \"git checkout\", we give a\n>> final warning to tell how to resurrect the commits being left behind since\n>> 8e2dc6a (commit: give final warning when reattaching HEAD to leave commits\n>> behind, 2011-02-18) rather loudly.\n>> \n>> This is a good safety measure for people who are not comfortable with the\n>> detached HEAD state, but the warning was given even to those who set the\n>> advice.detachedHead to false to decline the warning given when detaching,\n>> resulting in an asymmetric experience. Silent when going detached, and\n>> very loud when coming back.\n>\n> I'm somewhat negative on this. I think there are actually 5 distinct\n> pieces of information that git currently gives in going to and from a\n> detached HEAD, and the motivations for suppressing them may be\n> different:\n>\n> 1. On detaching, we indicate briefly that the HEAD has been detached\n> by saying \"HEAD is now at ...\" instead of \"Switched to branch ...\".\n>\n> 2. On detaching, we give a large warning on what the detached HEAD\n> state means, and advise on how to get out of it.\n>\n> 3. On leaving, if there are no orphaned commits, we indicate briefly\n> where the previous HEAD position was with \"Previous HEAD position\n> was...\".\n>\n> 4. On leaving, if there are orphaned commits, we list them.\n>\n> 5. On leaving, if there are orphaned commits, we give advice on how to\n> make branches out of them.\n>\n> Right now, advice.detachedhead suppresses (2); that is, we leave the\n> short indicator that provides distinct per-use information to the user\n> (1), but suppress the lengthy advice that is not helpful to advanced\n> user.\n>\n> So if you wanted symmetry, I think that would mean suppressing (5), but\n> leaving (4), which contains per-use information, intact.\n\nThe patch does leave per-use information by giving 3. \"HEAD was\" as you\nnoted above, and that is more than sufficient (you can also look at\nHEAD@{0}). If and only if the list is needed (i.e. the user wants to\nrecover), the user can run \"git log $that_commit\".\n\n> I can also see somebody wanting to suppress (4), either because it takes\n> too much time to compute, or because even though there is distinct\n> information in the message, it is lengthy. But I think that should be a\n> separate knob.\n\nOk, then a separate configuration that is.\n\n> I tend to think (3) is now just useless.\n\nQuite the contrary. If you do not want to pay the price of (4) that is\nuseless most of the time, (3) is a cheap, space efficient and useful\ninformation that is essential to allow you to get rid of (4) without\nhaving to look at reflog.\n"},{"id":"167301","messageId":"20110506232118.GA24794@sigill.intra.peff.net","threadId":"27285","inReplyTo":"7vbozfxwon.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-06T23:21:18Z","receivedAt":"2011-05-06T23:21:18Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 03:59:20PM -0700, Junio C Hamano wrote:\n\n> > I tend to think (3) is now just useless.\n> \n> Quite the contrary. If you do not want to pay the price of (4) that is\n> useless most of the time, (3) is a cheap, space efficient and useful\n> information that is essential to allow you to get rid of (4) without\n> having to look at reflog.\n\nSorry, I should have been more clear. If you turn off orphan-checking,\n(3) is just as useful as it always was. But _if_ you have\norphan-checking turned on, and it comes up negative, showing (3) is not\nparticularly useful. You already know you are not losing commits.\n\n-Peff\n"},{"id":"167304","messageId":"20110507013847.GA25771@sigill.intra.peff.net","threadId":"27285","inReplyTo":"7vbozfxwon.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-07T01:38:47Z","receivedAt":"2011-05-07T01:38:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, May 06, 2011 at 03:59:20PM -0700, Junio C Hamano wrote:\n\n> > I'm somewhat negative on this. I think there are actually 5 distinct\n> > pieces of information that git currently gives in going to and from a\n> > detached HEAD, and the motivations for suppressing them may be\n> > different:\n> >\n> > 1. On detaching, we indicate briefly that the HEAD has been detached\n> > by saying \"HEAD is now at ...\" instead of \"Switched to branch ...\".\n> >\n> > 2. On detaching, we give a large warning on what the detached HEAD\n> > state means, and advise on how to get out of it.\n> >\n> > 3. On leaving, if there are no orphaned commits, we indicate briefly\n> > where the previous HEAD position was with \"Previous HEAD position\n> > was...\".\n> >\n> > 4. On leaving, if there are orphaned commits, we list them.\n> >\n> > 5. On leaving, if there are orphaned commits, we give advice on how to\n> > make branches out of them.\n> >\n> > Right now, advice.detachedhead suppresses (2); that is, we leave the\n> > short indicator that provides distinct per-use information to the user\n> > (1), but suppress the lengthy advice that is not helpful to advanced\n> > user.\n> >\n> > So if you wanted symmetry, I think that would mean suppressing (5), but\n> > leaving (4), which contains per-use information, intact.\n> \n> The patch does leave per-use information by giving 3. \"HEAD was\" as you\n> noted above, and that is more than sufficient (you can also look at\n> HEAD@{0}). If and only if the list is needed (i.e. the user wants to\n> recover), the user can run \"git log $that_commit\".\n\nI think we're probably in agreement on how to go forward, but I wanted\nto note one final thing. It is actually not the list itself which is\nthat valuable to me. It's merely a convenience; if I know there is\nsomething worth looking at, I am perfectly capable of inspecting the\nhistory graph myself.\n\nThe real value in the orphan-check to me is whether it says \"hey, there\nare orphaned commits\" or not. Before we had that check, leaving the\ndetached state _always_ said \"Previous HEAD was...\", and I quickly\nlearned to ignore it as uninteresting noise in 99% of the cases. Whereas\nin the orphan warning case, I find that I want to do something about it\nin at least 50% of the cases. Thus I actually heed the warning, and it\nis effective.\n\nIt's possible that some people would find it useful to print only the\n\"warning: you are leaving orphaned commits\" message, but not show the\nlist of them. I don't think it is worth it, though. Leaving orphaned\ncommits is the uncommon case, and doing so without wanting to\ninvestigate is probably even less common. So the user is not too likely\nto get annoyed by a little extra verbosity in the uncommon false\npositive case.\n\n-Peff\n"},{"id":"168588","messageId":"7vzkmc6n1s.fsf@alter.siamese.dyndns.org","threadId":"27285","inReplyTo":"20110506223847.GC17848@sigill.intra.peff.net","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T17:11:43Z","receivedAt":"2011-05-24T17:11:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Hopefully the above made sense, but to be clear, what I think we should\n> do is:\n>\n> 1. Suppress the \"If you want to keep it...\" advice on exit with the\n> existing advice.detachedhead.\n>\n> 2. Optionally, if anybody cares (and I don't), introduce\n> advice.detachedOrphanCheck to suppress the check entirely.\n>\n> 3. Optionally remove \"Previous HEAD position\" in the non-orphan case.\n> I think it's superfluous, but it's so short that I don't really\n> care either way.\n\nI think the above makes sense (sorry for replying a three-week old thread,\nbut I am trying to rid as many topics as I can from the Stalled category),\nexcept that #3. might be useful after manually bisecting the existing\nhistory. You may not be losing any commit (all are connected), but you\nwould be losing the point you have spent efforts to locate by switching\nout.\n\nI also think #2 would not be necessary; scripts that know what they are\ndoing should be using -q to suppress output from checkout anyway, and the\ncheck is disabled in that case.\n\nSo on top of 8e2dc6a (commit: give final warning when reattaching HEAD to\nleave commits behind, 2011-02-18), here is a re-roll.\n\n-- >8 --\nSubject: checkout: make advice when reattaching the HEAD less loud\n\nWhen switching away from a detached HEAD with \"git checkout\", we give a\nlisting of the commits about to be lost, and then tell how to resurrect\nthem since 8e2dc6a (commit: give final warning when reattaching HEAD to\nleave commits behind, 2011-02-18).\n\nThis is a good safety measure for people who are not comfortable with the\ndetached HEAD state, but the advice on how to keep the state you just left\nwas given even to those who set advice.detachedHead to false.\n\nKeep the warning and informational commit listing, but honor the setting\nof advice.detachedHead to squelch the advice.\n\nHelped-by: Jeff King \nSigned-off-by: Junio C Hamano \n---\n builtin/checkout.c | 17 +++++++++--------\n 1 files changed, 9 insertions(+), 8 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex e44364c..feaf29f 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -634,14 +634,17 @@ static void suggest_reattach(struct commit *commit, struct rev_info *revs)\n \t\t\"Warning: you are leaving %d commit%s behind, \"\n \t\t\"not connected to\\n\"\n \t\t\"any of your branches:\\n\\n\"\n-\t\t\"%s\\n\"\n-\t\t\"If you want to keep them by creating a new branch, \"\n-\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n-\t\t\" git branch new_branch_name %s\\n\\n\",\n+\t\t\"%s\\n\",\n \t\tlost, ((1 < lost) ? \"s\" : \"\"),\n-\t\tsb.buf,\n-\t\tsha1_to_hex(commit->object.sha1));\n+\t\tsb.buf);\n \tstrbuf_release(&sb);\n+\n+\tif (advice_detached_head)\n+\t\tfprintf(stderr,\n+\t\t\t\"If you want to keep them by creating a new branch, \"\n+\t\t\t\"this may be a good time\\nto do so with:\\n\\n\"\n+\t\t\t\" git branch new_branch_name %s\\n\\n\",\n+\t\t\tsha1_to_hex(commit->object.sha1));\n }\n \n /*\n@@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)\n \t\tdie(\"internal error in revision walk\");\n \tif (!(commit->object.flags & UNINTERESTING))\n \t\tsuggest_reattach(commit, &revs);\n-\telse\n-\t\tdescribe_detached_head(\"Previous HEAD position was\", commit);\n }\n \n static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n"},{"id":"168595","messageId":"20110524202723.GG584@sigill.intra.peff.net","threadId":"27285","inReplyTo":"7vzkmc6n1s.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2011-05-24T20:27:23Z","receivedAt":"2011-05-24T20:27:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, May 24, 2011 at 10:11:43AM -0700, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > Hopefully the above made sense, but to be clear, what I think we should\n> > do is:\n> >\n> > 1. Suppress the \"If you want to keep it...\" advice on exit with the\n> > existing advice.detachedhead.\n> >\n> > 2. Optionally, if anybody cares (and I don't), introduce\n> > advice.detachedOrphanCheck to suppress the check entirely.\n> >\n> > 3. Optionally remove \"Previous HEAD position\" in the non-orphan case.\n> > I think it's superfluous, but it's so short that I don't really\n> > care either way.\n> \n> I think the above makes sense (sorry for replying a three-week old thread,\n> but I am trying to rid as many topics as I can from the Stalled category),\n\nNo problem. I have an embarrassing number of stalled topics myself.\n\n> except that #3. might be useful after manually bisecting the existing\n> history. You may not be losing any commit (all are connected), but you\n> would be losing the point you have spent efforts to locate by switching\n> out.\n\nYeah, I thought of the \"you have found this point via some work\" as\nsomething valuable but dismissed it because I couldn't think of a good\nexample. But bisection is one such example.\n\n> I also think #2 would not be necessary; scripts that know what they are\n> doing should be using -q to suppress output from checkout anyway, and the\n> check is disabled in that case.\n\nAh, I didn't realize that \"-q\" would suppress it, but yes, that makes\nperfect sense.\n\n> So on top of 8e2dc6a (commit: give final warning when reattaching HEAD to\n> leave commits behind, 2011-02-18), here is a re-roll.\n\nThe output looks good to me. I have an almost-complaint, though. I\napplied on top of 8e2dc6a and did a quick test. It is actually quite bad\nthere, because you get:\n\n Warning: you are leaving 1 commit behind, not connected to\n any of your branches:\n\n - some subject\n\nand nothing actually tells you the sha1 of the thing you are losing. :)\n\nHowever, once it is merged into master, you will get:\n\n abcd1234 some subject\n\nwhich is more helpful. Not a big deal, as that merge should happen\nbefore release. But you may want to just apply on top of my 0be240c\n(checkout: tweak detached-orphan warning format, 2011-03-20).\n\n> @@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)\n> \t\tdie(\"internal error in revision walk\");\n> \tif (!(commit->object.flags & UNINTERESTING))\n> \t\tsuggest_reattach(commit, &revs);\n> -\telse\n> -\t\tdescribe_detached_head(\"Previous HEAD position was\", commit);\n> }\n\nWait, I thought we were keeping this, per your argument above?\n\n-Peff\n"},{"id":"168600","messageId":"7vipsz7pwq.fsf@alter.siamese.dyndns.org","threadId":"27285","inReplyTo":"20110524202723.GG584@sigill.intra.peff.net","subject":"Re: [PATCH] checkout: honor advice.detachedHead when reattaching to a branch","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-05-24T21:24:37Z","receivedAt":"2011-05-24T21:24:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> @@ -668,8 +671,6 @@ static void orphaned_commit_warning(struct commit *commit)\n>> \t\tdie(\"internal error in revision walk\");\n>> \tif (!(commit->object.flags & UNINTERESTING))\n>> \t\tsuggest_reattach(commit, &revs);\n>> -\telse\n>> -\t\tdescribe_detached_head(\"Previous HEAD position was\", commit);\n>> }\n>\n> Wait, I thought we were keeping this, per your argument above?\n\nWell, I do not have very strong preference either way. I didn't really\nargue for or against. I just explained it might be useful.\n\nWill resurrect.\n"}]}