[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch

This is a reroll of sg/clone-refspec-from-command-line-config. Sorry for the delay, family visit.

The first patch is the updated version of what is now the first commit of that topic. The changes are those mentioned in [1]:

 - updated commit message,
 - renamed 'refspec_count' to 'refspec_nr',
 - use the parsed fetch refspecs returned by remote.c:remote_get()
   instead of parsing them ourselves (look at the third hunk of the
   diff of builtin/clone.c, how much shorter it looks),
 - modified tests to check that refs matching the default refspecs are
   transferred as well, and
 - added a test for the combination of '-c
   remote.<remote>.fetch=<refspec> --origin=<name>'.

The second patch is a doc update to warn users that not all configuration variables are supported via 'git clone -c ...' at the moment.

Patches 3 and 4 are the last two patches from Peff from this morning [2]. I picked those up, because his last patch required a bit of variable name adjustments. I didn't pick up his first patch, because using remote_get() already factors out refspec parsing.

[1] - http://public-inbox.org/git/xmqq4lwu7r0s.fsf@gitster.mtv.corp.google.com/T/#u [2] - http://public-inbox.org/git/20170515074617.wsdzogshc4ilnlsb@sigill.intra.peff.net/T/#m021eadff5d1e4351d99a27096090be39f53df961

Jeff King (2):
  remote: drop free_refspecs() function
  clone: use free_refspec() to free refspec list

SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  4 ++++
 builtin/clone.c             | 22 ++++++++++++++++------
 remote.c                    | 28 ++++++----------------------
 t/t5611-clone-config.sh     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 70 insertions(+), 28 deletions(-)

Due to limitations/bugs in the current implementation, some configuration variables specified via 'git clone -c var=val' (or 'git -c var=val clone') are ignored during the initial fetch and checkout.

Let the users know which configuration variables are known to be ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the documentation of 'git clone -c'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

A few notes to this patch, because I didn't like that "known to not take effect" expression, and looked into how some configuration variables are handled during the initial fetch and checkout. Far from comprehensive, but here they are anyway:

Concerning the initial fetch:

 - Configuration variables influencing the refspecs to be fetched are
   currently ignored.  These are the fetch refspecs, of course, and
   remote.<name>.{mirror,tagOpt}.
   This series makes fetch refspecs work.  The other two are mentioned
   in this patch, and are less urgent, because their functionality is
   or will soon be available through command line options (--mirror
   and --no-tags).
 - remote.<name>.url is strange.  It's not just ignored, it's ignored
   so much that it isn't even written to the config file when
   specified as 'git clone -c ...'.  Nonetheless, specifying it for
   'git clone' doesn't make much sense in the first place, does it?
   So I think it's actually good that it's ignored and it isn't worth
   mentioning it in the docs.
 - Some fetch-related config variables, e.g.
   remote.<name>.{prune,skipDefaultUpdate,skipFetchAll} or
   fetch.{prune,output}, don't matter during the initial fetch.
 - Other fetch-related config variables, e.g.
   fetch.{fsckObjects,unpackLimit}, are handled deep down in
   fetch-pack and work properly.
 - Transport-specific config variables, e.g. url.<base>.insteadOf,
   remote.<name>.{proxy,uploadpack,vcs}, or http.*, if applicable, are
   handled in the transport layer or remote helper.
 - I'm not sure about submodule-related config variables, but there
   are command line options for that.

I'm not sure about the initial checkout, in particular I'm not sure how many configuration variables there are that could/should influence the initial checkout (or any checkout, for that matter).

 - core.autocrlf works properly, we even have a test for that.
 - filter.<name>.smudge is read during initial checkout, but I'm not
   sure whether that should do anything, since no attributes files
   exist at that point.
 - I glanced through builtin/checkout.c to see whether it looks at any
   configuration variables that clone doesn't, and while it does so,
   it seems to only look at variables that are either irrelevant
   during the initial checkout, e.g. 'diff.ignoresubmodules' and
   'merge.conflictstyle', or are submodule-specific, and about those I
   have no idea.

Like I said, far from comprehensive, but I think at least the fetch part is well covered.

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables. This contradicts to 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. This implies that any fetch refspecs specified this way should already be taken into account during the initial fetch.

The reason is that the initial fetch is not a fully fledged 'git fetch' but a bunch of direct calls into the fetch/transport machinery, bypassing parts of 'git fetch' that processes configured fetch refspecs. The configured refspecs are, however, 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 configured fetch refspecs into account to retrieve all matching refs during the initial fetch. Note that the configuration at that point only includes the fetch refspecs specified by the user, but it doesn't include the default fetch refspec, so we have to append it manually at the end of the parsed refspecs array.

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 the remote name via '--origin=<name>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 20 +++++++++++++++-----
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 59 insertions(+), 5 deletions(-)

On Mon, May 15, 2017 at 01:05:54PM +0200, SZEDER Gábor wrote:

Minor gramm-o: s/to the/the/

Here we append to remote->fetch. We are assuming then that remote->fetch_refspec has already been parsed into remote->fetch. Which I think it always is by remote_get(), but given that it lazy-parses in some cases, it feels a little dangerous.

I also notice that you don't touch remote->fetch_refspec_nr, nor fetch_refspec_alloc. So the remote struct doesn't actually know about this entry. It would probably be wrong if you _did_ update them, because remote->fetch_refspec (the list of refspec strings) would not have a matching entry, and would potentially access uninitialized memory.

I think the whole thing would be a lot less messy if "struct remote" let you add a new refspec (as a string) after the initial parse, and it would handle the details. Just making the existing add_fetch_refspec() public isn't quite enough, because you'd need to invalidate and re-parse the matching "fetch" array, too. Something like:

On Tue, May 16, 2017 at 1:07 AM, Jeff King <peff@peff.net> wrote:

Right.

I'm not sure about lazy parsing. remote_get() returns a fully-parsed, cached struct remote instance without re-reading the configuration, so all fields directly corresponding to configuration variables stay the same. However, it does parse fetch and push refspecs on every invocation. So if it were to be called to return the origin remote more than once during cloning, then the default refspec would get lost on subsequent invocations. Is this what you meant with dangerous?

(Sidenote: and it would leak some memory, too, because it re-parses the refspecs without free()ing the results of the previous invocation.)

Your proposed function to add a refspec as a string would eliminate this danger.

It certainly looks better, see the patch below the scissors for reference, and I thought it works because until last night I only run the corresponding test script (t5611-clone-config), though I know very well that "Thou shalt always run the full test suite!" :)

Unfortunately, putting the default refspec into this temporary configuration environment breaks a few submodule tests (t5614-clone-submodules or t5614-clone-submodules-shallow (it's got renamed between this topic and master), t7407-submodule-foreach, t7410-submodule-checkout-to), because it "leaks" to the submodule environment.

 -- >8 --

Subject: [PATCH] clone: respect additional configured fetch refspecs during
 initial fetch

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables. 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 configured fetch refspecs into account to retrieve all matching refs during the initial fetch. Note that the configuration at that point only includes the fetch refspecs specified by the user, but it doesn't include the default fetch refspec. To keep the code simple and parsing and memory management of the refspecs in one place, add the default fetch refspec to the temporary configuration environment, so remote_get() can parse it along with all other refspecs that might have been specified on the command line.

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>'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 32 ++++++++++++++++----------------
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 60 insertions(+), 16 deletions(-)

On Fri, May 26, 2017 at 12:04:03PM +0200, SZEDER Gábor wrote:

More or less. I actually didn't look far enough to see under what circumstances we might re-parse (or might not have parsed when we add our extra refspec), but that's definitely the sort of thing I was worried about.

Yes, I'd argue that the current code is buggy, since:

  x = remote_get("foo");
  y = remote_get("foo");

is a guaranteed leak. It seems like remote_get_1() should protect the call to parse_fetch_refspec() by checking whether ret->fetch is NULL (and ditto for ret->push).

Yeah, I think it's not that hard to support. I'd just rather have the logic inside remote.c, rather than infecting the caller with the complexity.

Doh, of course. I didn't think of that. That's probably a bad direction, then, as there's no "just for this process" global config.

-Peff

Jeff King <peff@peff.net> writes:

Yuck, right. So... do we have a further plan for this topic, or are the patches more or less good as they are?

On Tue, May 30, 2017 at 12:53:47PM +0900, Junio C Hamano wrote:

I was hoping we'd see one more iteration with the abstracted function for adding to the "struct remote". Gábor, let me know if you're too sick of the topic, and I can try to re-spin it.

-Peff

On Tue, May 30, 2017 at 5:53 AM, Junio C Hamano <gitster@pobox.com> wrote:

Yup, here it comes.

The main change since v3 is that clone's default refspec is now added to the 'struct remote' using a function based on Peff's recommendation, with slight changes:

 - I call that function add_and_parse_fetch_refspec(), because, well,
   that's what it does, and
 - I dropped the if (remote->fetch) condition, because the function
   should work when there are no refspecs in the configuration, too.

I also dropped Peff's two patches that were included in v3, because:

 - his last patch doesn't apply anymore, because the variable it frees
   properly doesn't exist anymore, and
 - with that patch gone his second patch is not tangled up with this
   topic, so I think it should go on its own branch.

Interdiff against the first two patch of v3 below.

SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  4 ++++
 builtin/clone.c             | 34 +++++++++++++++++-----------------
 remote.c                    | 13 +++++++++++++
 remote.h                    |  1 +
 t/t5611-clone-config.sh     | 44 ++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 79 insertions(+), 17 deletions(-)

-- 2.13.0.35.g14b6294b1

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables. 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 configured fetch refspecs into account to retrieve all matching refs during the initial fetch. Note that the configuration at that point only includes the fetch refspecs specified by the user, but it doesn't include the default fetch refspec. Add a function to the remote API which encapsulates adding a fetch refspec to a 'struct remote' and parsing it, including all the necessary memory management housekeeping as well. Use this new function to add clone's default fetch refspec to the other refspecs the user might have specified.

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>'.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 34 +++++++++++++++++-----------------
 remote.c                | 13 +++++++++++++
 remote.h                |  1 +
 t/t5611-clone-config.sh | 44 ++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 75 insertions(+), 17 deletions(-)

Due to limitations/bugs in the current implementation, some configuration variables specified via 'git clone -c var=val' (or 'git -c var=val clone') are ignored during the initial fetch and checkout.

Let the users know which configuration variables are known to be ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the documentation of 'git clone -c'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 4 ++++
 1 file changed, 4 insertions(+)

On Tue, May 30, 2017 at 9:12 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

We should add to that: "Instead supply the --mirror and --no-tags options, respectively".

On Tue, May 30, 2017 at 11:01 AM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I don't think we can do that in this patch right now. This topic branches off from c3808ca69 (preparing for 2.10.3, 2016-12-05), I assume because, as a bugfix, it will be included in maintenance releases for older releases, and those won't have the '--no-tags' option. Perhaps as a follow-up, only on master.

On Wed, May 31, 2017 at 10:50 AM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Sorry, my misunderstanding. I didn't think we'd be updating docs like this in maint releases, thought this was for to-be-2.14.

Hi,

SZEDER Gábor wrote:

Makes sense.

Tiny nit: the paragraph of --config description is already a bit overwhelming, and I think this additional note takes it over the edge where I give up and stop reading. Could it go in a separate paragraph?

		the config file. This makes it safe, for example, to add
		additional fetch refspecs to the origin remote.
	+
	Due to limitations in the current implementation, some
	configuration variables do not take effect until after the
	initial fetch and checkout. Configuration variables known
	not to take effect are `remote.<name>.mirror` and
	`remote.<name>.tagOpt`.

Thanks, Jonathan

On Tue, May 30, 2017 at 09:12:43AM +0200, SZEDER Gábor wrote:

What happens here if remote->fetch isn't already initialized? I think we'd end up with a bunch of garbage values. That's what I was trying to protect against in my original suggestion.

I'm not sure if that's possible or not. We seem to initialize it in both remote_get() and for_each_remote(), and I don't think there are any other ways to get a remote. (In fact, I kind of wondered why we do this parsing lazily at all, but I think it's so that misconfigured remotes don't cause us to die() if we aren't accessing them at all).

If we assume that the contract that remote.c provides is that the fetch fields are always filled in before a "struct remote" is returned to a caller, and that only such callers would use this function, it's OK. It just seems more dangerous than it needs to be.

-Peff

On Wed, May 31, 2017 at 6:23 AM, Jeff King <peff@peff.net> wrote:

The only place creating remotes is remote.c:make_remote(), which calloc()s the required memory, making all of struct remote's fields zero-initialized. In case of clone the common case is that the user doesn't specify any additional fetch refspecs, so remote->fetch will still be NULL after full initialization and when add_and_parse_fetch_refspec() is called with the default fetch refspec, meaning we can't 'if (remote->fetch) { parse ... }'. OTOH, all functions involved can cope with the fetch-refspec-related fields being 0/NULL, and at the time remote->fetch_refspec_nr-1 is used for array indexing it's not 0 anymore.

On Wed, May 31, 2017 at 11:34:23AM +0200, SZEDER Gábor wrote:

Yeah, I agree it is safe now. I'm just worried about some function in remote.c later doing:

   read_config();
   add_and_parse_fetch_refspec(remotes[0], whatever);

which leaves the struct in an inconsistent state (we realloc NULL which allocates from scratch, and all of the other entries in remote->fetch end up uninitialized). Can we at least add an assertion like:

  if (!remote->fetch)
	BUG("cannot add refspec to an unparsed remote");

?

-Peff

On Mon, Jun 5, 2017 at 10:18 AM, Jeff King <peff@peff.net> wrote:

But as mentioned before, remote->fetch being NULL is not a bug in itself, it's a perfectly valid value even in a fully parsed remote when the remote has no fetch refspecs. Therefore, I think, the condition should instead be:

  remote->fetch_refspec_nr && !remote->fetch

We could even try to be extra helpful by checking this condition and calling parse_fetch_refspec() to initialize remote->fetch instead of BUG()ing out. However, that would mask the real issue, namely not using remote_get() to get the remote, so I don't actually think that's a good thing to do.

OTOH, having remote->fetch so closely related to, yet separate from remote->fetch_refspec{,_nr,_alloc} will always inherently be error prone. This assertion would catch one case where a less than careful dev could cause trouble, sure, but there will be still others left, e.g. he could still do:

  add_fetch_refspec(remote, ...);    // this doesn't update remote->fetch
  for (i = 0; i < remote->fetch_refspec_nr; i++)
        func(remote->fetch[i]);

and watch the array indexing blow up in the last iteration.

Or a non-hypothetical one: when I first tried to use remote_get() for an earlier version of this patch, I ALLOC_GROW()-ed remote->fetch to create room for the default refspec, because in struct remote not **fetch_refspec but *fetch is listed right above fetch_refspec_{nr,alloc}, being way past my bedtime may be my only excuse... It didn't work :) [1]

To put your worries to rest we should eliminate remote->fetch_refspec altogether and parse refspecs into remote->fetch right away, I'd think. After all, that's what's used in most places anyway, and it can be easily turned back to a single string where needed (I think in only 3 places in builtin/remote.c).

[1] - Though in the end this could be considered beneficial, because
      commits 53c5de29 (pickaxe: fix segfault with '-S<...>
      --pickaxe-regex', 2017-03-18), 59210dd56 (tests: make the
      'test_pause' helper work in non-verbose mode, 2017-03-18), and
      4ecae3c8c (tests: create an interactive gdb session with the
      'debug' helper, 2017-03-18) were all fallouts from the ensuing
      debugging session :)

On Tue, Jun 06, 2017 at 08:19:09PM +0200, SZEDER Gábor wrote:

Right, that would be a better check.

OK.

I don't think we can parse right away without regressing the error handling. If I have two remotes, one with a bogus refspec, like:

  [remote "one"]
  url = ...
  fetch = refs/heads/*:refs/remotes/one/*
  [remote "two"]
  url = ...
  fetch = ***bogus***

and I do:

  git fetch one

then read_config() will grab the data for _both_ of them, but only call remote_get() on the first one. If we parsed the refspecs during read_config(), we'd parse the bogus remote.two.fetch and die().

I guess that's a minor case, but as far as I can tell that's the motivation for the lazy parsing.

-Peff

On Tue, Jun 6, 2017 at 8:37 PM, Jeff King <peff@peff.net> wrote:

Yeah, I know, we'd need a parse_refspec_gently() or something. parse_refspec_internal() already has a 'verify' parameter which prevents it from die()ing while parsing a bogus refspec, but in its current form it doesn't tell us which refspec was bogus, so we'd need a bit more than that to let the user know what's wrong.

Hi,

SZEDER Gábor wrote:

[...]

Agh, subtle.

I'm hoping that longer term we can make fetch behave more like a library and make the initial fetch into a fully fledged 'git fetch' like thing again. But this smaller change is the logical fix in the meantime.

[...]

I realize its neighbors don't have this, but can this function have a brief comment explaining how it is meant to be used and what guarantees it makes?

For example:

	/** Adds a refspec to remote->fetch_refspec and remote->fetch. */
	void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);

I'm tempted to say that this one should be named add_fetch_refspec (or something like remote_add_refspec) --- this is the only way to add a fetch refspec in the public remote API, and the fact that it parses is an implementation detail. The private add_fetch_refpsec that builds the fetch_refspec as preparation for parsing them in a batch is not part of the exported API.

Also, now that the API is appending to remote->fetch instead of allocating it in one go, should it use the ALLOC_GROW heuristic / fetch_refspec_alloc size?

The caller adds one refspec right after calling remote_get. I'm starting to wonder if this could be done more simply by having a variant of remote_get that allows naming an additional refspec, so that remote->fetch could be immutable after construction like it was before. What do you think?

[...]

Allocating an array to put the parsed refspec in and then freeing it seems wasteful. Should parse_refspec_internal be changed to take an output parameter so it can put the refspec into remote->fetch directly?

[...]

[...]

nit: since it's not part of a key, value pair like value, default_refspec should probably go on its own line.

[...]

Can use <<-\EOF to save the reviewer from having to look for variable interpolations.

optional nit: might be easier to read with a blank line before the "cat >expect" line or the for-each-ref line. That way, it's easier to separate the validation of output from the commands being run at a glance and see what the test is about.

Likewise.

Likewise.  Nice.

The rest looks good.  Thanks for a pleasant read.

Sincerely, Jonathan

On Tue, Jun 13, 2017 at 05:48:16PM -0700, Jonathan Nieder wrote:

We talked about this earlier in the thread, but I doubt that will ever happen. Things like --single-branch means that clone has to actually look at what the remote has before it decides what to fetch.

Of course we _could_ teach all that logic to fetch, too, but I don't think you'll ever get the nice simple:

  1. configure refspecs
  2. fetch
  3. checkout

So the best thing is probably to push any repeated logic down into the transport layer, where it can be easily used by both commands.

It's not quite trivial to make that change. parse_refspec_internal() actually handles an array of refspecs. So its callers would all have to start allocating the correctly-sized array.

I doubt that one malloc per clone is worth caring about. I'd worry more about the trickiness that merited the comment above, but it's at least contained in this one function.

-Peff

On Wed, Jun 14, 2017 at 2:48 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

I kind of agree, but ...

First, there is an add_push_refspec() function as well, which, just like its fetch counterpart, doesn't parse the given refspec, only appends it to remote->push_refspec. Changing add_fetch_refspec() to parse, too, would break this symmetry.

Furthermore, at the moment we have both remote->fetch_refspec (for strings) and remote->fetch (for parsed refspecs), and parsing a refspec die()s if it's bogus, therefore I think that parsing is not an implementation detail that should be hidden.

That's such a very specific and narrow use case that I don't think it justifies a dedicated function. I don't think remote->fetch should be immutable; I think remote->{fetch,push}_refspec and the lazy parsing of refspecs should go away. Cleaning up this corner of the remote API is beyond the scope of this patch series.

No, I found that extracting the huge body of its loop into a helper function that fills an output parameter is much more useful.

Fun fact: they were never part of a key-value pair. While 'key' is indeed the name of a configuration variable, 'value' is not the value of that configuration variable, or any other configuration variable for that matter.

Best, Gábor

Only small, straightforward adjustments, mostly based on Jonathan's comments.

The one exception is passing the default refspec using strbuf_detach(), because add_fetch_refspec() doesn't make a copy of it internally. It doesn't make any difference in practice, because strbuf default_refspec remains unaltered til the very end, but this is the right thing to do.

SZEDER Gábor (2):
  clone: respect additional configured fetch refspecs during initial
    fetch
  Documentation/clone: document ignored configuration variables

 Documentation/git-clone.txt |  5 +++++
 builtin/clone.c             | 36 ++++++++++++++++++----------------
 remote.c                    | 13 +++++++++++++
 remote.h                    |  1 +
 t/t5611-clone-config.sh     | 47 +++++++++++++++++++++++++++++++++++++++++++++
 5 files changed, 85 insertions(+), 17 deletions(-)

Due to limitations/bugs in the current implementation, some configuration variables specified via 'git clone -c var=val' (or 'git -c var=val clone') are ignored during the initial fetch and checkout.

Let the users know which configuration variables are known to be ignored ('remote.origin.mirror' and 'remote.origin.tagOpt') under the documentation of 'git clone -c'.

Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 Documentation/git-clone.txt | 5 +++++
 1 file changed, 5 insertions(+)

On Fri, Jun 16 2017, SZEDER Gábor jotted:

Just so we're all on the same page, in CACBZZX740rcQKnfkRXgn0+fmeUDaWL-Kz5WzKeyUvBhXWPwPhg@mail.gmail.com I had the feedback that this patch didn't make sense on top of 2.13.0 since we have --no-tags now which should be documented here, but you replied in CAM0VKjmz7MpVt3oPBuwHiXNoLkZmdmrZ66ggk+aY5-4oVkE35A@mail.gmail.com in an answer I understood to mean that this was a patch not meant for master, but for the main track.

But this is now cooking in pu, Junio: is it clear that this patchu as-cooking ideally shouldn't land in next/master without the fix on top which I mentioned in my mail above? I can just submit that as a patch on top, but I'm confused about the current state with this cooking in pu, so I thought I'd ask first how this should be handled.

Ævar Arnfjörð Bjarmason wrote:

[...]

I think it's simplest to write a patch on top that discusses --no-tags. That way, Junio (and anyone else applying the patch) has the flexibility to apply or cherry-pick this change to old branches without the --no-tags discussion and newer branches with it.

Would you like to write it (or suggest wording), or would you prefer if someone else does?

Thanks, Jonathan

On Fri, Jun 16 2017, Jonathan Nieder jotted:

I can do that no problem.

I just first wanted to clarify what the status of this was, from SZEDER's comments in the referenced E-Mails I had the impression that this was only meant for an old maintenance release:

    SZEDER: "I assume because, as a bugfix, it will be included in
    maintenance releases for older releases, and those won't have the
    '--no-tags' option."

But thinking about it I don't see why we'd be doing such minor doc changes in an old maintenance release. I haven't read the whole backlog of this topic though, so maybe I missed something.

I initially suggested just adding "Instead supply the --mirror and --no-tags options, respectively" to the patch quoted above.

But actually, thinking about this again now, and being recently familiar with this code after having implemented  on --no-tags, I think this whole wording is just misleading. It makes it sound as though git clone is init + fetch, and that the initial fetch just ignores these two specific options because of some quirk of the implementation.

In reality clone doesn't use fetch at all, they just share some of the underlying fetch machinery.

I think something like this as a replacement is better, assuming this really needs to be applied to pre-2.13.0:

    diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
    index 35cc34b2fb..2169e5c97f 100644
    --- a/Documentation/git-clone.txt
    +++ b/Documentation/git-clone.txt
    @@ -189,6 +189,14 @@ objects from the source repository into a pack in the cloned repository.
            values are given for the same key, each value will be written to
            the config file. This makes it safe, for example, to add
            additional fetch refspecs to the origin remote.
    ++
    +The underlying implementation of `git clone` isn't equivalent to `git
    +init` followed by a `git fetch`, but the two share some of the
    +underlying fetch machinery. Because of this, setting configuration
    +variables which would impact `git fetch` doesn't have any effect on
    +`git clone` at all. For example, setting `remote.<name>.mirror` and
    +`remote.<name>.tagOpt` will do to change how the initial fetch is
    +carried out.

     --depth <depth>::
            Create a 'shallow' clone with a history truncated to the

Or, in case this just needs to be applied on top of master we can mention --no-tags:

    diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
    index 83c8e9b394..52a371176e 100644
    --- a/Documentation/git-clone.txt
    +++ b/Documentation/git-clone.txt
    @@ -189,6 +189,16 @@ objects from the source repository into a pack in the cloned repository.
            values are given for the same key, each value will be written to
            the config file. This makes it safe, for example, to add
            additional fetch refspecs to the origin remote.
    ++
    +The underlying implementation of `git clone` isn't equivalent to `git
    +init` followed by a `git fetch`, but the two share some of the
    +underlying fetch machinery. Because of this, setting configuration
    +variables which would impact `git fetch` doesn't have any effect on
    +`git clone` at all.
    ++
    +For example, setting `remote.<name>.mirror` and `remote.<name>.tagOpt`
    +will do to change how the initial fetch is carried out. Instead supply
    +the --mirror and --no-tags options, respectively.

     --depth <depth>::
            Create a 'shallow' clone with a history truncated to the

On Fri, Jun 16, 2017 at 11:10 PM, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:

I think this is unnecessarily technical and detailed to be included under the docs of the '--config' option.

This is not true, several such configuration variables work even during cloning, see:

  http://public-inbox.org/git/CAM0VKjkSMnemwPbFihXiQui3wm_wYmQeQmgYrFs5bfsH1jMg1A@mail.gmail.com/

SZEDER Gábor wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

SZEDER Gábor <szeder.dev@gmail.com> writes:

s/if/of/ (will squash in while queuing).

The initial fetch during a clone doesn't transfer refs matching additional fetch refspecs given on the command line as configuration variables. 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 configured fetch refspecs into account to retrieve all matching refs during the initial fetch. Note that the configuration at that point only includes the fetch refspecs specified by the user, but it doesn't include the default fetch refspec. Add a function to the remote API which encapsulates adding a fetch refspec to a 'struct remote' and parsing it, including all the necessary memory management housekeeping as well. Use this new function to add clone's default fetch refspec to the other refspecs the user might have specified.

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>'.

Helped-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c         | 36 +++++++++++++++++++-----------------
 remote.c                | 13 +++++++++++++
 remote.h                |  1 +
 t/t5611-clone-config.sh | 47 +++++++++++++++++++++++++++++++++++++++++++++++
 4 files changed, 80 insertions(+), 17 deletions(-)

SZEDER Gábor wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

On Fri, Jun 16, 2017 at 07:38:48PM +0200, SZEDER Gábor wrote:

I'd still prefer this to have:

  if (!remote->fetch && remote->fetch_refspec_nr)
	BUG("attempt to add refspec to uninitialized list");

at the top, as otherwise this case writes garbage into remote->fetch[0].

I see you have another series dealing with the lazy parsing, but I haven't looked at it yet (hopefully this danger would just go away after that).

Other than that, the patch looks fine to me.

-Peff

Jeff King <peff@peff.net> writes:

SZEDER? As long as the end result together with two series are safe, I do not have a strong preference, but given that the other one is a lot more invasive change [*1*], I think it is nicer to have this two-patch series already safe without the other one.

What's your take on Peff's point?

[Footnote]

*1* Especially the other branch does not merge cleanly into 'pu' and
    I haven't managed to include it in my tree yet.

Junio C Hamano <gitster@pobox.com> writes:

After infinite amount of time has passed, I no longer even remember what "the other" topic was X-<. I'll tentatively eject these two patches from my tree in preparation for starting the cycle after Git 2.14 early next week, but hopefully you can find time and energy to resurrect it.

Or somebody else may be interested to take the topic over--which is also fine by me, too.

Thanks.

On Tue, May 30, 2017 at 09:11:28AM +0200, SZEDER Gábor wrote:

Makes sense on both counts. Here's that stand-alone patch for reference, which can go onto its own topic.

-- >8 --
Subject: [PATCH] remote: drop free_refspecs() function

We already have free_refspec(), a public function which does the same thing as the static free_refspecs(). Let's just keep one. There are two minor differences between the functions:

  1. free_refspecs() is a noop when the refspec argument is
     NULL. This probably doesn't matter in practice.  The
     nr_refspec parameter would presumably be 0 in that
     case, skipping the loop. And free(NULL) is explicitly
     OK. But it doesn't hurt for us to port this extra
     safety to free_refspec(), as one of the callers passes
     a funny "i+1" count.

  2. The order of arguments is reversed between the two
     functions. This patch uses the already-public order of
     free_refspec(), as it matches the argument order on the
     parsing side.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

Jeff King wrote:

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.

From: Jeff King <peff@peff.net>

We already have free_refspec(), a public function which does the same thing as the static free_refspecs(). Let's just keep one. There are two minor differences between the functions:

  1. free_refspecs() is a noop when the refspec argument is
     NULL. This probably doesn't matter in practice.  The
     nr_refspec parameter would presumably be 0 in that
     case, skipping the loop. And free(NULL) is explicitly
     OK. But it doesn't hurt for us to port this extra
     safety to free_refspec(), as one of the callers passes
     a funny "i+1" count.

  2. The order of arguments is reversed between the two
     functions. This patch uses the already-public order of
     free_refspec(), as it matches the argument order on the
     parsing side.

Signed-off-by: Jeff King <peff@peff.net>
---
 remote.c | 28 ++++++----------------------
 1 file changed, 6 insertions(+), 22 deletions(-)

From: Jeff King <peff@peff.net>

Using free() on a refspec was always leaky, as its string fields also need freed. But it became more so when ad00f128d (clone: respect configured fetch respecs during initial fetch, 2016-03-30) taught clone to create a list of refspecs, each of which need to be freed.

[sg: adjusted the function parameters.]

Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: SZEDER Gábor <szeder.dev@gmail.com>
---
 builtin/clone.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

On Mon, May 15, 2017 at 1:05 PM, SZEDER Gábor <szeder.dev@gmail.com> wrote:

Erm... I should have given a bit more thought to this last patch, shouldn't I?

First, the unchanged commit message is now (i.e. by using the parsed refspecs returned by remote_get()) completely outdated. Second, while it properly frees those refspecs, i.e. the array and all its string fields, it will now leak the memory pointed by the 'refspec' variable. However, why free just that one field of the 'struct *remote'? Alas, we don't seem to have a free_remote() function...

On Mon, May 15, 2017 at 01:29:07PM +0200, SZEDER Gábor wrote:

Right, the "struct remote" fields are meant to last the program lifetime, and just go away when the program does. So they are never freed. We could just lump this in with them, and remove the free() entirely.

It's a little funny with your patch as-is, because "struct remote" doesn't actually know about the refspec we stuck on the end. But since it doesn't free its refspecs anyway, it's not any more leaky than all the other entries.

-Peff

SZEDER Gábor <szeder.dev@gmail.com> writes:

I was sifting entries in the draft "What's cooking" report to find topics to merge to 'next'. I read the series over and as Peff said in his <20170515224615.f6hnnfngwpierqk4@sigill.intra.peff.net>, I think the series overall is good.

Do you want to update the proposed log message for this one before the entire thing is merged to 'next'?

On Tue, May 23, 2017 at 04:38:07PM +0900, Junio C Hamano wrote:

I didn't quite say that it was good. I think the overall direction is good, but I had some comments on patch 1. Maybe we want to accept the ugliness there, but I had some suggestions that might make it less bad.

-Peff

On Tue, May 23, 2017 at 9:38 AM, Junio C Hamano <gitster@pobox.com> wrote:

I followed Peff's advice on adding the default fetch refspec to this temporary configuration environment (what's the proper term for that?), which resulted in the simplest and cleanest code yet, and, as a by product made this last patch moot, so I will drop it in the next version. The code is ready, but the change requires updates to the first patch's commit message, maybe I can get around to it in the evening.

On Mon, May 15, 2017 at 01:05:53PM +0200, SZEDER Gábor wrote:

No problem. Thanks for letting us know before it went to 'next'. ;)

Good.

Yeah, that is much nicer. It does feel a little dirty modifying remote->fetch, though. I'll comment on the specific patch.

Sounds good.

Good idea.

Makes sense. Thanks for including them.

-Peff