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

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

quoted

@@ -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