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

Re: [PATCH 6/6] fetch-pack: introduce negotiator API

From
Jonathan Nieder <jrnieder@gmail.com>
Date
Jun 6, 2018, 00:37 UTC
Message-ID
<20180606003745.GI9266@aiede.svl.corp.google.com>
In-Reply-To
<c7e504a4664bb6008de970671af13eda7d85dc5b.1527894919.git.jonathantanmy@google.com>
Jonathan Tan wrote:
Show 8 quoted lines
> Introduce the new files fetch-negotiator.{h,c}, which contains an API
> behind which the details of negotiation are abstracted. Currently, only
> one algorithm is available: the existing one.
>
> This patch is written to be more easily reviewed: static functions are
> moved verbatim from fetch-pack.c to negotiator/default.c, and it can be
> seen that the lines replaced by negotiator->X() calls are present in the
> X() functions respectively.
nit: s/more//
Show 14 quoted lines
> Signed-off-by: Jonathan Tan <jonathantanmy@google.com>
> ---
>  Makefile             |   2 +
>  fetch-negotiator.c   |   7 ++
>  fetch-negotiator.h   |  45 ++++++++++
>  fetch-pack.c         | 207 ++++++++++---------------------------------
>  negotiator/default.c | 173 ++++++++++++++++++++++++++++++++++++
>  negotiator/default.h |   8 ++
>  object.h             |   3 +-
>  7 files changed, 282 insertions(+), 163 deletions(-)
>  create mode 100644 fetch-negotiator.c
>  create mode 100644 fetch-negotiator.h
>  create mode 100644 negotiator/default.c
>  create mode 100644 negotiator/default.h
Looks like a job for --color-moved. :)
[...]
> --- /dev/null
> +++ b/fetch-negotiator.c
> @@ -0,0 +1,7 @@
> +#include "fetch-negotiator.h"

To avoid various standards weirdness, the first #include in all files should be git-compat-util.h, cache.h, or builtin.h. I tend to just use git-compat-util.h.

[...]
Show 6 quoted lines
> +++ b/fetch-negotiator.h
> @@ -0,0 +1,45 @@
> +#ifndef FETCH_NEGOTIATOR
> +#define FETCH_NEGOTIATOR
> +
> +#include "cache.h"
What does this need from cache.h?
> +#include "commit.h"

optional: could use a forward-declaration "struct commit;" since we only use pointers instead of the complete type. Do we document when to prefer forward-declaration and when to #include complete type definitions somewhere?

[...]
> +struct fetch_negotiator {

Neat. Can this file include an overview of the calling sequence / how I use this API? E.g.

	/*
	 * Standard calling sequence:
	 * 1. Obtain a struct fetch_negotiator * using [...]
	 * 2. For each [etc]
	 * 3. Free resources associated with the negotiator by calling
	 *    release(this).  This frees the pointer; it cannot be
	 *    reused.
	 */

That would be a good complement to the reference documentation in the struct definition.

[...]
Show 9 quoted lines
> +++ b/object.h
> @@ -28,7 +28,8 @@ struct object_array {
>  /*
>   * object flag allocation:
>   * revision.h:               0---------10                                26
> - * fetch-pack.c:             0----5
> + * fetch-pack.c:             01
> + * negotiator/default.c:       2--5
>   * walker.c:                 0-2
Nice!
[...]
> +++ b/fetch-pack.c
[...]
Show 6 quoted lines
> @@ -462,7 +348,7 @@ static int find_common(struct data *data, struct fetch_pack_args *args,
>  	retval = -1;
>  	if (args->no_dependents)
>  		goto done;
> -	while ((oid = get_rev(data))) {
> +	while ((oid = negotiator->next(negotiator))) {
[etc]
I like it. :)
[...]
Show 7 quoted lines
> @@ -988,7 +870,8 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	struct object_id oid;
>  	const char *agent_feature;
>  	int agent_len;
> -	struct data data = { { compare_commits_by_commit_date } };
> +	struct fetch_negotiator negotiator;
> +	fetch_negotiator_init(&negotiator);
Sane.
[...]
Show 20 quoted lines
> @@ -1061,19 +944,17 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
>  	if (!server_supports("deepen-relative") && args->deepen_relative)
>  		die(_("Server does not support --deepen"));
>  
> -	if (marked)
> -		for_each_ref(clear_marks, NULL);
> -	marked = 1;
> -	if (everything_local(&data, args, &ref, sought, nr_sought)) {
> +	if (everything_local(&negotiator, args, &ref, sought, nr_sought)) {
>  		packet_flush(fd[1]);
>  		goto all_done;
>  	}
> -	if (find_common(&data, args, fd, &oid, ref) < 0)
> +	if (find_common(&negotiator, args, fd, &oid, ref) < 0)
>  		if (!args->keep_pack)
>  			/* When cloning, it is not unusual to have
>  			 * no common commit.
>  			 */
>  			warning(_("no common commits"));
> +	negotiator.release(&negotiator);

Should this go after the all_done so that it doesn't get bypassed in the everything_local case?

[...]
Show 7 quoted lines
> @@ -1434,6 +1316,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
>  			continue;
>  		}
>  	}
> +	negotiator.release(&negotiator);
>  
>  	oidset_clear(&common);

nit: could put the negotiator.release(&negotiator) after the blank line, in the same paragraph as the other free calls like oidset_clear(&common).

[...]
> +++ b/negotiator/default.c
> @@ -0,0 +1,173 @@
> +#include "default.h"
First #include should be "git-compat-util.h".
[...]
Show 5 quoted lines
> +/*
> +   This function marks a rev and its ancestors as common.
> +   In some cases, it is desirable to mark only the ancestors (for example
> +   when only the server does not yet know that they are common).
> +*/

Not about this change: comments should have ' *' at the start of each line (could do in a preparatory patch or a followup).

[...]
Show 9 quoted lines
> --- /dev/null
> +++ b/negotiator/default.h
> @@ -0,0 +1,8 @@
> +#ifndef NEGOTIATOR_DEFAULT_H
> +#define NEGOTIATOR_DEFAULT_H
> +
> +#include "fetch-negotiator.h"
> +
> +void default_negotiator_init(struct fetch_negotiator *negotiator);

optional: same question about whether to use a forward declaration as in fetch-negotiator.h.

Except where noted above,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
Thanks for a nice cleanup.
Previous: Jonathan TanNext: Jonathan Tan
Message 19 of 47 in “Refactor fetch negotiation into its own API”
  1. 0/6 Refactor fetch negotiation into its own APIJonathan Tan, Jun 4, 2018
  2. 1/6 fetch-pack: clear marks before everything_local()Jonathan Tan, Jun 4, 2018
  3. Jonathan NiederJun 5, 2018
  4. Jonathan TanJun 6, 2018
  5. 2/6 fetch-pack: truly stop negotiation upon ACK readyJonathan Tan, Jun 4, 2018
  6. Jonathan NiederJun 5, 2018
  7. Jonathan NiederJun 5, 2018
  8. Jonathan TanJun 6, 2018
  9. 3/6 fetch-pack: in protocol v2, enqueue commons firstJonathan Tan, Jun 4, 2018
  10. Jonathan NiederJun 5, 2018
  11. Jonathan TanJun 6, 2018
  12. 4/6 fetch-pack: make negotiation-related vars localJonathan Tan, Jun 4, 2018
  13. Jonathan NiederJun 5, 2018
  14. Jonathan TanJun 6, 2018
  15. 5/6 fetch-pack: move common check and marking togetherJonathan Tan, Jun 4, 2018
  16. Jonathan NiederJun 6, 2018
  17. Jonathan TanJun 6, 2018
  18. 6/6 fetch-pack: introduce negotiator APIJonathan Tan, Jun 4, 2018
  19. Jonathan NiederJun 6, 2018
  20. Jonathan TanJun 6, 2018
  21. 0/8 Refactor fetch negotiation into its own APIJonathan Tan, Jun 6, 2018
  22. 1/8 fetch-pack: split up everything_local()Jonathan Tan, Jun 6, 2018
  23. Brandon WilliamsJun 14, 2018
  24. 2/8 fetch-pack: clear marks before re-markingJonathan Tan, Jun 6, 2018
  25. 4/8 fetch-pack: use ref adv. to prune "have" sentJonathan Tan, Jun 6, 2018
  26. Brandon WilliamsJun 14, 2018
  27. Junio C HamanoJun 14, 2018
  28. 5/8 fetch-pack: make negotiation-related vars localJonathan Tan, Jun 6, 2018
  29. Brandon WilliamsJun 14, 2018
  30. Junio C HamanoJun 14, 2018
  31. 6/8 fetch-pack: move common check and marking togetherJonathan Tan, Jun 6, 2018
  32. 8/8 negotiator/default: use better style in commentsJonathan Tan, Jun 6, 2018
  33. Brandon WilliamsJun 14, 2018
  34. 7/8 fetch-pack: introduce negotiator APIJonathan Tan, Jun 6, 2018
  35. 3/8 fetch-pack: directly end negotiation if ACK readyJonathan Tan, Jun 6, 2018
  36. Brandon WilliamsJun 14, 2018
  37. Brandon WilliamsJun 14, 2018
  38. 0/7 Refactor fetch negotiation into its own APIJonathan Tan, Jun 14, 2018
  39. 1/7 fetch-pack: split up everything_local()Jonathan Tan, Jun 14, 2018
  40. 2/7 fetch-pack: clear marks before re-markingJonathan Tan, Jun 14, 2018
  41. 3/7 fetch-pack: directly end negotiation if ACK readyJonathan Tan, Jun 14, 2018
  42. Junio C HamanoJun 15, 2018
  43. 4/7 fetch-pack: use ref adv. to prune "have" sentJonathan Tan, Jun 14, 2018
  44. 5/7 fetch-pack: make negotiation-related vars localJonathan Tan, Jun 14, 2018
  45. 6/7 fetch-pack: move common check and marking togetherJonathan Tan, Jun 14, 2018
  46. 7/7 fetch-pack: introduce negotiator APIJonathan Tan, Jun 14, 2018
  47. Brandon WilliamsJun 25, 2018

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.