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

From: Junio C Hamano <gitster@pobox.com>
Date: Apr 30, 2024, 16:29 UTC
Message-ID: <xmqqbk5qstjt.fsf@gitster.g>
In-Reply-To: <20240430014724.83813-2-james@jamesliu.io>

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.

Show 10 quoted lines

> 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.

Show 21 quoted lines

> 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.

Show 6 quoted lines

> +/*
> + * 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.

Show 10 quoted lines

> 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.

Show 10 quoted lines

> 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.

Show 6 quoted lines

> +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.