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

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

[Vsevolod Myalitsin]

The advice for configuring the default branch name
suggests disabling it with "git config set
advice.defaultBranchName false". This setting is
useless because it neither affects the current
repository nor newly created repositories.

Suggest using "git config --global" instead.

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

diff --git a/advice.c b/advice.c
index 63bf8b0c5f..64ca4613b4 100644
--- a/advice.c
+++ b/advice.c
@@ -96,7 +96,7 @@ 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 %s advice.%s false\"");
 
 static void vadvise(const char *advice, int display_instructions,
 		    const char *key, va_list params)
@@ -107,7 +107,8 @@ static void vadvise(const char *advice, int display_instructions,
 	strbuf_vaddf(&buf, advice, params);
 
 	if (display_instructions)
-		strbuf_addf(&buf, turn_off_instructions, key);
+		strbuf_addf(&buf, turn_off_instructions,
+			strcmp(key, "defaultBranchName") ? "set" : "--global", key);
 
 	for (cp = buf.buf; *cp; cp = np) {
 		np = strchrnul(cp, '\n');
-- 
2.50.1

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

[Ben Knoble]

> Le 7 sept. 2026 à 09:02, Vsevolod Myalitsin <ub4nal@mail.ru> a écrit :
> 
> The advice for configuring the default branch name
> suggests disabling it with "git config set
> advice.defaultBranchName false". This setting is
> useless because it neither affects the current
> repository nor newly created repositories.

Makes sense.

> Suggest using "git config --global" instead.

I think we should probably say “git config set --global …”
using the modern forms, no?

> Signed-off-by: Vsevolod Myalitsin <ub4nal@mail.ru>
> ---
> advice.c | 5 +++--
> 1 file changed, 3 insertions(+), 2 deletions(-)
> 
> diff --git a/advice.c b/advice.c
> index 63bf8b0c5f..64ca4613b4 100644
> --- a/advice.c
> +++ b/advice.c
> @@ -96,7 +96,7 @@ 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 %s advice.%s false\"");
> 
> static void vadvise(const char *advice, int display_instructions,
>            const char *key, va_list params)
> @@ -107,7 +107,8 @@ static void vadvise(const char *advice, int display_instructions,
>    strbuf_vaddf(&buf, advice, params);
> 
>    if (display_instructions)
> -        strbuf_addf(&buf, turn_off_instructions, key);
> +        strbuf_addf(&buf, turn_off_instructions,
> +            strcmp(key, "defaultBranchName") ? "set" : "--global", key);

This would be hard to extend later for other advice options that also make more sense at the global level. Perhaps extract a little helper is_global(key)?

Thanks

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

[Vsevolod Myalitsin]

I considered using an "is_global(key)" helper, but I think adding a field to "advice_setting" is cleaner.

The change is quite small:

 struct advice_setting {
     const char *key;
+    int global_hint;
     enum advice_level level;
 };

Then the scope is specified directly for the relevant advice:

	-[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName" },
	+[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName", 1 },

And used when building the hint:

	 static void vadvise(const char *advice, int display_instructions,
	-                    const char *key, va_list params)
	+                    const char *key, int global, va_list params)
	 {
	     ...
 
	     if (display_instructions)
	-        strbuf_addf(&buf, turn_off_instructions, key);
	+        strbuf_addf(&buf, turn_off_instructions,
	+                    global ? "--global" : "", key);
	 }

This keeps the information about the intended config scope in "advice_setting", rather than making "vadvise()" depend on specific advice keys.

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

[Ben Knoble]

> Le 8 sept. 2026 à 10:43, Vsevolod Myalitsin <ub4nal@mail.ru> a écrit :
> 
> 
> I considered using an "is_global(key)" helper, but I think adding a field to "advice_setting" is cleaner.
> 
> The change is quite small:
> 
> struct advice_setting {
>     const char *key;
> +    int global_hint;
>     enum advice_level level;
> };
> 
> Then the scope is specified directly for the relevant advice:
> 
>    -[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName" },
>    +[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName", 1 },
> 
> And used when building the hint:
> 
>     static void vadvise(const char *advice, int display_instructions,
>    -                    const char *key, va_list params)
>    +                    const char *key, int global, va_list params)
>     {
>         ...
> 
>         if (display_instructions)
>    -        strbuf_addf(&buf, turn_off_instructions, key);
>    +        strbuf_addf(&buf, turn_off_instructions,
>    +                    global ? "--global" : "", key);
>     }
> 
> This keeps the information about the intended config scope in "advice_setting", rather than making "vadvise()" depend on specific advice keys.

That also seems good to me. I think I prefer it. 

PS it is normal here to bottom-post and quote at least the
relevant parts of the message to which you reply ;)

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

[R4NC]

> That also seems good to me. I think I prefer it.
>
> PS it is normal here to bottom-post and quote at least the
> relevant parts of the message to which you reply 😉


Thank you for the review and for the formatting advice.
I will send v2 of the patch with the global_hint field added as suggested.
By the way, is my reply formatting correct this time?

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

[D. Ben Knoble]

On Tue, Sep 8, 2026 at 11:59 AM R4NC <ub4nal@mail.ru> wrote:
>
> > PS it is normal here to bottom-post and quote at least the
> > relevant parts of the message to which you reply 😉

[snip]

> By the way, is my reply formatting correct this time?

I think so, anyway :)

Thanks again!

-- 
D. Ben Knoble

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

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> I considered using an "is_global(key)" helper, but I think adding
> a field to "advice_setting" is cleaner.
>
> The change is quite small:
>
>  struct advice_setting {
>      const char *key;
> +    int global_hint;
>      enum advice_level level;
>  };

Should it only about "global vs local"?  I am wondering if we ever
want to suggest "system".  In any case, these three things are
called "scope" in "git config --help", so perhaps rename the new
member to "config_scope" or "scope_hint" or something?

> Then the scope is specified directly for the relevant advice:
>
> 	-[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName" },
> 	+[ADVICE_DEFAULT_BRANCH_NAME] = { "defaultBranchName", 1 },
>
> And used when building the hint:
>
> 	 static void vadvise(const char *advice, int display_instructions,
> 	-                    const char *key, va_list params)
> 	+                    const char *key, int global, va_list params)

Have you considered going in the other direction to narrow the
interface instead of widening?  Instead of passing .level and .key
separately from the caller to this function, I wonder if it makes
it more future-proof to pass &advice_setting[type].  A call in
advise_if_enabled() then would become

	vadvise(advice, &advice_settings[type], params);

and vadvise() is the only thing that needs to know what members are
in the advice_setting struct and how they affect the output.

> 	 {
> 	     ...
>  
> 	     if (display_instructions)
> 	-        strbuf_addf(&buf, turn_off_instructions, key);
> 	+        strbuf_addf(&buf, turn_off_instructions,
> 	+                    global ? "--global" : "", key);
> 	 }
>
> This keeps the information about the intended config scope in "advice_setting", rather than making "vadvise()" depend on specific advice keys.

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

[Vsevolod Myalitsin]

Hi Junio,

> Should it only about "global vs local"?  I am wondering if we ever
> want to suggest "system".  In any case, these three things are
> called "scope" in "git config --help", so perhaps rename the new
> member to "config_scope" or "scope_hint" or something?

Agreed. I will rename "global_hint" to "scope_hint" so that the field
describes the configuration scope rather than just the global case.

> Have you considered going in the other direction to narrow the
> interface instead of widening?  Instead of passing .level and .key
> separately from the caller to this function, I wonder if it makes
> it more future-proof to pass &advice_setting[type].

Yes, I agree that passing the "advice_setting" itself is cleaner and
more future-proof. I will change "vadvise()" to take a pointer to the
corresponding "advice_setting" instead.

Unfortunately, I did not notice your message in time and had already
sent v2. I will implement these changes in v3.

Thanks for the suggestions.

Best,
Vsevolod R4NC

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

[Junio C Hamano]

Vsevolod Myalitsin <ub4nal@mail.ru> writes:

> Yes, I agree that passing the "advice_setting" itself is cleaner and
> more future-proof. I will change "vadvise()" to take a pointer to the
> corresponding "advice_setting" instead.

One minor glitch is that there is an ad-hoc vadvise() call in
advise() that is not tied to any particular entry in the
advise_setting[] table.  I think we'd need to give a name to the
advice_setting struct type, instanciate an ad-hoc instance on stack,
and pass it down the callchain, perhaps like so:

	void advise(const char *advice, ...)
	{
		struct advice_setting ad_hoc = {
			.key = "",
			.scope = CONFIG_SCOPE_UNKNOWN,
			.level = 0,
		};
		va_list params;

		va_start(params, advise);
		vadvise(advise, &ad_hoc, params);
		va_end(params);
	}

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

[R4NC]

 > One minor glitch is that there is an ad-hoc vadvise() call in
 > advise() that is not tied to any particular entry in the
 > advise_setting[] table.

I agree that we should use a separate "advice_setting" structure for this.

 > I think we'd need to give a name to the advice_setting struct type,
 > instanciate an ad-hoc instance on stack, and pass it down the callchain.

I agree. However, "advise()" originally passed "0" for 
"display_instructions",
while "advise_if_enabled()" passed the negation of "level". With the new 
interface,
we need a non-zero value for the ad-hoc setting to suppress the 
instructions.
Using "ADVICE_LEVEL_ENABLED" or "ADVICE_LEVEL_DISABLED" would be a hack.


I suggest adding a dedicated "ADVICE_LEVEL_UNKNOWN" value to "enum 
advice_level" for this case.

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

[Jeff King]

On Tue, Sep 08, 2026 at 11:57:18AM -0700, Junio C Hamano wrote:

> Vsevolod Myalitsin <ub4nal@mail.ru> writes:
> 
> > Yes, I agree that passing the "advice_setting" itself is cleaner and
> > more future-proof. I will change "vadvise()" to take a pointer to the
> > corresponding "advice_setting" instead.
> 
> One minor glitch is that there is an ad-hoc vadvise() call in
> advise() that is not tied to any particular entry in the
> advise_setting[] table.  I think we'd need to give a name to the
> advice_setting struct type, instanciate an ad-hoc instance on stack,
> and pass it down the callchain, perhaps like so:

Isn't this a natural fit for NULL? That ad-hoc call wants to pass the
notion that there is no matching advice config (or at least not that it
knows about). And then vadvise() can check:

  if (conf && !conf->level)
	...show instructions...

which seems natural to me.

As a side note, I think this is revealing some existing shortcomings in
the callers.  Most of the calls to advise() are doing something like:

  if (advice_is_enabled(ADVICE_FOO))
	advise("ask your doctor about foo");

Those won't get the "turn this off with advice.foo instructions". Only:

  advise_if_enabled(ADVICE_FOO, "ask your doctor about foo");

will. So there are many missed opportunities for offering the turn-off
instructions. Nobody seems to have complained, which makes me wonder if
the turn-off instructions would be annoyingly chatty if we printed them
all the time. Most of those calls predate the addition if the turn-off
instructions and advise_if_enabled(), which was added in 2020. I wonder
how people would feel if we converted them all and started printing the
turn-off instructions everywhere.

Anyway, UI philosophizing aside, another obvious pattern for advise()
is:

  if (advice_is_enabled(ADVICE_FOO)) {
	/* do lots of work */
	advise("try %s", results_of_work);
  }

which _wouldn't_ want to convert to advise_if_enabled(). If that wants
the turn-off message, we'd want to be able to pass the advice enum to
advise(), like:

  advise(ADVICE_FOO, "try %s", results_of_work);

at which point we might need a way to pass the NULL advice marker
somehow (for those cases which really aren't tied to a config value,
though arguably that is an anti-pattern in itself).

I guess the caller could just do:

  advise_if_enabled(ADVICE_FOO, ...);

inside the block. We know that it's enabled, but it's not like the check
is expensive.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Tue, Sep 08, 2026 at 11:57:18AM -0700, Junio C Hamano wrote:
>
>> Vsevolod Myalitsin <ub4nal@mail.ru> writes:
>> 
>> > Yes, I agree that passing the "advice_setting" itself is cleaner and
>> > more future-proof. I will change "vadvise()" to take a pointer to the
>> > corresponding "advice_setting" instead.
>> 
>> One minor glitch is that there is an ad-hoc vadvise() call in
>> advise() that is not tied to any particular entry in the
>> advise_setting[] table.  I think we'd need to give a name to the
>> advice_setting struct type, instanciate an ad-hoc instance on stack,
>> and pass it down the callchain, perhaps like so:
>
> Isn't this a natural fit for NULL?

Perfect.

> As a side note, I think this is revealing some existing shortcomings in
> the callers.  Most of the calls to advise() are doing something like:
>
>   if (advice_is_enabled(ADVICE_FOO))
> 	advise("ask your doctor about foo");

Yes, but all of these callers call advice_enabled() without _is ;-)

>
> Those won't get the "turn this off with advice.foo instructions". Only:
>
>   advise_if_enabled(ADVICE_FOO, "ask your doctor about foo");
>
> will. So there are many missed opportunities for offering the turn-off
> instructions. Nobody seems to have complained, which makes me wonder if
> the turn-off instructions would be annoyingly chatty if we printed them
> all the time. Most of those calls predate the addition if the turn-off
> instructions and advise_if_enabled(), which was added in 2020. I wonder
> how people would feel if we converted them all and started printing the
> turn-off instructions everywhere.

Depends on how we do so, I guess.  Do you mean we should rewrite
advise() call above to advice_if_enabled(), even though the check
for ADVICE_FOO token appear redundant?

> Anyway, UI philosophizing aside, another obvious pattern for advise()
> is:
>
>   if (advice_is_enabled(ADVICE_FOO)) {
> 	/* do lots of work */
> 	advise("try %s", results_of_work);
>   }

Yes, checking with is-enabled primarily for the purpose of skipping
"do lots of work" is a very typical use.  I do not know why you
assume ...

>
> which _wouldn't_ want to convert to advise_if_enabled().

... this "try X" is something the users would not want to learn how
to disable, but assuming it is not, the existing code above as-is
should be what we want.

> If that wants
> the turn-off message, we'd want to be able to pass the advice enum to
> advise(), like:
>
>   advise(ADVICE_FOO, "try %s", results_of_work);
>
> at which point we might need a way to pass the NULL advice marker
> somehow (for those cases which really aren't tied to a config value,
> though arguably that is an anti-pattern in itself).
>
> I guess the caller could just do:
>
>   advise_if_enabled(ADVICE_FOO, ...);
>
> inside the block. We know that it's enabled, but it's not like the check
> is expensive.

Yes, I think we already have some callers that do so, in a pattern
where they want to skip the "do lots of work" part.  Or at least I
think I suggested the pattern in the past for somebody who wanted to
do that.

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

[Jeff King]

On Wed, Sep 09, 2026 at 11:51:03AM -0700, Junio C Hamano wrote:

> > will. So there are many missed opportunities for offering the turn-off
> > instructions. Nobody seems to have complained, which makes me wonder if
> > the turn-off instructions would be annoyingly chatty if we printed them
> > all the time. Most of those calls predate the addition if the turn-off
> > instructions and advise_if_enabled(), which was added in 2020. I wonder
> > how people would feel if we converted them all and started printing the
> > turn-off instructions everywhere.
> 
> Depends on how we do so, I guess.  Do you mean we should rewrite
> advise() call above to advice_if_enabled(), even though the check
> for ADVICE_FOO token appear redundant?

I mean we could mechanically rewrite:

  if (advice_enabled(ADVICE_FOO))
	advise(...);

to:

  advise_if_enabled(ADVICE_FOO, ...);

So the check wouldn't be redundant, but rather folded into the helper
function. The code becomes shorter, and the user-visible behavior
changes to produce the extra "turn-off" message.

> > Anyway, UI philosophizing aside, another obvious pattern for advise()
> > is:
> >
> >   if (advice_is_enabled(ADVICE_FOO)) {
> > 	/* do lots of work */
> > 	advise("try %s", results_of_work);
> >   }
> 
> Yes, checking with is-enabled primarily for the purpose of skipping
> "do lots of work" is a very typical use.  I do not know why you
> assume ...
> 
> >
> > which _wouldn't_ want to convert to advise_if_enabled().
> 
> ... this "try X" is something the users would not want to learn how
> to disable, but assuming it is not, the existing code above as-is
> should be what we want.

I meant only that they would not want the same mechanical conversion
above, because that would lose the ability to avoid the extra work.

> > I guess the caller could just do:
> >
> >   advise_if_enabled(ADVICE_FOO, ...);
> >
> > inside the block. We know that it's enabled, but it's not like the check
> > is expensive.
> 
> Yes, I think we already have some callers that do so, in a pattern
> where they want to skip the "do lots of work" part.  Or at least I
> think I suggested the pattern in the past for somebody who wanted to
> do that.

I think we do the same thing with trace_want() in a few spots.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 09, 2026 at 11:51:03AM -0700, Junio C Hamano wrote:
>
>> > will. So there are many missed opportunities for offering the turn-off
>> > instructions. Nobody seems to have complained, which makes me wonder if
>> > the turn-off instructions would be annoyingly chatty if we printed them
>> > all the time. Most of those calls predate the addition if the turn-off
>> > instructions and advise_if_enabled(), which was added in 2020. I wonder
>> > how people would feel if we converted them all and started printing the
>> > turn-off instructions everywhere.
>> 
>> Depends on how we do so, I guess.  Do you mean we should rewrite
>> advise() call above to advice_if_enabled(), even though the check
>> for ADVICE_FOO token appear redundant?
>
> I mean we could mechanically rewrite:
>
>   if (advice_enabled(ADVICE_FOO))
> 	advise(...);
>
> to:
>
>   advise_if_enabled(ADVICE_FOO, ...);

Surely, and I think we are pretty much on the same page.  Such a
mechanical rewrite is not too bad.  Here is what I came up with:

    $ edit tools/coccinelle/advice.cocci
    $ make coccicheck
    $ git add -N tools/coccinelle/advice.cocci
    $ git apply .build/tools/coccinelle/ALL.cocci.patch
    $ git add -p

    Some of the hunks I simply accepted with (y), but most of them
    needed (e)dit to make them presentable; otherwise we ended up
    with too many overly long lines.



 tools/coccinelle/advice.cocci |  7 +++++++
 advice.c                      | 13 ++++---------
 branch.c                      |  6 +++---
 builtin/am.c                  |  4 ++--
 builtin/checkout.c            |  4 ++--
 builtin/submodule--helper.c   |  4 ++--
 sequencer.c                   |  9 ++++-----
 7 files changed, 24 insertions(+), 23 deletions(-)

diff --git c/tools/coccinelle/advice.cocci w/tools/coccinelle/advice.cocci
new file mode 100644
index 0000000000..da4851c5d0
--- /dev/null
+++ w/tools/coccinelle/advice.cocci
@@ -0,0 +1,7 @@
+@@
+expression A;
+expression list args;
+@@
+-if (advice_enabled(A))
+-	advise(args);
++advice_if_enabled(A, args);
diff --git c/advice.c w/advice.c
index 63bf8b0c5f..c60b33ee33 100644
--- c/advice.c
+++ w/advice.c
@@ -216,13 +216,8 @@ int error_resolve_conflict(const char *me)
 	else
 		BUG("Unhandled conflict reason '%s'", me);
 
-	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
-		/*
-		 * Message used both when 'git commit' fails and when
-		 * other commands doing a merge do.
-		 */
-		advise(_("Fix them up in the work tree, and then use 'git add/rm <file>'\n"
-			 "as appropriate to mark resolution and make a commit."));
+	advice_if_enabled(ADVICE_RESOLVE_CONFLICT,
+			  _("Fix them up in the work tree, and then use 'git add/rm <file>'\n" "as appropriate to mark resolution and make a commit."));
 	return -1;
 }
 
@@ -235,8 +230,8 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
-		advise(_("Please, commit your changes before merging."));
+	advice_if_enabled(ADVICE_RESOLVE_CONFLICT,
+			  _("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
 
diff --git c/branch.c w/branch.c
index 22f4f46b96..a87facd311 100644
--- c/branch.c
+++ w/branch.c
@@ -812,9 +812,9 @@ void create_branches_recursively(struct repository *r, const char *name,
 			int code = die_message(
 				_("submodule '%s': unable to find submodule"),
 				submodule_entry_list.entries[i].submodule->name);
-			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
-				advise(_("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"),
-				       start_committish);
+			advice_if_enabled(ADVICE_SUBMODULES_NOT_UPDATED,
+					  _("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"),
+					  start_committish);
 			exit(code);
 		}
 
diff --git c/builtin/am.c w/builtin/am.c
index e9623b8307..6039b69475 100644
--- c/builtin/am.c
+++ w/builtin/am.c
@@ -1910,8 +1910,8 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_enabled(ADVICE_AM_WORK_DIR))
-				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
+			advice_if_enabled(ADVICE_AM_WORK_DIR,
+					  _("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
 		}
diff --git c/builtin/checkout.c w/builtin/checkout.c
index 2bc21aa49b..34f05d2381 100644
--- c/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -1612,8 +1612,8 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
 		 */
 		code = die_message(_("a branch is expected, got '%s'"), branch_info->name);
 
-	if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD))
-		advise(_("If you want to detach HEAD at the commit, try again with the --detach option."));
+	advice_if_enabled(ADVICE_SUGGEST_DETACHING_HEAD,
+			  _("If you want to detach HEAD at the commit, try again with the --detach option."));
 
 	exit(code);
 }
diff --git c/builtin/submodule--helper.c w/builtin/submodule--helper.c
index e7cd3225fa..5e4989a9aa 100644
--- c/builtin/submodule--helper.c
+++ w/builtin/submodule--helper.c
@@ -1806,8 +1806,8 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
-					advise(_(alternate_error_advice));
+				advice_if_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+						  _(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
 			case SUBMODULE_ALTERNATE_ERROR_INFO:
diff --git c/sequencer.c w/sequencer.c
index 65afd100d9..6d8be0c036 100644
--- c/sequencer.c
+++ w/sequencer.c
@@ -624,8 +624,8 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
-		advise(_("commit your changes or stash them to proceed."));
+	advice_if_enabled(ADVICE_COMMIT_BEFORE_MERGE,
+			  _("commit your changes or stash them to proceed."));
 	return -1;
 }
 
@@ -3497,9 +3497,8 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
-			advise(in_progress_advice,
-				advise_skip ? "--skip | " : "");
+		advice_if_enabled(ADVICE_SEQUENCER_IN_USE, in_progress_advice,
+				  advise_skip ? "--skip | " : "");
 		return -1;
 	}
 	if (mkdir(git_path_seq_dir(), 0777) < 0)

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

[Jeff King]

On Wed, Sep 09, 2026 at 09:23:19PM -0700, Junio C Hamano wrote:

> Surely, and I think we are pretty much on the same page.  Such a
> mechanical rewrite is not too bad.  Here is what I came up with:
> 
>     $ edit tools/coccinelle/advice.cocci
>     $ make coccicheck
>     $ git add -N tools/coccinelle/advice.cocci
>     $ git apply .build/tools/coccinelle/ALL.cocci.patch
>     $ git add -p
> 
>     Some of the hunks I simply accepted with (y), but most of them
>     needed (e)dit to make them presentable; otherwise we ended up
>     with too many overly long lines.
> 
> 
>  tools/coccinelle/advice.cocci |  7 +++++++
>  advice.c                      | 13 ++++---------
>  branch.c                      |  6 +++---
>  builtin/am.c                  |  4 ++--
>  builtin/checkout.c            |  4 ++--
>  builtin/submodule--helper.c   |  4 ++--
>  sequencer.c                   |  9 ++++-----
>  7 files changed, 24 insertions(+), 23 deletions(-)

This misses a few that have more complex conditionals like:

diff --git a/commit.c b/commit.c
index ad26f0b40a..5eedad6a2c 100644
--- a/commit.c
+++ b/commit.c
@@ -290,9 +290,9 @@ static int read_graft_file(struct repository *r, const char *graft_file)
 	struct strbuf buf = STRBUF_INIT;
 	if (!fp)
 		return -1;
-	if (!no_graft_file_deprecated_advice &&
-	    advice_enabled(ADVICE_GRAFT_FILE_DEPRECATED))
-		advise(_("Support for <GIT_DIR>/info/grafts is deprecated\n"
+	if (!no_graft_file_deprecated_advice)
+		advise_if_enabled(ADVICE_GRAFT_FILE_DEPRECATED,
+			_("Support for <GIT_DIR>/info/grafts is deprecated\n"
 			 "and will be removed in a future Git version.\n"
 			 "\n"
 			 "Please use \"git replace --convert-graft-file\"\n"

But I think the bigger question remains: if we did this, would people
find the extra lines giving the turn-off instructions ugly/overwhelming?
I'm not sure.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> But I think the bigger question remains: if we did this, would people
> find the extra lines giving the turn-off instructions ugly/overwhelming?
> I'm not sure.

Well, if they find them unnecessary then they would want to turn it
off and the instruction is already there ;-)

More seriously, if an advice item is found as such, then the item
either must (1) be beneficial enough to be always shown, or (2) be
so rarely shown that the turn-off instruction is unneeded.  It would
inherently be case-by-case basis but I do think we would converge
between unconditional advise() calls or advise_if_enabled() calls.

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

[Jeff King]

On Thu, Sep 10, 2026 at 05:18:21AM -0700, Junio C Hamano wrote:

> Jeff King <peff@peff.net> writes:
> 
> > But I think the bigger question remains: if we did this, would people
> > find the extra lines giving the turn-off instructions ugly/overwhelming?
> > I'm not sure.
> 
> Well, if they find them unnecessary then they would want to turn it
> off and the instruction is already there ;-)

Well, it would certainly increase my desire to turn each one off. ;) I
guess you can set it to "true" to suppress the turn-off instructions
(but keep the advice itself).

> More seriously, if an advice item is found as such, then the item
> either must (1) be beneficial enough to be always shown, or (2) be
> so rarely shown that the turn-off instruction is unneeded.  It would
> inherently be case-by-case basis but I do think we would converge
> between unconditional advise() calls or advise_if_enabled() calls.

Right, I was wondering specifically if there are items in (1), but you
said it much better than I did. I guess we wouldn't know until we try
it and see people's reactions, though.

-Peff

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

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Wed, Sep 09, 2026 at 11:51:03AM -0700, Junio C Hamano wrote:
>
>> > will. So there are many missed opportunities for offering the turn-off
>> > instructions. Nobody seems to have complained, which makes me wonder if
>> > the turn-off instructions would be annoyingly chatty if we printed them
>> > all the time. Most of those calls predate the addition if the turn-off
>> > instructions and advise_if_enabled(), which was added in 2020. I wonder
>> > how people would feel if we converted them all and started printing the
>> > turn-off instructions everywhere.
>> 
>> Depends on how we do so, I guess.  Do you mean we should rewrite
>> advise() call above to advice_if_enabled(), even though the check
>> for ADVICE_FOO token appear redundant?
>
> I mean we could mechanically rewrite:
>
>   if (advice_enabled(ADVICE_FOO))
> 	advise(...);
>
> to:
>
>   advise_if_enabled(ADVICE_FOO, ...);

Surely, and I think we are pretty much on the same page.  Such a
mechanical rewrite is not too bad.  Here is what I came up with:

    $ edit tools/coccinelle/advice.cocci
    $ make coccicheck
    $ git add -N tools/coccinelle/advice.cocci
    $ git apply .build/tools/coccinelle/ALL.cocci.patch
    $ git add -p

    Some of the hunks I simply accepted with (y), but most of them
    needed (e)dit to make them presentable; otherwise we ended up
    with too many overly long lines and losing some comments.

--- >8 ---
Subject: [PATCH] advice: use advise_if_enabled() more

One very common pattern is

	if (advice_enabled(ADVICE_FOO))
		advise(_("MESSAGE FOR FOO"));

but we have a perfect short-hand for that.  Using coccinelle,
rewrite the above as

	advise_if_enabled(ADVICE_FOO, _("MESSAGE FOR FOR"));

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 advice.c                      | 13 ++++---------
 branch.c                      |  6 +++---
 builtin/am.c                  |  4 ++--
 builtin/checkout.c            |  4 ++--
 builtin/submodule--helper.c   |  4 ++--
 sequencer.c                   |  9 ++++-----
 tools/coccinelle/advice.cocci |  7 +++++++
 7 files changed, 24 insertions(+), 23 deletions(-)
 create mode 100644 tools/coccinelle/advice.cocci

diff --git a/advice.c b/advice.c
index 63bf8b0c5f..c60b33ee33 100644
--- a/advice.c
+++ b/advice.c
@@ -216,13 +216,8 @@ int error_resolve_conflict(const char *me)
 	else
 		BUG("Unhandled conflict reason '%s'", me);
 
-	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
-		/*
-		 * Message used both when 'git commit' fails and when
-		 * other commands doing a merge do.
-		 */
-		advise(_("Fix them up in the work tree, and then use 'git add/rm <file>'\n"
-			 "as appropriate to mark resolution and make a commit."));
+	advice_if_enabled(ADVICE_RESOLVE_CONFLICT,
+			  _("Fix them up in the work tree, and then use 'git add/rm <file>'\n" "as appropriate to mark resolution and make a commit."));
 	return -1;
 }
 
@@ -235,8 +230,8 @@ void NORETURN die_resolve_conflict(const char *me)
 void NORETURN die_conclude_merge(void)
 {
 	error(_("You have not concluded your merge (MERGE_HEAD exists)."));
-	if (advice_enabled(ADVICE_RESOLVE_CONFLICT))
-		advise(_("Please, commit your changes before merging."));
+	advice_if_enabled(ADVICE_RESOLVE_CONFLICT,
+			  _("Please, commit your changes before merging."));
 	die(_("Exiting because of unfinished merge."));
 }
 
diff --git a/branch.c b/branch.c
index 22f4f46b96..a87facd311 100644
--- a/branch.c
+++ b/branch.c
@@ -812,9 +812,9 @@ void create_branches_recursively(struct repository *r, const char *name,
 			int code = die_message(
 				_("submodule '%s': unable to find submodule"),
 				submodule_entry_list.entries[i].submodule->name);
-			if (advice_enabled(ADVICE_SUBMODULES_NOT_UPDATED))
-				advise(_("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"),
-				       start_committish);
+			advice_if_enabled(ADVICE_SUBMODULES_NOT_UPDATED,
+					  _("You may try updating the submodules using 'git checkout --no-recurse-submodules %s && git submodule update --init'"),
+					  start_committish);
 			exit(code);
 		}
 
diff --git a/builtin/am.c b/builtin/am.c
index e9623b8307..6039b69475 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -1910,8 +1910,8 @@ static void am_run(struct am_state *state, int resume)
 			printf_ln(_("Patch failed at %s %.*s"), msgnum(state),
 				linelen(state->msg), state->msg);
 
-			if (advice_enabled(ADVICE_AM_WORK_DIR))
-				advise(_("Use 'git am --show-current-patch=diff' to see the failed patch"));
+			advice_if_enabled(ADVICE_AM_WORK_DIR,
+					  _("Use 'git am --show-current-patch=diff' to see the failed patch"));
 
 			die_user_resolve(state);
 		}
diff --git a/builtin/checkout.c b/builtin/checkout.c
index 2bc21aa49b..34f05d2381 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1612,8 +1612,8 @@ static void die_expecting_a_branch(const struct branch_info *branch_info)
 		 */
 		code = die_message(_("a branch is expected, got '%s'"), branch_info->name);
 
-	if (advice_enabled(ADVICE_SUGGEST_DETACHING_HEAD))
-		advise(_("If you want to detach HEAD at the commit, try again with the --detach option."));
+	advice_if_enabled(ADVICE_SUGGEST_DETACHING_HEAD,
+			  _("If you want to detach HEAD at the commit, try again with the --detach option."));
 
 	exit(code);
 }
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
index e7cd3225fa..5e4989a9aa 100644
--- a/builtin/submodule--helper.c
+++ b/builtin/submodule--helper.c
@@ -1806,8 +1806,8 @@ static int add_possible_reference_from_superproject(
 		} else {
 			switch (sas->error_mode) {
 			case SUBMODULE_ALTERNATE_ERROR_DIE:
-				if (advice_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE))
-					advise(_(alternate_error_advice));
+				advice_if_enabled(ADVICE_SUBMODULE_ALTERNATE_ERROR_STRATEGY_DIE,
+						  _(alternate_error_advice));
 				die(_("submodule '%s' cannot add alternate: %s"),
 				    sas->submodule_name, err.buf);
 			case SUBMODULE_ALTERNATE_ERROR_INFO:
diff --git a/sequencer.c b/sequencer.c
index 65afd100d9..6d8be0c036 100644
--- a/sequencer.c
+++ b/sequencer.c
@@ -624,8 +624,8 @@ static int error_dirty_index(struct repository *repo, struct replay_opts *opts)
 	error(_("your local changes would be overwritten by %s."),
 		_(action_name(opts)));
 
-	if (advice_enabled(ADVICE_COMMIT_BEFORE_MERGE))
-		advise(_("commit your changes or stash them to proceed."));
+	advice_if_enabled(ADVICE_COMMIT_BEFORE_MERGE,
+			  _("commit your changes or stash them to proceed."));
 	return -1;
 }
 
@@ -3497,9 +3497,8 @@ static int create_seq_dir(struct repository *r)
 	}
 	if (in_progress_error) {
 		error("%s", in_progress_error);
-		if (advice_enabled(ADVICE_SEQUENCER_IN_USE))
-			advise(in_progress_advice,
-				advise_skip ? "--skip | " : "");
+		advice_if_enabled(ADVICE_SEQUENCER_IN_USE, in_progress_advice,
+				  advise_skip ? "--skip | " : "");
 		return -1;
 	}
 	if (mkdir(git_path_seq_dir(), 0777) < 0)
diff --git a/tools/coccinelle/advice.cocci b/tools/coccinelle/advice.cocci
new file mode 100644
index 0000000000..da4851c5d0
--- /dev/null
+++ b/tools/coccinelle/advice.cocci
@@ -0,0 +1,7 @@
+@@
+expression A;
+expression list args;
+@@
+-if (advice_enabled(A))
+-	advise(args);
++advice_if_enabled(A, args);
-- 
2.55.0-967-gab67bff200

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

[Vsevolod Myalitsin]

Hi,

I've sent v3 with the suggested changes.

In particular, v3 uses NULL for advise() calls that are not associated with an advice_setting entry, as suggested by Peff.

Please continue the discussion based on v3.

Thanks,
Vsevolod