diff mbox series

[v3,03/11] promisor-remote: implement promisor_remote_get_direct()

Message ID 20190312132959.11764-4-chriscool@tuxfamily.org (mailing list archive)
State New, archived
Headers show
Series Many promisor remotes | expand

Commit Message

Christian Couder March 12, 2019, 1:29 p.m. UTC 
From: Christian Couder <christian.couder@gmail.com>

This is implemented for now by calling fetch_objects(). It fetches
from all the promisor remotes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 promisor-remote.c | 17 +++++++++++++++++
 promisor-remote.h |  1 +
 2 files changed, 18 insertions(+)

Comments

Junio C Hamano March 13, 2019, 4:23 a.m. UTC | #1 
Christian Couder <christian.couder@gmail.com> writes:

> @@ -98,3 +99,19 @@ int has_promisor_remote(void)
>  {
>  	return !!promisor_remote_find(NULL);
>  }
> +
> +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)
> +{
> +	struct promisor_remote *o;
> +
> +	promisor_remote_init();
> +
> +	for (o = promisors; o; o = o->next) {
> +		if (fetch_objects(o->remote_name, oids, oid_nr) < 0)
> +			continue;
> +		return 0;

Suppose the caller asks to fetch 3 objects, A, B and C, from two
promisors.  The first promisor can give you A and B but cannot give
you C.  The second promisor only can give you C.

Does fetch_objects() return failure after attempting to fetch A, B
and C from the first promisor?  Then we go on to the second promisor
but do we ask all three?  That would mean the second promisor will
also fail because it cannot give you A and B, and then the whole
thing would fail.  It would be nicer if the mechanism would allow us
to fetch what is still missing from later promisor, perhaps.

As the original "fetch" protocol only allows you to fetch a pack
with everything you asked for in it, instead of feeding you a pack
with best effort, I think the answer to the above is "it is very
hard to improve over what we have here", but people may have
interesting ideas ;-)

> +	}
> +
> +	return -1;
> +}
> +

Adding trailing blank line at the end?

> diff --git a/promisor-remote.h b/promisor-remote.h
> index bfbf7c0f21..f9f5825417 100644
> --- a/promisor-remote.h
> +++ b/promisor-remote.h
> @@ -13,5 +13,6 @@ struct promisor_remote {
>  extern struct promisor_remote *promisor_remote_new(const char *remote_name);
>  extern struct promisor_remote *promisor_remote_find(const char *remote_name);
>  extern int has_promisor_remote(void);
> +extern int promisor_remote_get_direct(const struct object_id *oids, int oid_nr);
>  
>  #endif /* PROMISOR_REMOTE_H */
Christian Couder April 1, 2019, 4:41 p.m. UTC | #2 
On Wed, Mar 13, 2019 at 5:24 AM Junio C Hamano <gitster@pobox.com> wrote:
>
> Christian Couder <christian.couder@gmail.com> writes:

> > +int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)
> > +{
> > +     struct promisor_remote *o;
> > +
> > +     promisor_remote_init();
> > +
> > +     for (o = promisors; o; o = o->next) {
> > +             if (fetch_objects(o->remote_name, oids, oid_nr) < 0)
> > +                     continue;
> > +             return 0;
>
> Suppose the caller asks to fetch 3 objects, A, B and C, from two
> promisors.  The first promisor can give you A and B but cannot give
> you C.  The second promisor only can give you C.
>
> Does fetch_objects() return failure after attempting to fetch A, B
> and C from the first promisor?

Yes, I think it should still send the objects it has and then fail.

Maybe it doesn't work like this right now (I haven't checked), or
maybe the failure should be different than a regular failure. In those
cases I can maybe improve on that in a later iteration.

> Then we go on to the second promisor
> but do we ask all three?  That would mean the second promisor will
> also fail because it cannot give you A and B, and then the whole
> thing would fail.  It would be nicer if the mechanism would allow us
> to fetch what is still missing from later promisor, perhaps.

Yeah, I implemented fetching only what is still missing in v4.

> As the original "fetch" protocol only allows you to fetch a pack
> with everything you asked for in it, instead of feeding you a pack
> with best effort, I think the answer to the above is "it is very
> hard to improve over what we have here", but people may have
> interesting ideas ;-)

I think we should make it best effort when acting as a promisor
remote. I will take a look at that.

> > +     }
> > +
> > +     return -1;
> > +}
> > +
>
> Adding trailing blank line at the end?

Removed.
diff mbox series

Patch

diff --git a/promisor-remote.c b/promisor-remote.c
index d2f574651e..f86f9d0b84 100644
--- a/promisor-remote.c
+++ b/promisor-remote.c
@@ -1,6 +1,7 @@ 
 #include "cache.h"
 #include "promisor-remote.h"
 #include "config.h"
+#include "fetch-object.h"
 
 static struct promisor_remote *promisors;
 static struct promisor_remote **promisors_tail = &promisors;
@@ -98,3 +99,19 @@  int has_promisor_remote(void)
 {
 	return !!promisor_remote_find(NULL);
 }
+
+int promisor_remote_get_direct(const struct object_id *oids, int oid_nr)
+{
+	struct promisor_remote *o;
+
+	promisor_remote_init();
+
+	for (o = promisors; o; o = o->next) {
+		if (fetch_objects(o->remote_name, oids, oid_nr) < 0)
+			continue;
+		return 0;
+	}
+
+	return -1;
+}
+
diff --git a/promisor-remote.h b/promisor-remote.h
index bfbf7c0f21..f9f5825417 100644
--- a/promisor-remote.h
+++ b/promisor-remote.h
@@ -13,5 +13,6 @@  struct promisor_remote {
 extern struct promisor_remote *promisor_remote_new(const char *remote_name);
 extern struct promisor_remote *promisor_remote_find(const char *remote_name);
 extern int has_promisor_remote(void);
+extern int promisor_remote_get_direct(const struct object_id *oids, int oid_nr);
 
 #endif /* PROMISOR_REMOTE_H */