Subject: Re: [PATCH v2 09/10] builtin/cat-file: deduplicate logic to iterate over all objects

From: Toon Claes <[email-masked]>
Date: Tue, 01 Apr 2025 14:13:57 +0200

Patrick Steinhardt <[email-masked]> writes:

> Pull out a common function that allows us to iterate over all objects in
> a repository. Right now the logic is trivial and would only require two
> function calls, making this refactoring a bit pointless. But in the next
> commit we will iterate on this logic to make use of bitmaps, so this is
> about to become a bit more complex.
>
> Signed-off-by: Patrick Steinhardt <[email-masked]>
> ---
>  builtin/cat-file.c | 85 ++++++++++++++++++++++++++++++------------------------
>  1 file changed, 48 insertions(+), 37 deletions(-)
>
> diff --git a/builtin/cat-file.c b/builtin/cat-file.c
> index 430320adfe9..6f5dbc821a2 100644
> --- a/builtin/cat-file.c
> +++ b/builtin/cat-file.c
> @@ -622,25 +622,18 @@ static int batch_object_cb(const struct object_id *oid, void *vdata)
>  	return 0;
>  }
>  
> -static int collect_loose_object(const struct object_id *oid,
> -				const char *path UNUSED,
> -				void *data)
> -{
> -	oid_array_append(data, oid);
> -	return 0;
> -}
> -
> -static int collect_packed_object(const struct object_id *oid,
> -				 struct packed_git *pack UNUSED,
> -				 uint32_t pos UNUSED,
> -				 void *data)
> +static int collect_object(const struct object_id *oid,
> +			  struct packed_git *pack UNUSED,
> +			  off_t offset UNUSED,
> +			  void *data)
>  {
>  	oid_array_append(data, oid);
>  	return 0;
>  }
>  
>  static int batch_unordered_object(const struct object_id *oid,
> -				  struct packed_git *pack, off_t offset,
> +				  struct packed_git *pack,
> +				  off_t offset,
>  				  void *vdata)
>  {
>  	struct object_cb_data *data = vdata;
> @@ -654,23 +647,6 @@ static int batch_unordered_object(const struct object_id *oid,
>  	return 0;
>  }
>  
> -static int batch_unordered_loose(const struct object_id *oid,
> -				 const char *path UNUSED,
> -				 void *data)
> -{
> -	return batch_unordered_object(oid, NULL, 0, data);
> -}
> -
> -static int batch_unordered_packed(const struct object_id *oid,
> -				  struct packed_git *pack,
> -				  uint32_t pos,
> -				  void *data)
> -{
> -	return batch_unordered_object(oid, pack,
> -				      nth_packed_object_offset(pack, pos),
> -				      data);
> -}
> -
>  typedef void (*parse_cmd_fn_t)(struct batch_options *, const char *,
>  			       struct strbuf *, struct expand_data *);
>  
> @@ -803,6 +779,45 @@ static void batch_objects_command(struct batch_options *opt,
>  
>  #define DEFAULT_FORMAT "%(objectname) %(objecttype) %(objectsize)"
>  
> +typedef int (*for_each_object_fn)(const struct object_id *oid, struct packed_git *pack,
> +				  off_t offset, void *data);
> +
> +struct for_each_object_payload {
> +	for_each_object_fn callback;
> +	void *payload;
> +};
> +
> +static int batch_one_object_loose(const struct object_id *oid,
> +				  const char *path UNUSED,
> +				  void *_payload)
> +{
> +	struct for_each_object_payload *payload = _payload;
> +	return payload->callback(oid, NULL, 0, payload->payload);
> +}
> +
> +static int batch_one_object_packed(const struct object_id *oid,
> +				   struct packed_git *pack,
> +				   uint32_t pos,
> +				   void *_payload)
> +{
> +	struct for_each_object_payload *payload = _payload;
> +	return payload->callback(oid, pack, nth_packed_object_offset(pack, pos),
> +				 payload->payload);
> +}
> +
> +static void batch_each_object(for_each_object_fn callback,
> +			      unsigned flags,
> +			      void *_payload)

Why is this `_payload` typeless? I see it only getting passed in
`struct object_cb_data`, is there a reason to hide this type? With
payload being wrapped in payload I think it's beneficial to keep type
info where possible.

-- 
Toon