On Tue, Jun 5, 2018 at 5:37 PM, Jonathan Nieder <jrnieder@xxxxxxxxx> wrote:
>> 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//
OK.
>> +#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.
OK.
>> +#include "cache.h"
>
> What does this need from cache.h?
If I remember correctly, I needed something, but I might not. I'll
remove it if so.
>> +#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?
I'll use the forward declaration then.
> [...]
>> +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.
OK.
>> @@ -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?
Good point - will do.
>> @@ -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).
OK.
>> +++ b/negotiator/default.c
>> @@ -0,0 +1,173 @@
>> +#include "default.h"
>
> First #include should be "git-compat-util.h".
OK.
> [...]
>> +/*
>> + 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).
I'll add 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.
I'll use a forward declaration.
