| Message ID | 20241121204119.1440773-2-jltobler@gmail.com (mailing list archive) |
|---|---|
| State | New |
| Headers | show |
| Series | propagate fsck message severity for bundle fetch | expand |
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).
On 24/11/22 10:21AM, Junio C Hamano wrote: > 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. 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
diff --git a/builtin/bundle.c b/builtin/bundle.c index 127518c2a8..15ac75ab51 100644 --- a/builtin/bundle.c +++ b/builtin/bundle.c @@ -218,7 +218,7 @@ static int cmd_bundle_unbundle(int argc, const char **argv, const char *prefix) strvec_pushl(&extra_index_pack_args, "-v", "--progress-title", _("Unbundling objects"), NULL); ret = !!unbundle(the_repository, &header, bundle_fd, - &extra_index_pack_args, 0) || + &extra_index_pack_args, NULL) || list_bundle_refs(&header, argc, argv); bundle_header_release(&header); 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; bundle_fd = read_bundle_header(file, &header); if (bundle_fd < 0) { @@ -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); if (result) { result = 1; goto cleanup; 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) { 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; strvec_pushl(&ip.args, "index-pack", "--fix-thin", "--stdin", NULL); @@ -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"); if (extra_index_pack_args) 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); +struct verify_bundle_opts { + enum verify_bundle_flags flags; +}; + /** * Unbundle after reading the header with read_bundle_header(). * @@ -49,12 +53,12 @@ int verify_bundle(struct repository *r, struct bundle_header *header, * (e.g. "-v" for verbose/progress), NULL otherwise. The provided * "extra_index_pack_args" (if any) will be strvec_clear()'d for you. * - * Before unbundling, this method will call verify_bundle() with the - * given 'flags'. + * Before unbundling, this method will call verify_bundle() with 'flags' + * provided in 'opts'. */ -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); int list_bundle_refs(struct bundle_header *header, int argc, const char **argv); 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 }; struct bundle_transport_data *data = transport->data; struct strvec extra_index_pack_args = STRVEC_INIT; int ret; @@ -185,9 +187,9 @@ static int fetch_refs_from_bundle(struct transport *transport, if (!data->get_refs_from_bundle_called) get_refs_from_bundle_inner(transport); + ret = unbundle(the_repository, &data->header, data->fd, - &extra_index_pack_args, - fetch_pack_fsck_objects() ? VERIFY_BUNDLE_FSCK : 0); + &extra_index_pack_args, &opts); transport->hash_algo = data->header.hash_algo; strvec_clear(&extra_index_pack_args);
Bundle verification performed as part of `unbundle()` is configurable via providing `verify_bundle_flags`. This is done by invoking `verify_bundle()` and propagating the set flags. If the `VERIFY_BUNDLE_FSCK` flag is provided, the `fsck-objects` flag is specified when invoking git-index-pack(1) to perform fsck checks on the objects in the bundle. Introduce a new type, `verify_bundle_opts`, and update `unbundle()` to accept this instead of `verify_bundle_flags` to perform the same verification configuration. In a subsequent commit, `verify_bundle_opts` will be extended to support configuration of fsck message severity. Signed-off-by: Justin Tobler <jltobler@gmail.com> --- builtin/bundle.c | 2 +- bundle-uri.c | 13 ++++++++----- bundle.c | 14 +++++++++----- bundle.h | 14 +++++++++----- transport.c | 6 ++++-- 5 files changed, 31 insertions(+), 18 deletions(-)