Re: [PATCH 2/6] odb: make backend-specific fields optional

[Justin Tobler <jltobler@gmail.com>]

On 26/06/24 02:19PM, Patrick Steinhardt wrote:
> The `struct object_info` carries two pieces of information
> about how an object was looked up:
> 
>   - The `whence` enum identifying the backend.
> 
>   - The backend-tagged union `u` exposing backend-specific details
>     (currently only the packed-source case, which records the owning
>     pack, offset and packed object type).
> 
> The union is populated unconditionally, even though most callers don't
> care about provenance at all.
> 
> Split the backend-specific union out into a new public type, `struct
> object_info_source`, and make the object info structure carry it via
> just another opt-in request pointer. As with all the other requestable
> information, callers that need source info allocate a `struct
> object_info_source` on the stack and point `sourcep` at it; callers that
> don't care about it simply leave the field as a `NULL` pointer. Adapt
> callers accordingly.

Since not all callers may require this information, requiring callers to
explicitly request it seems reasonable to me.

> Note that the `whence` enum is strictly-speaking also backend-specific
> information, so it would be another good candidate to be moved into the
> `struct object_info_source`. For now though it is left alone, as it will
> be replaced by a `struct odb_source` pointer in a subsequent commit.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/cat-file.c     |  8 +++++--
>  builtin/index-pack.c   |  8 +++++--
>  builtin/pack-objects.c | 15 +++++++++----
>  odb.c                  |  3 ++-
>  odb.h                  | 60 +++++++++++++++++++++++++++++++++-----------------
>  packfile.c             | 33 ++++++++++++++-------------
>  reachable.c            |  5 ++++-
>  7 files changed, 87 insertions(+), 45 deletions(-)
> 
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 8726485f1f..adc626ce30 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -835,7 +835,8 @@ static int batch_one_object_oi(const struct object_id *oid,
>  {
>  	struct for_each_object_payload *payload = _payload;
>  	if (oi && oi->whence == OI_PACKED)
> -		return payload->callback(oid, oi->u.packed.pack, oi->u.packed.offset,
> +		return payload->callback(oid, oi->sourcep->u.packed.pack,
> +					 oi->sourcep->u.packed.offset,
>  					 payload->payload);

We update callsites now that object source info is stored differently in
`struct object_info`.

>  	return payload->callback(oid, NULL, 0, payload->payload);
>  }
> @@ -906,7 +907,10 @@ static void batch_each_object(struct batch_options *opt,
>  						&payload, flags);
>  		}
>  	} else {
> -		struct object_info oi = { 0 };
> +		struct object_info_source oi_source;
> +		struct object_info oi = {
> +			.sourcep = &oi_source,
> +		};

Caller that wish to know information regarding the source of the object
are required to explicitly request it. Makes sense.

[snip]
>  struct object_info {
>  	/* Request */
>  	enum object_type *typep;
> @@ -269,32 +301,20 @@ struct object_info {
>  	 */
>  	time_t *mtimep;
>  
> +	/*
> +	 * Backend-specific information that tells the caller where exactly an
> +	 * object was looked up from. This information should help disambiguate
> +	 * object lookups in case the same object exists in multiple sources,
> +	 * or multiple times in the same source.
> +	 */
> +	struct object_info_source *sourcep;

To me, the name `sourcep` makes me think a pointer to `struct
odb_source`. This did confuse me slightly when initially reading, but
I'm not sure it's worth it to be overly verbose here.

[snip]
> diff --git a/packfile.c b/packfile.c
> index 2b741d7a76..688c410b35 100644
> --- a/packfile.c
> +++ b/packfile.c
> @@ -1422,22 +1422,25 @@ int packed_object_info_with_index_pos(struct odb_source_packed *source UNUSED,
>  	}
>  
>  	oi->whence = OI_PACKED;
> -	oi->u.packed.offset = obj_offset;
> -	oi->u.packed.pack = p;
>  
> -	switch (type) {
> -	case OBJ_NONE:
> -		oi->u.packed.type = PACKED_OBJECT_TYPE_UNKNOWN;
> -		break;
> -	case OBJ_REF_DELTA:
> -		oi->u.packed.type = PACKED_OBJECT_TYPE_REF_DELTA;
> -		break;
> -	case OBJ_OFS_DELTA:
> -		oi->u.packed.type = PACKED_OBJECT_TYPE_OFS_DELTA;
> -		break;
> -	default:
> -		oi->u.packed.type = PACKED_OBJECT_TYPE_FULL;
> -		break;
> +	if (oi->sourcep) {
> +		oi->sourcep->u.packed.offset = obj_offset;
> +		oi->sourcep->u.packed.pack = p;
> +
> +		switch (type) {
> +		case OBJ_NONE:
> +			oi->sourcep->u.packed.type = PACKED_OBJECT_TYPE_UNKNOWN;
> +			break;
> +		case OBJ_REF_DELTA:
> +			oi->sourcep->u.packed.type = PACKED_OBJECT_TYPE_REF_DELTA;
> +			break;
> +		case OBJ_OFS_DELTA:
> +			oi->sourcep->u.packed.type = PACKED_OBJECT_TYPE_OFS_DELTA;
> +			break;
> +		default:
> +			oi->sourcep->u.packed.type = PACKED_OBJECT_TYPE_FULL;
> +			break;
> +		}

Source information is no longer unconditionally set.

Overall, this patch looks good.

-Justin