Christian Couder <christian.couder@xxxxxxxxx> writes:
> On Fri, Jul 11, 2025 at 6:21 PM Karthik Nayak <karthik.188@xxxxxxxxx> wrote:
>
>> /*
>> * This is the same as for_each_fullref_in(), but it tries to iterate
>> * only over the patterns we'll care about. Note that it _doesn't_ do a full
>> @@ -2692,10 +2710,13 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
>> each_ref_fn cb,
>> void *cb_data)
>> {
>> + struct ref_iterator *iter;
>> + int flags = 0, ret = 0;
>> +
>> if (filter->kind & FILTER_REFS_ROOT_REFS) {
>> /* In this case, we want to print all refs including root refs. */
>> - return refs_for_each_include_root_refs(get_main_ref_store(the_repository),
>> - cb, cb_data);
>> + flags |= DO_FOR_EACH_INCLUDE_ROOT_REFS;
>> + goto non_prefix_iter;
>> }
>>
>> if (!filter->match_as_path) {
>> @@ -2704,8 +2725,7 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
>> * prefixes like "refs/heads/" etc. are stripped off,
>> * so we have to look at everything:
>> */
>> - return refs_for_each_fullref_in(get_main_ref_store(the_repository),
>> - "", NULL, cb, cb_data);
>> + goto non_prefix_iter;
>> }
>>
>> if (filter->ignore_case) {
>> @@ -2714,20 +2734,29 @@ static int for_each_fullref_in_pattern(struct ref_filter *filter,
>> * so just return everything and let the caller
>> * sort it out.
>> */
>> - return refs_for_each_fullref_in(get_main_ref_store(the_repository),
>> - "", NULL, cb, cb_data);
>> + goto non_prefix_iter;
>> }
>>
>> if (!filter->name_patterns[0]) {
>> /* no patterns; we have to look at everything */
>> - return refs_for_each_fullref_in(get_main_ref_store(the_repository),
>> - "", filter->exclude.v, cb, cb_data);
>> + goto non_prefix_iter;
>> }
>>
>> return refs_for_each_fullref_in_prefixes(get_main_ref_store(the_repository),
>> NULL, filter->name_patterns,
>> filter->exclude.v,
>> cb, cb_data);
>> +
>> +non_prefix_iter:
>> + iter = refs_ref_iterator_begin(get_main_ref_store(the_repository), "",
>> + NULL, 0, flags);
>> + if (filter->start_after)
>> + ret = start_ref_iterator_after(iter, filter->start_after);
>> +
>> + if (ret)
>> + return ret;
>> +
>> + return do_for_each_ref_iterator(iter, cb, cb_data);
>> }
>
> Nit: I wonder if what is under the 'non_prefix_iter' label could be in
> a new function and instead of `goto non_prefix_iter` we could return
> the result of the new function.
>
Yeah, that would work too. Let me do that and make it nicer! Thanks for
the suggestion.
>> /*
>> @@ -3197,9 +3226,11 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
>> init_contains_cache(&filter->internal.no_contains_cache);
>>
>> /* Simple per-ref filtering */
>> - if (!filter->kind)
>> + if (!filter->kind) {
>> die("filter_refs: invalid type");
>> - else {
>> + } else {
>
> Nit: the `else` could be removed altogether here, but maybe that
> should be done in a preparatory patch.
>
Indeed, since I plan to re-roll with the changes you've suggested, I
will add this in too.
>> + const char *prefix = NULL;
>> +
>
> [...]
>
>> +test_expect_success 'start after with specific directory and trailing slash' '
>> + cat >expect <<-\EOF &&
>> + refs/odd/spot
>> + refs/tags/annotated-tag
>> + refs/tags/doubly-annotated-tag
>> + refs/tags/doubly-signed-tag
>> + refs/tags/foo1.10
>> + refs/tags/foo1.3
>> + refs/tags/foo1.6
>> + refs/tags/four
>> + refs/tags/one
>> + refs/tags/signed-tag
>> + refs/tags/three
>> + refs/tags/two
>> + EOF
>> + git for-each-ref --format="%(refname)" --start-after=refs/lost >actual &&
>
> I don't see a trailing slash.
>
>> + test_cmp expect actual
>> +'
>> +
>> +test_expect_success 'start after, just behind a specific directory' '
>> + cat >expect <<-\EOF &&
>> + refs/odd/spot
>> + refs/tags/annotated-tag
>> + refs/tags/doubly-annotated-tag
>> + refs/tags/doubly-signed-tag
>> + refs/tags/foo1.10
>> + refs/tags/foo1.3
>> + refs/tags/foo1.6
>> + refs/tags/four
>> + refs/tags/one
>> + refs/tags/signed-tag
>> + refs/tags/three
>> + refs/tags/two
>> + EOF
>> + git for-each-ref --format="%(refname)" --start-after=refs/odd/ >actual &&
>
> Here there is a trailing slash though.
I think these tests would make more sense in the newer versions with the
values swapped. Let me do that.
Thanks Christian for the thorough review.
Attachment:
signature.asc
Description: PGP signature
