Re: [PATCH 3/6] odb: add `source` field to struct object_info_source

[Justin Tobler <jltobler@gmail.com>]

On 26/06/24 02:19PM, Patrick Steinhardt wrote:
> The previous commit introduced `struct object_info_source` as an opt-in
> container for backend-specific information, but for now we only moved
> preexisting data into this structure. Most importantly, the caller has
> no way yet to learn about which source an object was actually looked up
> from. Instead, callers have to rely on the `whence` enum to distinguish
> the object type, but cannot use that enum to tell the object source.
> 
> Add a `struct odb_source *source` field to the structure and populate it
> from each backend's lookup path.

Makes sense.

> The `whence` enum is still set and used by callers; it will be removed
> in a subsequent commit now that `sourcep->source` can identify the
> backend on its own.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.h                 | 3 +++
>  odb/source-inmemory.c | 3 +++
>  odb/source-loose.c    | 2 ++
>  packfile.c            | 6 +++++-
>  4 files changed, 13 insertions(+), 1 deletion(-)
> 
> diff --git a/odb.h b/odb.h
> index 770900289a..330a55879e 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -253,6 +253,9 @@ int odb_pretend_object(struct object_database *odb,
>   * more about how exactly it is stored.
>   */
>  struct object_info_source {
> +	/* The source that this object has been looked up from. */
> +	struct odb_source *source;

Here we add the `struct odb_source` so we can begin recording it.

> +
>  	/*
>  	 * Backend-specific information about the specific object. This can be
>  	 * used for example to uniquely identify a given object in case it
> diff --git a/odb/source-inmemory.c b/odb/source-inmemory.c
> index e004566d76..2328e62687 100644
> --- a/odb/source-inmemory.c
> +++ b/odb/source-inmemory.c
> @@ -52,6 +52,9 @@ static void populate_object_info(struct odb_source_inmemory *source,
>  		*oi->contentp = xmemdupz(object->buf, object->size);
>  	if (oi->mtimep)
>  		*oi->mtimep = 0;
> +	if (oi->sourcep)
> +		oi->sourcep->source = &source->base;

Here we set the source for the in-memory backend.

> +
>  	oi->whence = OI_CACHED;
>  }
>  
> diff --git a/odb/source-loose.c b/odb/source-loose.c
> index 66e6bb8d3f..5c4e9892b5 100644
> --- a/odb/source-loose.c
> +++ b/odb/source-loose.c
> @@ -196,6 +196,8 @@ static int read_object_info_from_path(struct odb_source_loose *loose,
>  			oi->typep = NULL;
>  		if (oi->delta_base_oid)
>  			oidclr(oi->delta_base_oid, loose->base.odb->repo->hash_algo);
> +		if (oi->sourcep && !ret)
> +			oi->sourcep->source = &loose->base;

Here it is set for the loose backend.

>  		if (!ret)
>  			oi->whence = OI_LOOSE;
>  	}
> diff --git a/packfile.c b/packfile.c
> index 688c410b35..fa22095b75 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1324,7 +1324,7 @@ static void add_delta_base_cache(struct packed_git *p, off_t base_offset,
>  	hashmap_add(&delta_base_cache, &ent->ent);
>  }
>  
> -int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
> +int packed_object_info_with_index_pos(struct odb_source_packed *source,
>  				      struct packed_git *p, off_t obj_offset,
>  				      uint32_t *maybe_index_pos, struct object_info *oi)
>  {
> @@ -1424,6 +1424,10 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
>  	oi->whence = OI_PACKED;
>  
>  	if (oi->sourcep) {
> +		if (!source)
> +			BUG("cannot request source without an owning source");
> +		oi->sourcep->source = &source->base;

And here it is set for the packed backend. Looks good.

Naive question: I understand that some `packed_info_object()` callers
may not have the `struct odb_source` on hand, but when the `struct
packed_git` is intially setup, is it not always known the ODB source it
comes from? It makes me wonder if the ODB source should also be recorded
when `struct packed_git` is initialized.

-Justin