{"thread":{"id":"43795","subject":"[PATCHv3] checkout: do not mention detach advice for explicit --detach option","startedAt":"2016-08-12T15:38:00Z","lastAt":"2016-08-12T16:28:43Z","messageCount":5,"participants":["Stefan Beller","Jeff King","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"298842","messageId":"20160812153744.15045-1-sbeller@google.com","threadId":"43795","inReplyTo":null,"subject":"[PATCHv3] checkout: do not mention detach advice for explicit --detach option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-12T15:37:44Z","receivedAt":"2016-08-12T15:38:00Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a user asked for a detached HEAD specifically with `--detach`,\nwe do not need to give advice on what a detached HEAD state entails as\nwe can assume they know what they're getting into as they asked for it.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n\nThanks for the review!\n\n> Is there a reason for not unsetting `advice.detachedHead` at the\n> end of the test?\n\ndone\n\nI did not consider to clean up after myself... what a selfish world!\n\nStefan\n\n builtin/checkout.c         |  2 +-\n t/t2020-checkout-detach.sh | 14 ++++++++++++++\n 2 files changed, 15 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 8d852d4..85408b1 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -658,7 +658,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\tupdate_ref(msg.buf, \"HEAD\", new->commit->object.oid.hash, NULL,\n \t\t\t   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);\n \t\tif (!opts->quiet) {\n-\t\t\tif (old->path && advice_detached_head)\n+\t\t\tif (old->path && advice_detached_head && !opts->force_detach)\n \t\t\t\tdetach_advice(new->name);\n \t\t\tdescribe_detached_head(_(\"HEAD is now at\"), new->commit);\n \t\t}\ndiff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh\nindex 5d68729..3ee60fb 100755\n--- a/t/t2020-checkout-detach.sh\n+++ b/t/t2020-checkout-detach.sh\n@@ -163,4 +163,18 @@ test_expect_success 'tracking count is accurate after orphan check' '\n \ttest_i18ncmp expect stdout\n '\n \n+test_expect_success 'no advice given for explicit detached head state' '\n+\tgit config advice.detachedHead false &&\n+\tgit checkout child &&\n+\tgit checkout --detach HEAD >expect &&\n+\tgit config advice.detachedHead true &&\n+\tgit checkout child &&\n+\tgit checkout --detach HEAD >actual &&\n+\ttest_cmp expect actual &&\n+\tgit checkout child &&\n+\tgit checkout HEAD >actual &&\n+\t! test_cmp expect actual &&\n+\tgit config --unset advice.detachedHead\n+'\n+\n test_done\n-- \n2.9.2.730.g46b112d\n"},{"id":"298844","messageId":"20160812154331.y2z6pv2w6cwsdsqw@sigill.intra.peff.net","threadId":"43795","inReplyTo":"20160812153744.15045-1-sbeller@google.com","subject":"Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-12T15:43:31Z","receivedAt":"2016-08-12T15:45:01Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 12, 2016 at 08:37:44AM -0700, Stefan Beller wrote:\n\n> > Is there a reason for not unsetting `advice.detachedHead` at the\n> > end of the test?\n> \n> done\n> \n> I did not consider to clean up after myself... what a selfish world!\n\nThe right way to do it is:\n\n  test_config advice.detachedHead false &&\n  ...\n\nso that the cleanup runs whether or not you may it to the end of the\nscript.\n\n> +test_expect_success 'no advice given for explicit detached head state' '\n> +\tgit config advice.detachedHead false &&\n> +\tgit checkout child &&\n> +\tgit checkout --detach HEAD >expect &&\n> +\tgit config advice.detachedHead true &&\n> +\tgit checkout child &&\n> +\tgit checkout --detach HEAD >actual &&\n> +\ttest_cmp expect actual &&\n> +\tgit checkout child &&\n> +\tgit checkout HEAD >actual &&\n> +\t! test_cmp expect actual &&\n> +\tgit config --unset advice.detachedHead\n> +'\n\nHmm. Using \"!test_cmp\" seems weird to me, just because it would falsely\nclaim success if something else unexpected changes. Our usual method for\nmaking sure some particular output does not appear is \"test_i18ngrep\"\nwith a liberal pattern.\n\n-Peff\n"},{"id":"298856","messageId":"CAGZ79kaOHdLrPownWdnZQSEzG6P6-iZqx2xu3MEG51DZU1U+Cg@mail.gmail.com","threadId":"43795","inReplyTo":"20160812154331.y2z6pv2w6cwsdsqw@sigill.intra.peff.net","subject":"Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-08-12T15:51:43Z","receivedAt":"2016-08-12T15:51:57Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Aug 12, 2016 at 8:43 AM, Jeff King <peff@peff.net> wrote:\n> On Fri, Aug 12, 2016 at 08:37:44AM -0700, Stefan Beller wrote:\n>\n>> > Is there a reason for not unsetting `advice.detachedHead` at the\n>> > end of the test?\n>>\n>> done\n>>\n>> I did not consider to clean up after myself... what a selfish world!\n>\n> The right way to do it is:\n>\n>   test_config advice.detachedHead false &&\n\nokay.\n\n>   ...\n>\n> so that the cleanup runs whether or not you may it to the end of the\n> script.\n>\n>> +test_expect_success 'no advice given for explicit detached head state' '\n>> +     git config advice.detachedHead false &&\n>> +     git checkout child &&\n>> +     git checkout --detach HEAD >expect &&\n>> +     git config advice.detachedHead true &&\n>> +     git checkout child &&\n>> +     git checkout --detach HEAD >actual &&\n>> +     test_cmp expect actual &&\n>> +     git checkout child &&\n>> +     git checkout HEAD >actual &&\n>> +     ! test_cmp expect actual &&\n>> +     git config --unset advice.detachedHead\n>> +'\n>\n> Hmm. Using \"!test_cmp\" seems weird to me, just because it would falsely\n> claim success if something else unexpected changes. Our usual method for\n> making sure some particular output does not appear is \"test_i18ngrep\"\n> with a liberal pattern.\n\nand I advanced the liberal a bit more. ;)\nSo maybe we'd be looking that no 'detached HEAD' occurs?\n\n    test_i18ngrep ! \"'detached HEAD'\" actual\n\nI think testing that we do not give out advice (i.e. behave the same as if\nnot giving out advice) is best tested to just compare the output to\nthe output of \"git -c advice.detachedHead=false ...\", which is what I do here.\nThis seems to be future proof to me no matter how we reword the advice or\nthe actual message on checkout?\n\nThanks,\nStefan\n\n>\n> -Peff\n"},{"id":"298859","messageId":"xmqqmvkix8yd.fsf@gitster.mtv.corp.google.com","threadId":"43795","inReplyTo":"20160812153744.15045-1-sbeller@google.com","subject":"Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-08-12T16:12:26Z","receivedAt":"2016-08-12T16:12:34Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> When a user asked for a detached HEAD specifically with `--detach`,\n> we do not need to give advice on what a detached HEAD state entails as\n> we can assume they know what they're getting into as they asked for it.\n>\n> Signed-off-by: Stefan Beller <sbeller@google.com>\n> ---\n>\n> Thanks for the review!\n>\n>> Is there a reason for not unsetting `advice.detachedHead` at the\n>> end of the test?\n>\n> done\n>\n> I did not consider to clean up after myself... what a selfish world!\n\n;-)\n\nUsing test_config instead of \"git config\" would help.\n\n> +test_expect_success 'no advice given for explicit detached head state' '\n> +\tgit config advice.detachedHead false &&\n> +\tgit checkout child &&\n> +\tgit checkout --detach HEAD >expect &&\n> +\tgit config advice.detachedHead true &&\n> +\tgit checkout child &&\n> +\tgit checkout --detach HEAD >actual &&\n> +\ttest_cmp expect actual &&\n\nThe above is meant to see explicit \"--detach\" gives the same message\nwhether advice.detachedHead is set to true or false, which is good.\nIt however does not make sure these messages do not caution about\ndetaching.\n\n> +\tgit checkout child &&\n> +\tgit checkout HEAD >actual &&\n\nThis goes to \"child\" branch and then does a no-op \"checkout HEAD\"\nthat does not even detach.\n\n> +\t! test_cmp expect actual &&\n\nAnd then it compares the message from a no-op \"checkout\" and\nthe previously obtained message from a detaching \"checkout\".\n\nAnother thing that this is not checking is that a detaching\n\"checkout\" without an explicit \"--detach\" still gives the advice\nwhen advice.detachedHead is set to true (or left to default, for\nthat matter).\n\nPerhaps you would want to do it a bit differently.  How does this\nlook?\n\n    # baseline\n    test_config advice.detachedHead true &&\n    git checkout child && git checkout HEAD^0 >expect.advice &&\n    test_config advice.detachedHead false &&\n    git checkout child && git checkout HEAD^0 >expect.no-advice &&\n    test_unconfig advice.detachedHead &&\n    # without configuration, the advice.* variables default to true\n    git checkout child && git checkout HEAD^0 >actual &&\n    test_cmp expect.advice actual &&\n\n    # with explicit --detach\n    # no configuration\n    test_unconfig advice.detachedHead &&\n    git checkout child && git checkout --detach HEAD >actual &&\n    test_cmp expect.no-advice actual &&\n\n    # explicitly ask advice\n    test_config advice.detachedHead true &&\n    git checkout child && git checkout --detach HEAD >actual &&\n    test_cmp expect.no-advice actual &&\n\n    # explicitly decline advice\n    test_config advice.detachedHead false &&\n    git checkout child && git checkout --detach HEAD >actual &&\n    test_cmp expect.no-advice actual\n\nIt might be controversial how the second from the last case should\nbehave, though.\n"},{"id":"298862","messageId":"20160812162825.omk23srrbx6ofizc@sigill.intra.peff.net","threadId":"43795","inReplyTo":"CAGZ79kaOHdLrPownWdnZQSEzG6P6-iZqx2xu3MEG51DZU1U+Cg@mail.gmail.com","subject":"Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-08-12T16:28:25Z","receivedAt":"2016-08-12T16:28:43Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 12, 2016 at 08:51:43AM -0700, Stefan Beller wrote:\n\n> > Hmm. Using \"!test_cmp\" seems weird to me, just because it would falsely\n> > claim success if something else unexpected changes. Our usual method for\n> > making sure some particular output does not appear is \"test_i18ngrep\"\n> > with a liberal pattern.\n> \n> and I advanced the liberal a bit more. ;)\n> So maybe we'd be looking that no 'detached HEAD' occurs?\n> \n>     test_i18ngrep ! \"'detached HEAD'\" actual\n\nI was thinking maybe \"advice:\" to look for any advice, but at this point\nwe are guessing about what might change in the future.\n\n> I think testing that we do not give out advice (i.e. behave the same as if\n> not giving out advice) is best tested to just compare the output to\n> the output of \"git -c advice.detachedHead=false ...\", which is what I do here.\n> This seems to be future proof to me no matter how we reword the advice or\n> the actual message on checkout?\n\nYeah, I guess that is reasonable. I thought at first you were comparing\nmore distinct commands, but if only the one thing is changing, that\nseems reasonable.\n\nYou may also want to grab stderr, too (I'm actually surprised advice\ngoes to stdout and not stderr, and that is something that seems\nplausible to change in the future).\n\n-Peff\n"}]}