Christian Couder [off-list ref] writes:

On Fri, Jul 11, 2025 at 6:21 PM Karthik Nayak [off-list ref] wrote:

quoted

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

quoted

 /*

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

quoted

+               const char *prefix = NULL;
+

[...]

quoted

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

quoted

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