[PATCHv3] checkout: do not mention detach advice for explicit --detach option

[PATCHv3] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

When a user asked for a detached HEAD specifically with `--detach`,
we do not need to give advice on what a detached HEAD state entails as
we can assume they know what they're getting into as they asked for it.

Signed-off-by: Stefan Beller <sbeller@google.com>
---

Thanks for the review!

> Is there a reason for not unsetting `advice.detachedHead` at the
> end of the test?

done

I did not consider to clean up after myself... what a selfish world!

Stefan

 builtin/checkout.c         |  2 +-
 t/t2020-checkout-detach.sh | 14 ++++++++++++++
 2 files changed, 15 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 8d852d4..85408b1 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -658,7 +658,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 		update_ref(msg.buf, "HEAD", new->commit->object.oid.hash, NULL,
 			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
-			if (old->path && advice_detached_head)
+			if (old->path && advice_detached_head && !opts->force_detach)
 				detach_advice(new->name);
 			describe_detached_head(_("HEAD is now at"), new->commit);
 		}
diff --git a/t/t2020-checkout-detach.sh b/t/t2020-checkout-detach.sh
index 5d68729..3ee60fb 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -163,4 +163,18 @@ test_expect_success 'tracking count is accurate after orphan check' '
 	test_i18ncmp expect stdout
 '
 
+test_expect_success 'no advice given for explicit detached head state' '
+	git config advice.detachedHead false &&
+	git checkout child &&
+	git checkout --detach HEAD >expect &&
+	git config advice.detachedHead true &&
+	git checkout child &&
+	git checkout --detach HEAD >actual &&
+	test_cmp expect actual &&
+	git checkout child &&
+	git checkout HEAD >actual &&
+	! test_cmp expect actual &&
+	git config --unset advice.detachedHead
+'
+
 test_done
-- 
2.9.2.730.g46b112d

Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Fri, Aug 12, 2016 at 08:37:44AM -0700, Stefan Beller wrote:

> > Is there a reason for not unsetting `advice.detachedHead` at the
> > end of the test?
> 
> done
> 
> I did not consider to clean up after myself... what a selfish world!

The right way to do it is:

  test_config advice.detachedHead false &&
  ...

so that the cleanup runs whether or not you may it to the end of the
script.

> +test_expect_success 'no advice given for explicit detached head state' '
> +	git config advice.detachedHead false &&
> +	git checkout child &&
> +	git checkout --detach HEAD >expect &&
> +	git config advice.detachedHead true &&
> +	git checkout child &&
> +	git checkout --detach HEAD >actual &&
> +	test_cmp expect actual &&
> +	git checkout child &&
> +	git checkout HEAD >actual &&
> +	! test_cmp expect actual &&
> +	git config --unset advice.detachedHead
> +'

Hmm. Using "!test_cmp" seems weird to me, just because it would falsely
claim success if something else unexpected changes. Our usual method for
making sure some particular output does not appear is "test_i18ngrep"
with a liberal pattern.

-Peff

Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

On Fri, Aug 12, 2016 at 8:43 AM, Jeff King <peff@peff.net> wrote:
> On Fri, Aug 12, 2016 at 08:37:44AM -0700, Stefan Beller wrote:
>
>> > Is there a reason for not unsetting `advice.detachedHead` at the
>> > end of the test?
>>
>> done
>>
>> I did not consider to clean up after myself... what a selfish world!
>
> The right way to do it is:
>
>   test_config advice.detachedHead false &&

okay.

>   ...
>
> so that the cleanup runs whether or not you may it to the end of the
> script.
>
>> +test_expect_success 'no advice given for explicit detached head state' '
>> +     git config advice.detachedHead false &&
>> +     git checkout child &&
>> +     git checkout --detach HEAD >expect &&
>> +     git config advice.detachedHead true &&
>> +     git checkout child &&
>> +     git checkout --detach HEAD >actual &&
>> +     test_cmp expect actual &&
>> +     git checkout child &&
>> +     git checkout HEAD >actual &&
>> +     ! test_cmp expect actual &&
>> +     git config --unset advice.detachedHead
>> +'
>
> Hmm. Using "!test_cmp" seems weird to me, just because it would falsely
> claim success if something else unexpected changes. Our usual method for
> making sure some particular output does not appear is "test_i18ngrep"
> with a liberal pattern.

and I advanced the liberal a bit more. ;)
So maybe we'd be looking that no 'detached HEAD' occurs?

    test_i18ngrep ! "'detached HEAD'" actual

I think testing that we do not give out advice (i.e. behave the same as if
not giving out advice) is best tested to just compare the output to
the output of "git -c advice.detachedHead=false ...", which is what I do here.
This seems to be future proof to me no matter how we reword the advice or
the actual message on checkout?

Thanks,
Stefan

>
> -Peff

Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Stefan Beller <sbeller@google.com> writes:

> When a user asked for a detached HEAD specifically with `--detach`,
> we do not need to give advice on what a detached HEAD state entails as
> we can assume they know what they're getting into as they asked for it.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>
> Thanks for the review!
>
>> Is there a reason for not unsetting `advice.detachedHead` at the
>> end of the test?
>
> done
>
> I did not consider to clean up after myself... what a selfish world!

;-)

Using test_config instead of "git config" would help.

> +test_expect_success 'no advice given for explicit detached head state' '
> +	git config advice.detachedHead false &&
> +	git checkout child &&
> +	git checkout --detach HEAD >expect &&
> +	git config advice.detachedHead true &&
> +	git checkout child &&
> +	git checkout --detach HEAD >actual &&
> +	test_cmp expect actual &&

The above is meant to see explicit "--detach" gives the same message
whether advice.detachedHead is set to true or false, which is good.
It however does not make sure these messages do not caution about
detaching.

> +	git checkout child &&
> +	git checkout HEAD >actual &&

This goes to "child" branch and then does a no-op "checkout HEAD"
that does not even detach.

> +	! test_cmp expect actual &&

And then it compares the message from a no-op "checkout" and
the previously obtained message from a detaching "checkout".

Another thing that this is not checking is that a detaching
"checkout" without an explicit "--detach" still gives the advice
when advice.detachedHead is set to true (or left to default, for
that matter).

Perhaps you would want to do it a bit differently.  How does this
look?

    # baseline
    test_config advice.detachedHead true &&
    git checkout child && git checkout HEAD^0 >expect.advice &&
    test_config advice.detachedHead false &&
    git checkout child && git checkout HEAD^0 >expect.no-advice &&
    test_unconfig advice.detachedHead &&
    # without configuration, the advice.* variables default to true
    git checkout child && git checkout HEAD^0 >actual &&
    test_cmp expect.advice actual &&

    # with explicit --detach
    # no configuration
    test_unconfig advice.detachedHead &&
    git checkout child && git checkout --detach HEAD >actual &&
    test_cmp expect.no-advice actual &&

    # explicitly ask advice
    test_config advice.detachedHead true &&
    git checkout child && git checkout --detach HEAD >actual &&
    test_cmp expect.no-advice actual &&

    # explicitly decline advice
    test_config advice.detachedHead false &&
    git checkout child && git checkout --detach HEAD >actual &&
    test_cmp expect.no-advice actual

It might be controversial how the second from the last case should
behave, though.

Re: [PATCHv3] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Fri, Aug 12, 2016 at 08:51:43AM -0700, Stefan Beller wrote:

> > Hmm. Using "!test_cmp" seems weird to me, just because it would falsely
> > claim success if something else unexpected changes. Our usual method for
> > making sure some particular output does not appear is "test_i18ngrep"
> > with a liberal pattern.
> 
> and I advanced the liberal a bit more. ;)
> So maybe we'd be looking that no 'detached HEAD' occurs?
> 
>     test_i18ngrep ! "'detached HEAD'" actual

I was thinking maybe "advice:" to look for any advice, but at this point
we are guessing about what might change in the future.

> I think testing that we do not give out advice (i.e. behave the same as if
> not giving out advice) is best tested to just compare the output to
> the output of "git -c advice.detachedHead=false ...", which is what I do here.
> This seems to be future proof to me no matter how we reword the advice or
> the actual message on checkout?

Yeah, I guess that is reasonable. I thought at first you were comparing
more distinct commands, but if only the one thing is changing, that
seems reasonable.

You may also want to grab stderr, too (I'm actually surprised advice
goes to stdout and not stderr, and that is something that seems
plausible to change in the future).

-Peff