Re: [PATCH 2/3] clone: respect additional configured fetch refspecs during initial fetch

[Jeff King <peff@peff.net>]

On Wed, Nov 14, 2018 at 11:46:19AM +0100, SZEDER Gábor wrote:

> The initial fetch during a clone doesn't transfer refs matching
> additional fetch refspecs given on the command line as configuration
> variables, e.g. '-c remote.origin.fetch=<refspec>'.  This contradicts
> the documentation stating that configuration variables specified via
> 'git clone -c <key>=<value> ...' "take effect immediately after the
> repository is initialized, but before the remote history is fetched"
> and the given example specifically mentions "adding additional fetch
> refspecs to the origin remote".  Furthermore, one-shot configuration
> variables specified via 'git -c <key>=<value> clone ...', though not
> written to the newly created repository's config file, live during the
> lifetime of the 'clone' command, including the initial fetch.  All
> this implies that any fetch refspecs specified this way should already
> be taken into account during the initial fetch.
> 
> The reason for this is that the initial fetch is not a fully fledged
> 'git fetch' but a bunch of direct calls into the fetch/transport
> machinery with clone's own refs-to-refspec matching logic, which
> bypasses parts of 'git fetch' processing configured fetch refspecs.
> This logic only considers a single default refspec, potentially
> influenced by options like '--single-branch' and '--mirror'.  The
> configured refspecs are, however, already read and parsed properly
> when clone calls remote.c:remote_get(), but it never looks at the
> parsed refspecs in the resulting 'struct remote'.
> 
> Modify clone to take the remote's configured fetch refspecs into
> account to retrieve all matching refs during the initial fetch.  Note
> that we have to explicitly add the default fetch refspec to the
> remote's refspecs, because at that point the remote only includes the
> fetch refspecs specified on the command line.

Nicely explained.

> Add tests to check that refspecs given both via 'git clone -c ...' and
> 'git -c ... clone' retrieve all refs matching either the default or
> the additional refspecs, and that it works even when the user
> specifies an alternative remote name via '--origin=<name>'.

Good. This is all sufficiently subtle that we definitely want to check
the related cases.

> @@ -1081,11 +1086,12 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_required_reference.nr || option_optional_reference.nr)
>  		setup_reference();
>  
> +	remote = remote_get(option_origin);
> +
>  	strbuf_addf(&default_refspec, "+%s*:%s*", src_ref_prefix,
>  		    branch_top.buf);
> -	refspec_append(&rs, default_refspec.buf);
> +	refspec_append(&remote->fetch, default_refspec.buf);

Wow, this is so much nicer than the older versions. :)

I wondered briefly whether this ought to only kick in when the user
didn't specify any refspecs. I.e., there is no way with this to say
"don't fetch refs/heads/*, but this other thing instead". But the way
the existing documentation is written, it's pretty clear that it's about
adding to the list of refspecs. We can always something like
"--no-default-refspec" later if somebody wants it.

> [...]

Most of the rest of the patch is just swapping out "rs" for
"remote->fetch", which makes sense. So the implementation looks good to
me.

> diff --git a/t/t5611-clone-config.sh b/t/t5611-clone-config.sh
> index 39329eb7a8..60c1ba951b 100755
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -45,6 +45,53 @@ test_expect_success 'clone -c config is available during clone' '
>  	test_cmp expect child/file
>  '
>  
> +test_expect_success 'clone -c remote.origin.fetch=<refspec> works' '
> +	rm -rf child &&
> +	git update-ref refs/grab/it refs/heads/master &&
> +	git update-ref refs/leave/out refs/heads/master &&

Checking one in and one out; nice.

> +	git clone -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" . child &&
> +	git -C child for-each-ref --format="%(refname)" >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	refs/grab/it
> +	refs/heads/master
> +	refs/remotes/origin/HEAD
> +	refs/remotes/origin/master
> +	EOF
> +	test_cmp expect actual
> +'

Makes sense.

> +test_expect_success 'git -c remote.origin.fetch=<refspec> clone works' '
> +	rm -rf child &&
> +	git -c "remote.origin.fetch=+refs/grab/*:refs/grab/*" clone . child &&
> +	git -C child for-each-ref --format="%(refname)" >actual &&
> +
> +	cat >expect <<-\EOF &&
> +	refs/grab/it
> +	refs/heads/master
> +	refs/remotes/origin/HEAD
> +	refs/remotes/origin/master
> +	EOF
> +	test_cmp expect actual
> +'

This relies on establishing grab/it in the previous test. A minor nit,
but we could break that more clear by breaking it out into its own
"set up refs for extra refspec tests" block.

> +test_expect_success 'clone -c remote.<remote>.fetch=<refspec> --origin=<name>' '
> +	rm -rf child &&
> +	git clone --origin=upstream \
> +		  -c "remote.upstream.fetch=+refs/grab/*:refs/grab/*" \
> +		  -c "remote.origin.fetch=+refs/leave/*:refs/leave/*" \
> +		  . child &&

Nice.

The whole thing looks very cleanly done.

-Peff