Let's see how well it fares by cooking it in 'next' ;-)
I think it was [off-list ref],
which needs a bit of massaging to apply to the current codebase.
-- >8 --
From: SZEDER Gábor <redacted>
Date: Wed, 30 Mar 2016 16:53:43 +0200
Subject: [PATCH] clone: respect configured fetch respecs during initial fetch
Conceptually 'git clone' should behave as if the following commands
were run:
git init
git config ... # set default configuration and origin remote
git fetch
git checkout # unless '--bare' is given
However, that initial 'git fetch' behaves differently from any
subsequent fetches, because it takes only the default fetch refspec
into account and ignores all other fetch refspecs that might have
been explicitly specified on the command line (e.g. 'git -c
remote.origin.fetch=<refspec> clone' or 'git clone -c ...').
Check whether there are any fetch refspecs configured for the origin
remote and take all of them into account during the initial fetch as
well.
Signed-off-by: SZEDER Gábor <redacted>
---
builtin/clone.c | 36 ++++++++++++++++++++++++++++--------
t/t5611-clone-config.sh | 24 ++++++++++++++++++++++++
2 files changed, 52 insertions(+), 8 deletions(-)
@@ -37,6 +37,30 @@ test_expect_success 'clone -c config is available during clone' 'test_cmpexpectchild/file'+test_expect_success'clone -c remote.origin.fetch=<refspec> works''+rm-rfchild&&+gitupdate-refrefs/grab/itrefs/heads/master&&+gitupdate-refrefs/keep/outrefs/heads/master&&+gitclone-c"remote.origin.fetch=+refs/grab/*:refs/grab/*".child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'++test_expect_success'git -c remote.origin.fetch=<refspec> clone works''+rm-rfchild&&+git-c"remote.origin.fetch=+refs/grab/*:refs/grab/*"clone.child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'+# Tests for the hidden file attribute on windows is_hidden(){# Use the output of `attrib`, ignore the absolute path
Let's see how well it fares by cooking it in 'next' ;-)
I think it was [off-list ref],
which needs a bit of massaging to apply to the current codebase.
Yeah, that is the most recent one I found.
I didn't actually review it very carefully before, but I'll do so now
(spoiler: a few nits, but it looks fine).
This "1" seems funny to up here by itself, as it must be kept in sync
with the later logic that feeds exactly one non-configured refspec into
our list. The current code isn't wrong, but it would be nice to have it
all together. I.e., replacing:
with:
refspec_count = 1;
if (config_fetch_patterns)
refspec_count += config_fetch_patterns->nr;
...
Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.
I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.
@@ -37,6 +37,30 @@ test_expect_success 'clone -c config is available during clone' 'test_cmpexpectchild/file'+test_expect_success'clone -c remote.origin.fetch=<refspec> works''+rm-rfchild&&+gitupdate-refrefs/grab/itrefs/heads/master&&+gitupdate-refrefs/keep/outrefs/heads/master&&+gitclone-c"remote.origin.fetch=+refs/grab/*:refs/grab/*".child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'++test_expect_success'git -c remote.origin.fetch=<refspec> clone works''+rm-rfchild&&+git-c"remote.origin.fetch=+refs/grab/*:refs/grab/*"clone.child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'
These look reasonable. Using "git -C for-each-ref" would save a
subshell, but that's minor.
If we wanted to be thorough, we could also check that the feature works
correctly with "--origin" (I think it does).
-Peff
This "1" seems funny to up here by itself, as it must be kept in sync
with the later logic that feeds exactly one non-configured refspec into
our list. The current code isn't wrong, but it would be nice to have it
all together. I.e., replacing:
with:
refspec_count = 1;
if (config_fetch_patterns)
refspec_count += config_fetch_patterns->nr;
...
Agreed.
Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.
Yeah, I did notice that you love argv_array :) I had to raise an
eyebrow recently while looking at send-pack and how it uses argv_array
to read refspecs from stdin into an array. I think argv_array feels a
bit out of place in both cases. Yes, it does exactly what's needed.
However, it's called *argv*_array, indicating that its contents is
destined to become the options of some command. But that's not the
case in these two cases, we are not dealing with arguments to a
command, these are just arrays of strings.
However, leveraging get_remote() makes it moot anyway.
I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.
Though get_remote() does give us this config, at this point the
default fetch refspec has not been configured yet, therefore it's not
included in the resulting remote->fetch array. The default refspec is
set in write_refspec_config(), but that's called only about two
screenfulls later. So there is a bit of extra work to do in order to
leverage get_remote()'s parsing.
I think the simplest way is to keep parsing the default fetch refspec
manually, and then append it to the remote->fetch array. Definitely
shorter and simpler than that parsing in the current patch.
Alternatively, we could set the default fetch refspec in the
configuration temporarily, only for the duration of the get_remote()
call, but it feels a bit too hackish...
@@ -37,6 +37,30 @@ test_expect_success 'clone -c config is available during clone' 'test_cmpexpectchild/file'+test_expect_success'clone -c remote.origin.fetch=<refspec> works''+rm-rfchild&&+gitupdate-refrefs/grab/itrefs/heads/master&&+gitupdate-refrefs/keep/outrefs/heads/master&&+gitclone-c"remote.origin.fetch=+refs/grab/*:refs/grab/*".child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'++test_expect_success'git -c remote.origin.fetch=<refspec> clone works''+rm-rfchild&&+git-c"remote.origin.fetch=+refs/grab/*:refs/grab/*"clone.child&&+(+cdchild&&+gitfor-each-ref--format="%(refname)"refs/grab/>../actual+)&&+echorefs/grab/it>expect&&+test_cmpexpectactual+'
These look reasonable. Using "git -C for-each-ref" would save a
subshell, but that's minor.
Loosing a subshell is good, but I subjectively like how the
indentation highlights that that command operates on a different
repository.
However, the tests should also check that refs matching the default
fetch refspec are transferred, too, i.e. that the clone has something
under refs/remotes/origin/ as well. Case in point is using the result
of get_remote(): at first I naively set out to use remote->fetch
as-is, which didn't include the default fetch refspec, hence didn't
fetch refs/heads/master, but the test succeeded nonetheless.
If we wanted to be thorough, we could also check that the feature works
correctly with "--origin" (I think it does).
I think it works, but I'm not quite sure what you mean with "works
correctly with --origin".
So just to be clear: the behaviour depends on whether the remote name
given in '-c remote.<name>.fetch=<refspec>' matches the name given to
the '--origin=<name>'. If they match, then refs matching the
additional refspec are transferred, too. That's good. However, if
the two remote names don't match, then refs matching the additional
refspec are NOT transferred. I think this is good, too, because only
the origin remote should be cloned, whatever it is called, and in this
case that additional refspec belongs to a different remote.
Gábor
Most of the changes here and elsewhere are just about passing along
multiple refspecs instead of a single, which makes sense.
The new parameter should perhaps be renamed to 'refspec_nr', though,
as '_nr' suffix seems to be more common in the codebase than '_count'.
Yeah, agreed.
quoted
Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.
Yeah, I did notice that you love argv_array :) I had to raise an
eyebrow recently while looking at send-pack and how it uses argv_array
to read refspecs from stdin into an array. I think argv_array feels a
bit out of place in both cases. Yes, it does exactly what's needed.
However, it's called *argv*_array, indicating that its contents is
destined to become the options of some command. But that's not the
case in these two cases, we are not dealing with arguments to a
command, these are just arrays of strings.
In my mind, "argv" is synonymous with "NULL-terminated array of
strings". If the name is the only thing keeping it from wider use, I'd
much prefer us to give it a more generic name. All I really care about
is simplifying memory management. :)
However, leveraging get_remote() makes it moot anyway.
Even better.
quoted
I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.
Though get_remote() does give us this config, at this point the
default fetch refspec has not been configured yet, therefore it's not
included in the resulting remote->fetch array. The default refspec is
set in write_refspec_config(), but that's called only about two
screenfulls later. So there is a bit of extra work to do in order to
leverage get_remote()'s parsing.
I think the simplest way is to keep parsing the default fetch refspec
manually, and then append it to the remote->fetch array. Definitely
shorter and simpler than that parsing in the current patch.
Alternatively, we could set the default fetch refspec in the
configuration temporarily, only for the duration of the get_remote()
call, but it feels a bit too hackish...
Yeah, I think manually combining the two here is fine. Though I won't
complain if you want to look into setting the config earlier. If the
refactoring isn't too bad, it would probably provide the nicest outcome.
However, the tests should also check that refs matching the default
fetch refspec are transferred, too, i.e. that the clone has something
under refs/remotes/origin/ as well. Case in point is using the result
of get_remote(): at first I naively set out to use remote->fetch
as-is, which didn't include the default fetch refspec, hence didn't
fetch refs/heads/master, but the test succeeded nonetheless.
Good point.
quoted
If we wanted to be thorough, we could also check that the feature works
correctly with "--origin" (I think it does).
I think it works, but I'm not quite sure what you mean with "works
correctly with --origin".
So just to be clear: the behaviour depends on whether the remote name
given in '-c remote.<name>.fetch=<refspec>' matches the name given to
the '--origin=<name>'. If they match, then refs matching the
additional refspec are transferred, too. That's good. However, if
the two remote names don't match, then refs matching the additional
refspec are NOT transferred. I think this is good, too, because only
the origin remote should be cloned, whatever it is called, and in this
case that additional refspec belongs to a different remote.
Yes, exactly. Mostly I was suggesting that if you do "--origin=foo",
then we do not fetch items named "remote.origin.fetch" (i.e., that the
code correctly uses the origin variable and not the hard-coded name
"origin").
-Peff
From: SZEDER Gábor <hidden> Date: 2017-05-03 14:43:05
Cc'ing Ævar because of his work on 'clone --no-tags'.
On Wed, Mar 15, 2017 at 6:08 PM, Jeff King [off-list ref] wrote:
On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote:
quoted
quoted
Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.
Yeah, I did notice that you love argv_array :) I had to raise an
eyebrow recently while looking at send-pack and how it uses argv_array
to read refspecs from stdin into an array. I think argv_array feels a
bit out of place in both cases. Yes, it does exactly what's needed.
However, it's called *argv*_array, indicating that its contents is
destined to become the options of some command. But that's not the
case in these two cases, we are not dealing with arguments to a
command, these are just arrays of strings.
In my mind, "argv" is synonymous with "NULL-terminated array of
strings". If the name is the only thing keeping it from wider use, I'd
much prefer us to give it a more generic name. All I really care about
is simplifying memory management. :)
Whether its name is the _only_ thing keeping it from wider use, I
don't know :)
All I can say is that I was aware of argv_array, but because of
its name it didn't even occur to me. And even if I had considered it,
I still wouldn't have used it here. Had it been called string_array,
I think I would have used it.
On a related note, we have string_list, which is not a list but an
ALLOC_GROW()-able array, and not that of strings (i.e. plan char*),
but of structs with a string and an additional data field.
Oh, well :)
quoted
quoted
I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.
Though get_remote() does give us this config, at this point the
default fetch refspec has not been configured yet, therefore it's not
included in the resulting remote->fetch array. The default refspec is
set in write_refspec_config(), but that's called only about two
screenfulls later. So there is a bit of extra work to do in order to
leverage get_remote()'s parsing.
I think the simplest way is to keep parsing the default fetch refspec
manually, and then append it to the remote->fetch array. Definitely
shorter and simpler than that parsing in the current patch.
Alternatively, we could set the default fetch refspec in the
configuration temporarily, only for the duration of the get_remote()
call, but it feels a bit too hackish...
Yeah, I think manually combining the two here is fine. Though I won't
complain if you want to look into setting the config earlier. If the
refactoring isn't too bad, it would probably provide the nicest outcome.
I did actually look into that, but don't think it's a good idea.
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.
Unfortunately, there is more to this issue. Earlier I though that the
fetch refspec is the only config that is ignored during a clone.
However, Ævar's 'clone --no-tags' patches[1] drew my attention to the
'remote.<name>.tagOpt' config variable, that I overlooked earlier...
and apparently 'git clone' overlooks it as well, grabbing all tags
even when it's set to '--no-tags'.
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.
I'm not sure what the right way forward would be.
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?
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).
Or we should libify more of 'builtin/fetch.c', but with all those
static variables and functions in there... Ugh :)
Gábor
From: Jeff King <hidden> Date: 2017-05-03 20:22:37
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
From: Sebastian Schuberth <hidden> Date: 2017-05-04 06:57:59
On 5/3/2017 22:22, Jeff King wrote:
quoted
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).
I agree. Let's fix one thing at a time, and not make this a overarching "account for all previously ignored config variables during clone" fix. Esp. as specifying the refspec is explicitly documented to work durig clone [1] I think we should get this in ASAP.
Thanks for your work so far!
[1] https://git-scm.com/docs/git-clone#git-clone---configltkeygtltvaluegt
Regards,
Sebastian
On Wed, May 3, 2017 at 4:42 PM, SZEDER Gábor [off-list ref] wrote:
Cc'ing Ævar because of his work on 'clone --no-tags'.
On Wed, Mar 15, 2017 at 6:08 PM, Jeff King [off-list ref] wrote:
quoted
On Sat, Mar 11, 2017 at 01:41:34AM +0100, SZEDER Gábor wrote:
quoted
quoted
Though if I'm bikeshedding, I'd probably have written the whole thing
with an argv_array and avoided counting at all.
Yeah, I did notice that you love argv_array :) I had to raise an
eyebrow recently while looking at send-pack and how it uses argv_array
to read refspecs from stdin into an array. I think argv_array feels a
bit out of place in both cases. Yes, it does exactly what's needed.
However, it's called *argv*_array, indicating that its contents is
destined to become the options of some command. But that's not the
case in these two cases, we are not dealing with arguments to a
command, these are just arrays of strings.
In my mind, "argv" is synonymous with "NULL-terminated array of
strings". If the name is the only thing keeping it from wider use, I'd
much prefer us to give it a more generic name. All I really care about
is simplifying memory management. :)
Whether its name is the _only_ thing keeping it from wider use, I
don't know :)
All I can say is that I was aware of argv_array, but because of
its name it didn't even occur to me. And even if I had considered it,
I still wouldn't have used it here. Had it been called string_array,
I think I would have used it.
On a related note, we have string_list, which is not a list but an
ALLOC_GROW()-able array, and not that of strings (i.e. plan char*),
but of structs with a string and an additional data field.
Oh, well :)
quoted
quoted
quoted
I do also notice that right _after_ this parsing, we use remote_get(),
which is supposed to give us this config anyway. Which makes me wonder
if we could just reorder this to put remote_get() first, and then read
the resulting refspecs from remote->fetch.
Though get_remote() does give us this config, at this point the
default fetch refspec has not been configured yet, therefore it's not
included in the resulting remote->fetch array. The default refspec is
set in write_refspec_config(), but that's called only about two
screenfulls later. So there is a bit of extra work to do in order to
leverage get_remote()'s parsing.
I think the simplest way is to keep parsing the default fetch refspec
manually, and then append it to the remote->fetch array. Definitely
shorter and simpler than that parsing in the current patch.
Alternatively, we could set the default fetch refspec in the
configuration temporarily, only for the duration of the get_remote()
call, but it feels a bit too hackish...
Yeah, I think manually combining the two here is fine. Though I won't
complain if you want to look into setting the config earlier. If the
refactoring isn't too bad, it would probably provide the nicest outcome.
I did actually look into that, but don't think it's a good idea.
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.
Unfortunately, there is more to this issue. Earlier I though that the
fetch refspec is the only config that is ignored during a clone.
However, Ævar's 'clone --no-tags' patches[1] drew my attention to the
'remote.<name>.tagOpt' config variable, that I overlooked earlier...
and apparently 'git clone' overlooks it as well, grabbing all tags
even when it's set to '--no-tags'.
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.
I'm not sure what the right way forward would be.
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?
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).
Or we should libify more of 'builtin/fetch.c', but with all those
static variables and functions in there... Ugh :)
Yes from my (limited) understanding of the code after hacking in
--no-tags this all seems correct. I.e. that a large part of my patch
wouldn't be needed if we were able to just set tagOpt earlier & then
call fetch.
But as you point out there's a big chicken & egg problem with the
likes of --single-branch where we'd either need to run upload-pack on
the remote side twice, once to get the branch and once to fetch (ew!).
The way to get around that that I can see would be to have some deep
hooking into the fetch machinery where first we set our config like
tagOpt=--no-tags for --no-tags, then we call `fetch`, but `fetch`
would need to have some hook where in the case of --single-branch it
would immediately write the branch name to the fetch spec in the
config, then do the actual fetching.