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

[Junio C Hamano]

Christian Couder  writes:

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

If it makes no sense, then detecting and erroring out would be a
sensible thing to do, no?

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

[Christian Couder]

On Tue, May 15, 2018 at 3:44 AM, Junio C Hamano  wrote:
> Christian Couder  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 = 
>> +
>> +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 = >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(>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.

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

[Ævar Arnfjörð Bjarmason]

On Sun, May 13 2018, Christian Couder wrote:

> "sha1_file.c"[...]

sha1-file.c these days :)

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

[Junio C Hamano]

Christian Couder  writes:

> diff --git a/odb-helper.h b/odb-helper.h
> new file mode 100644
> index 00..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 00..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 = 
> +
> +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 = >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", , , ) < 0)
> + return 0;
> +
> + o = find_or_create_helper(name, namelen);
> +
> + if (!strcmp(subkey, "promisorremote"))
> + return git_config_string(>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.

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

[Christian Couder]

The odb-remote.{c,h} files will contain the functions
that are called by the rest of Git mostly from
"sha1_file.c" to access the objects managed by the
odb remotes.

The odb-helper.{c,h} files will contain the functions to
actually implement communication with either the internal
functions or the external scripts or processes that will
manage and provide remote git objects.

For now only infrastructure to create helpers from the
config and to check if there are odb remotes and helpers
is implemented.

Helped-by: Jeff King 
Signed-off-by: Christian Couder 
---
 Makefile |  2 ++
 odb-helper.c | 16 
 odb-helper.h | 13 ++
 odb-remote.c | 72 
 odb-remote.h |  7 +
 5 files changed, 110 insertions(+)
 create mode 100644 odb-helper.c
 create mode 100644 odb-helper.h
 create mode 100644 odb-remote.c
 create mode 100644 odb-remote.h

diff --git a/Makefile b/Makefile
index ad880d1fc5..f64dea287b 100644
--- a/Makefile
+++ b/Makefile
@@ -896,6 +896,8 @@ LIB_OBJS += notes-cache.o
 LIB_OBJS += notes-merge.o
 LIB_OBJS += notes-utils.o
 LIB_OBJS += object.o
+LIB_OBJS += odb-helper.o
+LIB_OBJS += odb-remote.o
 LIB_OBJS += oidmap.o
 LIB_OBJS += oidset.o
 LIB_OBJS += packfile.o
diff --git a/odb-helper.c b/odb-helper.c
new file mode 100644
index 00..b4d403ffa9
--- /dev/null
+++ b/odb-helper.c
@@ -0,0 +1,16 @@
+#include "cache.h"
+#include "object.h"
+#include "argv-array.h"
+#include "odb-helper.h"
+#include "run-command.h"
+#include "sha1-lookup.h"
+
+struct odb_helper *odb_helper_new(const char *name, int namelen)
+{
+   struct odb_helper *o;
+
+   o = xcalloc(1, sizeof(*o));
+   o->name = xmemdupz(name, namelen);
+
+   return o;
+}
diff --git a/odb-helper.h b/odb-helper.h
new file mode 100644
index 00..61d2ad082b
--- /dev/null
+++ b/odb-helper.h
@@ -0,0 +1,13 @@
+#ifndef ODB_HELPER_H
+#define ODB_HELPER_H
+
+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 00..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 = 
+
+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 = >next;
+
+   return o;
+}
+
+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", , , ) < 0)
+   return 0;
+
+   o = find_or_create_helper(name, namelen);
+
+   if (!strcmp(subkey, "promisorremote"))
+   return git_config_string(>dealer, var, value);
+
+   return 0;
+}
+
+static void odb_remote_init(void)
+{
+   static int initialized;
+
+   if (initialized)
+   return;
+   initialized = 1;
+
+   git_config(odb_remote_config, NULL);
+}
+
+struct odb_helper *find_odb_helper(const char *dealer)
+{
+   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;
+
+   return NULL;
+}
+
+int has_odb_remote(void)
+{
+   return !!find_odb_helper(NULL);
+}
diff --git a/odb-remote.h b/odb-remote.h
new file mode 100644
index 00..68aa330244
--- /dev/null
+++ b/odb-remote.h
@@ -0,0 +1,7 @@
+#ifndef ODB_REMOTE_H
+#define ODB_REMOTE_H
+
+extern struct odb_helper *find_odb_helper(const char *dealer);
+extern int has_odb_remote(void);
+
+#endif /* ODB_REMOTE_H */
-- 
2.17.0.590.gbd05bfcafd