[PATCH 0/2] advice: add "all" option to disable all hints

[PATCH 0/2] advice: add "all" option to disable all hints

[James Liu]

Hello,

This patch series adds an "all" advice hint type that can be used as a
convenience option for disabling all advice hints. This is useful in a
server context where advice hints won't be seen by a human, and hints
that change over time may cause test failures.

This value should only be set to "false", at which point all hints will
be disabled. Individual hints can then be enabled by setting their
respective types to "true".

The series also modifies the `advise` test tool so it's able to test the
normal and special case branches in the advice_enabled() function, and
adds a few more test cases to verify behaviour.

Cheers,
James

James Liu (2):
  advice: allow advice type to be provided in tests
  advice: add "all" option to disable all hints

 Documentation/config/advice.txt |  5 +++
 advice.c                        |  8 +++++
 advice.h                        |  1 +
 t/helper/test-advise.c          | 20 +++++++----
 t/t0018-advice.sh               | 63 +++++++++++++++++++++++++++++++--
 5 files changed, 88 insertions(+), 9 deletions(-)

-- 
2.44.0

[PATCH 1/2] advice: allow advice type to be provided in tests

[James Liu]

advise_if_enabled() has a special branch to handle
backwards-compatibility with the `pushUpdateRejected` and
`pushNonFastForward` advice types, which went untested.

Modify the `test-tool advise` command so the advice type can be changed
between nestedTag (the previous behaviour) and pushUpdateRejected.

Signed-off-by: James Liu <james@jamesliu.io>
---
 t/helper/test-advise.c | 20 ++++++++++++++------
 t/t0018-advice.sh      | 30 +++++++++++++++++++++++++++---
 2 files changed, 41 insertions(+), 9 deletions(-)

diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
index 8a3fd0009a..c18b18e059 100644
--- a/t/helper/test-advise.c
+++ b/t/helper/test-advise.c
@@ -5,18 +5,26 @@
 
 int cmd__advise_if_enabled(int argc, const char **argv)
 {
-	if (argc != 2)
-		die("usage: %s <advice>", argv[0]);
+	if (argc != 3)
+		die("usage: %s nestedTag|pushUpdateRejected <advice>", argv[0]);
 
 	setup_git_directory();
 	git_config(git_default_config, NULL);
 
 	/*
-	 * Any advice type can be used for testing, but NESTED_TAG was
-	 * selected here and in t0018 where this command is being
-	 * executed.
+	 * Any advice type can be used for testing, but ADVICE_NESTED_TAG and
+	 * ADVICE_PUSH_UPDATE_REJECTED were selected here and used in t0018
+	 * where this command is being executed.
+	 *
+	 * This allows test cases to exercise the normal and special branch
+	 * within advice_enabled().
 	 */
-	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
+	if (!strcmp(argv[1], "nestedTag"))
+		advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[2]);
+	else if (!strcmp(argv[1], "pushUpdateRejected"))
+		advise_if_enabled(ADVICE_PUSH_UPDATE_REJECTED, "%s", argv[2]);
+	else
+		die("advice type should be nestedTag|pushUpdateRejected");
 
 	return 0;
 }
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 0dcfb760a2..8010796e1f 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -10,7 +10,16 @@ test_expect_success 'advice should be printed when config variable is unset' '
 	hint: This is a piece of advice
 	hint: Disable this message with "git config advice.nestedTag false"
 	EOF
-	test-tool advise "This is a piece of advice" 2>actual &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'advice should be printed when advice.pushUpdateRejected and its alias are unset' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	hint: Disable this message with "git config advice.pushUpdateRejected false"
+	EOF
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
 	test_cmp expect actual
 '
 
@@ -19,14 +28,29 @@ test_expect_success 'advice should be printed when config variable is set to tru
 	hint: This is a piece of advice
 	EOF
 	test_config advice.nestedTag true &&
-	test-tool advise "This is a piece of advice" 2>actual &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
 	test_cmp expect actual
 '
 
 test_expect_success 'advice should not be printed when config variable is set to false' '
 	test_config advice.nestedTag false &&
-	test-tool advise "This is a piece of advice" 2>actual &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
 	test_must_be_empty actual
 '
 
+test_expect_success 'advice should not be printed when advice.pushUpdateRejected is unset but its alias is set to false' '
+	test_config advice.pushNonFastForward false &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'advice should be printed when advice.pushUpdateRejected is set to true and its alias is unset' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test_config advice.pushUpdateRejected true &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH 1/2] advice: allow advice type to be provided in tests

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2703 bytes --]

On Wed, Apr 24, 2024 at 01:58:56PM +1000, James Liu wrote:
> advise_if_enabled() has a special branch to handle
> backwards-compatibility with the `pushUpdateRejected` and
> `pushNonFastForward` advice types, which went untested.
> 
> Modify the `test-tool advise` command so the advice type can be changed
> between nestedTag (the previous behaviour) and pushUpdateRejected.
> 
> Signed-off-by: James Liu <james@jamesliu.io>
> ---
>  t/helper/test-advise.c | 20 ++++++++++++++------
>  t/t0018-advice.sh      | 30 +++++++++++++++++++++++++++---
>  2 files changed, 41 insertions(+), 9 deletions(-)
> 
> diff --git a/t/helper/test-advise.c b/t/helper/test-advise.c
> index 8a3fd0009a..c18b18e059 100644
> --- a/t/helper/test-advise.c
> +++ b/t/helper/test-advise.c
> @@ -5,18 +5,26 @@
>  
>  int cmd__advise_if_enabled(int argc, const char **argv)
>  {
> -	if (argc != 2)
> -		die("usage: %s <advice>", argv[0]);
> +	if (argc != 3)
> +		die("usage: %s nestedTag|pushUpdateRejected <advice>", argv[0]);

We could retain the old behaviour here so that we don't have to update
all tests. So in case `argc == 2` we implicitly use `ADVICE_NESTED_TAG`,
if `argc == 3` we look up the advice passed by the caller. The usage
would thus essentially become something like this:

    usage: test-advise <msg> [<key>]

> -	if (argc != 2)
> -		die("usage: %s <advice>", argv[0]);
> +	if (argc != 3)
> +		die("usage: %s nestedTag|pushUpdateRejected <advice>", argv[0]);

>  	setup_git_directory();
>  	git_config(git_default_config, NULL);
>  
>  	/*
> -	 * Any advice type can be used for testing, but NESTED_TAG was
> -	 * selected here and in t0018 where this command is being
> -	 * executed.
> +	 * Any advice type can be used for testing, but ADVICE_NESTED_TAG and
> +	 * ADVICE_PUSH_UPDATE_REJECTED were selected here and used in t0018
> +	 * where this command is being executed.
> +	 *
> +	 * This allows test cases to exercise the normal and special branch
> +	 * within advice_enabled().
>  	 */
> -	advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[1]);
> +	if (!strcmp(argv[1], "nestedTag"))
> +		advise_if_enabled(ADVICE_NESTED_TAG, "%s", argv[2]);
> +	else if (!strcmp(argv[1], "pushUpdateRejected"))
> +		advise_if_enabled(ADVICE_PUSH_UPDATE_REJECTED, "%s", argv[2]);
> +	else
> +		die("advice type should be nestedTag|pushUpdateRejected");

Instead of singling out these specific advices, can we maybe expose the
`advice_setting[]` array and make this generic? We could for example add
a new `lookup_advice_by_name()` function that you pass the advice key,
which then walks through the array to look up the `enum advice_type`.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

[PATCH 2/2] advice: add "all" option to disable all hints

[James Liu]

Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
usages of Git where hints aren't necessary, it can be cumbersome to
maintain configuration to disable all advice hints. This is especially
the case if/when new advice hints are added.

Add a new "all" advice variable which acts as a toggle for all advice
types. When this is being used, individual advice hints can be enabled
by setting their respective configs to true.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/config/advice.txt |  5 +++++
 advice.c                        |  8 ++++++++
 advice.h                        |  1 +
 t/t0018-advice.sh               | 33 +++++++++++++++++++++++++++++++++
 4 files changed, 47 insertions(+)

diff --git a/Documentation/config/advice.txt b/Documentation/config/advice.txt
index 0e35ae5240..0516a23b6b 100644
--- a/Documentation/config/advice.txt
+++ b/Documentation/config/advice.txt
@@ -5,6 +5,11 @@ advice.*::
 	that you do not need the help message by setting these to `false`:
 +
 --
+	all::
+		Convenience option that allows all advice hints to be
+		disabled when set to false. When false, individual
+		advice hints can be enabled by setting them to true.
+		Setting this to true is a no-op.
 	addEmbeddedRepo::
 		Shown when the user accidentally adds one
 		git repo inside of another.
diff --git a/advice.c b/advice.c
index 75111191ad..9f0860f143 100644
--- a/advice.c
+++ b/advice.c
@@ -43,6 +43,7 @@ static struct {
 	const char *key;
 	enum advice_level level;
 } advice_setting[] = {
+	[ADVICE_ALL]					= { "all" },
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
@@ -132,6 +133,13 @@ int advice_enabled(enum advice_type type)
 		return enabled &&
 		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
 
+	/*
+	 * We still allow for specific advice hints to be enabled if
+	 * advice.all == false.
+	 */
+	if (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)
+		return advice_setting[type].level == ADVICE_LEVEL_ENABLED;
+
 	return enabled;
 }
 
diff --git a/advice.h b/advice.h
index c8d29f97f3..b5ac99a645 100644
--- a/advice.h
+++ b/advice.h
@@ -11,6 +11,7 @@ struct string_list;
  * Call advise_if_enabled to print your advice.
  */
 enum advice_type {
+	ADVICE_ALL,
 	ADVICE_ADD_EMBEDDED_REPO,
 	ADVICE_ADD_EMPTY_PATHSPEC,
 	ADVICE_ADD_IGNORED_FILE,
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 8010796e1f..19318cc9bb 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -53,4 +53,37 @@ test_expect_success 'advice should be printed when advice.pushUpdateRejected is
 	test_cmp expect actual
 '
 
+test_expect_success 'advice should not be printed when advice.all is set to false' '
+	test_config advice.all false &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '
+	test_config advice.all false &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_must_be_empty actual
+'
+
+test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test_config advice.all false &&
+	test_config advice.nestedTag true &&
+	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '
+	cat >expect <<-\EOF &&
+	hint: This is a piece of advice
+	EOF
+	test_config advice.all false &&
+	test_config advice.pushUpdateRejected true &&
+	test_config advice.pushNonFastForward true &&
+	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH 2/2] advice: add "all" option to disable all hints

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3207 bytes --]

On Wed, Apr 24, 2024 at 01:58:57PM +1000, James Liu wrote:
[snip]
> --- a/advice.c
> +++ b/advice.c
> @@ -43,6 +43,7 @@ static struct {
>  	const char *key;
>  	enum advice_level level;
>  } advice_setting[] = {
> +	[ADVICE_ALL]					= { "all" },
>  	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
>  	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
>  	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
> @@ -132,6 +133,13 @@ int advice_enabled(enum advice_type type)
>  		return enabled &&
>  		       advice_enabled(ADVICE_PUSH_UPDATE_REJECTED_ALIAS);
>  
> +	/*
> +	 * We still allow for specific advice hints to be enabled if
> +	 * advice.all == false.
> +	 */
> +	if (advice_setting[ADVICE_ALL].level == ADVICE_LEVEL_DISABLED)
> +		return advice_setting[type].level == ADVICE_LEVEL_ENABLED;

Makes sense. When the advice is unset we don't need to handle it at all,
and if it is enabled we basically want to behave the same as before.

>  	return enabled;
>  }
>  
> diff --git a/advice.h b/advice.h
> index c8d29f97f3..b5ac99a645 100644
> --- a/advice.h
> +++ b/advice.h
> @@ -11,6 +11,7 @@ struct string_list;
>   * Call advise_if_enabled to print your advice.
>   */
>  enum advice_type {
> +	ADVICE_ALL,
>  	ADVICE_ADD_EMBEDDED_REPO,
>  	ADVICE_ADD_EMPTY_PATHSPEC,
>  	ADVICE_ADD_IGNORED_FILE,
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index 8010796e1f..19318cc9bb 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -53,4 +53,37 @@ test_expect_success 'advice should be printed when advice.pushUpdateRejected is
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'advice should not be printed when advice.all is set to false' '
> +	test_config advice.all false &&
> +	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_expect_success 'advice should not be printed for pushUpdateRejected when advice.all is set to false' '
> +	test_config advice.all false &&
> +	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
> +	test_must_be_empty actual
> +'
> +
> +test_expect_success 'advice should be printed when advice.all is set to false, but specific advice is set to true' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test_config advice.all false &&
> +	test_config advice.nestedTag true &&
> +	test-tool advise nestedTag "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'
> +
> +test_expect_success 'advice should be printed when advice.all is set to false, but advice.pushUpdateRejected and its alias are set to true' '
> +	cat >expect <<-\EOF &&
> +	hint: This is a piece of advice
> +	EOF
> +	test_config advice.all false &&
> +	test_config advice.pushUpdateRejected true &&
> +	test_config advice.pushNonFastForward true &&
> +	test-tool advise pushUpdateRejected "This is a piece of advice" 2>actual &&
> +	test_cmp expect actual
> +'
>  test_done

Do we actually have to enable both advices here? If not, then this
should probably be two separate tests that check whether each of the
keys does its thing.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Junio C Hamano]

James Liu <james@jamesliu.io> writes:

> This patch series adds an "all" advice hint type that can be used as a
> convenience option for disabling all advice hints.

Hmph.  I thought this was rejected already and not in so distant
past, but I am not finding a discussion thread in the archive.

The design to support the advice.* variables to let individual ones
to be squelched, without allowing "all", is very much deliberate.

A user may think they are familiar with all the advices already, but
with "all", they'll never get a chance to learn new ones added after
they set the configuration.  Looking from the other direction, a new
advice message is a way for us to tell our users something important
for them to know.  For example, we may plan to improve a high-level
Porcelain command so that it will eventually error out when given an
input that used to be accepted but behaved in a way that newbies
felt confusing.  In the first step of such a transition, we will
introduce a new (and hopefully better) way to achieve what the user
wants to do, and add an advice message to tell the user, when they
trigger the codepath whose behaviour will change in the future, in
the old way that will eventually go away.

Do not close that communication channel on us.

Thanks.

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2963 bytes --]

On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:
> James Liu <james@jamesliu.io> writes:
> 
> > This patch series adds an "all" advice hint type that can be used as a
> > convenience option for disabling all advice hints.
> 
> Hmph.  I thought this was rejected already and not in so distant
> past, but I am not finding a discussion thread in the archive.
> 
> The design to support the advice.* variables to let individual ones
> to be squelched, without allowing "all", is very much deliberate.
> 
> A user may think they are familiar with all the advices already, but
> with "all", they'll never get a chance to learn new ones added after
> they set the configuration.  Looking from the other direction, a new
> advice message is a way for us to tell our users something important
> for them to know.  For example, we may plan to improve a high-level
> Porcelain command so that it will eventually error out when given an
> input that used to be accepted but behaved in a way that newbies
> felt confusing.  In the first step of such a transition, we will
> introduce a new (and hopefully better) way to achieve what the user
> wants to do, and add an advice message to tell the user, when they
> trigger the codepath whose behaviour will change in the future, in
> the old way that will eventually go away.
> 
> Do not close that communication channel on us.

While I agree that it might not be a good idea to set it for our users,
the usecase mentioned by this patch series is scripting. And here I very
much agree with the sentiment that it makes sense to give an easy knob
to disable all advice (disclosure: James is part of the Gitaly team at
GitLab, and that is where this feature comes from, so I am very much
biased).

It has happened multiple times to us that new advices were introduced
that then subsequently caused regressions in Gitaly because the output
of Git commands now looks different. While addressing such breakage is
easy enough to do, it does add up if we have to run Git with every
single advice that we may hit turned off individually.

Now one could say that we shouldn't execute porcelain tools in our
scripts because it is kind of expected that their output may change at
any point in time, and that is certainly true. But the reality is that
there aren't always good plumbing alternatives available.

Furthermore, we are often forced to parse fragile error messages from
such porcelain tools because Git doesn't provide a better way to dissect
errors. Error codes are mostly meaningless and there is no other data
channel available to us than the error message.

These are problems that run deeper than "We want to disable advices",
and we eventually want to address those over time. But it's a long road
to get there, and meanwhile disabling all advice would be something that
helps us make our scripted uses of Git at least a tiny bit more stable.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Phillip Wood]

Hi Patrick

On 24/04/2024 07:48, Patrick Steinhardt wrote:
> On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:
>> Do not close that communication channel on us.
> 
> While I agree that it might not be a good idea to set it for our users,
> the usecase mentioned by this patch series is scripting. And here I very
> much agree with the sentiment that it makes sense to give an easy knob
> to disable all advice (disclosure: James is part of the Gitaly team at
> GitLab, and that is where this feature comes from, so I am very much
> biased).

Maybe an environment variable would be a better fit for turning advice 
off in scripts?

> It has happened multiple times to us that new advices were introduced
> that then subsequently caused regressions in Gitaly because the output
> of Git commands now looks different. While addressing such breakage is
> easy enough to do, it does add up if we have to run Git with every
> single advice that we may hit turned off individually.

I'm sure you've considered these suggestions but (a) would it be 
possible for Gitaly to filter out lines beginning with "hint: " when it 
captures the output of commands and (b) would it be possible to have a 
script that parses advice_setting in advice.c to find the list of advice 
names so Gitaly can disable them? I think (a) would still leave some 
advice output from code that uses advice_enabled() rather than 
advise_if_enabled() but it should get rid of most of the advice messages.

Best Wishes

Phillip

> Now one could say that we shouldn't execute porcelain tools in our
> scripts because it is kind of expected that their output may change at
> any point in time, and that is certainly true. But the reality is that
> there aren't always good plumbing alternatives available.
> 
> Furthermore, we are often forced to parse fragile error messages from
> such porcelain tools because Git doesn't provide a better way to dissect
> errors. Error codes are mostly meaningless and there is no other data
> channel available to us than the error message.
> 
> These are problems that run deeper than "We want to disable advices",
> and we eventually want to address those over time. But it's a long road
> to get there, and meanwhile disabling all advice would be something that
> helps us make our scripted uses of Git at least a tiny bit more stable.
> 
> Patrick

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 2141 bytes --]

On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:
> Hi Patrick
> 
> On 24/04/2024 07:48, Patrick Steinhardt wrote:
> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:
> > > Do not close that communication channel on us.
> > 
> > While I agree that it might not be a good idea to set it for our users,
> > the usecase mentioned by this patch series is scripting. And here I very
> > much agree with the sentiment that it makes sense to give an easy knob
> > to disable all advice (disclosure: James is part of the Gitaly team at
> > GitLab, and that is where this feature comes from, so I am very much
> > biased).
> 
> Maybe an environment variable would be a better fit for turning advice off
> in scripts?

Sure, an environment variable would work just fine for our purposes. It
would probably also address the concern that users may disable all
advice and then miss out on some information.

> > It has happened multiple times to us that new advices were introduced
> > that then subsequently caused regressions in Gitaly because the output
> > of Git commands now looks different. While addressing such breakage is
> > easy enough to do, it does add up if we have to run Git with every
> > single advice that we may hit turned off individually.
> 
> I'm sure you've considered these suggestions but (a) would it be possible
> for Gitaly to filter out lines beginning with "hint: " when it captures the
> output of commands and (b) would it be possible to have a script that parses
> advice_setting in advice.c to find the list of advice names so Gitaly can
> disable them? I think (a) would still leave some advice output from code
> that uses advice_enabled() rather than advise_if_enabled() but it should get
> rid of most of the advice messages.

Filtering out advices would be doable. But we probably wouldn't want to
do so unconditionally whenever we execute Git commands. Which would
bring us back to the point where we have to address these new advices
whenever they come up, and it wouldn't be much less verbose than to just
disable advices we face.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

> Filtering out advices would be doable. But we probably wouldn't want to
> do so unconditionally whenever we execute Git commands.

Can you elaborate?  Would you only sometimes show advice messages
that come from "git" you invoke?  Based on what criteria would it
decide whether to filter or not to?  Is it purely per location in
your program (i.e., "every time the control flow reaches this call
to an equivalent of run_command(), we would filter the "hint:"
messages")?

In an invocation where you would not filter, what effect does users'
setting of advice.* configuration variables have, and what effect
does a new and unseen kind of advice messages have?

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1188 bytes --]

On Wed, Apr 24, 2024 at 07:59:16AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> > Filtering out advices would be doable. But we probably wouldn't want to
> > do so unconditionally whenever we execute Git commands.
> 
> Can you elaborate?  Would you only sometimes show advice messages
> that come from "git" you invoke?  Based on what criteria would it
> decide whether to filter or not to?  Is it purely per location in
> your program (i.e., "every time the control flow reaches this call
> to an equivalent of run_command(), we would filter the "hint:"
> messages")?
> 
> In an invocation where you would not filter, what effect does users'
> setting of advice.* configuration variables have, and what effect
> does a new and unseen kind of advice messages have?

The reason here is mostly that I do not know whether this can be rigged.
I cannot state without a doubt that no command may output "hint:" at the
start of a line that may _not_ actually be an advice. Think for example
(and I know this example is dumb because advice goes to stderr, not
stdout) git-cat-file(1) with a blob that contains a line saying "hint:".

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> > Filtering out advices would be doable. But we probably wouldn't want to
>> > do so unconditionally whenever we execute Git commands.
>> ...
> The reason here is mostly that I do not know whether this can be rigged.
> I cannot state without a doubt that no command may output "hint:" at the
> start of a line that may _not_ actually be an advice.

Ah, I mistook what you said to be "we may want to keep the advice
messages in certain situations while filtering out others".  

Surely, we may send stuff to the standard error stream that happens
to begin with "hint:" that is not created via advice.c:vadvise().

I do not however offhand know why anybody would want to filter such
lines out when they are filtering out all advice messages.  When we
emit a line that begins with "hint:", I do not think of a case when
we do not mean as a "hint", i.e., in the same spirit as real advice
messages.

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Dragan Simic]

Hello all,

On 2024-04-24 16:07, Patrick Steinhardt wrote:
> On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:
>> On 24/04/2024 07:48, Patrick Steinhardt wrote:
>> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:
>> > > Do not close that communication channel on us.
>> >
>> > While I agree that it might not be a good idea to set it for our users,
>> > the usecase mentioned by this patch series is scripting. And here I very
>> > much agree with the sentiment that it makes sense to give an easy knob
>> > to disable all advice (disclosure: James is part of the Gitaly team at
>> > GitLab, and that is where this feature comes from, so I am very much
>> > biased).
>> 
>> Maybe an environment variable would be a better fit for turning advice 
>> off
>> in scripts?
> 
> Sure, an environment variable would work just fine for our purposes. It
> would probably also address the concern that users may disable all
> advice and then miss out on some information.

Sounds good to me.  I'd support the addition of a new environment
variable for this purpose, perhaps GIT_NO_ADVICE or similar.

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Dragan Simic]

On 2024-04-24 18:14, Dragan Simic wrote:
> On 2024-04-24 16:07, Patrick Steinhardt wrote:
>> On Wed, Apr 24, 2024 at 02:52:39PM +0100, Phillip Wood wrote:
>>> On 24/04/2024 07:48, Patrick Steinhardt wrote:
>>> > On Tue, Apr 23, 2024 at 11:28:03PM -0700, Junio C Hamano wrote:
>>> > > Do not close that communication channel on us.
>>> >
>>> > While I agree that it might not be a good idea to set it for our users,
>>> > the usecase mentioned by this patch series is scripting. And here I very
>>> > much agree with the sentiment that it makes sense to give an easy knob
>>> > to disable all advice (disclosure: James is part of the Gitaly team at
>>> > GitLab, and that is where this feature comes from, so I am very much
>>> > biased).
>>> 
>>> Maybe an environment variable would be a better fit for turning 
>>> advice off
>>> in scripts?
>> 
>> Sure, an environment variable would work just fine for our purposes. 
>> It
>> would probably also address the concern that users may disable all
>> advice and then miss out on some information.
> 
> Sounds good to me.  I'd support the addition of a new environment
> variable for this purpose, perhaps GIT_NO_ADVICE or similar.

Actually, after reading Junio's comment, [1] I'd rather support
a new command-line option.  This is quite similar to disabling
the pager, so a new command-line option would IMHO fit rather well
into the grand scheme.

[1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Junio C Hamano]

Patrick Steinhardt <ps@pks.im> writes:

>> Do not close that communication channel on us.
>
> While I agree that it might not be a good idea to set it for our users,
> the usecase mentioned by this patch series is scripting. And here I very
> much agree with the sentiment that it makes sense to give an easy knob
> to disable all advice (disclosure: James is part of the Gitaly team at
> GitLab, and that is where this feature comes from, so I am very much
> biased).

Of course, as you mention, the kosher way is to use the plumbing
where there is no advice messages.  If certain functionalitly is
lacking in the set of plumbing commands, adding them would benefit
everybody, not just Gitaly.

If this is for scripting, make it easy for scripts to use while
making it very very hard for users to trigger in their interactive
environments.  A configuration variable, by design, is a very bad
choice to do so, as it is "set it once and forget about it", which
defeats the whole purpose of advice messages.  An environment
variable is very much in the same category in that you can set it in
~/.login or ~/.profile and forget about it.

A "git" wide command line option, similar to "--no-pager" or
"--literal-pathspecs", is probably an acceptable avenue.

Thanks.

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 1476 bytes --]

On Wed, Apr 24, 2024 at 07:31:04AM -0700, Junio C Hamano wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> 
> >> Do not close that communication channel on us.
> >
> > While I agree that it might not be a good idea to set it for our users,
> > the usecase mentioned by this patch series is scripting. And here I very
> > much agree with the sentiment that it makes sense to give an easy knob
> > to disable all advice (disclosure: James is part of the Gitaly team at
> > GitLab, and that is where this feature comes from, so I am very much
> > biased).
> 
> Of course, as you mention, the kosher way is to use the plumbing
> where there is no advice messages.  If certain functionalitly is
> lacking in the set of plumbing commands, adding them would benefit
> everybody, not just Gitaly.
> 
> If this is for scripting, make it easy for scripts to use while
> making it very very hard for users to trigger in their interactive
> environments.  A configuration variable, by design, is a very bad
> choice to do so, as it is "set it once and forget about it", which
> defeats the whole purpose of advice messages.  An environment
> variable is very much in the same category in that you can set it in
> ~/.login or ~/.profile and forget about it.
> 
> A "git" wide command line option, similar to "--no-pager" or
> "--literal-pathspecs", is probably an acceptable avenue.

Fair enough. So adding a global config like `--no-advice` it is.

Patrick

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH 0/2] advice: add "all" option to disable all hints

[Dragan Simic]

On 2024-04-24 08:28, Junio C Hamano wrote:
> James Liu <james@jamesliu.io> writes:
> 
>> This patch series adds an "all" advice hint type that can be used as a
>> convenience option for disabling all advice hints.
> 
> Hmph.  I thought this was rejected already and not in so distant
> past, but I am not finding a discussion thread in the archive.
> 
> The design to support the advice.* variables to let individual ones
> to be squelched, without allowing "all", is very much deliberate.
> 
> A user may think they are familiar with all the advices already, but
> with "all", they'll never get a chance to learn new ones added after
> they set the configuration.  Looking from the other direction, a new
> advice message is a way for us to tell our users something important
> for them to know.  For example, we may plan to improve a high-level
> Porcelain command so that it will eventually error out when given an
> input that used to be accepted but behaved in a way that newbies
> felt confusing.  In the first step of such a transition, we will
> introduce a new (and hopefully better) way to achieve what the user
> wants to do, and add an advice message to tell the user, when they
> trigger the codepath whose behaviour will change in the future, in
> the old way that will eventually go away.

I'd agree with Junio.  Having "advice.all" is like a chainsaw, very
useful, but also quite dangerous if not used properly.

[PATCH v2 0/1] advice: add --no-advice global option

[James Liu]

Hi,

This is v2 of the patch series to provide a mechanism for silencing all
advice hints. Based on feedback, the change now adds a `--no-advice` CLI
option which works independently of the individual hint toggles.

James Liu (1):
  advice: add --no-advice global option

 Documentation/git.txt |  5 ++++-
 advice.c              |  8 +++++++-
 environment.h         |  1 +
 git.c                 |  6 +++++-
 t/t0018-advice.sh     | 20 ++++++++++++++++++++
 5 files changed, 37 insertions(+), 3 deletions(-)

-- 
2.44.0

[PATCH v2 1/1] advice: add --no-advice global option

[James Liu]

Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
and scripted usages of Git where hints aren't necessary, it can be
cumbersome to maintain configuration to ensure all advice hints are
disabled in perpetuity. This is a particular concern in tests, where
new or changed hints can result in failed assertions.

Add a --no-advice global option to disable all advice hints from being
displayed. This is independent of the toggles for individual advice
hints.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/git.txt |  5 ++++-
 advice.c              |  8 +++++++-
 environment.h         |  1 +
 git.c                 |  6 +++++-
 t/t0018-advice.sh     | 20 ++++++++++++++++++++
 5 files changed, 37 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1b112a3e..ef1d9dce5d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,7 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--config-env=<name>=<envvar>] <command> [<args>]
+    [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]
 
 DESCRIPTION
 -----------
@@ -226,6 +226,9 @@ If you just want to run git as if it was started in `<path>` then use
 	linkgit:gitattributes[5]. This is equivalent to setting the
 	`GIT_ATTR_SOURCE` environment variable.
 
+--no-advice::
+	Disable all advice hints from being printed.
+
 GIT COMMANDS
 ------------
 
diff --git a/advice.c b/advice.c
index 75111191ad..f6282c3bde 100644
--- a/advice.c
+++ b/advice.c
@@ -2,6 +2,7 @@
 #include "advice.h"
 #include "config.h"
 #include "color.h"
+#include "environment.h"
 #include "gettext.h"
 #include "help.h"
 #include "string-list.h"
@@ -126,7 +127,12 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+	int enabled;
+
+	if (getenv(GIT_NO_ADVICE))
+		return 0;
+
+	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
 
 	if (type == ADVICE_PUSH_UPDATE_REJECTED)
 		return enabled &&
diff --git a/environment.h b/environment.h
index 05fd94d7be..30c2684269 100644
--- a/environment.h
+++ b/environment.h
@@ -56,6 +56,7 @@ const char *getenv_safe(struct strvec *argv, const char *name);
 #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
 #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
 #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
+#define GIT_NO_ADVICE "GIT_NO_ADVICE"
 
 /*
  * Environment variable used in handshaking the wire protocol.
diff --git a/git.c b/git.c
index 654d615a18..ffeb832ca9 100644
--- a/git.c
+++ b/git.c
@@ -38,7 +38,7 @@ const char git_usage_string[] =
 	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
 	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
 	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
+	   "           [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]");
 
 const char git_more_info_string[] =
 	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
@@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--no-advice")) {
+			setenv(GIT_NO_ADVICE, "1", 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else {
 			fprintf(stderr, _("unknown option: %s\n"), cmd);
 			usage(git_usage_string);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 0dcfb760a2..2ce2d4668a 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to
 	test_must_be_empty actual
 '
 
+test_expect_success 'advice should not be printed when --no-advice is used' '
+	cat << EOF > expect &&
+On branch master
+
+No commits yet
+
+Untracked files:
+	README
+
+nothing added to commit but untracked files present
+EOF
+
+	git init advice-test &&
+  test_when_finished "rm -fr advice-test" &&
+  cd advice-test &&
+  touch README &&
+  git --no-advice status > ../actual &&
+  test_cmp ../expect ../actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

Hello James,

Please see my comments below.

On 2024-04-29 03:09, James Liu wrote:
> Advice hints must be disabled individually by setting the relevant
> advice.* variables to false in the Git configuration. For server-side
> and scripted usages of Git where hints aren't necessary, it can be
> cumbersome to maintain configuration to ensure all advice hints are
> disabled in perpetuity. This is a particular concern in tests, where
> new or changed hints can result in failed assertions.
> 
> Add a --no-advice global option to disable all advice hints from being
> displayed. This is independent of the toggles for individual advice
> hints.
> 
> Signed-off-by: James Liu <james@jamesliu.io>
> ---
>  Documentation/git.txt |  5 ++++-
>  advice.c              |  8 +++++++-
>  environment.h         |  1 +
>  git.c                 |  6 +++++-
>  t/t0018-advice.sh     | 20 ++++++++++++++++++++
>  5 files changed, 37 insertions(+), 3 deletions(-)
> 
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 7a1b112a3e..ef1d9dce5d 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
>      [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
>      [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
> -    [--config-env=<name>=<envvar>] <command> [<args>]
> +    [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]

After having a more detailed look at the Git documentation,
I think that adding support for "--advice" at the same time
would be the right thing to do.  That option would override
all "advice.<hint>" options that may exist in the configuration
(and would override the GIT_NO_ADVICE environment variable,
if we end up supporting it), similarly to how "--paginate"
overrides all "pager.<cmd>" in the configuration, which would
be both consistent and rather useful in some situations.

>  DESCRIPTION
>  -----------
> @@ -226,6 +226,9 @@ If you just want to run git as if it was started
> in `<path>` then use
>  	linkgit:gitattributes[5]. This is equivalent to setting the
>  	`GIT_ATTR_SOURCE` environment variable.
> 
> +--no-advice::
> +	Disable all advice hints from being printed.
> +
>  GIT COMMANDS
>  ------------
> 
> diff --git a/advice.c b/advice.c
> index 75111191ad..f6282c3bde 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -2,6 +2,7 @@
>  #include "advice.h"
>  #include "config.h"
>  #include "color.h"
> +#include "environment.h"
>  #include "gettext.h"
>  #include "help.h"
>  #include "string-list.h"
> @@ -126,7 +127,12 @@ void advise(const char *advice, ...)
> 
>  int advice_enabled(enum advice_type type)
>  {
> -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> +	int enabled;
> +
> +	if (getenv(GIT_NO_ADVICE))
> +		return 0;

Huh, I was under impression that having an environment
variable to control this behavior was frowned upon by
Junio? [1]  To me, supporting such a variable would be
a somewhat acceptable risk, [2] but of course it's the
maintainer's opinion that matters most.

[1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
[2] 
https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/

> +
> +	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> 
>  	if (type == ADVICE_PUSH_UPDATE_REJECTED)
>  		return enabled &&
> diff --git a/environment.h b/environment.h
> index 05fd94d7be..30c2684269 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -56,6 +56,7 @@ const char *getenv_safe(struct strvec *argv, const
> char *name);
>  #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
>  #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
>  #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
> +#define GIT_NO_ADVICE "GIT_NO_ADVICE"

If we eventually end up supporting new "GIT_NO_ADVICE"
environment variable, which I actually doubt, the new
macro above should be named "GIT_NO_ADVICE_ENVIRONMENT"
instead, for consistency.

> 
>  /*
>   * Environment variable used in handshaking the wire protocol.
> diff --git a/git.c b/git.c
> index 654d615a18..ffeb832ca9 100644
> --- a/git.c
> +++ b/git.c
> @@ -38,7 +38,7 @@ const char git_usage_string[] =
>  	   "           [--exec-path[=<path>]] [--html-path] [--man-path]
> [--info-path]\n"
>  	   "           [-p | --paginate | -P | --no-pager]
> [--no-replace-objects] [--bare]\n"
>  	   "           [--git-dir=<path>] [--work-tree=<path>] 
> [--namespace=<name>]\n"
> -	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
> +	   "           [--config-env=<name>=<envvar>] [--no-advice]

Obviously, additional new configuration option "--advice"
would be also added here, mutually exclusive with "--no-advice",
and into the source code below.

> <command> [<args>]");
> 
>  const char git_more_info_string[] =
>  	N_("'git help -a' and 'git help -g' list available subcommands and 
> some\n"
> @@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int
> *argc, int *envchanged)
>  			setenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--no-advice")) {
> +			setenv(GIT_NO_ADVICE, "1", 1);
> +			if (envchanged)
> +				*envchanged = 1;
>  		} else {
>  			fprintf(stderr, _("unknown option: %s\n"), cmd);
>  			usage(git_usage_string);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index 0dcfb760a2..2ce2d4668a 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed
> when config variable is set to
>  	test_must_be_empty actual
>  '
> 
> +test_expect_success 'advice should not be printed when --no-advice is 
> used' '
> +	cat << EOF > expect &&
> +On branch master
> +
> +No commits yet
> +
> +Untracked files:
> +	README
> +
> +nothing added to commit but untracked files present
> +EOF
> +
> +	git init advice-test &&
> +  test_when_finished "rm -fr advice-test" &&
> +  cd advice-test &&
> +  touch README &&
> +  git --no-advice status > ../actual &&
> +  test_cmp ../expect ../actual
> +'
> +
>  test_done

There should also be a new test that checks how the new
"GIT_NO_ADVICE" environment variable affects the execution,
but I doubt it will eventually be supported.

Of course, an additional test should be added to check the
mutual exclusivity between the above-proposed "--advice"
and "--no-advice" options.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[James Liu]

Thanks for the feedback Dragan!

> After having a more detailed look at the Git documentation,
> I think that adding support for "--advice" at the same time
> would be the right thing to do.  That option would override
> all "advice.<hint>" options that may exist in the configuration
> (and would override the GIT_NO_ADVICE environment variable,
> if we end up supporting it), similarly to how "--paginate"
> overrides all "pager.<cmd>" in the configuration, which would
> be both consistent and rather useful in some situations.

I think this makes sense from a UX consistenty perspective, but I'm not
sure if we should increase the scope of this patch. It's also much
easier to re-enable previously silenced advice hints, so I'm unsure
whether an --advice option makes sense. We can also avoid the decision
of what to do if the user supplies --advice and --no-advice
simultaneously if we only have one option.

> > diff --git a/advice.c b/advice.c
> > index 75111191ad..f6282c3bde 100644
> > --- a/advice.c
> > +++ b/advice.c
> > @@ -2,6 +2,7 @@
> >  #include "advice.h"
> >  #include "config.h"
> >  #include "color.h"
> > +#include "environment.h"
> >  #include "gettext.h"
> >  #include "help.h"
> >  #include "string-list.h"
> > @@ -126,7 +127,12 @@ void advise(const char *advice, ...)
> > 
> >  int advice_enabled(enum advice_type type)
> >  {
> > -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> > +	int enabled;
> > +
> > +	if (getenv(GIT_NO_ADVICE))
> > +		return 0;
>
> Huh, I was under impression that having an environment
> variable to control this behavior was frowned upon by
> Junio? [1]  To me, supporting such a variable would be
> a somewhat acceptable risk, [2] but of course it's the
> maintainer's opinion that matters most.
>
> [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
> [2] 
> https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/

You're correct. I saw this pattern for a few of the other global CLI
options and followed it. I'm unsure what the best alternative for
passing this configuration down to the `advice_enabled()` function is.
I'd appreciate some guidance here.

Cheers,
James

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

On 2024-04-29 07:01, James Liu wrote:
> Thanks for the feedback Dragan!

Thank you for working on the patch.

>> After having a more detailed look at the Git documentation,
>> I think that adding support for "--advice" at the same time
>> would be the right thing to do.  That option would override
>> all "advice.<hint>" options that may exist in the configuration
>> (and would override the GIT_NO_ADVICE environment variable,
>> if we end up supporting it), similarly to how "--paginate"
>> overrides all "pager.<cmd>" in the configuration, which would
>> be both consistent and rather useful in some situations.
> 
> I think this makes sense from a UX consistenty perspective, but I'm not
> sure if we should increase the scope of this patch. It's also much
> easier to re-enable previously silenced advice hints, so I'm unsure
> whether an --advice option makes sense. We can also avoid the decision
> of what to do if the user supplies --advice and --no-advice
> simultaneously if we only have one option.

Regarding what to do if those two options are both supplied,
it's simple, just error out with an appropriate error message.
There are already similar situations in the code, e.g. with
the -k and --rfc options for git-format-patch(1).

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

On 2024-04-29 07:36, Dragan Simic wrote:
> On 2024-04-29 07:01, James Liu wrote:
>> Thanks for the feedback Dragan!
> 
> Thank you for working on the patch.
> 
>>> After having a more detailed look at the Git documentation,
>>> I think that adding support for "--advice" at the same time
>>> would be the right thing to do.  That option would override
>>> all "advice.<hint>" options that may exist in the configuration
>>> (and would override the GIT_NO_ADVICE environment variable,
>>> if we end up supporting it), similarly to how "--paginate"
>>> overrides all "pager.<cmd>" in the configuration, which would
>>> be both consistent and rather useful in some situations.
>> 
>> I think this makes sense from a UX consistenty perspective, but I'm 
>> not
>> sure if we should increase the scope of this patch. It's also much
>> easier to re-enable previously silenced advice hints, so I'm unsure
>> whether an --advice option makes sense. We can also avoid the decision
>> of what to do if the user supplies --advice and --no-advice
>> simultaneously if we only have one option.
> 
> Regarding what to do if those two options are both supplied,
> it's simple, just error out with an appropriate error message.
> There are already similar situations in the code, e.g. with
> the -k and --rfc options for git-format-patch(1).

Actually, the -p/--paginate and -P/--no-pager options can
currently be supplied together, which isn't the expected
behavior.  I'm preparing a patch that will cover this as
a case of the mutual option exclusivity.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Eric Sunshine]

On Mon, Apr 29, 2024 at 2:00 AM Dragan Simic <dsimic@manjaro.org> wrote:
> On 2024-04-29 07:36, Dragan Simic wrote:
> > Regarding what to do if those two options are both supplied,
> > it's simple, just error out with an appropriate error message.
> > There are already similar situations in the code, e.g. with
> > the -k and --rfc options for git-format-patch(1).
>
> Actually, the -p/--paginate and -P/--no-pager options can
> currently be supplied together, which isn't the expected
> behavior.  I'm preparing a patch that will cover this as
> a case of the mutual option exclusivity.

Please don't.

"Last wins" is an intentionally-supported mode of operation for many
Git options. It allows you to override an option which may have been
supplied by some other entity/mechanism. For instance, you may have a
Git alias, say `git mylog`, which employs --paginate, but you want to
avoid pagination on a particular run, so you invoke it as `git mylog
--no-pager`.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

Hello Eric,

On 2024-04-29 08:04, Eric Sunshine wrote:
> On Mon, Apr 29, 2024 at 2:00 AM Dragan Simic <dsimic@manjaro.org> 
> wrote:
>> On 2024-04-29 07:36, Dragan Simic wrote:
>> > Regarding what to do if those two options are both supplied,
>> > it's simple, just error out with an appropriate error message.
>> > There are already similar situations in the code, e.g. with
>> > the -k and --rfc options for git-format-patch(1).
>> 
>> Actually, the -p/--paginate and -P/--no-pager options can
>> currently be supplied together, which isn't the expected
>> behavior.  I'm preparing a patch that will cover this as
>> a case of the mutual option exclusivity.
> 
> Please don't.
> 
> "Last wins" is an intentionally-supported mode of operation for many
> Git options. It allows you to override an option which may have been
> supplied by some other entity/mechanism. For instance, you may have a
> Git alias, say `git mylog`, which employs --paginate, but you want to
> avoid pagination on a particular run, so you invoke it as `git mylog
> --no-pager`.

Ah, I see, thanks for pointing this out.  Makes perfect sense.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Jeff King]

On Mon, Apr 29, 2024 at 03:01:55PM +1000, James Liu wrote:

> > >  int advice_enabled(enum advice_type type)
> > >  {
> > > -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> > > +	int enabled;
> > > +
> > > +	if (getenv(GIT_NO_ADVICE))
> > > +		return 0;
> >
> > Huh, I was under impression that having an environment
> > variable to control this behavior was frowned upon by
> > Junio? [1]  To me, supporting such a variable would be
> > a somewhat acceptable risk, [2] but of course it's the
> > maintainer's opinion that matters most.
> >
> > [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
> > [2] 
> > https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/
> 
> You're correct. I saw this pattern for a few of the other global CLI
> options and followed it. I'm unsure what the best alternative for
> passing this configuration down to the `advice_enabled()` function is.
> I'd appreciate some guidance here.

You need an environment variable if you want the command-line option to
work consistently across commands that spawn external processes. E.g.:

  git --no-advice fetch --all

is going to spawn fetch sub-processes under the hood. You'd want them to
respect --no-advice, too, so we either have to propagate the
command-line option or use the environment. And when you consider an
external script like git-foo that runs a bunch of underlying Git
commands, then propagating becomes too cumbersome and error-prone.

You should use git_env_bool() to avoid the confusing behavior that
GIT_NO_ADVICE=false still turns off advice. ;)

You can also drop the "NO", which helps avoid awkward double negation.
For example, if you do:

  if (git_env_bool("GIT_ADVICE", 1))
	return 0;

then leaving that variable unset will act as if it is set to "1", but
you can still do GIT_ADVICE=0 to suppress it.

There are some older variables (e.g., GIT_NO_REPLACE_OBJECTS) that made
this mistake and we are stuck with, but I think we should avoid it for
newer ones.

-Peff

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

Hello Jeff,

On 2024-04-29 08:40, Jeff King wrote:
> On Mon, Apr 29, 2024 at 03:01:55PM +1000, James Liu wrote:
> 
>> > >  int advice_enabled(enum advice_type type)
>> > >  {
>> > > -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>> > > +	int enabled;
>> > > +
>> > > +	if (getenv(GIT_NO_ADVICE))
>> > > +		return 0;
>> >
>> > Huh, I was under impression that having an environment
>> > variable to control this behavior was frowned upon by
>> > Junio? [1]  To me, supporting such a variable would be
>> > a somewhat acceptable risk, [2] but of course it's the
>> > maintainer's opinion that matters most.
>> >
>> > [1] https://lore.kernel.org/git/xmqqfrva3k9j.fsf@gitster.g/
>> > [2] https://lore.kernel.org/git/462de4ec1fb1896fa7f26b3515deca57@manjaro.org/
>> 
>> You're correct. I saw this pattern for a few of the other global CLI
>> options and followed it. I'm unsure what the best alternative for
>> passing this configuration down to the `advice_enabled()` function is.
>> I'd appreciate some guidance here.
> 
> You need an environment variable if you want the command-line option to
> work consistently across commands that spawn external processes. E.g.:
> 
>   git --no-advice fetch --all
> 
> is going to spawn fetch sub-processes under the hood. You'd want them 
> to
> respect --no-advice, too, so we either have to propagate the
> command-line option or use the environment. And when you consider an
> external script like git-foo that runs a bunch of underlying Git
> commands, then propagating becomes too cumbersome and error-prone.

Well described, thanks.  Though, I'm afraid Junio isn't going
to like this new environment variable, but we'll see.

> You should use git_env_bool() to avoid the confusing behavior that
> GIT_NO_ADVICE=false still turns off advice. ;)
> 
> You can also drop the "NO", which helps avoid awkward double negation.
> For example, if you do:
> 
>   if (git_env_bool("GIT_ADVICE", 1))
> 	return 0;
> 
> then leaving that variable unset will act as if it is set to "1", but
> you can still do GIT_ADVICE=0 to suppress it.
> 
> There are some older variables (e.g., GIT_NO_REPLACE_OBJECTS) that made
> this mistake and we are stuck with, but I think we should avoid it for
> newer ones.

Makes sense to me.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> You need an environment variable if you want the command-line option to
> work consistently across commands that spawn external processes. E.g.:
> ...
> commands, then propagating becomes too cumbersome and error-prone.

Ah, you've explained this---thanks.  As an internal implementation
detail, this is inevitable, and I am OK as long as we document it as
such.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[James Liu]

On Mon Apr 29, 2024 at 4:40 PM AEST, Jeff King wrote:
> You need an environment variable if you want the command-line option to
> work consistently across commands that spawn external processes. E.g.:
>
>   git --no-advice fetch --all
>
> is going to spawn fetch sub-processes under the hood. You'd want them to
> respect --no-advice, too, so we either have to propagate the
> command-line option or use the environment. And when you consider an
> external script like git-foo that runs a bunch of underlying Git
> commands, then propagating becomes too cumbersome and error-prone.

Thanks for the explanation Jeff! Makes sense why the pattern is so
prevalent.

> You should use git_env_bool() to avoid the confusing behavior that
> GIT_NO_ADVICE=false still turns off advice. ;)
>
> You can also drop the "NO", which helps avoid awkward double negation.
> For example, if you do:
>
>   if (git_env_bool("GIT_ADVICE", 1))
> 	return 0;
>
> then leaving that variable unset will act as if it is set to "1", but
> you can still do GIT_ADVICE=0 to suppress it.

Awesome. I'll apply this suggestion in the next version of this patch.

Cheers,
James

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Huh, I was under impression that having an environment
> variable to control this behavior was frowned upon by
> Junio?

It is frowned upon.  It is secondary who frowns upon it ;-)

We _might_ be able to pass --no-advice to all internal invocations
of subprocesses, but that is a chore, and if a code path calls a
subprocess that is *not* a git program that in turn calls a git
program, such an approach would not work.  But an environment
variable would.

So using an environment variable as an internal detail and marking
it as "internal use only---do not set or unset it yourself" is the
best we could do, and I do not think I would mind.  At that point,
those who set the variable themselves are outside those whose
breakage we care about.

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Rubén Justo]

On Mon, Apr 29, 2024 at 11:09:25AM +1000, James Liu wrote:

>  int advice_enabled(enum advice_type type)
>  {
> -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> +	int enabled;
> +
> +	if (getenv(GIT_NO_ADVICE))
> +		return 0;
> +
> +	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>  

All hints are set to a default visibility value: "none", which means
implicitly enabled.  Maybe we can get this "no-advice" knob by making
this default value configurable:

diff --git a/advice.c b/advice.c
index 75111191ad..bc67b99ba7 100644
--- a/advice.c
+++ b/advice.c
@@ -39,6 +39,8 @@ enum advice_level {
        ADVICE_LEVEL_ENABLED,
 };
 
+static enum advice_level advice_default_level;
+
 static struct {
        const char *key;
        enum advice_level level;
@@ -126,7 +128,19 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-       int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+       static int once = 0;
+       int enabled;
+
+       if (!once++) {
+               const char* level = getenv("GIT_ADVICE_LEVEL");
+
+               if (level && !strcmp(level, "disable"))
+                       advice_default_level = ADVICE_LEVEL_DISABLED;
+       }
+
+       enabled = (advice_setting[type].level
+                  ? advice_setting[type].level
+                  : advice_default_level) != ADVICE_LEVEL_DISABLED;
 
        if (type == ADVICE_PUSH_UPDATE_REJECTED)
                return enabled &&

Re: [PATCH v2 1/1] advice: add --no-advice global option

[Dragan Simic]

On 2024-04-29 19:05, Rubén Justo wrote:
> On Mon, Apr 29, 2024 at 11:09:25AM +1000, James Liu wrote:
> 
>>  int advice_enabled(enum advice_type type)
>>  {
>> -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>> +	int enabled;
>> +
>> +	if (getenv(GIT_NO_ADVICE))
>> +		return 0;
>> +
>> +	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
>> 
> 
> All hints are set to a default visibility value: "none", which means
> implicitly enabled.  Maybe we can get this "no-advice" knob by making
> this default value configurable:

Please note that the new environment variable isn't supposed
to be used externally, [1] i.e. it isn't meant to be set by
the users in their ~/.bash_profile files (or any other shell
configuration files), so I think that extending the scope of
this patch into such a direction isn't something we should
aim at.

[1] https://lore.kernel.org/git/xmqqh6fk1dmq.fsf@gitster.g/

> diff --git a/advice.c b/advice.c
> index 75111191ad..bc67b99ba7 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -39,6 +39,8 @@ enum advice_level {
>         ADVICE_LEVEL_ENABLED,
>  };
> 
> +static enum advice_level advice_default_level;
> +
>  static struct {
>         const char *key;
>         enum advice_level level;
> @@ -126,7 +128,19 @@ void advise(const char *advice, ...)
> 
>  int advice_enabled(enum advice_type type)
>  {
> -       int enabled = advice_setting[type].level != 
> ADVICE_LEVEL_DISABLED;
> +       static int once = 0;
> +       int enabled;
> +
> +       if (!once++) {
> +               const char* level = getenv("GIT_ADVICE_LEVEL");
> +
> +               if (level && !strcmp(level, "disable"))
> +                       advice_default_level = ADVICE_LEVEL_DISABLED;
> +       }
> +
> +       enabled = (advice_setting[type].level
> +                  ? advice_setting[type].level
> +                  : advice_default_level) != ADVICE_LEVEL_DISABLED;
> 
>         if (type == ADVICE_PUSH_UPDATE_REJECTED)
>                 return enabled &&

[PATCH v3 0/1] advice: add --no-advice global option

[James Liu]

Hi,

This is v3 of the patch to add a global --no-advice option for silencing
all advice hints. The environment variable has been renamed to
GIT_ADVICE and marked for internal use only, and the conditional in the
advice_enabled() helper has been adjusted to use git_env_bool().

I explored the idea of adding another test to ensure the configuration
is propagated to subprocesses correctly. This would use `git fetch --all`
as the trigger, however it appears that advice is only printed when
`fetch_one()` is invoked, which I don't think spawns any child
processes. With that said, since this is a common pattern, I believe
the existing additional test case is sufficient.

Cheers,
James

James Liu (1):
  advice: add --no-advice global option

 Documentation/git.txt |  5 ++++-
 advice.c              |  8 +++++++-
 environment.h         |  7 +++++++
 git.c                 |  6 +++++-
 t/t0018-advice.sh     | 20 ++++++++++++++++++++
 5 files changed, 43 insertions(+), 3 deletions(-)

Range-diff against v2:
1:  0f2ecb7862 ! 1:  55d5559586 advice: add --no-advice global option
    @@ Commit message
     
         Add a --no-advice global option to disable all advice hints from being
         displayed. This is independent of the toggles for individual advice
    -    hints.
    +    hints. Use an internal environment variable (GIT_ADVICE) to ensure this
    +    configuration is propagated to the usage site, even if it executes in a
    +    subprocess.
     
         Signed-off-by: James Liu <james@jamesliu.io>
     
    @@ advice.c: void advise(const char *advice, ...)
     -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
     +	int enabled;
     +
    -+	if (getenv(GIT_NO_ADVICE))
    ++	if (!git_env_bool(GIT_ADVICE, 1))
     +		return 0;
     +
     +	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
    @@ advice.c: void advise(const char *advice, ...)
     
      ## environment.h ##
     @@ environment.h: const char *getenv_safe(struct strvec *argv, const char *name);
    - #define GIT_OPTIONAL_LOCKS_ENVIRONMENT "GIT_OPTIONAL_LOCKS"
      #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
      #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
    -+#define GIT_NO_ADVICE "GIT_NO_ADVICE"
      
    ++/*
    ++ * Environment variable used to propagate the --no-advice global option to the
    ++ * advice_enabled() helper, even when run in a subprocess.
    ++ * This is an internal variable that should not be set by the user.
    ++ */
    ++#define GIT_ADVICE "GIT_ADVICE"
    ++
      /*
       * Environment variable used in handshaking the wire protocol.
    +  * Contains a colon ':' separated list of keys with optional values
     
      ## git.c ##
     @@ git.c: const char git_usage_string[] =
    @@ git.c: static int handle_options(const char ***argv, int *argc, int *envchanged)
      			if (envchanged)
      				*envchanged = 1;
     +		} else if (!strcmp(cmd, "--no-advice")) {
    -+			setenv(GIT_NO_ADVICE, "1", 1);
    ++			setenv(GIT_ADVICE, "0", 1);
     +			if (envchanged)
     +				*envchanged = 1;
      		} else {
-- 
2.44.0

[PATCH v3 1/1] advice: add --no-advice global option

[James Liu]

Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
and scripted usages of Git where hints aren't necessary, it can be
cumbersome to maintain configuration to ensure all advice hints are
disabled in perpetuity. This is a particular concern in tests, where
new or changed hints can result in failed assertions.

Add a --no-advice global option to disable all advice hints from being
displayed. This is independent of the toggles for individual advice
hints. Use an internal environment variable (GIT_ADVICE) to ensure this
configuration is propagated to the usage site, even if it executes in a
subprocess.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/git.txt |  5 ++++-
 advice.c              |  8 +++++++-
 environment.h         |  7 +++++++
 git.c                 |  6 +++++-
 t/t0018-advice.sh     | 20 ++++++++++++++++++++
 5 files changed, 43 insertions(+), 3 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1b112a3e..ef1d9dce5d 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -13,7 +13,7 @@ SYNOPSIS
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
     [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--config-env=<name>=<envvar>] <command> [<args>]
+    [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]
 
 DESCRIPTION
 -----------
@@ -226,6 +226,9 @@ If you just want to run git as if it was started in `<path>` then use
 	linkgit:gitattributes[5]. This is equivalent to setting the
 	`GIT_ATTR_SOURCE` environment variable.
 
+--no-advice::
+	Disable all advice hints from being printed.
+
 GIT COMMANDS
 ------------
 
diff --git a/advice.c b/advice.c
index 75111191ad..abad9ccaa2 100644
--- a/advice.c
+++ b/advice.c
@@ -2,6 +2,7 @@
 #include "advice.h"
 #include "config.h"
 #include "color.h"
+#include "environment.h"
 #include "gettext.h"
 #include "help.h"
 #include "string-list.h"
@@ -126,7 +127,12 @@ void advise(const char *advice, ...)
 
 int advice_enabled(enum advice_type type)
 {
-	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+	int enabled;
+
+	if (!git_env_bool(GIT_ADVICE, 1))
+		return 0;
+
+	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
 
 	if (type == ADVICE_PUSH_UPDATE_REJECTED)
 		return enabled &&
diff --git a/environment.h b/environment.h
index 05fd94d7be..26e87843e1 100644
--- a/environment.h
+++ b/environment.h
@@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);
 #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
 #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
 
+/*
+ * Environment variable used to propagate the --no-advice global option to the
+ * advice_enabled() helper, even when run in a subprocess.
+ * This is an internal variable that should not be set by the user.
+ */
+#define GIT_ADVICE "GIT_ADVICE"
+
 /*
  * Environment variable used in handshaking the wire protocol.
  * Contains a colon ':' separated list of keys with optional values
diff --git a/git.c b/git.c
index 654d615a18..6283d126e5 100644
--- a/git.c
+++ b/git.c
@@ -38,7 +38,7 @@ const char git_usage_string[] =
 	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
 	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
 	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
+	   "           [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]");
 
 const char git_more_info_string[] =
 	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
@@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--no-advice")) {
+			setenv(GIT_ADVICE, "0", 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else {
 			fprintf(stderr, _("unknown option: %s\n"), cmd);
 			usage(git_usage_string);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 0dcfb760a2..2ce2d4668a 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to
 	test_must_be_empty actual
 '
 
+test_expect_success 'advice should not be printed when --no-advice is used' '
+	cat << EOF > expect &&
+On branch master
+
+No commits yet
+
+Untracked files:
+	README
+
+nothing added to commit but untracked files present
+EOF
+
+	git init advice-test &&
+  test_when_finished "rm -fr advice-test" &&
+  cd advice-test &&
+  touch README &&
+  git --no-advice status > ../actual &&
+  test_cmp ../expect ../actual
+'
+
 test_done
-- 
2.44.0

Re: [PATCH v3 1/1] advice: add --no-advice global option

[Patrick Steinhardt]

[-- Attachment #1: Type: text/plain, Size: 3574 bytes --]

On Tue, Apr 30, 2024 at 11:47:24AM +1000, James Liu wrote:
[snip]
> diff --git a/environment.h b/environment.h
> index 05fd94d7be..26e87843e1 100644
> --- a/environment.h
> +++ b/environment.h
> @@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);
>  #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
>  #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
>  
> +/*
> + * Environment variable used to propagate the --no-advice global option to the
> + * advice_enabled() helper, even when run in a subprocess.
> + * This is an internal variable that should not be set by the user.
> + */
> +#define GIT_ADVICE "GIT_ADVICE"

Almost all of the other defines in this file have an "_ENVIRONMENT"
suffix. We should probably do the same here to mark it as the name of
the environment variable, which otherwise wouldn't be obvious.

>  /*
>   * Environment variable used in handshaking the wire protocol.
>   * Contains a colon ':' separated list of keys with optional values
> diff --git a/git.c b/git.c
> index 654d615a18..6283d126e5 100644
> --- a/git.c
> +++ b/git.c
> @@ -38,7 +38,7 @@ const char git_usage_string[] =
>  	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
>  	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
>  	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
> -	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
> +	   "           [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]");
>  
>  const char git_more_info_string[] =
>  	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
> @@ -337,6 +337,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
>  			setenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);
>  			if (envchanged)
>  				*envchanged = 1;
> +		} else if (!strcmp(cmd, "--no-advice")) {
> +			setenv(GIT_ADVICE, "0", 1);
> +			if (envchanged)
> +				*envchanged = 1;
>  		} else {
>  			fprintf(stderr, _("unknown option: %s\n"), cmd);
>  			usage(git_usage_string);
> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index 0dcfb760a2..2ce2d4668a 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'advice should not be printed when --no-advice is used' '
> +	cat << EOF > expect &&

We typically write those `cat >expect <<-EOF`. I assume you cannot use
the dash here because the "README" string is indented by a tab. But the
order of redirects should still be reversed.

> +On branch master
> +
> +No commits yet
> +
> +Untracked files:
> +	README
> +
> +nothing added to commit but untracked files present
> +EOF
> +
> +	git init advice-test &&
> +  test_when_finished "rm -fr advice-test" &&

Let's run `test_when_finished` before creating the repo.

> +  cd advice-test &&
> +  touch README &&
> +  git --no-advice status > ../actual &&
> +  test_cmp ../expect ../actual

Nit: these should be indented by tabs, not spaces.

> +'

We could add two more tests that explicitly set the environment
variable, once to `true` and once to `false`. This would at least
somewhat represent the case of running Git subcommands, even though we
cannot test whether Git actually knows to set the envvar like this.

Patrick

>  test_done
> -- 
> 2.44.0
> 
> 

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 833 bytes --]

Re: [PATCH v3 1/1] advice: add --no-advice global option

[Dragan Simic]

Hello James,

On 2024-04-30 07:18, Patrick Steinhardt wrote:
> On Tue, Apr 30, 2024 at 11:47:24AM +1000, James Liu wrote:
>> +/*
>> + * Environment variable used to propagate the --no-advice global 
>> option to the
>> + * advice_enabled() helper, even when run in a subprocess.
>> + * This is an internal variable that should not be set by the user.
>> + */
>> +#define GIT_ADVICE "GIT_ADVICE"
> 
> Almost all of the other defines in this file have an "_ENVIRONMENT"
> suffix. We should probably do the same here to mark it as the name of
> the environment variable, which otherwise wouldn't be obvious.

Just for the record, I already pointed that out... [1]

> We could add two more tests that explicitly set the environment
> variable, once to `true` and once to `false`. This would at least
> somewhat represent the case of running Git subcommands, even though we
> cannot test whether Git actually knows to set the envvar like this.

... and this as well. [1]

[1] 
https://lore.kernel.org/git/37512328b1f3db4e8075bdb4beeb8929@manjaro.org/

Re: [PATCH v3 1/1] advice: add --no-advice global option

[Junio C Hamano]

James Liu <james@jamesliu.io> writes:

> Advice hints must be disabled individually by setting the relevant
> advice.* variables to false in the Git configuration. For server-side
> and scripted usages of Git where hints aren't necessary, it can be
> cumbersome to maintain configuration to ensure all advice hints are

It is not that scripted use decline hints because they are not
"necessary"; it is they find the hints harmful for whatever reason.

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 7a1b112a3e..ef1d9dce5d 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -13,7 +13,7 @@ SYNOPSIS
>      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
>      [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
>      [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
> -    [--config-env=<name>=<envvar>] <command> [<args>]
> +    [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]

Move the new one near [--no-replace-objects] to group the --no-*
options together.

This is not a fault of this patch, but I notice "--no-lazy-fetch"
and "--no-optional-locks" are not listed here (there may be others,
I didn't check too deeply).  It might make sense to have a separate
clean-up step to move the --no-* "ordinarily we would never want to
disable these wholesale, but for very special needs here are the
knobs to toggle them off" options together in the DESCRIPTION
section and add missing ones to the SYNOPSIS section.

> diff --git a/advice.c b/advice.c
> index 75111191ad..abad9ccaa2 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -2,6 +2,7 @@
>  #include "advice.h"
>  #include "config.h"
>  #include "color.h"
> +#include "environment.h"
>  #include "gettext.h"
>  #include "help.h"
>  #include "string-list.h"
> @@ -126,7 +127,12 @@ void advise(const char *advice, ...)
>  
>  int advice_enabled(enum advice_type type)
>  {
> -	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
> +	int enabled;
> +
> +	if (!git_env_bool(GIT_ADVICE, 1))
> +		return 0;

How expensive is it to check git_env_bool() every time without
caching the parsing of the value given to the environment variable?
Everybody pays this price but for most users who do not set the
environment variable it is not a price we want them to pay.

One way to solve that might be to just insert these

	static int advice_enabled = -1;

	if (advice_enabled < 0)
		advice_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1)
	if (!advice_enabled)
		return 0;

and leave everything else intact.  We do not even need to split the
declalation+initialization into a ...

> +	enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;

... separate assignment.

> +/*
> + * Environment variable used to propagate the --no-advice global option to the
> + * advice_enabled() helper, even when run in a subprocess.
> + * This is an internal variable that should not be set by the user.
> + */
> +#define GIT_ADVICE "GIT_ADVICE"

As Patrick pointed out, this should be GIT_ADVICE_ENVIRONMENT or
something.

> diff --git a/git.c b/git.c
> index 654d615a18..6283d126e5 100644
> --- a/git.c
> +++ b/git.c
> @@ -38,7 +38,7 @@ const char git_usage_string[] =
>  	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
>  	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
>  	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
> -	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
> +	   "           [--config-env=<name>=<envvar>] [--no-advice] <command> [<args>]");

Ditto.

> diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
> index 0dcfb760a2..2ce2d4668a 100755
> --- a/t/t0018-advice.sh
> +++ b/t/t0018-advice.sh
> @@ -29,4 +29,24 @@ test_expect_success 'advice should not be printed when config variable is set to
>  	test_must_be_empty actual
>  '
>  
> +test_expect_success 'advice should not be printed when --no-advice is used' '
> +	cat << EOF > expect &&

Style.  Between the redirection operator and the file that the
operator operates on, there should be no SP.

> +On branch master
> +
> +No commits yet
> +
> +Untracked files:
> +	README

Something like

	q_to_tab >expect <<\-EOF
	On branch master
	...
	Untracked files:
	QREADME
	
	nothing added to ...
	EOF

or

	sed -e "s/^|//" >expect <<\-EOF
	On branch master
	...
	Untracked files:
	|	README
	
	nothing added to ...
	EOF

would work better.

> +	git init advice-test &&
> +  test_when_finished "rm -fr advice-test" &&

Funny indentation.  Also if "git init" fails in the middle, after
creating the directory but before completing, your when-finished
handler fails to register.

> +  cd advice-test &&

Do not chdir around without being inside a subshell.  If we have to
add new tests later to this script, you do not want them to start in
the directory that you are going to remove at the end of this test.

> +  touch README &&

When you do not care about the timestamp, avoid using "touch".  Writing

	>README &&

instead would make it clear that you ONLY care about the presense of
the file, not even its contents.

> +  git --no-advice status > ../actual &&
> +  test_cmp ../expect ../actual

So, taken together:

	(
		cd advice-test &&
		>README &&
		git --no-advice status
	) >actual &&
	test_cmp expect actual

> +'
> +
>  test_done

Thanks.

[PATCH v4 0/3] advice: add "all" option to disable all hints

[James Liu]

Hi,

Thank you all for the comprehensive feedback. This is v4 of the patch
series to introduce a --no-advice option for silencing advice hints.

The main changes are:

- Two preliminary commits to reorder/clean up the options documentation
  and usage string.
- Caching the value of GIT_ADVICE.
- Adding tests which explicitly set GIT_ADVICE to false and true.

Cheers,
James

James Liu (3):
  doc: clean up usage documentation for --no-* opts
  doc: add spacing around paginate options
  advice: add --no-advice global option

 Documentation/git.txt | 18 +++++++-----
 advice.c              |  7 +++++
 environment.h         |  7 +++++
 git.c                 | 11 +++++--
 t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 101 insertions(+), 10 deletions(-)

Range-diff against v3:
1:  55d5559586 < -:  ---------- advice: add --no-advice global option
-:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for --no-* opts
-:  ---------- > 2:  1b0019026a doc: add spacing around paginate options
-:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option
-- 
2.44.0

[PATCH v4 1/3] doc: clean up usage documentation for --no-* opts

[James Liu]

We'll be adding another option to the --no-* class of options soon.

Clean up the existing options by grouping them together in the OPTIONS
section, and adding missing ones to the SYNOPSIS.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/git.txt | 14 +++++++-------
 git.c                 |  6 +++---
 2 files changed, 10 insertions(+), 10 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7a1b112a3e..7fa75350b2 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,9 +11,9 @@ SYNOPSIS
 [verse]
 'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
-    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
-    [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
-    [--config-env=<name>=<envvar>] <command> [<args>]
+    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
+    [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
+    [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
 
 DESCRIPTION
 -----------
@@ -186,6 +186,10 @@ If you just want to run git as if it was started in `<path>` then use
 	This is equivalent to setting the `GIT_NO_LAZY_FETCH`
 	environment variable to `1`.
 
+--no-optional-locks::
+	Do not perform optional operations that require locks. This is
+	equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
+
 --literal-pathspecs::
 	Treat pathspecs literally (i.e. no globbing, no pathspec magic).
 	This is equivalent to setting the `GIT_LITERAL_PATHSPECS` environment
@@ -207,10 +211,6 @@ If you just want to run git as if it was started in `<path>` then use
 	Add "icase" magic to all pathspec. This is equivalent to setting
 	the `GIT_ICASE_PATHSPECS` environment variable to `1`.
 
---no-optional-locks::
-	Do not perform optional operations that require locks. This is
-	equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
-
 --list-cmds=<group>[,<group>...]::
 	List commands by group. This is an internal/experimental
 	option and may change or be removed in the future. Supported
diff --git a/git.c b/git.c
index 654d615a18..7654571b75 100644
--- a/git.c
+++ b/git.c
@@ -36,9 +36,9 @@ struct cmd_struct {
 const char git_usage_string[] =
 	N_("git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]\n"
 	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
-	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--bare]\n"
-	   "           [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]\n"
-	   "           [--config-env=<name>=<envvar>] <command> [<args>]");
+	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n"
+	   "           [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]\n"
+	   "           [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]");
 
 const char git_more_info_string[] =
 	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
-- 
2.44.0

Re: [PATCH v4 1/3] doc: clean up usage documentation for --no-* opts

[Junio C Hamano]

James Liu <james@jamesliu.io> writes:

> We'll be adding another option to the --no-* class of options soon.
>
> Clean up the existing options by grouping them together in the OPTIONS
> section, and adding missing ones to the SYNOPSIS.

Nice.  

> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 7a1b112a3e..7fa75350b2 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -11,9 +11,9 @@ SYNOPSIS
>  [verse]
>  'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
>      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
> -    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
> -    [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
> -    [--config-env=<name>=<envvar>] <command> [<args>]
> +    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
> +    [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
> +    [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]

Looks sensible.

There still are a few options (like noglob-pathspecs) missing, but
cleaning them up from this part of the documentation is totally
outside the scope of this topic (#leftoverbits -- we either make
this exhaustive, or make it clear that this is not exhaustive).

Thanks.

Re: [PATCH v4 1/3] doc: clean up usage documentation for --no-* opts

[James Liu]

On Sat May 4, 2024 at 3:30 AM AEST, Junio C Hamano wrote:
> James Liu <james@jamesliu.io> writes:
>
> > We'll be adding another option to the --no-* class of options soon.
> >
> > Clean up the existing options by grouping them together in the OPTIONS
> > section, and adding missing ones to the SYNOPSIS.
>
> Nice.  
>
> > diff --git a/Documentation/git.txt b/Documentation/git.txt
> > index 7a1b112a3e..7fa75350b2 100644
> > --- a/Documentation/git.txt
> > +++ b/Documentation/git.txt
> > @@ -11,9 +11,9 @@ SYNOPSIS
> >  [verse]
> >  'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
> >      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
> > -    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare]
> > -    [--git-dir=<path>] [--work-tree=<path>] [--namespace=<name>]
> > -    [--config-env=<name>=<envvar>] <command> [<args>]
> > +    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
> > +    [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
> > +    [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
>
> Looks sensible.
>
> There still are a few options (like noglob-pathspecs) missing, but
> cleaning them up from this part of the documentation is totally
> outside the scope of this topic (#leftoverbits -- we either make
> this exhaustive, or make it clear that this is not exhaustive).
>
> Thanks.

I did notice noglob-pathspecs, but I wasn't sure if it came under the
same purview as the `--no-*` options (where a dash exists after "no").

Cheers,
James

[PATCH v4 2/3] doc: add spacing around paginate options

[James Liu]

Make the documentation page consistent with the usage string printed by
git.c

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/git.txt | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index 7fa75350b2..d11d3d0c86 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -11,7 +11,7 @@ SYNOPSIS
 [verse]
 'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
-    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
+    [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]
     [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
     [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
 
-- 
2.44.0

Re: [PATCH v4 2/3] doc: add spacing around paginate options

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 966 bytes --]

James Liu <james@jamesliu.io> writes:

> Make the documentation page consistent with the usage string printed by
> git.c
>

Nit: missing full-stop here.

> Signed-off-by: James Liu <james@jamesliu.io>
> ---
>  Documentation/git.txt | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/Documentation/git.txt b/Documentation/git.txt
> index 7fa75350b2..d11d3d0c86 100644
> --- a/Documentation/git.txt
> +++ b/Documentation/git.txt
> @@ -11,7 +11,7 @@ SYNOPSIS
>  [verse]
>  'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
>      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
> -    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
> +    [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]
>      [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
>      [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
>
> --
> 2.44.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4 2/3] doc: add spacing around paginate options

[Junio C Hamano]

Karthik Nayak <karthik.188@gmail.com> writes:

> James Liu <james@jamesliu.io> writes:
>
>> Make the documentation page consistent with the usage string printed by
>> git.c
>>
>
> Nit: missing full-stop here.

Yes, but "printed by git.c" is an unsatisfying way to say this.  We
can easily illustrate what the end user does to get it, for example:

    ... usage string given by "git help git".

It also makes it internally consistent with "[-v | --version]" that
exists in the same document, which is even better reason to do this
change.

Thanks, both.

>
>> Signed-off-by: James Liu <james@jamesliu.io>
>> ---
>>  Documentation/git.txt | 2 +-
>>  1 file changed, 1 insertion(+), 1 deletion(-)
>>
>> diff --git a/Documentation/git.txt b/Documentation/git.txt
>> index 7fa75350b2..d11d3d0c86 100644
>> --- a/Documentation/git.txt
>> +++ b/Documentation/git.txt
>> @@ -11,7 +11,7 @@ SYNOPSIS
>>  [verse]
>>  'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
>>      [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
>> -    [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--no-lazy-fetch]
>> +    [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]
>>      [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
>>      [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
>>
>> --
>> 2.44.0

[PATCH v4 3/3] advice: add --no-advice global option

[James Liu]

Advice hints must be disabled individually by setting the relevant
advice.* variables to false in the Git configuration. For server-side
and scripted usages of Git where hints can be a hindrance, it can be
cumbersome to maintain configuration to ensure all advice hints are
disabled in perpetuity. This is a particular concern in tests, where
new or changed hints can result in failed assertions.

Add a --no-advice global option to disable all advice hints from being
displayed. This is independent of the toggles for individual advice
hints. Use an internal environment variable (GIT_ADVICE) to ensure this
configuration is propagated to the usage site, even if it executes in a
subprocess.

Signed-off-by: James Liu <james@jamesliu.io>
---
 Documentation/git.txt |  8 +++--
 advice.c              |  7 +++++
 environment.h         |  7 +++++
 git.c                 |  9 ++++--
 t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 95 insertions(+), 4 deletions(-)

diff --git a/Documentation/git.txt b/Documentation/git.txt
index d11d3d0c86..a0c07f1db8 100644
--- a/Documentation/git.txt
+++ b/Documentation/git.txt
@@ -12,8 +12,9 @@ SYNOPSIS
 'git' [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]
     [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]
     [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]
-    [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]
-    [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]
+    [--no-optional-locks] [--no-advice] [--bare] [--git-dir=<path>]
+    [--work-tree=<path>] [--namespace=<name>] [--config-env=<name>=<envvar>]
+    <command> [<args>]
 
 DESCRIPTION
 -----------
@@ -190,6 +191,9 @@ If you just want to run git as if it was started in `<path>` then use
 	Do not perform optional operations that require locks. This is
 	equivalent to setting the `GIT_OPTIONAL_LOCKS` to `0`.
 
+--no-advice::
+	Disable all advice hints from being printed.
+
 --literal-pathspecs::
 	Treat pathspecs literally (i.e. no globbing, no pathspec magic).
 	This is equivalent to setting the `GIT_LITERAL_PATHSPECS` environment
diff --git a/advice.c b/advice.c
index 75111191ad..0a122c2020 100644
--- a/advice.c
+++ b/advice.c
@@ -2,6 +2,7 @@
 #include "advice.h"
 #include "config.h"
 #include "color.h"
+#include "environment.h"
 #include "gettext.h"
 #include "help.h"
 #include "string-list.h"
@@ -127,6 +128,12 @@ void advise(const char *advice, ...)
 int advice_enabled(enum advice_type type)
 {
 	int enabled = advice_setting[type].level != ADVICE_LEVEL_DISABLED;
+	static int globally_enabled = -1;
+
+	if (globally_enabled < 0)
+		globally_enabled = git_env_bool(GIT_ADVICE_ENVIRONMENT, 1);
+	if (!globally_enabled)
+		return 0;
 
 	if (type == ADVICE_PUSH_UPDATE_REJECTED)
 		return enabled &&
diff --git a/environment.h b/environment.h
index 05fd94d7be..0b2d457f07 100644
--- a/environment.h
+++ b/environment.h
@@ -57,6 +57,13 @@ const char *getenv_safe(struct strvec *argv, const char *name);
 #define GIT_TEXT_DOMAIN_DIR_ENVIRONMENT "GIT_TEXTDOMAINDIR"
 #define GIT_ATTR_SOURCE_ENVIRONMENT "GIT_ATTR_SOURCE"
 
+/*
+ * Environment variable used to propagate the --no-advice global option to the
+ * advice_enabled() helper, even when run in a subprocess.
+ * This is an internal variable that should not be set by the user.
+ */
+#define GIT_ADVICE_ENVIRONMENT "GIT_ADVICE"
+
 /*
  * Environment variable used in handshaking the wire protocol.
  * Contains a colon ':' separated list of keys with optional values
diff --git a/git.c b/git.c
index 7654571b75..637c61ca9c 100644
--- a/git.c
+++ b/git.c
@@ -37,8 +37,9 @@ const char git_usage_string[] =
 	N_("git [-v | --version] [-h | --help] [-C <path>] [-c <name>=<value>]\n"
 	   "           [--exec-path[=<path>]] [--html-path] [--man-path] [--info-path]\n"
 	   "           [-p | --paginate | -P | --no-pager] [--no-replace-objects] [--no-lazy-fetch]\n"
-	   "           [--no-optional-locks] [--bare] [--git-dir=<path>] [--work-tree=<path>]\n"
-	   "           [--namespace=<name>] [--config-env=<name>=<envvar>] <command> [<args>]");
+	   "           [--no-optional-locks] [--no-advice] [--bare] [--git-dir=<path>]\n"
+	   "           [--work-tree=<path>] [--namespace=<name>] [--config-env=<name>=<envvar>]\n"
+	   "           <command> [<args>]");
 
 const char git_more_info_string[] =
 	N_("'git help -a' and 'git help -g' list available subcommands and some\n"
@@ -337,6 +338,10 @@ static int handle_options(const char ***argv, int *argc, int *envchanged)
 			setenv(GIT_ATTR_SOURCE_ENVIRONMENT, cmd, 1);
 			if (envchanged)
 				*envchanged = 1;
+		} else if (!strcmp(cmd, "--no-advice")) {
+			setenv(GIT_ADVICE_ENVIRONMENT, "0", 1);
+			if (envchanged)
+				*envchanged = 1;
 		} else {
 			fprintf(stderr, _("unknown option: %s\n"), cmd);
 			usage(git_usage_string);
diff --git a/t/t0018-advice.sh b/t/t0018-advice.sh
index 0dcfb760a2..b02448ea16 100755
--- a/t/t0018-advice.sh
+++ b/t/t0018-advice.sh
@@ -29,4 +29,72 @@ test_expect_success 'advice should not be printed when config variable is set to
 	test_must_be_empty actual
 '
 
+test_expect_success 'advice should not be printed when --no-advice is used' '
+	q_to_tab >expect <<-\EOF &&
+On branch master
+
+No commits yet
+
+Untracked files:
+QREADME
+
+nothing added to commit but untracked files present
+EOF
+
+	test_when_finished "rm -fr advice-test" &&
+	git init advice-test &&
+	(
+		cd advice-test &&
+		>README &&
+		git --no-advice status
+	) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '
+	q_to_tab >expect <<-\EOF &&
+On branch master
+
+No commits yet
+
+Untracked files:
+QREADME
+
+nothing added to commit but untracked files present
+EOF
+
+	test_when_finished "rm -fr advice-test" &&
+	git init advice-test &&
+	(
+		cd advice-test &&
+		>README &&
+		GIT_ADVICE=false git status
+	) >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '
+	q_to_tab >expect <<-\EOF &&
+On branch master
+
+No commits yet
+
+Untracked files:
+  (use "git add <file>..." to include in what will be committed)
+QREADME
+
+nothing added to commit but untracked files present (use "git add" to track)
+EOF
+
+	test_when_finished "rm -fr advice-test" &&
+	git init advice-test &&
+	(
+		cd advice-test &&
+		>README &&
+		GIT_ADVICE=true git status
+	) >actual &&
+	cat actual > /tmp/actual &&
+	test_cmp expect actual
+'
+
 test_done
-- 
2.44.0

[PATCH v4 4/3] t0018: two small fixes

[Junio C Hamano]

Even though the three tests that were recently added started their
here-doc with "<<-\EOF", it did not take advantage of that and
instead wrote the here-doc payload abut to the left edge.  Use a tabs
to indent these lines.

More importantly, because these all hardcode the expected output,
which contains the current branch name, they break the CI job that
uses 'main' as the default branch name.

Use

    GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk
    export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME

between the test_description line and ". ./test-lib.sh" line to
force the initial branch name to 'trunk' and expect it to show in
the output.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * I should have noticed these before merging the series to 'next',
   so here is a fix-up patch on top of the three patches.

 t/t0018-advice.sh | 41 ++++++++++++++++++++++-------------------
 1 file changed, 22 insertions(+), 19 deletions(-)

diff --git c/t/t0018-advice.sh w/t/t0018-advice.sh
index b02448ea16..29306b367c 100755
--- c/t/t0018-advice.sh
+++ w/t/t0018-advice.sh
@@ -2,6 +2,9 @@
 
 test_description='Test advise_if_enabled functionality'
 
+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=trunk
+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
+
 TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
@@ -31,15 +34,15 @@ test_expect_success 'advice should not be printed when config variable is set to
 
 test_expect_success 'advice should not be printed when --no-advice is used' '
 	q_to_tab >expect <<-\EOF &&
-On branch master
+	On branch trunk
 
-No commits yet
+	No commits yet
 
-Untracked files:
-QREADME
+	Untracked files:
+	QREADME
 
-nothing added to commit but untracked files present
-EOF
+	nothing added to commit but untracked files present
+	EOF
 
 	test_when_finished "rm -fr advice-test" &&
 	git init advice-test &&
@@ -53,15 +56,15 @@ EOF
 
 test_expect_success 'advice should not be printed when GIT_ADVICE is set to false' '
 	q_to_tab >expect <<-\EOF &&
-On branch master
+	On branch trunk
 
-No commits yet
+	No commits yet
 
-Untracked files:
-QREADME
+	Untracked files:
+	QREADME
 
-nothing added to commit but untracked files present
-EOF
+	nothing added to commit but untracked files present
+	EOF
 
 	test_when_finished "rm -fr advice-test" &&
 	git init advice-test &&
@@ -75,16 +78,16 @@ EOF
 
 test_expect_success 'advice should be printed when GIT_ADVICE is set to true' '
 	q_to_tab >expect <<-\EOF &&
-On branch master
+	On branch trunk
 
-No commits yet
+	No commits yet
 
-Untracked files:
-  (use "git add <file>..." to include in what will be committed)
-QREADME
+	Untracked files:
+	  (use "git add <file>..." to include in what will be committed)
+	QREADME
 
-nothing added to commit but untracked files present (use "git add" to track)
-EOF
+	nothing added to commit but untracked files present (use "git add" to track)
+	EOF
 
 	test_when_finished "rm -fr advice-test" &&
 	git init advice-test &&

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Dragan Simic]

Hello James,

On 2024-05-03 09:17, James Liu wrote:
>  Documentation/git.txt | 18 +++++++-----
>  advice.c              |  7 +++++
>  environment.h         |  7 +++++
>  git.c                 | 11 +++++--
>  t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+), 10 deletions(-)
> 
> Range-diff against v3:
> 1:  55d5559586 < -:  ---------- advice: add --no-advice global option
> -:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for
> --no-* opts
> -:  ---------- > 2:  1b0019026a doc: add spacing around paginate 
> options
> -:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option

Just a small suggestion...  Perhaps the creation factor needs adjusting
for the range diff to actually be produced.  To be clear, I don't mind
that it's missing here.

Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> Hello James,
> ...
>> Range-diff against v3:
>> 1:  55d5559586 < -:  ---------- advice: add --no-advice global option
>> -:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for
>> --no-* opts
>> -:  ---------- > 2:  1b0019026a doc: add spacing around paginate
>> options
>> -:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option
>
> Just a small suggestion...  Perhaps the creation factor needs adjusting
> for the range diff to actually be produced.  To be clear, I don't mind
> that it's missing here.

I see this happen to too many series I see on the list.  There are
cases when the user knows that they are comparing an old and a new
iterations of the same series, e.g. running it from format-patch.
We probably should use a much higher creation factor than default to
run range-diff in such a context.

IOW, this shouldn't have to be done by individual users, but by the
tool.

When I do a post-am check myself, I often use --creation-factor=999
to force matching.

Perhaps something along this line may not be a bad idea.

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] format-patch: run range-diff with larger creation-factor

We see too often that a range-diff added to format-patch output
shows too many "unmatched" patches.  This is because the default
value for creation-factor is set to a relatively low value.

It may be justified for other uses (like you have a yet-to-be-sent
new iteration of your series, and compare it against the 'seen'
branch that has an older iteration, probably with the '--left-only'
option, to pick out only your patches while ignoring the others) of
"range-diff" command, but when the command is run as part of the
format-patch, the user _knows_ and expects that the patches in the
old and the new iterations roughly correspond to each other, so we
can and should use a much higher default.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 builtin/log.c | 2 +-
 range-diff.h  | 6 ++++++
 2 files changed, 7 insertions(+), 1 deletion(-)

diff --git c/builtin/log.c w/builtin/log.c
index 4da7399905..7a019476c3 100644
--- c/builtin/log.c
+++ w/builtin/log.c
@@ -2294,7 +2294,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (creation_factor < 0)
-		creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+		creation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;
 	else if (!rdiff_prev)
 		die(_("the option '%s' requires '%s'"), "--creation-factor", "--range-diff");
 
diff --git c/range-diff.h w/range-diff.h
index 04ffe217be..2f69f6a434 100644
--- c/range-diff.h
+++ w/range-diff.h
@@ -6,6 +6,12 @@
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
+/*
+ * A much higher value than the default, when we KNOW we are comparing
+ * the same series (e.g., used when format-patch calls range-diff).
+ */
+#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999
+
 struct range_diff_options {
 	int creation_factor;
 	unsigned dual_color:1;

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Eric Sunshine]

On Fri, May 3, 2024 at 2:00 PM Junio C Hamano <gitster@pobox.com> wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> > Just a small suggestion...  Perhaps the creation factor needs adjusting
> > for the range diff to actually be produced. [...]
>
> I see this happen to too many series I see on the list.  There are
> cases when the user knows that they are comparing an old and a new
> iterations of the same series, e.g. running it from format-patch.
> We probably should use a much higher creation factor than default to
> run range-diff in such a context.
>
> IOW, this shouldn't have to be done by individual users, but by the
> tool.
>
> Perhaps something along this line may not be a bad idea.
>
> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] format-patch: run range-diff with larger creation-factor
>
> We see too often that a range-diff added to format-patch output
> shows too many "unmatched" patches.  This is because the default
> value for creation-factor is set to a relatively low value.
>
> It may be justified for other uses (like you have a yet-to-be-sent
> new iteration of your series, and compare it against the 'seen'
> branch that has an older iteration, probably with the '--left-only'
> option, to pick out only your patches while ignoring the others) of
> "range-diff" command, but when the command is run as part of the
> format-patch, the user _knows_ and expects that the patches in the
> old and the new iterations roughly correspond to each other, so we
> can and should use a much higher default.
>
> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Ævar had posted[1] pretty much the exact same patch a few years ago.
At the time, I had trouble understanding why `git range-diff` and `git
format-patch --range-dif` would need separate default creation
factors[2], and I still have trouble understanding why they should be
different. Aren't both commands addressing the same use-case of
comparing one version of a series against a subsequent version? In
your response[3], you seemed to agree with that observation and
suggested instead simply increasing the default creation factor for
both commands (which sounds reasonable to me).

[1]: https://lore.kernel.org/git/87y35g9l18.fsf@evledraar.gmail.com/
[2]: https://lore.kernel.org/git/CAPig+cRMiEcXVRYrgp+B3tcDreh41-a5_k0zABe+HNce0G=Cyw@mail.gmail.com/
[3]: https://lore.kernel.org/git/xmqqzhps4uyq.fsf@gitster-ct.c.googlers.com/

> ---
> diff --git c/builtin/log.c w/builtin/log.c
> index 4da7399905..7a019476c3 100644
> --- c/builtin/log.c
> +++ w/builtin/log.c
> @@ -2294,7 +2294,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
>         if (creation_factor < 0)
> -               creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
> +               creation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;
>         else if (!rdiff_prev)
>                 die(_("the option '%s' requires '%s'"), "--creation-factor", "--range-diff");
> diff --git c/range-diff.h w/range-diff.h
> index 04ffe217be..2f69f6a434 100644
> --- c/range-diff.h
> +++ w/range-diff.h
> @@ -6,6 +6,12 @@
>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
>
> +/*
> + * A much higher value than the default, when we KNOW we are comparing
> + * the same series (e.g., used when format-patch calls range-diff).
> + */
> +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

Eric Sunshine <sunshine@sunshineco.com> writes:

> different. Aren't both commands addressing the same use-case of
> comparing one version of a series against a subsequent version? In
> your response[3], you seemed to agree with that observation and
> suggested instead simply increasing the default creation factor for
> both commands (which sounds reasonable to me).

I think Dscho's use case was compare only single updated series of
his with seen that has tons of irrelevant other patches, to have the
command sift the patches belong to other topics away while making
comparison with the older incarnation of his topic.  I never use the
command for such a comparison, but I can imagine that in such a
context lower criteria may help reduce false matches of his new
commits with unrelated commits on 'seen'.

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

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

> Eric Sunshine <sunshine@sunshineco.com> writes:
>
>> different. Aren't both commands addressing the same use-case of
>> comparing one version of a series against a subsequent version? In
>> your response[3], you seemed to agree with that observation and
>> suggested instead simply increasing the default creation factor for
>> both commands (which sounds reasonable to me).
>
> I think Dscho's use case was compare only single updated series of
> his with seen that has tons of irrelevant other patches, to have the
> command sift the patches belong to other topics away while making
> comparison with the older incarnation of his topic.  I never use the
> command for such a comparison, but I can imagine that in such a
> context lower criteria may help reduce false matches of his new
> commits with unrelated commits on 'seen'.

It seems that Dscho was in agreement that format-patch's use case
should try to be more aggressive at least back then.  In the message
in the thread you pointed

 https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/

he does not give us the exact reason why he does not think the "more
aggressive" mode is not suitable for other use cases, though.

A similar thread was raised more recently:

 https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/

Also, there was an attempt to clarify what the "factor" really
meant, but we did not get a real conclusion other than the UNIT to
measure the "factor" is called "percent" (without specifying what
100% is relative to, "percent" does not mean much to guide users
to guess what the right value would be).

 https://lore.kernel.org/git/85snn12q-po05-osqs-n1o0-n6040392q01q@tzk.qr/

So, yes, --creation-factor=<value> is messy, I think a very high
value, much higher than the hardcoded default of 60, is more
appropriate for use in format-patch, but we do not know what bad
effect it would have if we used much higher default everywhere.

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Eric Sunshine]

On Fri, May 3, 2024 at 4:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> It seems that Dscho was in agreement that format-patch's use case
> should try to be more aggressive at least back then.  In the message
> in the thread you pointed
>
>  https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/
>
> he does not give us the exact reason why he does not think the "more
> aggressive" mode is not suitable for other use cases, though.

Having an answer to that question could be helpful.

> A similar thread was raised more recently:
>
>  https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/

I think I missed this thread.

> Also, there was an attempt to clarify what the "factor" really
> meant, but we did not get a real conclusion other than the UNIT to
> measure the "factor" is called "percent" (without specifying what
> 100% is relative to, "percent" does not mean much to guide users
> to guess what the right value would be).
>
>  https://lore.kernel.org/git/85snn12q-po05-osqs-n1o0-n6040392q01q@tzk.qr/

I recall this discussion, as well as its followup (in which I
proclaimed my continuing lack of understanding of what creation-factor
really represents):

https://lore.kernel.org/git/CAPig+cRp4N=6EktoisKAH09aVAPkPgZfHJYcB5pJFJ-CUpBHFA@mail.gmail.com/

> So, yes, --creation-factor=<value> is messy, I think a very high
> value, much higher than the hardcoded default of 60, is more
> appropriate for use in format-patch, but we do not know what bad
> effect it would have if we used much higher default everywhere.

At this point in time, I'm not particularly against giving `git
format-patch` its own default creation-factor.

My only worry (and perhaps it's very minor) is that separate default
creation-factors for `git range-diff` and `git format-patch
--range-diff` could catch the unwary off-guard when invoking `git
range-diff` to manually check the difference between an old an new
version of a series before finally invoking `git format-patch
--range-diff` to actually create the patches for sending. (I've done
this myself on a very few occasions, but perhaps not often enough to
warrant the concern(?).)

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Johannes Schindelin]

[-- Attachment #1: Type: text/plain, Size: 1767 bytes --]

Hi,

On Fri, 3 May 2024, Eric Sunshine wrote:

> On Fri, May 3, 2024 at 4:08 PM Junio C Hamano <gitster@pobox.com> wrote:
> > It seems that Dscho was in agreement that format-patch's use case
> > should try to be more aggressive at least back then.  In the message
> > in the thread you pointed
> >
> >  https://lore.kernel.org/git/nycvar.QRO.7.76.6.1903211209280.41@tvgsbejvaqbjf.bet/
> >
> > he does not give us the exact reason why he does not think the "more
> > aggressive" mode is not suitable for other use cases, though.
>
> Having an answer to that question could be helpful.

Sure.

The `creation factor` essentially tells Git how confident the caller is that the
two provided commit ranges contain only matching pairs of patches, no
matter how different they look.

When calling `git format-patch --range-diff`, we can be _quite_ certain.
Not necessarily 100% (in which case the creation factor would need to be a
lot higher than 100), as it does happen that contributors have to drop a
patch or two, and add a patch or two, sometimes in the same iteration.

When calling `git range-diff` in general, we can be less certain about
that. In fact, users like me often call `git range-diff` to _determine_
whether there are obvious matches (for example, to see whether commits in
GitGitGadget PRs have corresponding commits in `seen`).

That difference in certainty is the entire reason why I contend that
`range-diff` and `format-patch --range-diff` need different defaults for
the creation factor.

> > A similar thread was raised more recently:
> >
> >  https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/
>
> I think I missed this thread.

Heh. I had forgotten about it.

Ciao,
Johannes

Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> That difference in certainty is the entire reason why I contend that
> `range-diff` and `format-patch --range-diff` need different defaults for
> the creation factor.
>
>> > A similar thread was raised more recently:
>> >
>> >  https://lore.kernel.org/git/rq6919s9-qspp-rn6o-n704-r0400q10747r@tzk.qr/
>>
>> I think I missed this thread.
>
> Heh. I had forgotten about it.

So what's the conclusion of this discussion (have we reached one
yet)?

to me it sounds like everybody is on board to raise the default used
for format-patch.  In the old thread I said "I use something
unreasonably high like 9999", but my "unreasonably high" number
these days is 999, which was used in the earlier weatherbaloon
patch.  The old thread ended with an academic "we know different
defaults are appropriate, but what is the right number then?" but
anything higher (like 999 or even 9999) is better than the default
for range-diff which is 60, I would think.

Here is the patch, this time with a bit of documentation stolen from
the old weather-balloon by Ævar.

----- >8 --------- >8 --------- >8 --------- >8 -----
Subject: [PATCH] format-patch: run range-diff with larger creation-factor

We see too often that a range-diff added to format-patch output
shows too many "unmatched" patches.  This is because the default
value for creation-factor is set to a relatively low value.

It may be justified for other uses (like you have a yet-to-be-sent
new iteration of your series, and compare it against the 'seen'
branch that has an older iteration, probably with the '--left-only'
option, to pick out only your patches while ignoring the others) of
"range-diff" command, but when the command is run as part of the
format-patch, the user _knows_ and expects that the patches in the
old and the new iterations roughly correspond to each other, so we
can and should use a much higher default.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 Documentation/git-format-patch.txt | 5 +++++
 builtin/log.c                      | 2 +-
 range-diff.h                       | 6 ++++++
 3 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/Documentation/git-format-patch.txt b/Documentation/git-format-patch.txt
index 728bb3821c..b72f87b114 100644
--- a/Documentation/git-format-patch.txt
+++ b/Documentation/git-format-patch.txt
@@ -346,6 +346,11 @@ material (this may change in the future).
 	between the previous and current series of patches by adjusting the
 	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
 	for details.
++
+Defaults to 999 (the linkgit:git-range-diff[1] uses 60), as the use
+case is to show comparison with an older iteration of the same
+topic and the tool should find more correspondence between the two
+sets of patches.
 
 --notes[=<ref>]::
 --no-notes::
diff --git a/builtin/log.c b/builtin/log.c
index c0a8bb95e9..73608ffef9 100644
--- a/builtin/log.c
+++ b/builtin/log.c
@@ -2274,7 +2274,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 	}
 
 	if (creation_factor < 0)
-		creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
+		creation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;
 	else if (!rdiff_prev)
 		die(_("the option '%s' requires '%s'"), "--creation-factor", "--range-diff");
 
diff --git a/range-diff.h b/range-diff.h
index 04ffe217be..2f69f6a434 100644
--- a/range-diff.h
+++ b/range-diff.h
@@ -6,6 +6,12 @@
 
 #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
 
+/*
+ * A much higher value than the default, when we KNOW we are comparing
+ * the same series (e.g., used when format-patch calls range-diff).
+ */
+#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999
+
 struct range_diff_options {
 	int creation_factor;
 	unsigned dual_color:1;
-- 
2.45.0-31-gd4cc1ec35f

Re: Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Dragan Simic]

Hello Junio,

On 2024-05-06 18:40, Junio C Hamano wrote:
> So what's the conclusion of this discussion (have we reached one
> yet)?
> 
> to me it sounds like everybody is on board to raise the default used
> for format-patch.  In the old thread I said "I use something
> unreasonably high like 9999", but my "unreasonably high" number
> these days is 999, which was used in the earlier weatherbaloon
> patch.  The old thread ended with an academic "we know different
> defaults are appropriate, but what is the right number then?" but
> anything higher (like 999 or even 9999) is better than the default
> for range-diff which is 60, I would think.
> 
> Here is the patch, this time with a bit of documentation stolen from
> the old weather-balloon by Ævar.

After going through all the linked discussions, I think that introducing
the new default value for the creation factor in "git format-patch" is
the way to go.  However, I still have one remaining concern.

See, the creation factor is described in the documentation as some fudge
factor, specified as a percentage.  Without going through the actual
source code in detail, a question that pops up in my mind is why do we
need to use 999 or 9999 as the new default value?  Shouldn't 99 or 100
be good enough instead, if it's a percentage?  I'd assume that the same
question might be asked by other Git users.

Admittedly, I haven't produced numerous range diffs, but for what I did,
90 worked very well as the creation factor value.  I even created some
aliases that specify 90 as the creation factor value.

> ----- >8 --------- >8 --------- >8 --------- >8 -----
> Subject: [PATCH] format-patch: run range-diff with larger 
> creation-factor
> 
> We see too often that a range-diff added to format-patch output
> shows too many "unmatched" patches.  This is because the default
> value for creation-factor is set to a relatively low value.
> 
> It may be justified for other uses (like you have a yet-to-be-sent
> new iteration of your series, and compare it against the 'seen'
> branch that has an older iteration, probably with the '--left-only'
> option, to pick out only your patches while ignoring the others) of
> "range-diff" command, but when the command is run as part of the
> format-patch, the user _knows_ and expects that the patches in the
> old and the new iterations roughly correspond to each other, so we
> can and should use a much higher default.
> 
> Signed-off-by: Junio C Hamano <gitster@pobox.com>
> ---
>  Documentation/git-format-patch.txt | 5 +++++
>  builtin/log.c                      | 2 +-
>  range-diff.h                       | 6 ++++++
>  3 files changed, 12 insertions(+), 1 deletion(-)
> 
> diff --git a/Documentation/git-format-patch.txt
> b/Documentation/git-format-patch.txt
> index 728bb3821c..b72f87b114 100644
> --- a/Documentation/git-format-patch.txt
> +++ b/Documentation/git-format-patch.txt
> @@ -346,6 +346,11 @@ material (this may change in the future).
>  	between the previous and current series of patches by adjusting the
>  	creation/deletion cost fudge factor. See linkgit:git-range-diff[1])
>  	for details.
> ++
> +Defaults to 999 (the linkgit:git-range-diff[1] uses 60), as the use
> +case is to show comparison with an older iteration of the same
> +topic and the tool should find more correspondence between the two
> +sets of patches.
> 
>  --notes[=<ref>]::
>  --no-notes::
> diff --git a/builtin/log.c b/builtin/log.c
> index c0a8bb95e9..73608ffef9 100644
> --- a/builtin/log.c
> +++ b/builtin/log.c
> @@ -2274,7 +2274,7 @@ int cmd_format_patch(int argc, const char
> **argv, const char *prefix)
>  	}
> 
>  	if (creation_factor < 0)
> -		creation_factor = RANGE_DIFF_CREATION_FACTOR_DEFAULT;
> +		creation_factor = CREATION_FACTOR_FOR_THE_SAME_SERIES;
>  	else if (!rdiff_prev)
>  		die(_("the option '%s' requires '%s'"), "--creation-factor", 
> "--range-diff");
> 
> diff --git a/range-diff.h b/range-diff.h
> index 04ffe217be..2f69f6a434 100644
> --- a/range-diff.h
> +++ b/range-diff.h
> @@ -6,6 +6,12 @@
> 
>  #define RANGE_DIFF_CREATION_FACTOR_DEFAULT 60
> 
> +/*
> + * A much higher value than the default, when we KNOW we are comparing
> + * the same series (e.g., used when format-patch calls range-diff).
> + */
> +#define CREATION_FACTOR_FOR_THE_SAME_SERIES 999
> +
>  struct range_diff_options {
>  	int creation_factor;
>  	unsigned dual_color:1;

Re: Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

Dragan Simic <dsimic@manjaro.org> writes:

> See, the creation factor is described in the documentation as some fudge
> factor, specified as a percentage.  Without going through the actual
> source code in detail, a question that pops up in my mind is why do we
> need to use 999 or 9999 as the new default value?  Shouldn't 99 or 100
> be good enough instead, if it's a percentage?  I'd assume that the same
> question might be asked by other Git users.

It is very much deliberate to choose a value beyond 100.  Setting
the value to such a large value was designed to force somebody to
ask that question: "what does the value mean?" [*], and in a sense
it already has achieved one half of its objective ;-).

We never had an satisfactory update to the range-diff documentation
when we discussed this parameter in the past.  It was stated that
the unit to express the value for creation-factor was called
"percent", but it seems that nobody had a good explanation that can
be given to a layperson what that percent means (e.g., what is it
relative to?  what it means to have that value at 100% as opposed to
say 50%?).  Somebody should come up with an easy-to-understand
explanation suitable for users after reading and digesting the
overly technical "algorithm" section of the manual, and forcing
ourselves to do a good job at it is the other half of its objective.


[Footnote]

 * Having more than 100 as a value that is measured in "percent" is
   not unusual, by the way.  You can zoom into a picture at
   magnification level of 400%, for example.

Re: Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Dragan Simic]

On 2024-05-07 02:21, Junio C Hamano wrote:
> Dragan Simic <dsimic@manjaro.org> writes:
> 
>> See, the creation factor is described in the documentation as some 
>> fudge
>> factor, specified as a percentage.  Without going through the actual
>> source code in detail, a question that pops up in my mind is why do we
>> need to use 999 or 9999 as the new default value?  Shouldn't 99 or 100
>> be good enough instead, if it's a percentage?  I'd assume that the 
>> same
>> question might be asked by other Git users.
> 
> It is very much deliberate to choose a value beyond 100.  Setting
> the value to such a large value was designed to force somebody to
> ask that question: "what does the value mean?" [*], and in a sense
> it already has achieved one half of its objective ;-).

Ah, I see, it's also meant to provoke users' curiosity a bit. :)

> We never had an satisfactory update to the range-diff documentation
> when we discussed this parameter in the past.  It was stated that
> the unit to express the value for creation-factor was called
> "percent", but it seems that nobody had a good explanation that can
> be given to a layperson what that percent means (e.g., what is it
> relative to?  what it means to have that value at 100% as opposed to
> say 50%?).  Somebody should come up with an easy-to-understand
> explanation suitable for users after reading and digesting the
> overly technical "algorithm" section of the manual, and forcing
> ourselves to do a good job at it is the other half of its objective.

Yes, the whole percentage thing is a bit unclear.  I'm going through
the ALGORITHM section of the git-range-diff(1) man page, which could
also use some wording and formatting improvements.  I'll see to send
a patch that improves it a bit later.

> [Footnote]
> 
>  * Having more than 100 as a value that is measured in "percent" is
>    not unusual, by the way.  You can zoom into a picture at
>    magnification level of 400%, for example.

Of course.  For example, I use 120% as the default scaling in my web
browser, to compensate for the rather high value of the screen DPI.

Re: Re* [PATCH v4 0/3] advice: add "all" option to disable all hints

[Dragan Simic]

Hello Eric,

On 2024-05-03 23:24, Eric Sunshine wrote:
> On Fri, May 3, 2024 at 4:08 PM Junio C Hamano <gitster@pobox.com> 
> wrote:
>> So, yes, --creation-factor=<value> is messy, I think a very high
>> value, much higher than the hardcoded default of 60, is more
>> appropriate for use in format-patch, but we do not know what bad
>> effect it would have if we used much higher default everywhere.
> 
> At this point in time, I'm not particularly against giving `git
> format-patch` its own default creation-factor.
> 
> My only worry (and perhaps it's very minor) is that separate default
> creation-factors for `git range-diff` and `git format-patch
> --range-diff` could catch the unwary off-guard when invoking `git
> range-diff` to manually check the difference between an old an new
> version of a series before finally invoking `git format-patch
> --range-diff` to actually create the patches for sending. (I've done
> this myself on a very few occasions, but perhaps not often enough to
> warrant the concern(?).)

I agree that having two different default values for the creation
factor in "git format-patch" and "git range-diff" is a valid concern,
but I think that the resulting benefits, in form of patch submissions
that don't lack the range diff contents, outweigh this concern.

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 1290 bytes --]

Hello James,

James Liu <james@jamesliu.io> writes:

> Hi,
>
> Thank you all for the comprehensive feedback. This is v4 of the patch
> series to introduce a --no-advice option for silencing advice hints.
>
> The main changes are:
>
> - Two preliminary commits to reorder/clean up the options documentation
>   and usage string.
> - Caching the value of GIT_ADVICE.
> - Adding tests which explicitly set GIT_ADVICE to false and true.
>
> Cheers,
> James
>

This version looks good to me.

Thanks
Karthik

> James Liu (3):
>   doc: clean up usage documentation for --no-* opts
>   doc: add spacing around paginate options
>   advice: add --no-advice global option
>
>  Documentation/git.txt | 18 +++++++-----
>  advice.c              |  7 +++++
>  environment.h         |  7 +++++
>  git.c                 | 11 +++++--
>  t/t0018-advice.sh     | 68 +++++++++++++++++++++++++++++++++++++++++++
>  5 files changed, 101 insertions(+), 10 deletions(-)
>
> Range-diff against v3:
> 1:  55d5559586 < -:  ---------- advice: add --no-advice global option
> -:  ---------- > 1:  ae3f45dadc doc: clean up usage documentation for --no-* opts
> -:  ---------- > 2:  1b0019026a doc: add spacing around paginate options
> -:  ---------- > 3:  31e73e6c0e advice: add --no-advice global option
> --
> 2.44.0

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[James Liu]

On Sat May 4, 2024 at 12:35 AM AEST, Karthik Nayak wrote:
> Hello James,
>
> James Liu <james@jamesliu.io> writes:
>
> > Hi,
> >
> > Thank you all for the comprehensive feedback. This is v4 of the patch
> > series to introduce a --no-advice option for silencing advice hints.
> >
> > The main changes are:
> >
> > - Two preliminary commits to reorder/clean up the options documentation
> >   and usage string.
> > - Caching the value of GIT_ADVICE.
> > - Adding tests which explicitly set GIT_ADVICE to false and true.
> >
> > Cheers,
> > James
> >
>
> This version looks good to me.
>
> Thanks
> Karthik
>

Thanks for the review Karthik!

Cheers,
James

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

> Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

Huh?  Do we have "all" option that disable everything?

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[James Liu]

On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
> > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>
> Huh?  Do we have "all" option that disable everything?

Oops, I think I mistakenly copied the subject from the original cover
letter. I don't suppose this warrants a v5, does it?

Cheers,
James

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[James Liu]

On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:
> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
> > > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
> >
> > Huh?  Do we have "all" option that disable everything?
>
> Oops, I think I mistakenly copied the subject from the original cover
> letter. I don't suppose this warrants a v5, does it?
>
> Cheers,
> James

Ahh, I didn't see Karthik's feedback. Will prepare a new version.

Cheers,
James

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

"James Liu" <james@jamesliu.io> writes:

> On Mon May 6, 2024 at 9:20 AM AEST, James Liu wrote:
>> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
>> > > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>> >
>> > Huh?  Do we have "all" option that disable everything?
>>
>> Oops, I think I mistakenly copied the subject from the original cover
>> letter. I don't suppose this warrants a v5, does it?
>>
>> Cheers,
>> James
>
> Ahh, I didn't see Karthik's feedback. Will prepare a new version.

I count two messages from Karthik for v4, one of them being "this
round looks good", to which you said "thanks".

If you mean the other comment about a non-sentence in the proposed
log message of [2/3], the copy I have in 'seen' says this:

    doc: add spacing around paginate options

    Make the documentation page consistent with the usage string printed by
    "git help git" and consistent with the description of "[-v | --version]"
    option.

    Signed-off-by: James Liu <james@jamesliu.io>
    Signed-off-by: Junio C Hamano <gitster@pobox.com>

If that looks good enough to both of you, then we can probably
declare victory and mark the topic for 'next'.

Thanks.

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[James Liu]

On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:
> I count two messages from Karthik for v4, one of them being "this
> round looks good", to which you said "thanks".
>
> If you mean the other comment about a non-sentence in the proposed
> log message of [2/3], the copy I have in 'seen' says this:
>
>     doc: add spacing around paginate options
>
>     Make the documentation page consistent with the usage string printed by
>     "git help git" and consistent with the description of "[-v | --version]"
>     option.
>
>     Signed-off-by: James Liu <james@jamesliu.io>
>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>
> If that looks good enough to both of you, then we can probably
> declare victory and mark the topic for 'next'.
>
> Thanks.

Yes I'm happy with that. Thanks Junio!

Cheers,
James

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Karthik Nayak]

[-- Attachment #1: Type: text/plain, Size: 940 bytes --]

Hello,

"James Liu" <james@jamesliu.io> writes:

> On Tue May 7, 2024 at 2:47 AM AEST, Junio C Hamano wrote:
>> I count two messages from Karthik for v4, one of them being "this
>> round looks good", to which you said "thanks".
>>
>> If you mean the other comment about a non-sentence in the proposed
>> log message of [2/3], the copy I have in 'seen' says this:
>>
>>     doc: add spacing around paginate options
>>
>>     Make the documentation page consistent with the usage string printed by
>>     "git help git" and consistent with the description of "[-v | --version]"
>>     option.
>>
>>     Signed-off-by: James Liu <james@jamesliu.io>
>>     Signed-off-by: Junio C Hamano <gitster@pobox.com>
>>
>> If that looks good enough to both of you, then we can probably
>> declare victory and mark the topic for 'next'.
>>
>> Thanks.
>
> Yes I'm happy with that. Thanks Junio!
>
> Cheers,
> James

Looks good to me too :)

Thanks
Karthik

[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]

Re: [PATCH v4 0/3] advice: add "all" option to disable all hints

[Junio C Hamano]

"James Liu" <james@jamesliu.io> writes:

> On Sat May 4, 2024 at 3:25 AM AEST, Junio C Hamano wrote:
>> > Subject: Re: [PATCH v4 0/3] advice: add "all" option to disable all hints
>>
>> Huh?  Do we have "all" option that disable everything?
>
> Oops, I think I mistakenly copied the subject from the original cover
> letter. I don't suppose this warrants a v5, does it?

No, I was just teasing you ;-).