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

[Christian Couder <christian.couder@gmail.com>]

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.

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

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

> +       test_cmp expect actual
> +'