On Wed Dec 04, 2024 at 03:20, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Bence Ferdinandy <bence@xxxxxxxxxxxxxx> 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".
Ah, nice, this has the added benefit of a bad configuration where
"warn-if-not-" doesn't actually specify a branch to fall back to just "warn".
>
>> 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.
--
bence.ferdinandy.com
