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