On Tue, May 15, 2018 at 3:44 AM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 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.
