| Message ID | 20240430014724.83813-2-james@jamesliu.io (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | advice: add --no-advice global option | expand |
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 > >
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/
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.
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
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(-)