[PATCH 1/2] advice: preset advice preferences to -1

[PATCH 1/2] advice: preset advice preferences to -1

[Stefan Beller]

In the next patch we want to make a distinction if a the advice was
requested explicitly or is set implicit. To do that we need to have
different values for the default and a value that is set explicitly.

Signed-off-by: Stefan Beller <sbeller@google.com>
---
 advice.c | 30 +++++++++++++++---------------
 1 file changed, 15 insertions(+), 15 deletions(-)

diff --git a/advice.c b/advice.c
index b84ae49..56b70b6 100644
--- a/advice.c
+++ b/advice.c
@@ -1,20 +1,20 @@
 #include "cache.h"
 
-int advice_push_update_rejected = 1;
-int advice_push_non_ff_current = 1;
-int advice_push_non_ff_matching = 1;
-int advice_push_already_exists = 1;
-int advice_push_fetch_first = 1;
-int advice_push_needs_force = 1;
-int advice_status_hints = 1;
-int advice_status_u_option = 1;
-int advice_commit_before_merge = 1;
-int advice_resolve_conflict = 1;
-int advice_implicit_identity = 1;
-int advice_detached_head = 1;
-int advice_set_upstream_failure = 1;
-int advice_object_name_warning = 1;
-int advice_rm_hints = 1;
+int advice_push_update_rejected = -1;
+int advice_push_non_ff_current = -1;
+int advice_push_non_ff_matching = -1;
+int advice_push_already_exists = -1;
+int advice_push_fetch_first = -1;
+int advice_push_needs_force = -1;
+int advice_status_hints = -1;
+int advice_status_u_option = -1;
+int advice_commit_before_merge = -1;
+int advice_resolve_conflict = -1;
+int advice_implicit_identity = -1;
+int advice_detached_head = -1;
+int advice_set_upstream_failure = -1;
+int advice_object_name_warning = -1;
+int advice_rm_hints = -1;
 
 static struct {
 	const char *name;
-- 
2.9.2.730.g525ad04.dirty

[PATCH 2/2] 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>
---

 Junio writes:
 > It might be controversial how the second from the last case should
 > behave, though.
 
 I agree. I think if the advice is configured explicitly we can still give it.
 That makes the code a bit more complicated though.
 
 Also note I added stderr to stdout redirections as suggested by Peff.
 
 Thanks,
 Stefan
 
 builtin/checkout.c         |  4 +++-
 t/t2020-checkout-detach.sh | 28 ++++++++++++++++++++++++++++
 2 files changed, 31 insertions(+), 1 deletion(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 4866111..6196b40 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -658,7 +658,9 @@ 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 == 1 ||
+			     (advice_detached_head == -1 && !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..fe311a1 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -163,4 +163,32 @@ 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' '
+	# baseline
+	test_config advice.detachedHead true &&
+	git checkout child && git checkout HEAD^0 >expect.advice 2>&1 &&
+	test_config advice.detachedHead false &&
+	git checkout child && git checkout HEAD^0 >expect.no-advice 2>&1 &&
+	test_unconfig advice.detachedHead &&
+	# without configuration, the advice.* variables default to true
+	git checkout child && git checkout HEAD^0 >actual 2>&1 &&
+	test_cmp expect.advice actual &&
+
+	# with explicit --detach
+	# no configuration
+	test_unconfig advice.detachedHead &&
+	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+	test_cmp expect.no-advice actual &&
+
+	# explicitly ask advice
+	test_config advice.detachedHead true &&
+	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+	test_cmp expect.advice actual &&
+
+	# explicitly decline advice
+	test_config advice.detachedHead false &&
+	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
+	test_cmp expect.no-advice actual
+'
+
 test_done
-- 
2.9.2.730.g525ad04.dirty

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 11:40:21AM -0700, Stefan Beller wrote:

> 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>
> ---
> 
>  Junio writes:
>  > It might be controversial how the second from the last case should
>  > behave, though.
>  
>  I agree. I think if the advice is configured explicitly we can still give it.
>  That makes the code a bit more complicated though.

So....I guess. But has anybody in the history of git ever explicitly
configured advice.* to true?

It has never produced any change of behavior, and the whole point of
"advice.*" was that git would advise by default, and you would use
advice.* to shut it up once you were sufficiently educated.

I don't think doing it this way is _wrong_. It just feels sort of
pointlessly over-engineered. It's also a little weird that all of the:

  if (advice_foo)

will trigger because "advice_foo" is set to -1. I think it does the
right thing, but it feels like a bug (the value is now a tri-state, and
we silently collapse two states into one).

-Peff

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

On Mon, Aug 15, 2016 at 11:47 AM, Jeff King <peff@peff.net> wrote:
> On Mon, Aug 15, 2016 at 11:40:21AM -0700, Stefan Beller wrote:
>
>> 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>
>> ---
>>
>>  Junio writes:
>>  > It might be controversial how the second from the last case should
>>  > behave, though.
>>
>>  I agree. I think if the advice is configured explicitly we can still give it.
>>  That makes the code a bit more complicated though.
>
> So....I guess. But has anybody in the history of git ever explicitly
> configured advice.* to true?

An admin/teacher of a university that wants to teach Git to students
in a class? Better be safe than sorry and explicitly ask for advice because...
we cannot trust the default?

>
> It has never produced any change of behavior, and the whole point of
> "advice.*" was that git would advise by default, and you would use
> advice.* to shut it up once you were sufficiently educated.

And now I am arguing that "by default" we should not give advice 100%
of the time, but only when we think it is appropriate. You may disagree
(as a teacher see above), so you can slightly change the setting to give
out advice more often again?

>
> I don't think doing it this way is _wrong_. It just feels sort of
> pointlessly over-engineered. It's also a little weird that all of the:
>
>   if (advice_foo)
>
> will trigger because "advice_foo" is set to -1. I think it does the
> right thing, but it feels like a bug (the value is now a tri-state, and
> we silently collapse two states into one).

I think this is what I did in some of the submodule code as well, which
is why I assumed it's ok (or rather the projects groupthink on how to do
"default on but still different than explicit on")

If you think this is wrong, what is the idiomatic way to solve this problem?

Thanks,
Stefan


>
> -Peff

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 11:54:53AM -0700, Stefan Beller wrote:

> > So....I guess. But has anybody in the history of git ever explicitly
> > configured advice.* to true?
> 
> An admin/teacher of a university that wants to teach Git to students
> in a class? Better be safe than sorry and explicitly ask for advice because...
> we cannot trust the default?
> 
> >
> > It has never produced any change of behavior, and the whole point of
> > "advice.*" was that git would advise by default, and you would use
> > advice.* to shut it up once you were sufficiently educated.
> 
> And now I am arguing that "by default" we should not give advice 100%
> of the time, but only when we think it is appropriate. You may disagree
> (as a teacher see above), so you can slightly change the setting to give
> out advice more often again?

I don't think it's quite the same thing. It is fine not to bother
advising because the advice is not really applicable, and that is what
is happening here. We do not need to lecture the user on something they
explicitly asked for.

But that is different than "by default, in situations where we have
useful advice to give, give it".

I guess you are indicating that somebody may disagree on "applicable"
here. Which I suppose is possible, but it seems like a bit of a made-up
hypothetical.


I had also thought at first you were arguing from the position of "let's
handle advice.detachedHEAD=true in case somebody has set it". That seems
even more silly, because almost certainly nobody _has_ set it. But if
your position is "let's make it do something useful in case somebody
wants to set it", then...I still think it's silly, but at least there is
room for debate. :)

> > I don't think doing it this way is _wrong_. It just feels sort of
> > pointlessly over-engineered. It's also a little weird that all of the:
> >
> >   if (advice_foo)
> >
> > will trigger because "advice_foo" is set to -1. I think it does the
> > right thing, but it feels like a bug (the value is now a tri-state, and
> > we silently collapse two states into one).
> 
> I think this is what I did in some of the submodule code as well, which
> is why I assumed it's ok (or rather the projects groupthink on how to do
> "default on but still different than explicit on")
> 
> If you think this is wrong, what is the idiomatic way to solve this problem?

I don't think it's wrong (didn't I say so :) ).

It's just that idiomatic use of a tri-state like this is generally
something like:

  1. Set the option to -1 for "not specified"

  2. Fill in the values as 0/1 if the user asks for it.

  3. Canonicalize any remaining unspecified value to 0/1, depending on
     the default. Usually this happens after we know all setup is done,
     but sometimes is done lazily by an accessor function.

  3. Check the canonical value with "if (option)", or "if (option())" if
     using a lazy accessor.

And we have fixed many bugs in the past where some non-canonical value
slipped past step 3, and did the wrong thing in step 4.

Here it's OK because "if (option)" means that "unspecified" collapses to
"true", and that is the default for each of these options. It's just
that it's hard to distinguish from the buggy case.

I suppose the more idiomatic way would be:

  static int advice_wanted(int x)
  {
	if (advice_values[x] < 0)
		advice_values[x] = 1; /* all advice defaults to on */
	return advice_values[x];
  }

  ...
  if (advice_wanted(ADVICE_DETACHED_HEAD))
	...

but perhaps it is not worth that amount of boilerplate (but maybe at
least a comment explaining the situation). You'd also need to check with
your patch that each user of the advice variables is checking just for a
non-zero value, and not explicitly for "1" (which is almost certainly
the case, but it needs to be considered).

-Peff

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I don't think doing it this way is _wrong_. It just feels sort of
> pointlessly over-engineered. It's also a little weird that all of the:
>
>   if (advice_foo)
>
> will trigger because "advice_foo" is set to -1. I think it does the
> right thing, but it feels like a bug (the value is now a tri-state, and
> we silently collapse two states into one).

Guilty as charged.  I do agree that this is over-engineered.

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> I don't think doing it this way is _wrong_. It just feels sort of
>> pointlessly over-engineered. It's also a little weird that all of the:
>>
>>   if (advice_foo)
>>
>> will trigger because "advice_foo" is set to -1. I think it does the
>> right thing, but it feels like a bug (the value is now a tri-state, and
>> we silently collapse two states into one).
>
> Guilty as charged.  I do agree that this is over-engineered.

Let's discard 1/2 and amend 2/2 with this incremental.



 builtin/checkout.c         | 3 +--
 t/t2020-checkout-detach.sh | 5 -----
 2 files changed, 1 insertion(+), 7 deletions(-)

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2a32b5f..337c35a 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
 			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
 		if (!opts->quiet) {
 			if (old->path &&
-			    (advice_detached_head == 1 ||
-			     (advice_detached_head == -1 && !opts->force_detach)))
+			    advice_detached_head == 1 && !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 fe311a1..fbb4ee9 100755
--- a/t/t2020-checkout-detach.sh
+++ b/t/t2020-checkout-detach.sh
@@ -180,11 +180,6 @@ test_expect_success 'no advice given for explicit detached head state' '
 	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
 	test_cmp expect.no-advice actual &&
 
-	# explicitly ask advice
-	test_config advice.detachedHead true &&
-	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
-	test_cmp expect.advice actual &&
-
 	# explicitly decline advice
 	test_config advice.detachedHead false &&
 	git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Jeff King]

On Mon, Aug 15, 2016 at 02:10:36PM -0700, Junio C Hamano wrote:

> > Guilty as charged.  I do agree that this is over-engineered.
> 
> Let's discard 1/2 and amend 2/2 with this incremental.

Looks good (though I really am OK the other way if people feel
strongly).

> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2a32b5f..337c35a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
>  		if (!opts->quiet) {
>  			if (old->path &&
> -			    (advice_detached_head == 1 ||
> -			     (advice_detached_head == -1 && !opts->force_detach)))
> +			    advice_detached_head == 1 && !opts->force_detach)

Maybe s/== 1//?

That's more idiomatic, though maybe you were future-proofing in case we
turn it into a tri-state later. :)

-Peff

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Stefan Beller]

On Mon, Aug 15, 2016 at 2:10 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Junio C Hamano <gitster@pobox.com> writes:
>
>> Jeff King <peff@peff.net> writes:
>>
>>> I don't think doing it this way is _wrong_. It just feels sort of
>>> pointlessly over-engineered. It's also a little weird that all of the:
>>>
>>>   if (advice_foo)
>>>
>>> will trigger because "advice_foo" is set to -1. I think it does the
>>> right thing, but it feels like a bug (the value is now a tri-state, and
>>> we silently collapse two states into one).
>>
>> Guilty as charged.  I do agree that this is over-engineered.
>
> Let's discard 1/2 and amend 2/2 with this incremental.

That is fine with me.

Thanks,
Stefan

>
>
>
>  builtin/checkout.c         | 3 +--
>  t/t2020-checkout-detach.sh | 5 -----
>  2 files changed, 1 insertion(+), 7 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 2a32b5f..337c35a 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -656,8 +656,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>                            REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
>                 if (!opts->quiet) {
>                         if (old->path &&
> -                           (advice_detached_head == 1 ||
> -                            (advice_detached_head == -1 && !opts->force_detach)))
> +                           advice_detached_head == 1 && !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 fe311a1..fbb4ee9 100755
> --- a/t/t2020-checkout-detach.sh
> +++ b/t/t2020-checkout-detach.sh
> @@ -180,11 +180,6 @@ test_expect_success 'no advice given for explicit detached head state' '
>         git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
>         test_cmp expect.no-advice actual &&
>
> -       # explicitly ask advice
> -       test_config advice.detachedHead true &&
> -       git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&
> -       test_cmp expect.advice actual &&
> -
>         # explicitly decline advice
>         test_config advice.detachedHead false &&
>         git checkout child && git checkout --detach HEAD^0 >actual 2>&1 &&

Re: [PATCH 2/2] checkout: do not mention detach advice for explicit --detach option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

>>  			   REF_NODEREF, UPDATE_REFS_DIE_ON_ERR);
>>  		if (!opts->quiet) {
>>  			if (old->path &&
>> -			    (advice_detached_head == 1 ||
>> -			     (advice_detached_head == -1 && !opts->force_detach)))
>> +			    advice_detached_head == 1 && !opts->force_detach)
>
> Maybe s/== 1//?

Surely, and thanks.