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

From: Patrick Steinhardt <ps@pks.im>
Date: Nov 26, 2024, 09:08 UTC
Message-ID: <Z0WQC7mFvT4pKn0n@pks.im>
In-Reply-To: <20241121204119.1440773-2-jltobler@gmail.com>

On Thu, Nov 21, 2024 at 02:41:15PM -0600, Justin Tobler wrote:

Show 13 quoted lines

> 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.

Show 7 quoted lines

> 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.

Show 17 quoted lines

> +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