Re: [PATCH v3 1/4] fetch: add remote.<name>.refmap

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

"Harald Nordgren via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: Harald Nordgren <haraldnordgren@gmail.com>
>
> Add a per-remote config variable, remote.<name>.refmap, that provides
> the default value for --refmap the same way remote.<name>.fetch
> already provides the default refspecs to fetch. It only takes effect
> when there is something explicit to fetch, on the command line or via

This ...

> remote.<name>.fetch, matching how --refmap itself already behaves.
>
> Signed-off-by: Harald Nordgren <haraldnordgren@gmail.com>

> +remote.<name>.refmap::
> +	The default value of the `--refmap` option for linkgit:git-fetch[1].
> +	Only takes effect when the fetch names what to fetch explicitly,
> +	either on the command line or via `remote.<name>.fetch`. See the
> +	`--refmap` entry in linkgit:git-fetch[1].

... and this made me a bit puzzled.  It may be a philosophical
difference, but I've always viewed --refmap=<src>:<dst> to "take
effect" whenever they are given, regardless of 0, 1, or more
explicit things to fetch.  It is just when you have zero explicit
things to fetch, 0 things are mapped via the refmap mechanism and 0
things are fetched.

In other words, what does not "take effect" when 0 things are given
explicitly to fetch is not the effect of refmap alone, but the
entire 'git fetch' operation itself.

    The default value of the `--refmap` option for linkgit:git-fetch[1].
    Used to map remote refs being fetched to remote-tracking refs to
    store.  See the `--refmap` entry in linkgit:git-fetch[1].

> @@ -244,6 +244,9 @@ endif::git-pull[]
>  	refspecs and rely entirely on the refspecs supplied as
>  	command-line arguments. See section on "Configured Remote-tracking
>  	Branches" for details.
> ++
> +`remote.<name>.refmap` provides the default value for this option, the
> +same way `remote.<name>.fetch` provides the default refspecs to fetch.

This is perfect.

> diff --git a/builtin/fetch.c b/builtin/fetch.c
> index 533fdfe7d8..7651b41139 100644
> --- a/builtin/fetch.c
> +++ b/builtin/fetch.c
> @@ -509,6 +509,8 @@ static struct ref *get_ref_map(struct remote *remote,
>  	struct ref *rm;
>  	struct ref *ref_map = NULL;
>  	struct ref **tail = &ref_map;
> +	struct refspec *effective_refmap =
> +		refmap.nr ? &refmap : remote ? &remote->refmap : NULL;
>  
>  	/* opportunistically-updated references: */
>  	struct ref *orefs = NULL, **oref_tail = &orefs;
> @@ -552,14 +554,14 @@ static struct ref *get_ref_map(struct remote *remote,
>  		 * by ref_remove_duplicates() in favor of one of these
>  		 * opportunistic entries with FETCH_HEAD_IGNORE.
>  		 */
> -		if (refmap.nr)
> -			fetch_refspec = &refmap;
> +		if (effective_refmap && effective_refmap->nr)
> +			fetch_refspec = effective_refmap;
>  		else
>  			fetch_refspec = &remote->fetch;
>  
>  		for (i = 0; i < fetch_refspec->nr; i++)
>  			get_fetch_map(ref_map, &fetch_refspec->items[i], &oref_tail, 1);
> -	} else if (refmap.nr) {
> +	} else if (effective_refmap && effective_refmap->nr) {
>  		die("--refmap option is only meaningful with command-line refspec(s)");
>  	} else {
>  		/* Use the defaults */

Looking good.  One of these days, we probably should reduce our
reliance on the file-scope static "global variables" but that is
clearly outside the scope of this topic.  Perhaps once the dust
settles after this topic stabilizes.  I wonder if most of them can
be added as members to "struct fetch_config" and then we can pass
one instance of such struct around in the call chain, or if it
needs a lot more involved changes.