Re: [PATCH] refs/ref-cache: stop seeking into empty directories

[Patrick Steinhardt <ps@pks.im>]

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