From: Dragan Simic Date: Mon, 29 Apr 2024 04:15:33 GMT Subject: Re: [PATCH v2 1/1] advice: add --no-advice global option 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: > 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 > --- > 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[=]] [--html-path] [--man-path] [--info-path] > [-p|--paginate|-P|--no-pager] [--no-replace-objects] [--bare] > [--git-dir=] [--work-tree=] [--namespace=] > - [--config-env==] [] > + [--config-env==] [--no-advice] [] 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." 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." 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 `` 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[=]] [--html-path] [--man-path] > [--info-path]\n" > " [-p | --paginate | -P | --no-pager] > [--no-replace-objects] [--bare]\n" > " [--git-dir=] [--work-tree=] > [--namespace=]\n" > - " [--config-env==] []"); > + " [--config-env==] [--no-advice] Obviously, additional new configuration option "--advice" would be also added here, mutually exclusive with "--no-advice", and into the source code below. > []"); > > 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.