[PATCH v3] advice: use global config for default branch name

[PATCH v3] advice: use global config for default branch name

[Vsevolod Myalitsin]

Some advice messages suggest disabling the advice with
"git config set advice.<name> false", even when the
corresponding configuration should be set at a different scope.

Add a scope hint to advice settings so that the suggested
command uses the appropriate config scope.

Pass the advice setting itself to vadvise() instead of passing
its fields separately. Use NULL for advise() calls that are not
associated with an advice setting.

Signed-off-by: Vsevolod Myalitsin <ub4nal@mail.ru>
---
 advice.c | 43 ++++++++++++++++++++++++++++++++-----------
 1 file changed, 32 insertions(+), 11 deletions(-)

diff --git a/advice.c b/advice.c
index 63bf8b0c5f..80cc388215 100644
--- a/advice.c
+++ b/advice.c
@@ -40,10 +40,19 @@ enum advice_level {
 	ADVICE_LEVEL_ENABLED,
 };
 
-static struct {
+enum advice_scope {
+	ADVICE_SCOPE_LOCAL = 0,
+	ADVICE_SCOPE_GLOBAL,
+	ADVICE_SCOPE_SYSTEM,
+};
+
+struct advice_setting {
 	const char *key;
+	enum advice_scope scope_hint;
 	enum advice_level level;
-} advice_setting[] = {
+};
+
+static struct advice_setting advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
@@ -51,7 +60,7 @@ static struct {
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
-	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName" },
+	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName", ADVICE_SCOPE_GLOBAL },
 	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
 	[ADVICE_DIVERGING]				= { "diverging" },
 	[ADVICE_FETCH_SET_HEAD_WARN]			= { "fetchRemoteHEADWarn" },
@@ -96,18 +105,31 @@ static struct {
 
 static const char turn_off_instructions[] =
 N_("\n"
-   "Disable this message with \"git config set advice.%s false\"");
+   "Disable this message with \"git config set%s advice.%s false\"");
 
-static void vadvise(const char *advice, int display_instructions,
-		    const char *key, va_list params)
+static void vadvise(const char *advice,
+	const struct advice_setting *setting, va_list params)
 {
 	struct strbuf buf = STRBUF_INIT;
 	const char *cp, *np;
 
 	strbuf_vaddf(&buf, advice, params);
 
-	if (display_instructions)
-		strbuf_addf(&buf, turn_off_instructions, key);
+	if (setting && setting->level == 0) {
+		const char *scope = "";
+		switch (setting->scope_hint) {
+			case ADVICE_SCOPE_LOCAL:
+				break;
+			case ADVICE_SCOPE_GLOBAL:
+				scope = " --global";
+				break;
+			case ADVICE_SCOPE_SYSTEM:
+				scope = " --system";
+				break;
+		}
+		strbuf_addf(&buf, turn_off_instructions,
+				scope, setting->key);
+	}
 
 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
@@ -126,7 +148,7 @@ void advise(const char *advice, ...)
 {
 	va_list params;
 	va_start(params, advice);
-	vadvise(advice, 0, "", params);
+	vadvise(advice, NULL, params);
 	va_end(params);
 }
 
@@ -155,8 +177,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
-		params);
+	vadvise(advice, &advice_setting[type], params);
 	va_end(params);
 }
 
-- 
2.50.1

Re: [PATCH v3] advice: use global config for default branch name

[Jeff King]

On Sun, Aug 29, 2027 at 03:49:58AM +0300, Vsevolod Myalitsin wrote:

> Add a scope hint to advice settings so that the suggested
> command uses the appropriate config scope.
> 
> Pass the advice setting itself to vadvise() instead of passing
> its fields separately. Use NULL for advise() calls that are not
> associated with an advice setting.

Thanks, this looks OK to me. A few small nits/observations:

> @@ -96,18 +105,31 @@ static struct {
>  
>  static const char turn_off_instructions[] =
>  N_("\n"
> -   "Disable this message with \"git config set advice.%s false\"");
> +   "Disable this message with \"git config set%s advice.%s false\"");

Translators will need to update their message translations, and I wonder
if seeing this "set%s" in isolation might be confusing. Probably it
should be obvious that they should leave everything within the
double-quotes alone. But the alternative is adding a comment with
"TRANSLATORS" in it, I think.

See below, also.

> -	if (display_instructions)
> -		strbuf_addf(&buf, turn_off_instructions, key);
> +	if (setting && setting->level == 0) {

I left this comparison as something like "!setting->level" in my earlier
suggestion, which I think would be OK. But really it is an enum, and if
we are going to use "==" we should probably spell out the whole name
rather than 0, like:

  if (setting && setting->level == ADVICE_LEVEL_NONE)

> +		const char *scope = "";
> +		switch (setting->scope_hint) {
> +			case ADVICE_SCOPE_LOCAL:
> +				break;
> +			case ADVICE_SCOPE_GLOBAL:
> +				scope = " --global";
> +				break;
> +			case ADVICE_SCOPE_SYSTEM:
> +				scope = " --system";
> +				break;
> +		}

I had somehow hoped we could reuse the existing CONFIG_SCOPE enum
without having to redeclare it ourselves. But there are a lot more
scopes than these three! On the other hand, I think it would be possible
to use config_scope_name() to convert them into options.

That makes the translation more lego-like, but maybe it would actually
make it easier to understand, because we could pull the whole command
out into a single placeholder. Like:

diff --git a/advice.c b/advice.c
index cbb0f2f428..789f01c7e1 100644
--- a/advice.c
+++ b/advice.c
@@ -40,15 +40,9 @@ enum advice_level {
 	ADVICE_LEVEL_ENABLED,
 };
 
-enum advice_scope {
-	ADVICE_SCOPE_LOCAL = 0,
-	ADVICE_SCOPE_GLOBAL,
-	ADVICE_SCOPE_SYSTEM,
-};
-
 struct advice_setting {
 	const char *key;
-	enum advice_scope scope_hint;
+	enum config_scope scope_hint;
 	enum advice_level level;
 };
 
@@ -60,7 +54,7 @@ static struct advice_setting advice_setting[] = {
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
-	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName", ADVICE_SCOPE_GLOBAL },
+	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName", CONFIG_SCOPE_GLOBAL },
 	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
 	[ADVICE_DIVERGING]				= { "diverging" },
 	[ADVICE_FETCH_SET_HEAD_WARN]			= { "fetchRemoteHEADWarn" },
@@ -105,7 +99,7 @@ static struct advice_setting advice_setting[] = {
 
 static const char turn_off_instructions[] =
 N_("\n"
-   "Disable this message with \"git config set%s advice.%s false\"");
+   "Disable this message with \"%s");
 
 static void vadvise(const char *advice,
 	const struct advice_setting *setting, va_list params)
@@ -116,19 +110,16 @@ static void vadvise(const char *advice,
 	strbuf_vaddf(&buf, advice, params);
 
 	if (setting && setting->level == 0) {
-		const char *scope = "";
-		switch (setting->scope_hint) {
-			case ADVICE_SCOPE_LOCAL:
-				break;
-			case ADVICE_SCOPE_GLOBAL:
-				scope = " --global";
-				break;
-			case ADVICE_SCOPE_SYSTEM:
-				scope = " --system";
-				break;
-		}
-		strbuf_addf(&buf, turn_off_instructions,
-				scope, setting->key);
+		struct strbuf cmd = STRBUF_INIT;
+
+		strbuf_addstr(&cmd, "git config set");
+		if (setting->scope_hint &&
+		    setting->scope_hint != CONFIG_SCOPE_LOCAL)
+			strbuf_addf(&cmd, " --%s",
+				    config_scope_name(setting->scope_hint));
+		strbuf_addf(&cmd, "advice.%s false", setting->key);
+		strbuf_addf(&buf, turn_off_instructions, cmd.buf);
+		strbuf_release(&cmd);
 	}
 
 	for (cp = buf.buf; *cp; cp = np) {


Having typed that, I'm not sure if it is more or less confusing. It does
avoid replicating the CONFIG_SCOPE enum. There is some lego-string
construction, but it is all within the code and for the non-translated
command. It would obviously be nonsense with CONFIG_SCOPE_FILE, but
there is no reason to think we'd ever pass that.

So I dunno. I could take or leave it as a further cleanup.

-Peff

Re: [PATCH v3] advice: use global config for default branch name

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I had somehow hoped we could reuse the existing CONFIG_SCOPE enum
> without having to redeclare it ourselves. But there are a lot more
> scopes than these three! On the other hand, I think it would be possible
> to use config_scope_name() to convert them into options.

I had the same thought, and do not have strong opinion myself either
way.

For everything else you suggested in your review, I think we would
want a hopefully small and final reroll.

Thanks.

Re: [PATCH v3] advice: use global config for default branch name

[Vsevolod Myalitsin]

Hi Peff,

> Translators will need to update their message translations, and I wonder
> if seeing this "set%s" in isolation might be confusing.

I agree that using a single placeholder for the whole command is clearer for translators. I'll use this approach.

> But really it is an enum, and if we are going to use "==" we should
> probably spell out the whole name rather than 0, like:

if (setting && setting->level == ADVICE_LEVEL_NONE)

I simply forgot to include this change in the patch. I'll change it to use ADVICE_LEVEL_NONE.

> I had somehow hoped we could reuse the existing CONFIG_SCOPE enum
> without having to redeclare it ourselves.

One concern about reusing enum config_scope: since CONFIG_SCOPE_UNKNOWN is 0, all existing advice_setting entries without an explicitly specified scope_hint would default to CONFIG_SCOPE_UNKNOWN rather than CONFIG_SCOPE_LOCAL.

I believe this is incorrect, since the existing behavior is local scope by default. However, if you consider CONFIG_SCOPE_UNKNOWN appropriate here and it satisfies the intended requirements, I have no objection to using the existing enum.

Thanks,
Vsevolod

Re: [PATCH v3] advice: use global config for default branch name

[Jeff King]

On Thu, Sep 10, 2026 at 12:22:13AM +0300, Vsevolod Myalitsin wrote:

> > I had somehow hoped we could reuse the existing CONFIG_SCOPE enum
> > without having to redeclare it ourselves.
> 
> One concern about reusing enum config_scope: since
> CONFIG_SCOPE_UNKNOWN is 0, all existing advice_setting entries without
> an explicitly specified scope_hint would default to
> CONFIG_SCOPE_UNKNOWN rather than CONFIG_SCOPE_LOCAL.
> 
> I believe this is incorrect, since the existing behavior is local
> scope by default. However, if you consider CONFIG_SCOPE_UNKNOWN
> appropriate here and it satisfies the intended requirements, I have no
> objection to using the existing enum.

Any config can work at any scope. These are really just recommendations
on where the user might want to write a value. So I think it would be
fine to treat UNKNOWN as "just suggest the default location for
writing", as we do now.

TBH, I am not really sure what the criteria are for suggesting one
advice option as --global or not. I'd think most of them are about
squelching advice that the user already knows about, and thus they would
go into --global. I didn't really follow the earlier discussion that led
up to this patch, though.

-Peff

Re: [PATCH v3] advice: use global config for default branch name

[Vsevolod Myalitsin]

> Any config can work at any scope. These are really just recommendations on where the user might want to write a value.

Looking at it from that perspective, this seems obvious to me now. I'll reuse the existing CONFIG_SCOPE enum and treat CONFIG_SCOPE_UNKNOWN as the default location.

> TBH, I am not really sure what the criteria are for suggesting one advice option as --global or not.

The motivation for this patch was that "advice.defaultBranchName" currently suggests:

"git config set advice.defaultBranchName false"

Without an explicit scope, this writes to the local ".git/config". After the repository has been initialized, that particular scenario won't occur again in that repository. However, when the user initializes a new repository, the advice will appear again, which may make them wonder why they ran the command in the first place.

Therefore, I think "defaultBranchName" should suggest using the global scope.

> I'd think most of them are about squelching advice that the user already knows about, and thus they would go into --global.

I agree that this may apply to many of the advice messages. For this patch, though, I'm specifically addressing "defaultBranchName", where the global scope seems appropriate for the reason above.

> I didn't really follow the earlier discussion that led up to this patch, though.

The original motivation was specifically the behavior of "defaultBranchName" after initializing a new repository, which is why I considered a global scope recommendation here.

Vsevolod

Re: [PATCH v3] advice: use global config for default branch name

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> Some advice messages suggest disabling the advice with
> "git config set advice.<name> false", even when the
> corresponding configuration should be set at a different scope.
>
> Add a scope hint to advice settings so that the suggested
> command uses the appropriate config scope.
>
> Pass the advice setting itself to vadvise() instead of passing
> its fields separately. Use NULL for advise() calls that are not
> associated with an advice setting.

"""Use this new mechanism to suggest setting advice.defaultBranchName 
in per-user configuration, not in per-repository configuration, as
it is way too late once a repository is initialized.""" or something
along that line is missing here.

> +enum advice_scope {
> +	ADVICE_SCOPE_LOCAL = 0,
> +	ADVICE_SCOPE_GLOBAL,
> +	ADVICE_SCOPE_SYSTEM,
> +};
> +
> +struct advice_setting {
>  	const char *key;
> +	enum advice_scope scope_hint;
>  	enum advice_level level;
> -} advice_setting[] = {
> +};

Looking good.

> +static struct advice_setting advice_setting[] = {
>  	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
>  	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
>  	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
> @@ -51,7 +60,7 @@ static struct {
>  	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
>  	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
>  	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
> -	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName" },
> +	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName", ADVICE_SCOPE_GLOBAL },
>  	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
>  	[ADVICE_DIVERGING]				= { "diverging" },
>  	[ADVICE_FETCH_SET_HEAD_WARN]			= { "fetchRemoteHEADWarn" },
> @@ -96,18 +105,31 @@ static struct {
>  
>  static const char turn_off_instructions[] =
>  N_("\n"
> -   "Disable this message with \"git config set advice.%s false\"");
> +   "Disable this message with \"git config set%s advice.%s false\"");
>  
> -static void vadvise(const char *advice, int display_instructions,
> -		    const char *key, va_list params)
> +static void vadvise(const char *advice,
> +	const struct advice_setting *setting, va_list params)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	const char *cp, *np;
>  
>  	strbuf_vaddf(&buf, advice, params);
>  
> -	if (display_instructions)
> -		strbuf_addf(&buf, turn_off_instructions, key);
> +	if (setting && setting->level == 0) {
> +		const char *scope = "";
> +		switch (setting->scope_hint) {
> +			case ADVICE_SCOPE_LOCAL:
> +				break;
> +			case ADVICE_SCOPE_GLOBAL:
> +				scope = " --global";
> +				break;
> +			case ADVICE_SCOPE_SYSTEM:
> +				scope = " --system";
> +				break;
> +		}

Style.  In our codebase, switch and case are indented to the same
tabstop.

> +		strbuf_addf(&buf, turn_off_instructions,
> +				scope, setting->key);
> +	}
>  
>  	for (cp = buf.buf; *cp; cp = np) {
>  		np = strchrnul(cp, '\n');
> @@ -126,7 +148,7 @@ void advise(const char *advice, ...)
>  {
>  	va_list params;
>  	va_start(params, advice);
> -	vadvise(advice, 0, "", params);
> +	vadvise(advice, NULL, params);
>  	va_end(params);
>  }
>  
> @@ -155,8 +177,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
>  		return;
>  
>  	va_start(params, advice);
> -	vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
> -		params);
> +	vadvise(advice, &advice_setting[type], params);
>  	va_end(params);
>  }

The change to narrow the interface into vadvise() needs to be
described in the proposed log message.

Ideally, this would be a three-patch series.  API change to
vadvise() would come first, and then the introduction of advice
scope mechanism, and finally making defaultBranchName a global
scope variable.

Other than that, the end shape looks good to me.

Thanks.

Re: [PATCH v3] advice: use global config for default branch name

[Vsevolod Myalitsin]

Hi Junio,

Thanks for the review.

> """Use this new mechanism to suggest setting advice.defaultBranchName
> in per-user configuration, not in per-repository configuration, as
> it is way too late once a repository is initialized.""" or something
> along that line is missing here.

Agreed. I'll add this motivation to the commit message.

> The change to narrow the interface into vadvise() needs to be
> described in the proposed log message.

I'll describe this change in the appropriate commit message.

> Ideally, this would be a three-patch series. API change to
> vadvise() would come first, and then the introduction of advice
> scope mechanism, and finally making defaultBranchName a global
> scope variable.

Agreed. I'll split the changes into three patches in this order.

I have one question about how the series should be organized. Since the
three patches will have different purposes, should each patch have its
own subject and commit message describing the changes introduced by that
patch? Or should they share a common subject/theme, with the individual
changes described in the respective commit messages?

> Style. In our codebase, switch and case are indented to the same
tabstop.

I'll fix the indentation.

> Other than that, the end shape looks good to me.

Thanks!

Vsevolod

Re: [PATCH v3] advice: use global config for default branch name

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> I have one question about how the series should be organized. Since the
> three patches will have different purposes, should each patch have its
> own subject and commit message describing the changes introduced by that
> patch? Or should they share a common subject/theme, with the individual
> changes described in the respective commit messages?

Sorry, but I do not quite understand what is being asked.

For example, if you had a 4-patch series like

  https://lore.kernel.org/git/20260909-758-introduce-hook-v9-0-3043d417e0ee@gmail.com/

how would you characterize each patch in it?  These 4 patches share
the same goal in bigger picture (after all that is why they are in a
single series) yet each step has its own agenda (each of them can be
explained separately as a logical unit, and that is why you are
making them separate patches to ease reading and understanding).
Each patch comes with its own title and explian the background (the
observation of the status quo) and what it wants to solve and how.

Your three-patch series would be quite similar.  If you want to
describe the motivation and overall structure of the solution, a
cover letter would make a good place to do so, and then each patch
does so in a smaller scale in its proposed log message.
The contents of each message may begin like so:

 [0/3] defaultBranchName advice is useless

 It does not make much sense to set the advice.defaultBranchName
 configuration variable in a per-repository configuration file, as
 once a repository is initialized, the advice will never fire.  We
 need to mechanism to mark such advice messages so that the message
 to tell what advice.* variable to tweak can suggest doing so in a
 per-user or even per-system configuration files.

 This series consists of three steps, ...

 [1/3] advice: pass the entire advice_setting to vadvise()

 The internal function vadvice() takes values taken from members of
 an advice_settings struct individually, which is cumbersome to
 extend.  Instead, pass the advice_settings instance so that the
 function can be extended by adding new members ot advnce_settings
 struct, without changing the signature of vadvise() function.

 [2/3] advice: introduce advice scoping mechanism

 The hint on how to squelch advice message told users to set
 advice.X configuration variable to false to squelch it, but for
 some variables, setting it globally in per-user configuration file
 is more appropriate.  Add a new member to advice_settings struct to
 indicate which config scope the variable should be set, and adjust
 the message.

...

By the way, when you prepare a v4, make sure that the cover letter
of the 3-patch series is a reply to your v3 patch, and each patch in
the series is a reply to the cover letter of v4.  That would give us
a nice threading on the mailing list archive and help automation.

Thanks.

[PATCH v4 0/3] defaultBranchName advice is useless

[Vsevolod Myalitsin]

The advice for setting the default branch name currently suggests
disabling it with the following command:

git config set advice.defaultBranchName false

Without an explicit scope, this command writes the setting to the
current repository's configuration. This is not particularly useful
here, since the advice is shown while initializing a repository, and
the initialization scenario does not repeat for that repository.
As a result, the setting only affects the repository where the advice
has already been shown, while the advice appears again when a new
repository is initialized.

Suggest using the global configuration scope instead, so that disabling
the advice applies to future repositories as well.

This series prepares the advice infrastructure for specifying a
configuration scope and then uses it for defaultBranchName advice.

The series is structured as follows:

1. Pass the entire advice_setting structure to vadvise().
2. Introduce a configuration scope hint for advice settings.
3. Suggest the global configuration scope for defaultBranchName advice.

Vsevolod Myalitsin (3):
  advice: pass the entire advice_setting to vadvise()
  advice: introduce advice scoping mechanism
  advice: use global config for default branch name

 advice.c | 45 ++++++++++++++++++++++++++++++++++-----------
 1 file changed, 34 insertions(+), 11 deletions(-)

-- 
2.50.1

[PATCH v4 1/3] advice: pass the entire advice_setting to vadvise()

[Vsevolod Myalitsin]

Currently, vadvise() takes the advice level and configuration key as
separate arguments. Pass the entire advice_setting structure instead.

This keeps the advice configuration together and makes it possible for
vadvise() to access additional properties of an advice setting without
changing its interface again.

Signed-off-by: Vsevolod Myalitsin <ub4nal@mail.ru>
---
 advice.c | 20 +++++++++++---------
 1 file changed, 11 insertions(+), 9 deletions(-)

diff --git a/advice.c b/advice.c
index 63bf8b0c5f..b556c8b38e 100644
--- a/advice.c
+++ b/advice.c
@@ -40,10 +40,12 @@ enum advice_level {
 	ADVICE_LEVEL_ENABLED,
 };
 
-static struct {
+struct advice_setting {
 	const char *key;
 	enum advice_level level;
-} advice_setting[] = {
+};
+
+static struct advice_setting advice_setting[] = {
 	[ADVICE_ADD_EMBEDDED_REPO]			= { "addEmbeddedRepo" },
 	[ADVICE_ADD_EMPTY_PATHSPEC]			= { "addEmptyPathspec" },
 	[ADVICE_ADD_IGNORED_FILE]			= { "addIgnoredFile" },
@@ -98,16 +100,17 @@ static const char turn_off_instructions[] =
 N_("\n"
    "Disable this message with \"git config set advice.%s false\"");
 
-static void vadvise(const char *advice, int display_instructions,
-		    const char *key, va_list params)
+static void vadvise(const char *advice,
+	const struct advice_setting *setting, va_list params)
 {
 	struct strbuf buf = STRBUF_INIT;
 	const char *cp, *np;
 
 	strbuf_vaddf(&buf, advice, params);
 
-	if (display_instructions)
-		strbuf_addf(&buf, turn_off_instructions, key);
+	if (setting && setting->level == ADVICE_LEVEL_NONE) {
+		strbuf_addf(&buf, turn_off_instructions,
+					setting->key);
 
 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
@@ -126,7 +129,7 @@ void advise(const char *advice, ...)
 {
 	va_list params;
 	va_start(params, advice);
-	vadvise(advice, 0, "", params);
+	vadvise(advice, NULL, params);
 	va_end(params);
 }
 
@@ -155,8 +158,7 @@ void advise_if_enabled(enum advice_type type, const char *advice, ...)
 		return;
 
 	va_start(params, advice);
-	vadvise(advice, !advice_setting[type].level, advice_setting[type].key,
-		params);
+	vadvise(advice, &advice_setting[type], params);
 	va_end(params);
 }
 
-- 
2.50.1

Re: [PATCH v4 1/3] advice: pass the entire advice_setting to vadvise()

[SZEDER Gábor]

On Thu, Sep 10, 2026 at 11:53:51AM +0300, Vsevolod Myalitsin wrote:
> @@ -98,16 +100,17 @@ static const char turn_off_instructions[] =
>  N_("\n"
>     "Disable this message with \"git config set advice.%s false\"");
>  
> -static void vadvise(const char *advice, int display_instructions,
> -		    const char *key, va_list params)
> +static void vadvise(const char *advice,
> +	const struct advice_setting *setting, va_list params)
>  {
>  	struct strbuf buf = STRBUF_INIT;
>  	const char *cp, *np;
>  
>  	strbuf_vaddf(&buf, advice, params);
>  
> -	if (display_instructions)
> -		strbuf_addf(&buf, turn_off_instructions, key);
> +	if (setting && setting->level == ADVICE_LEVEL_NONE) {

There is an opening brace at the end of this line ...

> +		strbuf_addf(&buf, turn_off_instructions,
> +					setting->key);

... but there is no corresponding closing brace here, leading to
compilation errors.

Please make sure that each and every commit you submit can be built.

>  
>  	for (cp = buf.buf; *cp; cp = np) {
>  		np = strchrnul(cp, '\n');

[PATCH v4 2/3] advice: introduce advice scoping mechanism

[Vsevolod Myalitsin]

The advice settings currently do not distinguish between configuration
scopes. Add a scope hint to advice_setting so that an advice can
recommend a specific configuration scope when disabling it.

Use the existing enum config_scope to represent the scope, with
CONFIG_SCOPE_UNKNOWN indicating that the default configuration scope
should be used.

Signed-off-by: Vsevolod Myalitsin <ub4nal@mail.ru>
---
 advice.c | 25 +++++++++++++++++++++++--
 1 file changed, 23 insertions(+), 2 deletions(-)

diff --git a/advice.c b/advice.c
index b556c8b38e..12a68ea716 100644
--- a/advice.c
+++ b/advice.c
@@ -42,6 +42,7 @@ enum advice_level {
 
 struct advice_setting {
 	const char *key;
+	enum config_scope scope_hint;
 	enum advice_level level;
 };
 
@@ -96,9 +97,16 @@ static struct advice_setting advice_setting[] = {
 	[ADVICE_WORKTREE_ADD_ORPHAN]			= { "worktreeAddOrphan" },
 };
 
+/*
+ * TRANSLATORS: This is a command line that the user should run.
+ *              Do not translate the part inside double quotes.
+ *              The first %s is the config scope (e.g. " --global"),
+ *              the second %s is the advice key (e.g. "defaultBranchName").
+ */
+
 static const char turn_off_instructions[] =
 N_("\n"
-   "Disable this message with \"git config set advice.%s false\"");
+   "Disable this message with \"git config set%s advice.%s false\"");
 
 static void vadvise(const char *advice,
 	const struct advice_setting *setting, va_list params)
@@ -109,8 +117,21 @@ static void vadvise(const char *advice,
 	strbuf_vaddf(&buf, advice, params);
 
 	if (setting && setting->level == ADVICE_LEVEL_NONE) {
+		const char *scope = "";
+		switch (setting->scope_hint) {
+		case CONFIG_SCOPE_LOCAL:
+		case CONFIG_SCOPE_UNKNOWN:
+			break;
+		case CONFIG_SCOPE_GLOBAL:
+			scope = " --global";
+			break;
+		case CONFIG_SCOPE_SYSTEM:
+			scope = " --system";
+			break;
+		}
 		strbuf_addf(&buf, turn_off_instructions,
-					setting->key);
+				scope, setting->key);
+	}
 
 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
-- 
2.50.1

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> @@ -109,8 +117,21 @@ static void vadvise(const char *advice,
>  	strbuf_vaddf(&buf, advice, params);
>  
>  	if (setting && setting->level == ADVICE_LEVEL_NONE) {
> +		const char *scope = "";
> +		switch (setting->scope_hint) {
> +		case CONFIG_SCOPE_LOCAL:
> +		case CONFIG_SCOPE_UNKNOWN:
> +			break;
> +		case CONFIG_SCOPE_GLOBAL:
> +			scope = " --global";
> +			break;
> +		case CONFIG_SCOPE_SYSTEM:
> +			scope = " --system";
> +			break;
> +		}

make DEVELOPER=YesPlease would die due to

advice.c: In function 'vadvise':
advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_WORKTREE' not handled in switch [-Werror=switch]
  123 |                 switch (setting->scope_hint) {
      |                 ^~~~~~
advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_COMMAND' not handled in switch [-Werror=switch]
advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_SUBMODULE' not handled in switch [-Werror=switch]

We probably should have

		default:
			BUG("advice settings at wrong config scope");

or something there.

>  		strbuf_addf(&buf, turn_off_instructions,
> -					setting->key);
> +				scope, setting->key);
> +	}
>  
>  	for (cp = buf.buf; *cp; cp = np) {
>  		np = strchrnul(cp, '\n');

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Jeff King]

On Thu, Sep 10, 2026 at 08:36:59AM -0700, Junio C Hamano wrote:

> > @@ -109,8 +117,21 @@ static void vadvise(const char *advice,
> >  	strbuf_vaddf(&buf, advice, params);
> >  
> >  	if (setting && setting->level == ADVICE_LEVEL_NONE) {
> > +		const char *scope = "";
> > +		switch (setting->scope_hint) {
> > +		case CONFIG_SCOPE_LOCAL:
> > +		case CONFIG_SCOPE_UNKNOWN:
> > +			break;
> > +		case CONFIG_SCOPE_GLOBAL:
> > +			scope = " --global";
> > +			break;
> > +		case CONFIG_SCOPE_SYSTEM:
> > +			scope = " --system";
> > +			break;
> > +		}
> 
> make DEVELOPER=YesPlease would die due to
> 
> advice.c: In function 'vadvise':
> advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_WORKTREE' not handled in switch [-Werror=switch]
>   123 |                 switch (setting->scope_hint) {
>       |                 ^~~~~~
> advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_COMMAND' not handled in switch [-Werror=switch]
> advice.c:123:17: error: enumeration value 'CONFIG_SCOPE_SUBMODULE' not handled in switch [-Werror=switch]
> 
> We probably should have
> 
> 		default:
> 			BUG("advice settings at wrong config scope");
> 
> or something there.

It is funny that we would handle LOCAL here (which we do not expect
anybody to pass) but would BUG() on other stuff like WORKTREE (which we
also would not expect).

So if we are going to do a switch statement, then I'd expect:

  switch (setting->scope_hint) {
  case CONFIG_SCOPE_GLOBAL:
	scope = " --global";
	break;
  case CONFIG_SCOPE_SYSTEM:
	scope = " --system";
	break;
  default:
	/*
	 * Scope is local or otherwise unsupported; just recommend
	 * the usual unadorned config command.
         */
	break;
  }

I guess maybe that would surprise somebody who tried to add
CONFIG_SCOPE_WORKTREE support, and they'd rather see a BUG(). I dunno.

I was hoping we could avoid enumerating things at all here, but using
config_scope_name() did involve a bit more string construction (and a
hidden assumption that each scope name has a matching "--foo" option).

I'm really not sure why anybody would use those other flags, though (or
even --system, for that matter). After reading the thread again, I get
why we want "--global" for advice that only affects new repository
creation (like defaultBranchName), since otherwise it could never have
any effect. But why would you ever want --system?

I feel like we are maybe leading poor Vsevolod in circles, though. At
some point there are diminishing returns for polishing this.

-Peff

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Vsevolod Myalitsin]

Hi, Jeff!

> I'm really not sure why anybody would use those other flags, though (or
> even --system, for that matter). After reading the thread again, I get
> why we want "--global" for advice that only affects new repository
> creation (like defaultBranchName), since otherwise it could never have
> any effect. But why would you ever want --system?

I initially looked at Junio's suggestion and, based on his experience, didn't argue with it, and then I didn't come back to that message. I think the patch should contain not + enum config_scope scope_hint; but + bool is_global_hint;, since I myself can't find any scenarios where advice should be disabled at the system level.

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Jeff King]

On Thu, Sep 10, 2026 at 08:54:15PM +0300, Vsevolod Myalitsin wrote:

> > I'm really not sure why anybody would use those other flags, though (or
> > even --system, for that matter). After reading the thread again, I get
> > why we want "--global" for advice that only affects new repository
> > creation (like defaultBranchName), since otherwise it could never have
> > any effect. But why would you ever want --system?
> 
> I initially looked at Junio's suggestion and, based on his experience,
> didn't argue with it, and then I didn't come back to that message. I
> think the patch should contain not + enum config_scope scope_hint; but
> + bool is_global_hint;, since I myself can't find any scenarios where
> advice should be disabled at the system level.

Yeah, it feels like handling arbitrary scopes is introducing all of
these extra questions. But all we really need is that original bool you
had. I think there's some YAGNI principle here, too. Later if somebody
comes along and really wants to advise the user to use "git config
--system", they can do the bool-to-scope conversion then. I'd be
surprised if that happens.

-Peff

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'm really not sure why anybody would use those other flags, though (or
> even --system, for that matter). After reading the thread again, I get
> why we want "--global" for advice that only affects new repository
> creation (like defaultBranchName), since otherwise it could never have
> any effect. But why would you ever want --system?

No particular concrete expected use case in mind.  But I figured
that it would not be too much additional effort to allow other
scopes once we need to add support to allow "--global" to be added
to the message.  I didn't think of "--worktree", but now you have
mentioned it, I tend to think it is more plausible to have real use
case than "--system" (which users often do not even have power to
set).

The primary reason why I didn't think of "--worktree" is because
output of "git config --help" has room for improvements.  This is a
tangent, but one of its SYNOPSIS item reads like this:

	git config set [<file-option>] [--type=<type>] [--all] \
		[--value=<pattern>] [--fixed-value] <name> <value>

And nowhere in the body of the documentation there is any
description on what <file-option> is.  There is this sentence

    ... and options --system, --global, --local, --worktree and
    --file <filename> can be used to tell the command to read from
    only that location.

in one paragraph that gives enough hints that these five options are
related to each other and give the closest thing as the definition
of <file-option>, but I wouldn't call it a very good form of
documentation.

There is a section called FILES, at the end of which has

       You can limit which configuration sources are read from or
       written to by specifying the path of a file with the --file
       option, or by specifying a configuration scope with --system,
       --global, --local, or --worktree. For more, see the section
       called “OPTIONS” above.

but it is not explicit that the section is talking about
<file-option>, either.

--- >8 ---
Subject: [PATCH] doc: clarify <file-option> in "git config --help"

The SYNOPSIS section of "git config --help" refers to <file-option>
without explaining what they really mean.

I *think* they meant to refer to the mechanism to limit the file(s)
read from or written to by giving the scope options or the '--file
<filename>' option.  Spell it out early in the description.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---

 * The SYNOPSIS section also refers to <display-option> for many
   operations; I have no idea what it means.  I left a needswork
   comment there.  We should either clarify it in a similar way, or
   remove it if it does not refer to anything.

 Documentation/git-config.adoc | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
index 57af010ade..3673226505 100644
--- a/Documentation/git-config.adoc
+++ b/Documentation/git-config.adoc
@@ -39,6 +39,12 @@ outgoing values are canonicalize-able under the given <type>.  If no
 `--type=<type>` is given, no canonicalization will be performed. Callers may
 unset an existing `--type` specifier with `--no-type`.
 
+The `<file-option>` in the SYNOPSIS refers to options that limit the
+read/write operations to a specific scope (see <<SCOPES>>) or a single
+file (see <<FILES>>).
+
+// NEEDSWORK: What is the `<display-option>` meant to refer to?
+
 When reading, the values are read from the system, global and
 repository local configuration files by default, and options
 `--system`, `--global`, `--local`, `--worktree` and
-- 
2.56.0-rc0-135-g9520983108

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Jeff King]

On Thu, Sep 10, 2026 at 11:35:50AM -0700, Junio C Hamano wrote:

> The primary reason why I didn't think of "--worktree" is because
> output of "git config --help" has room for improvements.  This is a
> tangent, but one of its SYNOPSIS item reads like this:
> 
> 	git config set [<file-option>] [--type=<type>] [--all] \
> 		[--value=<pattern>] [--fixed-value] <name> <value>

If it makes you feel any better, I did not even know --worktree existed
until today. ;) I only discovered it when looking at the possible values
returned by config_scope_name().

I still have trouble imagining why a particular piece of advice would
make sense only in --worktree mode. The only concrete case I've seen for
any advice scoping is that clone/init advice config does not make sense
in repo config. And --global is the sensible solution to that (--system
works, too, but it is not a very helpful recommendation).

I kind of wonder if _all_ advice should just say "--global". I cannot
think of an advice flag that is really repo specific. They are about
silencing extra help because the _user_ understands the situation and
wants Git to be less chatty.

> --- >8 ---
> Subject: [PATCH] doc: clarify <file-option> in "git config --help"
> 
> The SYNOPSIS section of "git config --help" refers to <file-option>
> without explaining what they really mean.
> 
> I *think* they meant to refer to the mechanism to limit the file(s)
> read from or written to by giving the scope options or the '--file
> <filename>' option.  Spell it out early in the description.

I agree that we should use the term <file-option> to refer to it. I
think the paragraphs just below what you touched try to explain these,
but don't use the term.

Something like the patch below uses the term. There's also a lot of
duplication between the reading/writing paragraphs that could be
condensed (but I didn't do it here).

diff --git a/Documentation/git-config.adoc b/Documentation/git-config.adoc
index 8d080e301b..18cee89f84 100644
--- a/Documentation/git-config.adoc
+++ b/Documentation/git-config.adoc
@@ -40,16 +40,14 @@ outgoing values are canonicalize-able under the given <type>.  If no
 unset an existing `--type` specifier with `--no-type`.
 
 When reading, the values are read from the system, global and
-repository local configuration files by default, and options
-`--system`, `--global`, `--local`, `--worktree` and
-`--file <filename>` can be used to tell the command to read from only
+repository local configuration files by default. Provide a
+`<file-option>` (`--system`, `--global`, `--local`, `--worktree`,
+or `--file <filename>`) to tell the command to read from only
 that location (see <<FILES>>).
 
 When writing, the new value is written to the repository local
-configuration file by default, and options `--system`, `--global`,
-`--worktree`, `--file <filename>` can be used to tell the command to
-write to that location (you can say `--local` but that is the
-default).
+configuration file by default. A `<file-options>` can be used to tell
+the command to write to that location.
 
 This command will fail with non-zero status upon error.  Some exit
 codes are:


I also considered that the options themselves should be grouped as
sub-entries of a <file-options>:: entry, but I think that may create
other awkwardness.

There is also --blob, which affects the source/dest of config, but isn't
really a "file" option. It is really more of a "location" option (and
that is what it is called in the macro grouping within the code, though
that is never exposed to the user).

>  * The SYNOPSIS section also refers to <display-option> for many
>    operations; I have no idea what it means.  I left a needswork
>    comment there.  We should either clarify it in a similar way, or
>    remove it if it does not refer to anything.

It comes from 14970509c6 (builtin/config: introduce "list" subcommand,
2024-05-06), and there's similar macro magic. It really just means
"stuff that changes the list output".

I think the manpage could probably be rewritten to focus on the
different command modes, and have a section for "here are the useful
options in list mode". Whereas historically, "--list" was just another
option. That would be a much bigger rewrite of the page, though.

-Peff

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I kind of wonder if _all_ advice should just say "--global". I cannot
> think of an advice flag that is really repo specific. They are about
> silencing extra help because the _user_ understands the situation and
> wants Git to be less chatty.

I think there are two things in play.

 * If applicability of a piece of advice depends on the workflow
   employed, and a user who works on multiple projects that use
   different workflows, set of advice messages may want to be
   squelched per project, hence "--global" may not be appropriate.

 * "I, a physical single person, understand this piece of advice" is
   inherently per user, so squelching a piece of advice that the
   physical single person understands globally may make sense very
   well.

In hindsight, the latter argument should have been given more
weight, but I think the primary thinking back when we designed the
customizable advice messages was instead the former.

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Jeff King]

On Thu, Sep 10, 2026 at 12:54:21PM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > I kind of wonder if _all_ advice should just say "--global". I cannot
> > think of an advice flag that is really repo specific. They are about
> > silencing extra help because the _user_ understands the situation and
> > wants Git to be less chatty.
> 
> I think there are two things in play.
> 
>  * If applicability of a piece of advice depends on the workflow
>    employed, and a user who works on multiple projects that use
>    different workflows, set of advice messages may want to be
>    squelched per project, hence "--global" may not be appropriate.
> 
>  * "I, a physical single person, understand this piece of advice" is
>    inherently per user, so squelching a piece of advice that the
>    physical single person understands globally may make sense very
>    well.
> 
> In hindsight, the latter argument should have been given more
> weight, but I think the primary thinking back when we designed the
> customizable advice messages was instead the former.

Yeah, my contention is that the first thing doesn't really exist. But I
admit I didn't carefully go through the list of advice looking for
counter-examples.

I'd be surprised if anybody really thought carefully about it, though.
When I introduced advice.* in 2009 (geez, has it really been that long?)
I had assumed people would just set it in their user config. The actual
"git config" command advice came much later, but I don't see any
discussion of global vs local in that thread:

  https://lore.kernel.org/git/pull.548.git.1581311049547.gitgitgadget@gmail.com/

Amusingly that thread also touches on some of the "could we just convert
everything to advise_if_enabled()" issues we've discussed here. I had
zero recollection of it, despite participating.

-Peff

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> I'd be surprised if anybody really thought carefully about it, though.
> When I introduced advice.* in 2009 (geez, has it really been that long?)
> I had assumed people would just set it in their user config. The actual
> "git config" command advice came much later, but I don't see any
> discussion of global vs local in that thread:
>
>   https://lore.kernel.org/git/pull.548.git.1581311049547.gitgitgadget@gmail.com/
>
> Amusingly that thread also touches on some of the "could we just convert
> everything to advise_if_enabled()" issues we've discussed here. I had
> zero recollection of it, despite participating.

I do not think I added much input into the topic at the
philosophical design level---just the usual usability and
correctness review.  No wonder I do not recall anything particular I
contributed to the discussion there ;-)

It is very much understandable if we didn't mean the "use 'git
config advice.foo false' to disable" as a cut-and-paste ready
instruction, and rather meant as a general instruction that any
intelligent users would tweak for their own situation.  And it is
not surprising, from such a stance, the 'git config' hint would not
come with any scope indicator.

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Vsevolod Myalitsin]

Junio C Hamano <gitster@pobox.com> writes:

> It is very much understandable if we didn't mean the "use 'git
> config advice.foo false' to disable" as a cut-and-paste ready
> instruction, and rather meant as a general instruction that any
> intelligent users would tweak for their own situation.  And it is
> not surprising, from such a stance, the 'git config' hint would not
> come with any scope indicator.

I think that since advice.* was originally assumed to be disabled
globally (as Jeff mentions, he expected it to be set in the user
config), adding "--global" to the hint is a good solution.  It makes
the hint actually cut-and-paste ready while still matching the
original intent.

As for "--system", "--worktree" and the like, I don't think it makes
sense to support them until there is a proven need.  I propose to
choose between local and global via a boolean flag, and treat all the
other scopes as YAGNI for now.

Thanks.

Re: [PATCH v4 2/3] advice: introduce advice scoping mechanism

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> Junio C Hamano <gitster@pobox.com> writes:
>
>> It is very much understandable if we didn't mean the "use 'git
>> config advice.foo false' to disable" as a cut-and-paste ready
>> instruction, and rather meant as a general instruction that any
>> intelligent users would tweak for their own situation.  And it is
>> not surprising, from such a stance, the 'git config' hint would not
>> come with any scope indicator.
>
> I think that since advice.* was originally assumed to be disabled
> globally (as Jeff mentions, he expected it to be set in the user
> config), adding "--global" to the hint is a good solution.  It makes
> the hint actually cut-and-paste ready while still matching the
> original intent.

The original intent was more like "the users are intelligent enough
to be able to decide which scope they want to use", I think.  I
agree that even with "--global" they can still cut-and-paste and
tweak if they wanted to, so I am OK with that move, but my point was
it probably is not even needed to mark each ones for which scope
they are suggested to be set (iow, we can just change the message to
always say "--global" without changing anything else).

Thanks.

[PATCH v4 3/3] advice: use global config for default branch name

[Vsevolod Myalitsin]

The advice for setting the default branch name currently suggests
disabling it with a local configuration value.

This makes the advice appear again when a new repository is initialized.
Suggest using the global configuration scope instead, so that disabling
the advice applies to future repositories as well.

Signed-off-by: Vsevolod Myalitsin <ub4nal@mail.ru>
---
 advice.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/advice.c b/advice.c
index 12a68ea716..6964a6e2ba 100644
--- a/advice.c
+++ b/advice.c
@@ -54,7 +54,7 @@ static struct advice_setting advice_setting[] = {
 	[ADVICE_AM_WORK_DIR] 				= { "amWorkDir" },
 	[ADVICE_CHECKOUT_AMBIGUOUS_REMOTE_BRANCH_NAME] 	= { "checkoutAmbiguousRemoteBranchName" },
 	[ADVICE_COMMIT_BEFORE_MERGE]			= { "commitBeforeMerge" },
-	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName" },
+	[ADVICE_DEFAULT_BRANCH_NAME]			= { "defaultBranchName", CONFIG_SCOPE_GLOBAL },
 	[ADVICE_DETACHED_HEAD]				= { "detachedHead" },
 	[ADVICE_DIVERGING]				= { "diverging" },
 	[ADVICE_FETCH_SET_HEAD_WARN]			= { "fetchRemoteHEADWarn" },
-- 
2.50.1