threads / discuss / 43795

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

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

## tl;dr

5 messages between Aug 12, 2016 and Aug 12, 2016.

replies: 4people: 3as markdown or json

Stefan Beller· Aug 12, 2016, 15:37 UTC · lore

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
Jeff King· Aug 12, 2016, 15:43 UTC · re: Stefan Beller · lore

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

On Fri, Aug 12, 2016 at 08:37:44AM -0700, Stefan Beller wrote:
Show 6 quoted lines
> > 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.

Show 13 quoted lines
> +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
Stefan Beller· Aug 12, 2016, 15:51 UTC · re: Jeff King · lore

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

On Fri, Aug 12, 2016 at 8:43 AM, Jeff King <peff@peff.net> wrote:
Show 12 quoted lines
> 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.
Show 23 quoted lines
>   ...
>
> 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
Jeff King· Aug 12, 2016, 16:28 UTC · re: Stefan Beller · lore

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

On Fri, Aug 12, 2016 at 08:51:43AM -0700, Stefan Beller wrote:
Show 9 quoted lines
> > 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.

Show 5 quoted lines
> 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
Junio C Hamano· Aug 12, 2016, 16:12 UTC · re: Stefan Beller · lore

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

Stefan Beller <sbeller@google.com> writes:
Show 15 quoted lines
> 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.
Show 8 quoted lines
> +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.

← back to recent threads