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

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

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