fetch.c: defer fetch.followRemoteHEAD validation The value of the fetch.followRemoteHEAD configuration variable is validated while the configuration file is being parsed, which produces a warning even when this particular "git fetch" invocation will never consult it. Store the raw config string instead, and resolve/validate it lazily at the one place in do_fetch() that actually uses it, so a mistyped value only warns, and a missing value only dies, when this fetch would have consulted it. remote.c's handle_config() has the same problem for remote.<name>.followRemoteHEAD, but is left unaddressed here since it touches shared remote-parsing infrastructure used well beyond fetch. Leave a NEEDSWORK comment at remote.c:handle_config() that has a defect similar to what is fixed by this patch, so the remaining scope is easy to find for a follow-up patch. Signed-off-by: Colin Hinton <colinlewishinton@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/builtin/fetch.c b/builtin/fetch.c index b2decc6..4fee710 100644 --- a/builtin/fetch.c +++ b/builtin/fetch.c
@@ -103,7 +103,8 @@ static struct string_list negotiation_include = STRING_LIST_INIT_NODUP; struct fetch_config { enum display_format display_format; - enum follow_remote_head_settings follow_remote_head; + char *follow_remote_head_raw; + int follow_remote_head_seen; int all; int prune; int prune_tags; @@ -176,24 +177,31 @@ static int git_fetch_config(const char *k, const char *v, } if (!strcmp(k, "fetch.followremotehead")) { - if (!v) - return config_error_nonbool(k); - else if (!strcmp(v, "never")) - fetch_config->follow_remote_head = FOLLOW_REMOTE_NEVER; - else if (!strcmp(v, "create")) - fetch_config->follow_remote_head = FOLLOW_REMOTE_CREATE; - else if (!strcmp(v, "warn")) - fetch_config->follow_remote_head = FOLLOW_REMOTE_WARN; - else if (!strcmp(v, "always")) - fetch_config->follow_remote_head = FOLLOW_REMOTE_ALWAYS; - else - warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), v); + free(fetch_config->follow_remote_head_raw); + fetch_config->follow_remote_head_raw = xstrdup_or_null(v); + fetch_config->follow_remote_head_seen = 1; return 0; } return git_default_config(k, v, ctx, cb); } +static enum follow_remote_head_settings get_follow_remote_head(const char *setting) +{ + if (!setting) + die(_("missing value for 'fetch.followRemoteHEAD'")); + else if (!strcmp(setting, "never")) + return FOLLOW_REMOTE_NEVER; + else if (!strcmp(setting, "create")) + return FOLLOW_REMOTE_CREATE; + else if (!strcmp(setting, "warn")) + return FOLLOW_REMOTE_WARN; + else if (!strcmp(setting, "always")) + return FOLLOW_REMOTE_ALWAYS; + warning(_("unrecognized fetch.followRemoteHEAD value '%s' ignored"), setting); + return BUILTIN_FOLLOW_REMOTE_HEAD_DFLT; +} + static int parse_refmap_arg(const struct option *opt, const char *arg, int unset) { BUG_ON_OPT_NEG(unset); @@ -1918,11 +1926,10 @@ static int do_fetch(struct transport *transport, TRANSPORT_LS_REFS_OPTIONS_INIT; struct fetch_head fetch_head = { 0 }; struct strbuf err = STRBUF_INIT; - int do_set_head = 0; struct ref_update_display_info_array display_array = { 0 }; struct strmap rejected_refs = STRMAP_INIT; int summary_width = 0; - int follow_remote_head; + int follow_remote_head = FOLLOW_REMOTE_NEVER; if (tags == TAGS_DEFAULT) { if (transport->remote->fetch_tags == 2) @@ -1938,22 +1945,6 @@ static int do_fetch(struct transport *transport, goto cleanup; } - /* - * NEEDSWORK: By the time this function executes, we have already parsed - * all such followRemoteHEAD values from the external configuration, - * potentially emitting warning messages for bogus values. Ideally, if - * this fetch ends up not needing to consult these values, then git would - * not ever output a value warning. (eg: when pulling from a URL directly - - * rather than a configured remote, or when a remote's followRemoteHEAD - * overrides the fallback fetch setting) - */ - if (transport->remote->follow_remote_head) - follow_remote_head = transport->remote->follow_remote_head; - else if (config->follow_remote_head) - follow_remote_head = config->follow_remote_head; - else - follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT; - if (rs->nr) { refspec_ref_prefixes(rs, &transport_ls_refs_options.ref_prefixes); } else { @@ -1962,8 +1953,16 @@ static int do_fetch(struct transport *transport, if (transport->remote->fetch.nr) { refspec_ref_prefixes(&transport->remote->fetch, &transport_ls_refs_options.ref_prefixes); - if (follow_remote_head != FOLLOW_REMOTE_NEVER) - do_set_head = 1; + /* + * See remote.c's handling of remote.<name>.followRemoteHEAD + * for the analogous, still-unresolved case. + */ + if (transport->remote->follow_remote_head) + follow_remote_head = transport->remote->follow_remote_head; + else if (config->follow_remote_head_seen) + follow_remote_head = get_follow_remote_head(config->follow_remote_head_raw); + else + follow_remote_head = BUILTIN_FOLLOW_REMOTE_HEAD_DFLT; } if (branch && branch_has_merge_config(branch) && !strcmp(branch->remote_name, transport->remote->name)) { @@ -1987,7 +1986,7 @@ static int do_fetch(struct transport *transport, strvec_push(&transport_ls_refs_options.ref_prefixes, "refs/tags/"); - if (do_set_head) + if (follow_remote_head != FOLLOW_REMOTE_NEVER) strvec_push(&transport_ls_refs_options.ref_prefixes, "HEAD"); @@ -2164,7 +2163,7 @@ static int do_fetch(struct transport *transport, "you need to specify exactly one branch with the --set-upstream option")); } } - if (do_set_head) { + if (follow_remote_head != FOLLOW_REMOTE_NEVER) { /* * Way too many cases where this can go wrong so let's just * ignore errors and fail silently for now. @@ -2509,7 +2508,8 @@ int cmd_fetch(int argc, { struct fetch_config config = { .display_format = DISPLAY_FORMAT_FULL, - .follow_remote_head = FOLLOW_REMOTE_UNCONFIGURED, + .follow_remote_head_raw = NULL, + .follow_remote_head_seen = 0, .prune = -1, .prune_tags = -1, .show_forced_updates = 1, @@ -2929,5 +2929,6 @@ int cmd_fetch(int argc, cleanup: string_list_clear(&list, 0); list_objects_filter_release(&filter_options); + free(config.follow_remote_head_raw); return result; }
diff --git a/remote.c b/remote.c index fe62068..58f3436 100644 --- a/remote.c +++ b/remote.c
@@ -582,6 +582,13 @@ static int handle_config(const char *key, const char *value, &remote->negotiation_include); } else if (!strcmp(subkey, "followremotehead")) { const char *no_warn_branch; + /* + * NEEDSWORK: this is validated/warned about here, during config + * parsing, regardless of whether the fetch that triggered this + * parse will ever consult it for this particular remote. See + * fetch.c's deferred handling of fetch.followRemoteHEAD for the + * pattern this should likely follow. + */ if (!strcmp(value, "never")) remote->follow_remote_head = FOLLOW_REMOTE_NEVER; else if (!strcmp(value, "create"))