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

> Bence Ferdinandy <bence@ferdinandy.com> writes:
>
>> +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.
>
>> -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".

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