Re: [PATCH v1 2/8] Add initial odb remote support

[Christian Couder <christian.couder@gmail.com>]

On Tue, May 15, 2018 at 3:44 AM, Junio C Hamano <gitster@pobox.com> wrote:
> Christian Couder <christian.couder@gmail.com> writes:

>> --- /dev/null
>> +++ b/odb-helper.h
>> @@ -0,0 +1,13 @@
>> +#ifndef ODB_HELPER_H
>> +#define ODB_HELPER_H
>
> Here is a good space to write a comment on what this structure and
> its fields are about.  Who are the dealers of helpers anyway?

Ok I added a few comments and renamed "dealer" to "remote".

>> +static struct odb_helper *helpers;
>> +static struct odb_helper **helpers_tail = &helpers;
>> +
>> +static struct odb_helper *find_or_create_helper(const char *name, int len)
>> +{
>> +     struct odb_helper *o;
>> +
>> +     for (o = helpers; o; o = o->next)
>> +             if (!strncmp(o->name, name, len) && !o->name[len])
>> +                     return o;
>> +
>> +     o = odb_helper_new(name, len);
>> +     *helpers_tail = o;
>> +     helpers_tail = &o->next;
>> +
>> +     return o;
>> +}
>
> This is a tangent, but I wonder if we can do better than
> hand-rolling these singly-linked list of custom structure types
> every time we add new code.  I am just guessing (because it is not
> described in the log message) that the expectation is to have only
> just a handful of helpers so looking for a helper by name is OK at
> O(n), as long as we very infrequently look up a helper by name.

Yeah, I think there will be very few helpers. I added a comment in the
version I will send really soon now.

>> +     if (!strcmp(subkey, "promisorremote"))
>> +             return git_config_string(&o->dealer, var, value);
>
> If the field is meant to record the name of the promisor remote,
> then it is better to name it as such, no?

Yeah, it is renamed "remote" now.

>> +     for (o = helpers; o; o = o->next)
>> +             if (!strcmp(o->dealer, dealer))
>> +                     return o;
>
> The code to create a new helper tried to avoid creating a helper
> with duplicated name, but nobody tries to create more than one
> helpers that point at the same promisor remote.  Yet here we try to
> grab the first one that happens to point at the given promisor.
>
> That somehow smells wrong.

For each promisor remote I think it makes no sense to have more than
one remote odb pointing to it. So I am not sure what to do here.