Re: git-clone --config order & fetching extra refs during initial clone

[Jeff King <peff@peff.net>]

On Wed, May 03, 2017 at 04:42:58PM +0200, SZEDER Gábor wrote:

> write_refspec_config() nicely encapsulates writing the proper fetch
> refspec configuration according to the given command line options.  Of
> course these options are already known right at the start, so solely
> in this regard we could call this function earlier.  However, in some
> cases, e.g. '--single-branch', the refspec to be written to the config
> depends not only on the given options but on the refs in the remote
> repository, too, so it can only be written after we got the remote's
> refs.

Good point. We can't really consider clone to be a blind "init + config
+ fetch + checkout" because those middle two steps sometimes overlap
each other.  It really does need to be its own beast.

> The root issue is that 'git clone' calls directly into the fetch
> machinery instead of running 'git fetch' (either via run_command() or
> cmd_fetch()), and some of these "higher-level" config variables are
> only handled in 'builtin/fetch.c' but not in 'git clone'.  By
> "handle" I mean "parse and act accordingly": as it happens, these
> config values are parsed alright when 'git clone' calls remote_get(),
> but it never looks at the relevant fields in the resulting 'struct
> remote'.
> 
> Luckily, many "lower-level" config variables are working properly even
> during 'git clone', because they are handled in the transport layer,
> e.g. 'git clone -c url.https://github.com/.insteadof=gh: gh:git/git'
> does the right thing.

Yeah, and I think that insteadOf is working as intended there (clone
sets it early enough that all of the rest of the code sees it). And the
bug is just that there's special handling in builtin/fetch.c that clone
needs to replicate.

The right solution there is probably pushing that logic down into the
transport layer. Or at the very least abstracting it into a function so
that both clone and fetch can call it without replicating the logic.

> My patch deals with 'remote.<name>.refspec', i.e. 'remote->fetch'.
> Apparently some extra care is necessary for 'remote.<name>.tagOpt' and
> 'remote->fetch_tags', too.  Perhaps there are more, I haven't checked
> again, and maybe we'll add similar config variables in the future.  So
> I don't think that dealing with such config variables one by one in
> 'git clone', too, is the right long-term solution...  but perhaps it's
> sufficient for the time being?

I think your patch is a strict improvement and we don't need to hold up
waiting for a perfect fix (and because of the --single-branch thing you
mentioned, this may be the best we can do anyway).

> Running a fully-fledged 'git fetch' seems to be simpler and safer
> conceptually, as it would naturally handle all fetch-related config
> variables, present and future.  However, it's not without drawbacks:
> 'git clone' must set the proper config before running 'git fetch' (or
> at least set equivalent cmdline options), which in some cases requires
> the refs in the remote repository, making an additional "list remote
> refs" step necessary (i.e. both 'clone' and 'fetch' would have to
> retrieve the refs from the remote, resulting in more network I/O).

I don't think we ever want to request two ref advertisements; they're
too expensive. If clone needs to do work between the advertisement and
the actual fetch (and it sounds like it does), then it should be using
the transport layer directly. Which is what it's already doing.

-Peff