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

From: Karthik Nayak <karthik.188@gmail.com>
Date: Jul 3, 2025, 10:02 UTC
Message-ID: <CAOLa=ZQvY6+fU-xDTH5jnrZxAsx2LxB2t-2QDu-8WpNrFpwjHQ@mail.gmail.com>
In-Reply-To: <aGYbZcXP2voT2IYE@pks.im>

Patrick Steinhardt <ps@pks.im> writes:

Show 30 quoted lines

> On Tue, Jul 01, 2025 at 05:03:30PM +0200, Karthik Nayak wrote:
>> diff --git a/Documentation/git-for-each-ref.adoc b/Documentation/git-for-each-ref.adoc
>> index 5ef89fc0fe..4bf7c66b8c 100644
>> --- a/Documentation/git-for-each-ref.adoc
>> +++ b/Documentation/git-for-each-ref.adoc
>> @@ -14,7 +14,7 @@ SYNOPSIS
>>  		   [--points-at=<object>]
>>  		   [--merged[=<object>]] [--no-merged[=<object>]]
>>  		   [--contains[=<object>]] [--no-contains[=<object>]]
>> -		   [--exclude=<pattern> ...]
>> +		   [--exclude=<pattern> ...] [--skip-until=<pattern>]
>>
>>  DESCRIPTION
>>  -----------
>> @@ -108,6 +108,10 @@ TAB %(refname)`.
>>  --include-root-refs::
>>  	List root refs (HEAD and pseudorefs) apart from regular refs.
>>
>> +--skip-until::
>> +    Skip references up to the specified pattern. Cannot be used with
>> +    general pattern matching.
>> +
>>  FIELD NAMES
>>  -----------
>>
>
> Is it "up to and including the specified pattern" or "up to but
> excluding the specified pattern"? It would help to make it very explicit
> whether the pattern itself would be yielded or not.
>

It is "up to and including", will modify to make this more clearer.

Show 35 quoted lines

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

Show 15 quoted lines

>> +	if (ret)
>> +		return ret;
>> +
>> +	return do_for_each_ref_iterator(iter, cb, cb_data);
>>  }
>>
>>  /*
>> @@ -3200,6 +3210,8 @@ static int do_filter_refs(struct ref_filter *filter, unsigned int type, each_ref
>>  	if (!filter->kind)
>>  		die("filter_refs: invalid type");
>>  	else {
>
> The `if` branch now needs to be updated to have curly braces, as well.
>
> Patrick

Yes, will add.

Thanks for the review!