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

[Junio C Hamano <gitster@pobox.com>]

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.