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