Re: [PATCH 10/40] external-odb: implement external_odb_get_direct

From: Christian Couder <christian.couder@gmail.com>
Date: Jan 15, 2018, 14:47 UTC
Message-ID: <CAP8UFD2MR_VCgFKVmLEaO989uM_AvX8_134PdoT=4MyJVPD1nA@mail.gmail.com>
In-Reply-To: <4ce872c0-8f4b-602a-9ee3-cf429cc6a146@jeffhostetler.com>

On Thu, Jan 4, 2018 at 6:44 PM, Jeff Hostetler <git@jeffhostetler.com> wrote:

Show 41 quoted lines

>
>
> On 1/3/2018 11:33 AM, Christian Couder wrote:
>>
>> This is implemented only in the promisor remote mode
>> for now by calling fetch_object().
>>
>> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
>> ---
>>   external-odb.c | 15 +++++++++++++++
>>   external-odb.h |  1 +
>>   odb-helper.c   | 13 +++++++++++++
>>   odb-helper.h   |  3 ++-
>>   4 files changed, 31 insertions(+), 1 deletion(-)
>>
>> diff --git a/external-odb.c b/external-odb.c
>> index d26e63d8b1..5d0afb9762 100644
>> --- a/external-odb.c
>> +++ b/external-odb.c
>> @@ -76,3 +76,18 @@ int external_odb_has_object(const unsigned char *sha1)
>>                         return 1;
>>         return 0;
>>   }
>> +
>> +int external_odb_get_direct(const unsigned char *sha1)
>> +{
>> +       struct odb_helper *o;
>> +
>> +       external_odb_init();
>> +
>> +       for (o = helpers; o; o = o->next) {
>> +               if (odb_helper_get_direct(o, sha1) < 0)
>> +                       continue;
>> +               return 0;
>
>> +     }
>
> Would this be simpler said as:
>         for (o = ...)
>                 if (!odb_helper_get_direct(...))
>                         return 0;

At then end of the series the content of the loop is:

        if (!(o->supported_capabilities & ODB_HELPER_CAP_GET_DIRECT))
            continue;
        if (odb_helper_get_direct(o, sha1) < 0)
            continue;
        return 0;

And I think it is fine like that, so I don't think changing this commit is a good idea.

Show 43 quoted lines

>> +       return -1;
>> +}
>> diff --git a/external-odb.h b/external-odb.h
>> index 9a3c2f01b3..fd6708163e 100644
>> --- a/external-odb.h
>> +++ b/external-odb.h
>> @@ -4,5 +4,6 @@
>>   extern int has_external_odb(void);
>>   extern const char *external_odb_root(void);
>>   extern int external_odb_has_object(const unsigned char *sha1);
>> +extern int external_odb_get_direct(const unsigned char *sha1);
>>     #endif /* EXTERNAL_ODB_H */
>> diff --git a/odb-helper.c b/odb-helper.c
>> index 1404393807..4b70b287af 100644
>> --- a/odb-helper.c
>> +++ b/odb-helper.c
>> @@ -4,6 +4,7 @@
>>   #include "odb-helper.h"
>>   #include "run-command.h"
>>   #include "sha1-lookup.h"
>> +#include "fetch-object.h"
>>     struct odb_helper *odb_helper_new(const char *name, int namelen)
>>   {
>> @@ -52,3 +53,15 @@ int odb_helper_has_object(struct odb_helper *o, const
>> unsigned char *sha1)
>>         return !!odb_helper_lookup(o, sha1);
>>   }
>>   +int odb_helper_get_direct(struct odb_helper *o,
>> +                         const unsigned char *sha1)
>> +{
>> +       int res = 0;
>> +       uint64_t start = getnanotime();
>> +
>> +       fetch_object(o->dealer, sha1);
>> +
>> +       trace_performance_since(start, "odb_helper_get_direct");
>> +
>> +       return res;
>
>
> 'res' will always be 0, so the external_odb_get_direct() will
> only do the first helper.  i haven't looked at the rest of the
> series yet, so maybe you've already addressed this.

That's why I previously suggested in one of your or Jonathan's patch that fetch_object() should return an int that tells the caller if the object has been fetched instead of void.

If we make it possible at one point to have the objects fetched fetch_object() in different remotes (and I think that's a straightforward goal), this will actually fail but callers will have no simple way to know that.

> Also, I put a TODO comment in the fetch_object() header to
> consider returning an error/success, so maybe that could help
> here too.

Yeah, indeed.