Web lists-archives.com

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




Jonathan Tan wrote:

> 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//

> Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx>
> ---
>  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.

[...]
> +++ 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.

[...]
> +++ 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
[...]
> @@ -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. :)

[...]
> @@ -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.

[...]
> @@ -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?

[...]
> @@ -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".

[...]
> +/*
> +   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).

[...]
> --- /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@xxxxxxxxx>

Thanks for a nice cleanup.