| Message ID | 20200918004909.32474-2-alipman88@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Commit | a1b19aa5d43c46fc570f6946ab5ad001dccc8cdf |
| Headers | show |
| Series | ref-filter: merged & no-merged touch-ups | expand |
On Thu, Sep 17, 2020 at 8:49 PM Aaron Lipman <alipman88@gmail.com> wrote: > ref-filter: refactor do_merge_filter() > > Rename to reach_filter() to be more descriptive. > > Separate parameters to explicitly identify what data is used by the > function, instead of passing an entire ref_filter_cbdata struct. > > Rename and move associated preprocessor macros from header to source > file, as they're only used internally in a single location. One thing that this commit message lacks is a high-level explanation of why these changes are being made. One possible rewrite would be: ref-filter: make internal reachable-filter API more precise The internal reachable-filter API is a bit loose and imprecise; it also bleeds unnecessarily into the public header. Tighten the API by: * renaming do_merge_filter() to reach_filter() * separating parameters to explicitly identify what data is used by the function instead of passing an entire ref_filter_cbdata struct * renaming and moving internal constants from header to source file > Signed-off-by: Aaron Lipman <alipman88@gmail.com> > --- > diff --git a/ref-filter.c b/ref-filter.c > @@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array) > +static void reach_filter(struct ref_array *array, > + struct commit_list *check_reachable, > + int include_reached) > { > [...] > + if (!check_reachable) > return; I would probably drop this conditional return altogether since it incorrectly conveys to the reader that it is legal to call this function with NULL for 'check_reachable', whereas, in reality, it would be pointless to do so. If this function were public, and the possible list of callers open-ended, then it might be reasonable for it to do this to alert callers early of their error: if (!check_reachable) BUG("check_reachable must not be NULL"); However, as this function is private, and the set of callers can be precisely determined, it's not necessary to make the check at all. If anyone adds a new call in this file which incorrectly passes NULL, they will discover the error quickly enough when the program crashes at the new call site. If you were to drop this conditional, then that change would deserve another bullet point in the commit message.
On Fri, Sep 18, 2020 at 1:03 AM Eric Sunshine <sunshine@sunshineco.com> wrote: > > + if (!check_reachable) > > return; > > I would probably drop this conditional return altogether since it > incorrectly conveys to the reader that it is legal to call this > function with NULL for 'check_reachable', whereas, in reality, it > would be pointless to do so. [...] Nevermind. I'm an idiot. I forgot that filter->reachable_from and filter->unreachable_from may indeed validly be NULL.
Eric Sunshine <sunshine@sunshineco.com> writes: > One thing that this commit message lacks is a high-level explanation > of why these changes are being made. One possible rewrite would be: > > ref-filter: make internal reachable-filter API more precise > > The internal reachable-filter API is a bit loose and imprecise; it > also bleeds unnecessarily into the public header. Tighten the API > by: > > * renaming do_merge_filter() to reach_filter() > > * separating parameters to explicitly identify what data is used > by the function instead of passing an entire ref_filter_cbdata > struct > > * renaming and moving internal constants from header to source > file Sounds good. Aaron? >> +static void reach_filter(struct ref_array *array, >> + struct commit_list *check_reachable, >> + int include_reached) >> { >> [...] >> + if (!check_reachable) >> return; > > I would probably drop this conditional return altogether ... As you said downthread, this part is fine as-is.
diff --git a/ref-filter.c b/ref-filter.c index 785785a757..5550a0d34c 100644 --- a/ref-filter.c +++ b/ref-filter.c @@ -2231,19 +2231,18 @@ void ref_array_clear(struct ref_array *array) } } -static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable) +#define EXCLUDE_REACHED 0 +#define INCLUDE_REACHED 1 +static void reach_filter(struct ref_array *array, + struct commit_list *check_reachable, + int include_reached) { struct rev_info revs; int i, old_nr; - struct ref_array *array = ref_cbdata->array; struct commit **to_clear = xcalloc(sizeof(struct commit *), array->nr); - struct commit_list *rl; + struct commit_list *cr; - struct commit_list *check_reachable_list = reachable ? - ref_cbdata->filter->reachable_from : - ref_cbdata->filter->unreachable_from; - - if (!check_reachable_list) + if (!check_reachable) return; repo_init_revisions(the_repository, &revs, NULL); @@ -2254,8 +2253,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable) to_clear[i] = item->commit; } - for (rl = check_reachable_list; rl; rl = rl->next) { - struct commit *merge_commit = rl->item; + for (cr = check_reachable; cr; cr = cr->next) { + struct commit *merge_commit = cr->item; merge_commit->object.flags |= UNINTERESTING; add_pending_object(&revs, &merge_commit->object, ""); } @@ -2273,7 +2272,7 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable) int is_merged = !!(commit->object.flags & UNINTERESTING); - if (is_merged == reachable) + if (is_merged == include_reached) array->items[array->nr++] = array->items[i]; else free_array_item(item); @@ -2281,8 +2280,8 @@ static void do_merge_filter(struct ref_filter_cbdata *ref_cbdata, int reachable) clear_commit_marks_many(old_nr, to_clear, ALL_REV_FLAGS); - while (check_reachable_list) { - struct commit *merge_commit = pop_commit(&check_reachable_list); + while (check_reachable) { + struct commit *merge_commit = pop_commit(&check_reachable); clear_commit_marks(merge_commit, ALL_REV_FLAGS); } @@ -2337,8 +2336,8 @@ int filter_refs(struct ref_array *array, struct ref_filter *filter, unsigned int clear_contains_cache(&ref_cbdata.no_contains_cache); /* Filters that need revision walking */ - do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_REACHABLE); - do_merge_filter(&ref_cbdata, DO_MERGE_FILTER_UNREACHABLE); + reach_filter(array, filter->reachable_from, INCLUDE_REACHED); + reach_filter(array, filter->unreachable_from, EXCLUDE_REACHED); return ret; } diff --git a/ref-filter.h b/ref-filter.h index 2d13928455..feaef4a8fd 100644 --- a/ref-filter.h +++ b/ref-filter.h @@ -23,9 +23,6 @@ #define FILTER_REFS_DETACHED_HEAD 0x0020 #define FILTER_REFS_KIND_MASK (FILTER_REFS_ALL | FILTER_REFS_DETACHED_HEAD) -#define DO_MERGE_FILTER_UNREACHABLE 0 -#define DO_MERGE_FILTER_REACHABLE 1 - struct atom_value; struct ref_sorting {
Rename to reach_filter() to be more descriptive. Separate parameters to explicitly identify what data is used by the function, instead of passing an entire ref_filter_cbdata struct. Rename and move associated preprocessor macros from header to source file, as they're only used internally in a single location. Signed-off-by: Aaron Lipman <alipman88@gmail.com> --- ref-filter.c | 29 ++++++++++++++--------------- ref-filter.h | 3 --- 2 files changed, 14 insertions(+), 18 deletions(-)