git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCHv4 1/2] clone: respect additional configured fetch refspecs during initial fetch

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jun 14, 2017, 00:48 UTC
Message-ID
<20170614004816.GR133952@aiede.mtv.corp.google.com>
In-Reply-To
<20170530071244.32257-1-szeder.dev@gmail.com>
Hi,
SZEDER Gábor wrote:
Show 6 quoted lines
> 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
[...]
> 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.
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.

[...]
Show 35 quoted lines
> diff --git a/remote.c b/remote.c
> index ad6c5424e..b8fd09dc9 100644
> --- a/remote.c
> +++ b/remote.c
> @@ -626,6 +626,19 @@ struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec)
>  	return parse_refspec_internal(nr_refspec, refspec, 1, 0);
>  }
>  
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec)
> +{
> +	struct refspec *rs;
> +
> +	add_fetch_refspec(remote, refspec);
> +	rs = parse_fetch_refspec(1, &refspec);
> +	REALLOC_ARRAY(remote->fetch, remote->fetch_refspec_nr);
> +	remote->fetch[remote->fetch_refspec_nr - 1] = *rs;
> +
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);
> +}
> +
>  static struct refspec *parse_push_refspec(int nr_refspec, const char **refspec)
>  {
>  	return parse_refspec_internal(nr_refspec, refspec, 0, 0);
> diff --git a/remote.h b/remote.h
> index 924881169..9ad8c1085 100644
> --- a/remote.h
> +++ b/remote.h
> @@ -164,6 +164,7 @@ struct ref *ref_remove_duplicates(struct ref *ref_map);
>  
>  int valid_fetch_refspec(const char *refspec);
>  struct refspec *parse_fetch_refspec(int nr_refspec, const char **refspec);
> +void add_and_parse_fetch_refspec(struct remote *remote, const char *refspec);
>  
>  void free_refspec(int nr_refspec, struct refspec *refspec);

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?

[...]
> +	/* Not free_refspecs(), as we copied its pointers above */
> +	free(rs);

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?

[...]
> +++ b/builtin/clone.c
[...]
Show 6 quoted lines
> @@ -848,16 +853,13 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	const struct ref *our_head_points_at;
>  	struct ref *mapped_refs;
>  	const struct ref *ref;
> -	struct strbuf key = STRBUF_INIT, value = STRBUF_INIT;
> +	struct strbuf key = STRBUF_INIT, default_refspec = STRBUF_INIT;

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

[...]
Show 20 quoted lines
> --- a/t/t5611-clone-config.sh
> +++ b/t/t5611-clone-config.sh
> @@ -37,6 +37,50 @@ 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 &&
> +	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
> +'

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.

Show 11 quoted lines
> +
> +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
Likewise.
Show 16 quoted lines
> +	test_cmp expect actual
> +'
> +
> +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 &&
> +	git -C child for-each-ref --format="%(refname)" >actual &&
> +	cat >expect <<-EOF &&
> +	refs/grab/it
> +	refs/heads/master
> +	refs/remotes/upstream/HEAD
> +	refs/remotes/upstream/master
> +	EOF
Likewise.  Nice.
Show 6 quoted lines
> +	test_cmp expect actual
> +'
> +
>  # Tests for the hidden file attribute on windows
>  is_hidden () {
>  	# Use the output of `attrib`, ignore the absolute path
The rest looks good.  Thanks for a pleasant read.

Sincerely, Jonathan

Previous: SZEDER GáborNext: Jeff King
Message 23 of 48 in “[PATCHv3 0/4] clone: respect configured fetch respecs during initial fetch”
  1. SZEDER GáborMay 15, 2017
  2. 2/4 Documentation/clone: document ignored configuration variablesSZEDER Gábor, May 15, 2017
  3. SZEDER GáborMay 26, 2017
  4. 1/4 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, May 15, 2017
  5. Jeff KingMay 15, 2017
  6. SZEDER GáborMay 26, 2017
  7. Jeff KingMay 26, 2017
  8. Junio C HamanoMay 30, 2017
  9. Jeff KingMay 30, 2017
  10. SZEDER GáborMay 30, 2017
  11. 1/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, May 30, 2017
  12. 2/2 Documentation/clone: document ignored configuration variablesSZEDER Gábor, May 30, 2017
  13. Ævar Arnfjörð BjarmasonMay 30, 2017
  14. SZEDER GáborMay 31, 2017
  15. Ævar Arnfjörð BjarmasonMay 31, 2017
  16. Jonathan NiederJun 13, 2017
  17. Jeff KingMay 31, 2017
  18. SZEDER GáborMay 31, 2017
  19. Jeff KingJun 5, 2017
  20. SZEDER GáborJun 6, 2017
  21. Jeff KingJun 6, 2017
  22. SZEDER GáborJun 7, 2017
  23. Jonathan NiederJun 14, 2017
  24. Jeff KingJun 14, 2017
  25. SZEDER GáborJun 16, 2017
  26. 0/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, Jun 16, 2017
  27. 2/2 Documentation/clone: document ignored configuration variablesSZEDER Gábor, Jun 16, 2017
  28. Ævar Arnfjörð BjarmasonJun 16, 2017
  29. Jonathan NiederJun 16, 2017
  30. Ævar Arnfjörð BjarmasonJun 16, 2017
  31. SZEDER GáborJun 16, 2017
  32. Jonathan NiederJun 16, 2017
  33. Junio C HamanoJun 16, 2017
  34. 1/2 clone: respect additional configured fetch refspecs during initial fetchSZEDER Gábor, Jun 16, 2017
  35. Jonathan NiederJun 16, 2017
  36. Jeff KingJun 17, 2017
  37. Junio C HamanoJun 22, 2017
  38. Junio C HamanoAug 12, 2017
  39. remote: drop free_refspecs() functionJeff King, May 31, 2017
  40. Jonathan NiederJun 14, 2017
  41. 3/4 remote: drop free_refspecs() functionSZEDER Gábor, May 15, 2017
  42. 4/4 clone: use free_refspec() to free refspec listSZEDER Gábor, May 15, 2017
  43. SZEDER GáborMay 15, 2017
  44. Jeff KingMay 15, 2017
  45. Junio C HamanoMay 23, 2017
  46. Jeff KingMay 23, 2017
  47. SZEDER GáborMay 23, 2017
  48. Jeff KingMay 15, 2017

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.