Re: [PATCH v5 10/40] Add initial external odb support

Christian Couder [off-list ref] writes:

+int external_odb_has_object(const unsigned char *sha1)
+{
+	struct odb_helper *o;
+
+	external_odb_init();
+
+	for (o = helpers; o; o = o->next)
+		if (odb_helper_has_object(o, sha1))
+			return 1;
+	return 0;
+}
+
+int external_odb_get_object(const unsigned char *sha1)
+{
+	struct odb_helper *o;
+	const char *path;
+
+	if (!external_odb_has_object(sha1))
+		return -1;

This probably would not matter, as I do not expect one repository to
connect to and backed by very many external odb instances, but I
would have expected that the interaction would go more like "ah, we
need this object that is lacking locally. let's see if there is
anybody with the object. now we found who claims to have the object,
let's ask that guy (and nobody else) to give the object to us".

IOW, I would have expected two functions:

 - struct odb_helper *external_odb_with(struct object_id *oid);
 - int external_odb_get(struct object_id *oid, struct odb_helper *odb);

where the latter may start like

    if (!odb) {
	odb = external_odb_with(oid);
	if (!odb)
	    return -1;
    }
    ... go ask that odb for the object ...

Even though ancient codebase of ours deliberately omitted them, I
think our recent trend is to explicitly spell "extern " in headers.

Likewise.

On Thu, Aug 03, 2017 at 12:34:25PM -0700, Junio C Hamano wrote:

quoted

+int external_odb_get_object(const unsigned char *sha1)
+{
+	struct odb_helper *o;
+	const char *path;
+
+	if (!external_odb_has_object(sha1))
+		return -1;

This probably would not matter, as I do not expect one repository to
connect to and backed by very many external odb instances, but I
would have expected that the interaction would go more like "ah, we
need this object that is lacking locally. let's see if there is
anybody with the object. now we found who claims to have the object,
let's ask that guy (and nobody else) to give the object to us".

IOW, I would have expected two functions:

 - struct odb_helper *external_odb_with(struct object_id *oid);
 - int external_odb_get(struct object_id *oid, struct odb_helper *odb);

One advantage of walking through them linearly and asking "can you get
it?" is that it gracefully handles external odbs which aren't available.
That can be used for redundancy, or for situations where a preferred
odb isn't always available (e.g., a fast server which is only available
when you're on a particular network).

-Peff

On Thu, Aug 3, 2017 at 9:34 PM, Junio C Hamano [off-list ref] wrote:

Christian Couder [off-list ref] writes:

quoted

diff --git a/external-odb.h b/external-odb.h
new file mode 100644
index 0000000000..9989490c9e
--- /dev/null
+++ b/external-odb.h

@@ -0,0 +1,8 @@
+#ifndef EXTERNAL_ODB_H
+#define EXTERNAL_ODB_H
+
+const char *external_odb_root(void);
+int external_odb_has_object(const unsigned char *sha1);
+int external_odb_get_object(const unsigned char *sha1);

Even though ancient codebase of ours deliberately omitted them, I
think our recent trend is to explicitly spell "extern " in headers.

quoted

diff --git a/odb-helper.h b/odb-helper.h
new file mode 100644
index 0000000000..5800661704
--- /dev/null
+++ b/odb-helper.h

Likewise.

Ok, I am adding "extern " to the headers.