Re: [RFC PATCH v1 1/2] fetch set_head: add warn-if-not-$branch option

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 4, 2024, 02:20 UTC
Message-ID: <xmqqh67k5gv8.fsf@gitster.g>
In-Reply-To: <20241203215713.135068-2-bence@ferdinandy.com>

Bence Ferdinandy <bence@ferdinandy.com> writes:

Show 8 quoted lines

> +static void set_head_advice_msg(const char *remote, const char *head_name) {
> +
> +	printf("Run 'git remote set-head %s %s' to follow the change, or\n"
> +		"'git config set remote.%s.warn-if-not-%s' to disable this warning\n"
> +		"until the remote changes HEAD again.\n",
> +		remote, head_name, remote, head_name);
> +
> +}

Style. "{" that encloses the function body sits on a line of its own.

Perhaps use the advise_if_enabled(), so that those who already learned how to deal with the situation can squelch the "how to fix" message.

Show 15 quoted lines

> -static int set_head(const struct ref *remote_refs, int follow_remote_head)
> +static int set_head(const struct ref *remote_refs, int follow_remote_head,
> +		const char *no_warn_branch)
>  {
>  	int result = 0, create_only, is_bare, was_detached;
>  	struct strbuf b_head = STRBUF_INIT, b_remote_head = STRBUF_INIT,
> @@ -1660,7 +1668,10 @@ static int set_head(const struct ref *remote_refs, int follow_remote_head)
>  		result = 1;
>  		goto cleanup;
>  	}
> -	if (follow_remote_head == FOLLOW_REMOTE_WARN && verbosity >= 0)
> +	if ((follow_remote_head == FOLLOW_REMOTE_WARN ||
> +		(follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH &&
> +		strcmp(no_warn_branch, head_name))
> +		) && verbosity >= 0)

Reorder conditions combined with &&- to have more expensive ones later.

	if (verbosity >= 0 &&
            (follow_remote_head == FOLLOW_REMOTE_WARN ||
	    (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH &&
	     strcmp(no_warn_branch, head_name)))

As human readers, we may know that no_warn_branch is always valid when (follow_remote_head == FOLLOW_REMOTE_WARN_IF_NOT_BRANCH), but semi clever compilers may not realize it and give a false warning about using no_warn_branch potentially uninitialized.

We could do without adding FOLLOW_REMOTE_WARN_IF_NOT_BRANCH and reuse FOLLOW_REMOTE_WARN, like so:

	if (verbosity >= 0 &&
            follow_remote_head == FOLLOW_REMOTE_WARN &&
	    (!no_warn_branch || strcmp(no_warn_branch, head_name)))

That is, "if set to remote-warn, always warn, or no_warn_branch is not NULL, only warn if the current head is different from it".

Show 20 quoted lines

> diff --git a/remote.c b/remote.c
> index 0b18840d43..f0e1b1b76a 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -515,6 +515,7 @@ static int handle_config(const char *key, const char *value,
>  		return parse_transport_option(key, value,
>  					      &remote->server_options);
>  	} else if (!strcmp(subkey, "followremotehead")) {
> +		const char *no_warn_branch;
>  		if (!strcmp(value, "never"))
>  			remote->follow_remote_head = FOLLOW_REMOTE_NEVER;
>  		else if (!strcmp(value, "create"))
> @@ -523,6 +524,10 @@ static int handle_config(const char *key, const char *value,
>  			remote->follow_remote_head = FOLLOW_REMOTE_WARN;
>  		else if (!strcmp(value, "always"))
>  			remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
> +		else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) {
> +			remote->follow_remote_head = FOLLOW_REMOTE_WARN_IF_NOT_BRANCH;
> +			remote->no_warn_branch = no_warn_branch;
> +		}

If we were to do without FOLLOW_REMOTE_WARN_IF_NOT_BRANCH, then the above becomes

			remote->follow_remote_head = FOLLOW_REMOTE_WARN;
			remote->no_warn_branch = NULL;
		} else if (skip_prefix(value, "warn-if-not-", &no_warn_branch)) {
			remote->follow_remote_head = FOLLOW_REMOTE_WARN;
			remote->no_warn_branch = no_warn_branch;
		} else if (!strcmp(value, "always")) {
			remote->follow_remote_head = FOLLOW_REMOTE_ALWAYS;
		} else {
			warn(_("unrecognized followRemoteHEAD value '%s' ignored"),
			     value);
		}

We'd want the new choice documented before we graduate this topic out of the RFC status.

Thanks.