Re: [PATCH 4/4] for-each-ref: introduce a '--skip-until' option

[Patrick Steinhardt <ps@pks.im>]

On Thu, Jul 03, 2025 at 03:02:03AM -0700, Karthik Nayak wrote:
> Patrick Steinhardt <ps@pks.im> writes:
> > On Tue, Jul 01, 2025 at 05:03:30PM +0200, Karthik Nayak wrote:
> >> diff --git a/ref-filter.c b/ref-filter.c
> >> index 7a274633cf..9d0255d5db 100644
> >> --- a/ref-filter.c
> >> +++ b/ref-filter.c
> >> @@ -2714,20 +2716,28 @@ 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->seek)
> >> +		ret = ref_iterator_seek(iter, filter->seek, 0);
> >
> > Hm, this interface is somewhat weird now, as we have a split in what the
> > prefix-string meeks when creating the iterator and seeking it. I think
> > we should align those two functions.
> >
> 
> The `refs_ref_iterator_begin()` takes in a `prefix` string, which sets
> the prefix.
> 
> The `ref_iterator_seek()` takes in a `seek` string, but a flag allows it
> also set the prefix.
> 
> I think this is okay since the naming matches what it does.
> 
> The alternate would be to `refs_ref_iterator_begin()` to also take in a
> `seek` string with a flag to also set the prefix. What do you think? I'm
> okay either ways.

I just think that the interface is a bit confusing. It's weird that the
needle that we're seeking for may or may not be used to update internal
state, and that this is inconsistent with the similar fields that you
pass to the iterator when creating it. So after seeking it sometimes
acts like you have created a new iterator with the needle, sometimes it
does not becaus we retain internal state. This kind of inconsistency
invites mistakes.

Patrick