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

From: Junio C Hamano <gitster@pobox.com>
Date: May 15, 2018, 01:44 UTC
Message-ID: <xmqqh8n9ae17.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20180513103232.17514-3-chriscool@tuxfamily.org>

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

Show 9 quoted lines

> 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?

Show 38 quoted lines

> +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.

Show 14 quoted lines

> +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.

Show 11 quoted lines

> +{
> +	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.