On 24/11/22 10:21AM, Junio C Hamano wrote:
> Justin Tobler <jltobler@xxxxxxxxx> 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.
Thanks for the feedback. I'll revert the unnecessary changes in the next
version and avoid doing this in the future.
> > -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).
Good to know, I saw this pattern used in the reftable library. I'll
update per your suggestion in the next version.
> > {
> > 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.
Good point, at this point in the series its not obvious why the added
compexity is good or worth it. I'll defer making this change to the
following patch in the next version.
>
> Until you start needing other fields of opts in the function,
> perhaps in a later step, that is.
Ya, this is its intent. Having a default options to fallback to is more
useful once there are other fields present.
-Justin
