| Message ID | 3a88c99d9bc765bf4728fe0f0df1eed86adace0e.1645638911.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | Partial bundles | expand |
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > static int try_bitmap_count(struct rev_info *revs, > - struct list_objects_filter_options *filter, > int filter_provided_objects) This makes quite a lot of sense as filter is now available as revs->filter. > { > uint32_t commit_count = 0, > @@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs, > */ > max_count = revs->max_count; > > - bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects); > + bitmap_git = prepare_bitmap_walk(revs, revs->filter, > + filter_provided_objects); And we should be able to do the same to prepare_bitmap_walk(). It is OK if such a change comes later and not as part of this commit. Perhaps it is deliberate. Unlike the helpers this step touches, namely, try_bitmap_count(), try_bitmap_traversal(), and try_bitmap_disk_usage(), prepare_bitmap_walk() is not a file-scope static helper and updating it will need touching many more places. > @@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) > } > > if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) { #leftoverbit. We need to remember to clean this up, now "--filter" is well established (I am assuming this literal-string pasting is because we didn't know what the right and final word to be used as the option name back when this code was originally written), when the code around here is quiescent. > - parse_list_objects_filter(&filter_options, arg); > - if (filter_options.choice && !revs.blob_objects) > + if (!revs.filter) > + CALLOC_ARRAY(revs.filter, 1); > + parse_list_objects_filter(revs.filter, arg); > + if (revs.filter->choice && !revs.blob_objects) > die(_("object filtering requires --objects")); > continue; OK. The original "filter_options" was a structure and not a pointer to a structure; now we have a pointer to a structure in revs as a member so we need an on-demand allocation. CALLOC_ARRAY() instead of xcalloc(), when we know we are creating one element and not an array of elements whose size happens to be one, is not wrong but it does look strange. Was there a reason why we avoid xcalloc()? Makes me also wonder how big the filter_options structure is; because we will not use unbounded many revs structure, it may have been a simpler conversion to turn a static struct into an embedded struct member in a struct (instead of a member of a struct that is a pointer to a struct). That way, we did not have to ... > } > if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) { > - list_objects_filter_set_no_filter(&filter_options); > + if (!revs.filter) > + CALLOC_ARRAY(revs.filter, 1); ... repeat the on-demand allocation. If some code used to pass &filter_options in a parameter to helper functions, and such calling sites get rewritten to pass the value in the revs.filter pointer, and if revs hasn't gone through this codepath, these helper functions will start receiving NULL in their filter_options parameter, which they may or may not be prepared to take. This "we get rid of a global struct and replace it with an on-demand allocated structure, pointer to which is stored in the revs structure" rewrite somehow makes me nervous. > diff --git a/revision.h b/revision.h > index 3c58c18c63a..1ddb73ab82e 100644 > --- a/revision.h > +++ b/revision.h > @@ -81,6 +81,7 @@ struct rev_cmdline_info { > > struct oidset; > struct topo_walk_info; > +struct list_objects_filter_options; Is the forward-declaration the only reason why we needed to have a pointer to a(n opaque) struct, not an embedded struct, as a member? > struct rev_info { > /* Starting list */ > @@ -94,6 +95,9 @@ struct rev_info { > /* The end-points specified by the end user */ > struct rev_cmdline_info cmdline; > > + /* Object filter options. NULL for no filtering. */ > + struct list_objects_filter_options *filter; > + > /* excluding from --branches, --refs, etc. expansion */ > struct string_list *ref_excludes;
On 3/4/2022 5:15 PM, Junio C Hamano wrote: > "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> static int try_bitmap_count(struct rev_info *revs, >> - struct list_objects_filter_options *filter, >> int filter_provided_objects) > > This makes quite a lot of sense as filter is now available as > revs->filter. > >> { >> uint32_t commit_count = 0, >> @@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs, >> */ >> max_count = revs->max_count; >> >> - bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects); >> + bitmap_git = prepare_bitmap_walk(revs, revs->filter, >> + filter_provided_objects); > > And we should be able to do the same to prepare_bitmap_walk(). It > is OK if such a change comes later and not as part of this commit. > > Perhaps it is deliberate. Unlike the helpers this step touches, > namely, try_bitmap_count(), try_bitmap_traversal(), and > try_bitmap_disk_usage(), prepare_bitmap_walk() is not a file-scope > static helper and updating it will need touching many more places. I'm making a note that this cleanup can happen in a follow-up series. >> @@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) >> } >> >> if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) { > > #leftoverbit. We need to remember to clean this up, now "--filter" > is well established (I am assuming this literal-string pasting is > because we didn't know what the right and final word to be used as > the option name back when this code was originally written), when > the code around here is quiescent. Good point. >> - parse_list_objects_filter(&filter_options, arg); >> - if (filter_options.choice && !revs.blob_objects) >> + if (!revs.filter) >> + CALLOC_ARRAY(revs.filter, 1); >> + parse_list_objects_filter(revs.filter, arg); >> + if (revs.filter->choice && !revs.blob_objects) >> die(_("object filtering requires --objects")); >> continue; > > OK. The original "filter_options" was a structure and not a pointer > to a structure; now we have a pointer to a structure in revs as a > member so we need an on-demand allocation. CALLOC_ARRAY() instead > of xcalloc(), when we know we are creating one element and not an > array of elements whose size happens to be one, is not wrong but it > does look strange. Was there a reason why we avoid xcalloc()? I think I've been using CALLOC_ARRAY(..., 1) over "... = xcalloc()" for simplicity's sake for a while. I see quite a few across the codebase, too, but I can swap the usage here if you feel that is important. > Makes me also wonder how big the filter_options structure is; > because we will not use unbounded many revs structure, it may have > been a simpler conversion to turn a static struct into an embedded > struct member in a struct (instead of a member of a struct that is a > pointer to a struct). That way, we did not have to ... >>> } >> if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) { >> - list_objects_filter_set_no_filter(&filter_options); >> + if (!revs.filter) >> + CALLOC_ARRAY(revs.filter, 1); > > ... repeat the on-demand allocation. If some code used to pass > &filter_options in a parameter to helper functions, and such calling > sites get rewritten to pass the value in the revs.filter pointer, > and if revs hasn't gone through this codepath, these helper functions > will start receiving NULL in their filter_options parameter, which > they may or may not be prepared to take. This "we get rid of a > global struct and replace it with an on-demand allocated structure, > pointer to which is stored in the revs structure" rewrite somehow > makes me nervous. I think the main idea is that the filter being NULL indicates "no filter is used. Do a full object walk." If we use a static struct, then we need to instead use revs->filter.filter_spec.nr, but that is already being used as a BUG() statement: const char *list_objects_filter_spec(struct list_objects_filter_options *filter) { if (!filter->filter_spec.nr) BUG("no filter_spec available for this filter"); so a non-NULL filter with no specs is considered invalid, while a NULL filter _is_ currently considered valid. While we _could_ make that switch to using a static struct and change our checks to allow empty specs, that would be a more involved change. Maybe we can leave it for a follow up? Thanks, -Stolee
Derrick Stolee <derrickstolee@github.com> writes: >> member so we need an on-demand allocation. CALLOC_ARRAY() instead >> of xcalloc(), when we know we are creating one element and not an >> array of elements whose size happens to be one, is not wrong but it >> does look strange. Was there a reason why we avoid xcalloc()? > > I think I've been using CALLOC_ARRAY(..., 1) over "... = xcalloc()" > for simplicity's sake for a while. I see quite a few across the > codebase, too, but I can swap the usage here if you feel that is > important. Not at all. I think it was just me who was confused; CALLOC_ARRAY() vs xcalloc() is not confusing. Both are capable of and meant for allocating an array of these elements, and use of it for a single element non-array is the same either way. Nothing to complain about. >> they may or may not be prepared to take. This "we get rid of a >> global struct and replace it with an on-demand allocated structure, >> pointer to which is stored in the revs structure" rewrite somehow >> makes me nervous. > > I think the main idea is that the filter being NULL indicates "no > filter is used. Do a full object walk." If we use a static struct, > then we need to instead use revs->filter.filter_spec.nr, but that > is already being used as a BUG() statement: Thanks. My observation was primarily that it looked deviating from the original code but that is not an objection unless the original was without room for improvement. And in fact in this case, I think the global variable that was a static struct should have been a global variable that is a pointer to a struct which is NULL unless the filtering capability is being used. So in that sense, the conversion done in this series is an improvement and going in the right direction. > While we _could_ make that switch to using a static struct and > change our checks to allow empty specs, that would be a more > involved change. Maybe we can leave it for a follow up? No, there is no need to. A pointer that is NULL when unused is the right thing to do. Thanks.
diff --git a/builtin/rev-list.c b/builtin/rev-list.c index 777558e9b06..6f2b91d304e 100644 --- a/builtin/rev-list.c +++ b/builtin/rev-list.c @@ -62,7 +62,6 @@ static const char rev_list_usage[] = static struct progress *progress; static unsigned progress_counter; -static struct list_objects_filter_options filter_options; static struct oidset omitted_objects; static int arg_print_omitted; /* print objects omitted by filter */ @@ -400,7 +399,6 @@ static inline int parse_missing_action_value(const char *value) } static int try_bitmap_count(struct rev_info *revs, - struct list_objects_filter_options *filter, int filter_provided_objects) { uint32_t commit_count = 0, @@ -436,7 +434,8 @@ static int try_bitmap_count(struct rev_info *revs, */ max_count = revs->max_count; - bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects); + bitmap_git = prepare_bitmap_walk(revs, revs->filter, + filter_provided_objects); if (!bitmap_git) return -1; @@ -453,7 +452,6 @@ static int try_bitmap_count(struct rev_info *revs, } static int try_bitmap_traversal(struct rev_info *revs, - struct list_objects_filter_options *filter, int filter_provided_objects) { struct bitmap_index *bitmap_git; @@ -465,7 +463,8 @@ static int try_bitmap_traversal(struct rev_info *revs, if (revs->max_count >= 0) return -1; - bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects); + bitmap_git = prepare_bitmap_walk(revs, revs->filter, + filter_provided_objects); if (!bitmap_git) return -1; @@ -475,7 +474,6 @@ static int try_bitmap_traversal(struct rev_info *revs, } static int try_bitmap_disk_usage(struct rev_info *revs, - struct list_objects_filter_options *filter, int filter_provided_objects) { struct bitmap_index *bitmap_git; @@ -483,7 +481,7 @@ static int try_bitmap_disk_usage(struct rev_info *revs, if (!show_disk_usage) return -1; - bitmap_git = prepare_bitmap_walk(revs, filter, filter_provided_objects); + bitmap_git = prepare_bitmap_walk(revs, revs->filter, filter_provided_objects); if (!bitmap_git) return -1; @@ -597,13 +595,17 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) } if (skip_prefix(arg, ("--" CL_ARG__FILTER "="), &arg)) { - parse_list_objects_filter(&filter_options, arg); - if (filter_options.choice && !revs.blob_objects) + if (!revs.filter) + CALLOC_ARRAY(revs.filter, 1); + parse_list_objects_filter(revs.filter, arg); + if (revs.filter->choice && !revs.blob_objects) die(_("object filtering requires --objects")); continue; } if (!strcmp(arg, ("--no-" CL_ARG__FILTER))) { - list_objects_filter_set_no_filter(&filter_options); + if (!revs.filter) + CALLOC_ARRAY(revs.filter, 1); + list_objects_filter_set_no_filter(revs.filter); continue; } if (!strcmp(arg, "--filter-provided-objects")) { @@ -688,11 +690,11 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) progress = start_delayed_progress(show_progress, 0); if (use_bitmap_index) { - if (!try_bitmap_count(&revs, &filter_options, filter_provided_objects)) + if (!try_bitmap_count(&revs, filter_provided_objects)) return 0; - if (!try_bitmap_disk_usage(&revs, &filter_options, filter_provided_objects)) + if (!try_bitmap_disk_usage(&revs, filter_provided_objects)) return 0; - if (!try_bitmap_traversal(&revs, &filter_options, filter_provided_objects)) + if (!try_bitmap_traversal(&revs, filter_provided_objects)) return 0; } @@ -733,7 +735,7 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix) oidset_init(&missing_objects, DEFAULT_OIDSET_SIZE); traverse_commit_list_filtered( - &filter_options, &revs, show_commit, show_object, &info, + revs.filter, &revs, show_commit, show_object, &info, (arg_print_omitted ? &omitted_objects : NULL)); if (arg_print_omitted) { diff --git a/revision.h b/revision.h index 3c58c18c63a..1ddb73ab82e 100644 --- a/revision.h +++ b/revision.h @@ -81,6 +81,7 @@ struct rev_cmdline_info { struct oidset; struct topo_walk_info; +struct list_objects_filter_options; struct rev_info { /* Starting list */ @@ -94,6 +95,9 @@ struct rev_info { /* The end-points specified by the end user */ struct rev_cmdline_info cmdline; + /* Object filter options. NULL for no filtering. */ + struct list_objects_filter_options *filter; + /* excluding from --branches, --refs, etc. expansion */ struct string_list *ref_excludes;