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
