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

[Junio C Hamano <gitster@pobox.com>]

Christian Couder <christian.couder@gmail.com> writes:

> diff --git a/odb-helper.h b/odb-helper.h
> new file mode 100644
> index 0000000000..61d2ad082b
> --- /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?

> +struct odb_helper {
> +	const char *name;
> +	const char *dealer;
> +
> +	struct odb_helper *next;
> +};
> +
> +extern struct odb_helper *odb_helper_new(const char *name, int namelen);
> +
> +#endif /* ODB_HELPER_H */
> diff --git a/odb-remote.c b/odb-remote.c
> new file mode 100644
> index 0000000000..e03b953ec6
> --- /dev/null
> +++ b/odb-remote.c
> @@ -0,0 +1,72 @@
> +#include "cache.h"
> +#include "odb-remote.h"
> +#include "odb-helper.h"
> +#include "config.h"
> +
> +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.

> +static int odb_remote_config(const char *var, const char *value, void *data)
> +{
> +	struct odb_helper *o;
> +	const char *name;
> +	int namelen;
> +	const char *subkey;
> +
> +	if (parse_config_key(var, "odb", &name, &namelen, &subkey) < 0)
> +		return 0;
> +
> +	o = find_or_create_helper(name, namelen);
> +
> +	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?

> +struct odb_helper *find_odb_helper(const char *dealer)

Ditto.

> +{
> +	struct odb_helper *o;
> +
> +	odb_remote_init();
> +
> +	if (!dealer)
> +		return helpers;
> +
> +	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.