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

[Jeff King <peff@peff.net>]

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