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

[Jonathan Tan <jonathantanmy@google.com>]

On Tue, Jun 5, 2018 at 5:37 PM, Jonathan Nieder <jrnieder@gmail.com> 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.