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.
