git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
DSDragan Simic <dsimic@manjaro.org>
Date
Apr 29, 2024, 04:15 UTC
Message-ID
<37512328b1f3db4e8075bdb4beeb8929@manjaro.org>
In-Reply-To
<20240429010925.93205-2-james@jamesliu.io>
Hello James,
Please see my comments below.
On 2024-04-29 03:09, James Liu wrote:
Show 30 quoted lines
> 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.

Show 34 quoted lines
>  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/

Show 15 quoted lines
> +
> +	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.

Show 16 quoted lines
> 
>  /*
>   * 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.

Show 48 quoted lines
> <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.

Previous: James LiuNext: James Liu
Message 20 of 67 in “advice: add "all" option to disable all hints”
  1. 0/2 advice: add "all" option to disable all hintsJames Liu, Apr 24, 2024
  2. 1/2 advice: allow advice type to be provided in testsJames Liu, Apr 24, 2024
  3. Patrick SteinhardtApr 24, 2024
  4. 2/2 advice: add "all" option to disable all hintsJames Liu, Apr 24, 2024
  5. Patrick SteinhardtApr 24, 2024
  6. Junio C HamanoApr 24, 2024
  7. Patrick SteinhardtApr 24, 2024
  8. Phillip WoodApr 24, 2024
  9. Patrick SteinhardtApr 24, 2024
  10. Junio C HamanoApr 24, 2024
  11. Patrick SteinhardtApr 25, 2024
  12. Junio C HamanoApr 25, 2024
  13. Dragan SimicApr 24, 2024
  14. Dragan SimicApr 24, 2024
  15. Junio C HamanoApr 24, 2024
  16. Patrick SteinhardtApr 25, 2024
  17. Dragan SimicApr 24, 2024
  18. 0/1 advice: add --no-advice global optionJames Liu, Apr 29, 2024
  19. 1/1 advice: add --no-advice global optionJames Liu, Apr 29, 2024
  20. Dragan SimicApr 29, 2024
  21. James LiuApr 29, 2024
  22. Dragan SimicApr 29, 2024
  23. Dragan SimicApr 29, 2024
  24. Eric SunshineApr 29, 2024
  25. Dragan SimicApr 29, 2024
  26. Jeff KingApr 29, 2024
  27. Dragan SimicApr 29, 2024
  28. Junio C HamanoApr 29, 2024
  29. James LiuApr 30, 2024
  30. Junio C HamanoApr 29, 2024
  31. Rubén JustoApr 29, 2024
  32. Dragan SimicApr 29, 2024
  33. 0/1 advice: add --no-advice global optionJames Liu, Apr 30, 2024
  34. 1/1 advice: add --no-advice global optionJames Liu, Apr 30, 2024
  35. Patrick SteinhardtApr 30, 2024
  36. Dragan SimicApr 30, 2024
  37. Junio C HamanoApr 30, 2024
  38. 0/3 advice: add "all" option to disable all hintsJames Liu, May 3, 2024
  39. 1/3 doc: clean up usage documentation for --no-* optsJames Liu, May 3, 2024
  40. Junio C HamanoMay 3, 2024
  41. James LiuMay 6, 2024
  42. 2/3 doc: add spacing around paginate optionsJames Liu, May 3, 2024
  43. Karthik NayakMay 3, 2024
  44. Junio C HamanoMay 3, 2024
  45. 3/3 advice: add --no-advice global optionJames Liu, May 3, 2024
  46. 4/3 t0018: two small fixesJunio C Hamano, May 8, 2024
  47. Dragan SimicMay 3, 2024
  48. Re* [PATCH v4 0/3] advice: add "all" option to disable all hintsJunio C Hamano, May 3, 2024
  49. Eric SunshineMay 3, 2024
  50. Junio C HamanoMay 3, 2024
  51. Junio C HamanoMay 3, 2024
  52. Eric SunshineMay 3, 2024
  53. Johannes SchindelinMay 5, 2024
  54. Re* Re* [PATCH v4 0/3] advice: add "all" option to disable all hintsJunio C Hamano, May 6, 2024
  55. Dragan SimicMay 7, 2024
  56. Junio C HamanoMay 7, 2024
  57. Dragan SimicMay 7, 2024
  58. Dragan SimicMay 7, 2024
  59. Karthik NayakMay 3, 2024
  60. James LiuMay 5, 2024
  61. Junio C HamanoMay 3, 2024
  62. James LiuMay 5, 2024
  63. James LiuMay 6, 2024
  64. Junio C HamanoMay 6, 2024
  65. James LiuMay 6, 2024
  66. Karthik NayakMay 7, 2024
  67. Junio C HamanoMay 6, 2024

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.