Re: [PATCH 1/5] bundle: add bundle verification options type

[Patrick Steinhardt <ps@pks.im>]

On Thu, Nov 21, 2024 at 02:41:15PM -0600, Justin Tobler wrote:
> diff --git a/bundle-uri.c b/bundle-uri.c
> index 0df66e2872..ed3afcaeb3 100644
> --- a/bundle-uri.c
> +++ b/bundle-uri.c
> @@ -361,12 +361,16 @@ static int copy_uri_to_file(const char *filename, const char *uri)
>  
>  static int unbundle_from_file(struct repository *r, const char *file)
>  {
> -	int result = 0;
> -	int bundle_fd;
> +	struct verify_bundle_opts opts = {
> +		.flags = VERIFY_BUNDLE_QUIET |
> +			 (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0)

This is missing its trailing comma.

> diff --git a/bundle.h b/bundle.h
> index 5ccc9a061a..bddf44c267 100644
> --- a/bundle.h
> +++ b/bundle.h
> @@ -39,6 +39,10 @@ enum verify_bundle_flags {
>  int verify_bundle(struct repository *r, struct bundle_header *header,
>  		  enum verify_bundle_flags flags);

Curious. I would have expected that `verify_bundle()` should receive the
full `verify_bundle_opts` because these are so similarly named. Like
this we have the weird situation where `unbundle()` seemingly receives
the options that are intended for `verify_bundle()`, but we never pass
them through in the first place.

> +struct verify_bundle_opts {
> +	enum verify_bundle_flags flags;
> +};
> +
>  /**
>   * Unbundle after reading the header with read_bundle_header().
>   *
> diff --git a/transport.c b/transport.c
> index 47fda6a773..7e0ec4adc9 100644
> --- a/transport.c
> +++ b/transport.c
> @@ -176,6 +176,8 @@ static int fetch_refs_from_bundle(struct transport *transport,
>  				  int nr_heads UNUSED,
>  				  struct ref **to_fetch UNUSED)
>  {
> +	struct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?
> +							    VERIFY_BUNDLE_FSCK : 0 };

This is weirdly formatted and should rather follow what you have done
further up in `unbundle_from_file()`.

Patrick