Patrick Steinhardt <ps@xxxxxx> writes:
> 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.
>> 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.
>> + 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!
Attachment:
signature.asc
Description: PGP signature
