Re: [PATCH v4] fetch.c: defer fetch.followRemoteHEAD validation

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

Colin Hinton <colinlewishinton@gmail.com> writes:

> My rationale for this recent change came from when I was evaluating
> what calls get_follow_remote_head() in my patch.
>
> With the current design, get_follow_remote_head() is only called in
> do_fetch() in this conditional else if
> (config->follow_remote_head_raw).
>
> If followRemoteHEAD is now NULL because we set it as such in
> fetch_config->follow_remote_head_raw = xstrdup_or_null(v); Then this
> conditional is skipped, and we will never call the die(), and alert
> the user that their value is blank.
>
> To fully fix based on your suggestion, I suppose the design question
> is, should empty string warn or die?

I think we should behave the same when we see "nvere".  We do not
understand what they wanted us to do in either case, so we should
behave the same way, be it warn-and-ignore or complain-and-die.

Given that we now check the validity of the value only after we
determine that we need it, I think it is OK to tighten the rules
to die() instead of warn().  The historical behavior of not dying,
and instead warning and ignoring, was a weak excuse for leaving
configuration parsing broken and checking the validity of the value
in the wrong place.

This patch rectifies the situation, which is a very good step
toward doing the right thing.  It is perfectly fine to tighten
the rules as a separate topic after this patch lands and things
stabilize, but this patch lays the groundwork for us to move in
that direction.

> If empty string should warn, I likely will need to add some value in
> the fetch_config struct such as follow_remote_head_seen, and use this
> as our conditional in do_fetch() rather than the
> follow_remote_head_raw, to account for when followRemoteHEAD was set
> to anything. Then when the check in get_follow_remote_head() occurs,
> we know to die or warn based on NULL, or bogus.

... because?  Ah, because then you lose distinction between "the
configuration variable not set at all" and "the configuration
variable is set to the valueless true"?

If so, you'd need to be able to tell _three_ cases.  The empty
string you use as a stand in for "valueless true" should be
distinguishable from the empty string the user set (by mistake).

> Visually, it would look something like this.
>
> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 2cb0bcca8b..af22f63954 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -104,6 +104,7 @@ static struct string_list negotiation_include =
> STRING_LIST_INIT_NODUP;
>  struct fetch_config {
>         enum display_format display_format;
>         char *follow_remote_head_raw;
> +       int follow_remote_head_seen;

That would certainly work, and might be easier to work with than
what I would have done, which is not to bother with this extra
variable and instead to have a

	static const char *valueless_true = "true";

in the file scope.  Then use that ...

>         int all;
>         int prune;
>         int prune_tags;
> @@ -177,10 +178,8 @@ static int git_fetch_config(const char *k, const char *v,
>
>         if (!strcmp(k, "fetch.followremotehead")) {
>                 free(fetch_config->follow_remote_head_raw);
> -               if (!v)
> -                       fetch_config->follow_remote_head_raw = xstrdup("");
> -               else
> -                       fetch_config->follow_remote_head_raw = xstrdup(v);
> +               fetch_config->follow_remote_head_raw = xstrdup_or_null(v);

... here like so:

		if (fetch_config->follow_remote_head_raw != valueless_true)
			free(fetch_config->follow_remote_head_raw);
		if (!v)
			fetch_config->follow_remote_head_raw = valueless_true;
		else
			...


> @@ -189,7 +188,7 @@ static int git_fetch_config(const char *k, const char *v,
>
>  static enum follow_remote_head_settings get_follow_remote_head(const
> char *setting)
>  {
> -       if (!setting || !*setting)
> +       if (!setting) /*!*setting would return true on "" removing to
> warn instead*/
>                 die(_("missing value for 'fetch.followRemoteHEAD'"));
>         else if (!strcmp(setting, "never"))
>                 return FOLLOW_REMOTE_NEVER;

... and deal with the setting that is equal to valueless_true here.

I think either way would work, and the way you outlined would be
better.