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

[Junio C Hamano <gitster@pobox.com>]

Justin Tobler <jltobler@gmail.com> writes:

> 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)
> +	};
>  	struct bundle_header header = BUNDLE_HEADER_INIT;
> -	struct string_list_item *refname;
>  	struct strbuf bundle_ref = STRBUF_INIT;
> +	struct string_list_item *refname;
>  	size_t bundle_prefix_len;
> +	int result = 0;
> +	int bundle_fd;

Unrelated changes to reorder the lines, without any justification
worth describing in the proposed commit log message, distracts and
discourages the reviewers from reading further on.  I would avoid
making such changes if I were doing this patch.

The _real_ change in the above hunk is that a new struct instance
"opts" is defined, with its .flags member initialized based on what
fetch_pack_fsck_object() says.  That helper function requires us to
be in a repository, but because you must have a repository to
unbundle into, that call is safe.

> @@ -379,8 +383,7 @@ static int unbundle_from_file(struct repository *r, const char *file)
>  	 * a reachable ref pointing to the new tips, which will reach
>  	 * the prerequisite commits.
>  	 */
> -	result = unbundle(r, &header, bundle_fd, NULL,
> -			  VERIFY_BUNDLE_QUIET | (fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0));
> +	result = unbundle(r, &header, bundle_fd, NULL, &opts);

We can see that .flags in the new structure gets the same value we
used to pass in the original, which is good.

> diff --git a/bundle.c b/bundle.c
> index 4773b51eb1..db17f50ee0 100644
> --- a/bundle.c
> +++ b/bundle.c
> @@ -626,13 +626,17 @@ int create_bundle(struct repository *r, const char *path,
>  	return ret;
>  }
>  
> -int unbundle(struct repository *r, struct bundle_header *header,
> -	     int bundle_fd, struct strvec *extra_index_pack_args,
> -	     enum verify_bundle_flags flags)
> +int unbundle(struct repository *r, struct bundle_header *header, int bundle_fd,
> +	     struct strvec *extra_index_pack_args,
> +	     struct verify_bundle_opts *_opts)

Again, unrelated rewrapping of lines distracts and discourages the
reviewers from reading further on.  It looked as if the patch is
adding an extra parameter, until I read it again.

The real change here is that the enum is replaced with a struct that
has the same enum as one of its members, which is good.

Name the external-facing one (like this new parameter) _without_
funnies, and call it straight "opts".  The internal stand-in object
you create below can use funny convention but using "_" as prefix is
usually for system stuff (and the language standard forbids it, even
though people often do so in practice, from programs).

>  {
>  	struct child_process ip = CHILD_PROCESS_INIT;
> +	struct verify_bundle_opts opts = { 0 };
>  
> -	if (verify_bundle(r, header, flags))
> +	if (_opts)
> +		opts = *_opts;
> +
> +	if (verify_bundle(r, header, opts.flags))
>  		return -1;

This is an arrangement that looks strange, especially at this stage
of the series without reading the rest.  If verify_bundle() takes
the enum and not &opts, there is no need for stand-in opts struct.
You can have a local enum "flags" that is initialized to 0 and only
when parameter "opts" is not NULL, assign opts->flags to it and use
it throughout the rest of the function.  Reviewers will be left
confused wondering why the patch does this in an unnecessarily more
complex way by using an extra structure instance.

Until you start needing other fields of opts in the function,
perhaps in a later step, that is.

> @@ -641,7 +645,7 @@ int unbundle(struct repository *r, struct bundle_header *header,
>  	if (header->filter.choice)
>  		strvec_push(&ip.args, "--promisor=from-bundle");
>  
> -	if (flags & VERIFY_BUNDLE_FSCK)
> +	if (opts.flags & VERIFY_BUNDLE_FSCK)
>  		strvec_push(&ip.args, "--fsck-objects");

And this is a fallout of the above "strange" arrangement.

>  {
> +	struct verify_bundle_opts opts = { .flags = fetch_pack_fsck_objects() ?
> +							    VERIFY_BUNDLE_FSCK : 0 };

	struct verify_bundle_opts opts = {
		.flags = fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0,
	};

to avoid overly long lines, and prepare for a future you add more
members to the structure (the trailing comma helps).