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

From: Jeff King <peff@peff.net>
Date: Sep 10, 2026, 15:52 UTC
Message-ID: <20260910155247.GA251185@coredump.intra.peff.net>
In-Reply-To: <xmqqzexpf78k.fsf@gitster.g>

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

Show 32 quoted lines

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