Re: [PATCH v4 4/4] for-each-ref: introduce a '--start-after' option

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 15, 2025, 08:42 UTC
Message-ID: <CAOLa=ZSyVh1cWZL=F8Gk-URh7p6GTnPpOpWjGwCbwYX=a+-TNQ@mail.gmail.com>
In-Reply-To: <CAP8UFD1wRxZDCRC76VuuA8_rpNn__TQnL9RnNumCE33wAjSrMQ@mail.gmail.com>

Christian Couder <christian.couder@gmail.com> writes:

Show 68 quoted lines

> On Fri, Jul 11, 2025 at 6:21 PM Karthik Nayak <karthik.188@gmail.com> 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.

Show 14 quoted lines

>>  /*
>> @@ -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.

Show 24 quoted lines

>> +               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.
>

Show 21 quoted lines

>> +       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.