Re: [PATCH 28/40] pack-objects: don't pack objects in external odbs

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Jan 4, 2018, 20:54 UTC
Message-ID: <8de3597a-01bd-a721-fffb-20769701d0af@jeffhostetler.com>
In-Reply-To: <20180103163403.11303-29-chriscool@tuxfamily.org>

On 1/3/2018 11:33 AM, Christian Couder wrote:

Show 28 quoted lines

> Objects managed by an external ODB should not be put into
> pack files. They should be transfered using other mechanism
> that can be specific to the external odb.
> 
> Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
> ---
>   builtin/pack-objects.c | 4 ++++
>   1 file changed, 4 insertions(+)
> 
> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
> index 6c71552cdf..4ed66c7677 100644
> --- a/builtin/pack-objects.c
> +++ b/builtin/pack-objects.c
> @@ -28,6 +28,7 @@
>   #include "argv-array.h"
>   #include "mru.h"
>   #include "packfile.h"
> +#include "external-odb.h"
>   
>   static const char *pack_usage[] = {
>   	N_("git pack-objects --stdout [<options>...] [< <ref-list> | < <object-list>]"),
> @@ -1026,6 +1027,9 @@ static int want_object_in_pack(const struct object_id *oid,
>   			return want;
>   	}
>   
> +	if (external_odb_has_object(oid->hash))
> +		return 0;
> +

I worry about the performance of this in light of my comments earlier in the patch series about the expense of building the "have" sets.

Since we've already checked for a loose object and we are about to walk thru the local packfiles, so if we don't find it in any of them, then we don't have it locally. Only then do we need to worry about external odbs.

If we don't have it locally, does the caller of this function have sufficient "promisor" infomation infer that the object should exist on the promisor remote? Since you're going to omit it from the packfile anyway, you don't really need to know if the remote actually has it.

>   	for (entry = packed_git_mru.head; entry; entry = entry->next) {
>   		struct packed_git *p = entry->item;
>   		off_t offset;
>

Jeff