patchrefs/ref-cache: stop seeking into empty directories

> The 'cache_ref_iterator_seek()' function is used to seek the
> `ref_iterator` to the desired reference in the ref-cache mechanism. We
> use the seeking functionality to implement the '--start-after' flag in
> 'git-for-each-ref(1)'.
> 
> When using the files-backend with packed-refs, it is possible that some
> of the refs directories are empty. For e.g. just after repacking, the
> 'refs/heads' directory would be empty. The ref-cache seek mechanism
> doesn't take this into consideration, causing SEGFAULT as we try to
> access entries within the directory.

> Fix this by breaking out of the
> loop when we enter an empty directory.
> 
> Add tests which simulate this behavior and also provide coverage over
> using the feature over packed-refs.

> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
> ---
>  refs/ref-cache.c               |  3 ++
>  t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++
>  2 files changed, 68 insertions(+)
> 
> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index c180e0aad7..8a260028ec 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>  			slash = strchr(slash, '/');
>  			len = slash ? (size_t)(slash - refname) : strlen(refname);
>  
> +			if (dir->nr == 0)
> +				break;

>  			for (idx = 0; idx < dir->nr; idx++) {
>  				cmp = strncmp(refname, dir->entries[idx]->name, len);
>  				if (cmp <= 0)
> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
> index 9b80ea1e3b..d14567cb62 100755
> --- a/t/t6302-for-each-ref-filter.sh
> +++ b/t/t6302-for-each-ref-filter.sh
> @@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '
>  	test_cmp expect actual
>  '
>  
> +test_expect_success 'start after with packed refs' '
> +	test_when_finished "rm -rf repo" &&
> +	git init repo &&
> +	(
> +		cd repo &&
> +		test_commit default &&
> +
> +		git update-ref --stdin <<-EOF &&

> On Wed, Sep 24, 2025 at 09:55:51AM +0200, Karthik Nayak wrote:
>> The 'cache_ref_iterator_seek()' function is used to seek the
>> `ref_iterator` to the desired reference in the ref-cache mechanism. We
>> use the seeking functionality to implement the '--start-after' flag in
>> 'git-for-each-ref(1)'.
>>
>> When using the files-backend with packed-refs, it is possible that some
>> of the refs directories are empty. For e.g. just after repacking, the
>> 'refs/heads' directory would be empty. The ref-cache seek mechanism
>> doesn't take this into consideration, causing SEGFAULT as we try to
>> access entries within the directory.
>
> Why do we even try to access any entry in an empty directory? We have
> `dir->nr`, so shouldn't we check that `idx < dir->nr` every time we try
> to deref an entry?
>
> In other words, I wonder whether we fix a symptom of a missing bounds
> check instead of addressing that missing bounds check as the root cause
> directly.
>

>> Fix this by breaking out of the
>> loop when we enter an empty directory.
>>
>> Add tests which simulate this behavior and also provide coverage over
>> using the feature over packed-refs.
>
> Nit: the commit subject might be adjusted to mention the symptom we're
> fixing rather than what we're doing in the commit. At least, it feels
> like the segfault is the more important thing to talk about here.
>

>> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
>> ---
>>  refs/ref-cache.c               |  3 ++
>>  t/t6302-for-each-ref-filter.sh | 65 ++++++++++++++++++++++++++++++++++++++++++
>>  2 files changed, 68 insertions(+)
>>
>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>> index c180e0aad7..8a260028ec 100644
>> --- a/refs/ref-cache.c
>> +++ b/refs/ref-cache.c
>> @@ -507,6 +507,9 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>  			slash = strchr(slash, '/');
>>  			len = slash ? (size_t)(slash - refname) : strlen(refname);
>>
>> +			if (dir->nr == 0)
>> +				break;
>
> Can't we break before sorting already? Avoids a couple of no-op changes.
>

>>  			for (idx = 0; idx < dir->nr; idx++) {
>>  				cmp = strncmp(refname, dir->entries[idx]->name, len);
>>  				if (cmp <= 0)
>> diff --git a/t/t6302-for-each-ref-filter.sh b/t/t6302-for-each-ref-filter.sh
>> index 9b80ea1e3b..d14567cb62 100755
>> --- a/t/t6302-for-each-ref-filter.sh
>> +++ b/t/t6302-for-each-ref-filter.sh
>> @@ -754,4 +754,69 @@ test_expect_success 'start after used with custom sort order' '
>>  	test_cmp expect actual
>>  '
>>
>> +test_expect_success 'start after with packed refs' '
>> +	test_when_finished "rm -rf repo" &&
>> +	git init repo &&
>> +	(
>> +		cd repo &&
>> +		test_commit default &&
>> +
>> +		git update-ref --stdin <<-EOF &&
>
> s/EOF/\EOF/
>
> The same is true for the other test.
>
> Patrick

> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
> index c180e0aad7..e5e5df16d8 100644
> --- a/refs/ref-cache.c
> +++ b/refs/ref-cache.c
> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>  				 */
>  				break;
>  			}
> -		} while (slash);
> +		} while (slash && dir->nr);
>  	}

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>> index c180e0aad7..e5e5df16d8 100644
>> --- a/refs/ref-cache.c
>> +++ b/refs/ref-cache.c
>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>  				 */
>>  				break;
>>  			}
>> -		} while (slash);
>> +		} while (slash && dir->nr);
>>  	}
>
> This is at the tail of a "do { ... } while (...);" loop, but inside
> the loop I see this construct:
>
> 			for (idx = 0; idx < dir->nr; idx++) {
> 				cmp = strncmp(refname, dir->entries[idx]->name, len);
> 				if (cmp <= 0)
> 					break;
> 			}
> 			/* don't overflow the index */
> 			idx = idx >= dir->nr ? dir->nr - 1 : idx;
>
> i.e., if we scan all the dir->entries[] elements in the innter loop
> and did not find any hit, idx would become dir->nr and this inner
> loop runs to the end.  If (dir->nr == 0), then ?: operator [*] would
> become the idx = (dir->nr - 1);  And that idx is used for a while
> before we get to this "while (slash && dir->nr)".
>

> And then tha tis used like this.
>
> 			if (slash)
> 				slash = slash + 1;
>
> 			level->index = idx;
> 			if (dir->entries[idx]->flag & REF_DIR) {
> 				...
>
> IOW, isn't this check a bit too late?  I wonder if we can leave at
> the beginning of the outer loop, even before sort_ref_dir(dir), when
> dir->nr is zero, or something?
>
>

>
> [Side note]
>
>  * I found the problematic ?: extremely hard to read, given its
>    contrast with the terminating condnition of for loop.  If you
>    apply the discipline to keep the textual order match the actual
>    order, i.e.
>
> 	for (idx = 0; idx < dir->nr; idx++)
> 		...;
>
> 	idx = dir->nr <= idx ? dir->nr - 1 : idx;
>
>    people would have spotted this more easily.
>
>    But perhaps that may be just me.  Anyway...

> Junio C Hamano <gitster@pobox.com> writes:
>
>> Karthik Nayak <karthik.188@gmail.com> writes:
>>
>>> diff --git a/refs/ref-cache.c b/refs/ref-cache.c
>>> index c180e0aad7..e5e5df16d8 100644
>>> --- a/refs/ref-cache.c
>>> +++ b/refs/ref-cache.c
>>> @@ -539,7 +539,7 @@ static int cache_ref_iterator_seek(struct ref_iterator *ref_iterator,
>>>  				 */
>>>  				break;
>>>  			}
>>> -		} while (slash);
>>> +		} while (slash && dir->nr);
>>>  	}
>>
>> This is at the tail of a "do { ... } while (...);" loop, but inside
>> the loop I see this construct:
>>
>> 			for (idx = 0; idx < dir->nr; idx++) {
>> 				cmp = strncmp(refname, dir->entries[idx]->name, len);
>> 				if (cmp <= 0)
>> 					break;
>> 			}
>> 			/* don't overflow the index */
>> 			idx = idx >= dir->nr ? dir->nr - 1 : idx;
>>
>> i.e., if we scan all the dir->entries[] elements in the innter loop
>> and did not find any hit, idx would become dir->nr and this inner
>> loop runs to the end.  If (dir->nr == 0), then ?: operator [*] would
>> become the idx = (dir->nr - 1);  And that idx is used for a while
>> before we get to this "while (slash && dir->nr)".
>>
>
> This wouldn't happen because before the loop starts, we set:
>
>    dir = get_ref_dir(iter->cache->root);
>
> So, `dir` will always have an entry (i.e. 'refs') in the first
> iteration, after that, the checks in the while section  of the 'do {}
> while (...)' loop will kick in.
>
>> And then tha tis used like this.
>>
>> 			if (slash)
>> 				slash = slash + 1;
>>
>> 			level->index = idx;
>> 			if (dir->entries[idx]->flag & REF_DIR) {
>> 				...
>>
>> IOW, isn't this check a bit too late?  I wonder if we can leave at
>> the beginning of the outer loop, even before sort_ref_dir(dir), when
>> dir->nr is zero, or something?
>>
>>
>
> We could add it there too. I can't see the merit of one over the other.
> But if you see it being more readable. I'll happily make that change.
>

> This wouldn't happen because before the loop starts, we set:
>
>    dir = get_ref_dir(iter->cache->root);
>
> So, `dir` will always have an entry (i.e. 'refs') in the first
> iteration, after that, the checks in the while section  of the 'do {}
> while (...)' loop will kick in.

>> I wonder if we can leave at
>> the beginning of the outer loop, even before sort_ref_dir(dir), when
>> dir->nr is zero, or something?
>
> We could add it there too. I can't see the merit of one over the other.

> Karthik Nayak <karthik.188@gmail.com> writes:
>
>> This wouldn't happen because before the loop starts, we set:
>>
>>    dir = get_ref_dir(iter->cache->root);
>>
>> So, `dir` will always have an entry (i.e. 'refs') in the first
>> iteration, after that, the checks in the while section  of the 'do {}
>> while (...)' loop will kick in.
>
> Ah, OK.  So we know dir->nr != 0 when the loop is entered for the
> first time.  We may descend into a new subdirectory and at that
> point we might hit dir->nr == 0 and that is why we want to catch it
> at the end of "do {} while ()".  Makes sense.
>
>>> I wonder if we can leave at
>>> the beginning of the outer loop, even before sort_ref_dir(dir), when
>>> dir->nr is zero, or something?
>>
>> We could add it there too. I can't see the merit of one over the other.
>
> No, I don't, either.  As long as we know dir->nr != 0 upon the entry
> into the loop, the check at the tail end in while() is sufficient.
>
> Thanks for clarifying.  I wonder if a bit of update to the log
> message would help future readers of the code, though.
>
>     When using the files-backend with packed-refs, it is possible that some
>     of the refs directories are empty. For e.g. just after repacking, the
>     'refs/heads' directory would be empty. The ref-cache seek mechanism,
>     doesn't take this into consideration when descending into a subdirectory,
>     and makes an out of bounds access, causing SEGFAULT as we try to
>     access entries within the directory. Fix this by breaking out of the
>     loop when we enter an empty directory.
>
> or something?  I dunno.
>
> Thanks.