Re: [PATCH v2 01/17] refs/files: simplify iterating through root refs

> When iterating through root refs we first need to determine the
> directory in which the refs live. This is done by retrieving the root of
> the loose refs via `refs->loose->root->name`, and putting it through
> `files_ref_path()` to derive the final path.
>
> This is somewhat redundant though: the root name of the loose files
> cache is always going to be the empty string. As such, we always end up
> passing that empty string to `files_ref_path()` as the ref hierarchy we
> want to start. And this actually makes sense: `files_ref_path()` already
> computes the location of the root directory, so of course we need to
> pass the empty string for the ref hierarchy itself. So going via the
> loose ref cache to figure out that the root of a ref hierarchy is empty
> is only causing confusion.
>
> But next to the added confusion, it can also lead to a segfault. The
> loose ref cache is populated lazily, so it may not always be set. It
> seems to be sheer luck that this is a condition we do not currently hit.
> The right thing to do would be to call `get_loose_ref_cache()`, which
> knows to populate the cache if required.
>
> Simplify the code and fix the potential segfault by simply removing the
> indirection via the loose ref cache completely.
>
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  refs/files-backend.c | 11 +++--------
>  1 file changed, 3 insertions(+), 8 deletions(-)
>
> diff --git a/refs/files-backend.c b/refs/files-backend.c
> index 6f6f76a8d8..297739f203 100644
> --- a/refs/files-backend.c
> +++ b/refs/files-backend.c
> @@ -354,13 +354,11 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			     void *cb_data)
>  {
>  	struct strbuf path = STRBUF_INIT, refname = STRBUF_INIT;
> -	const char *dirname = refs->loose->root->name;
>  	struct dirent *de;
> -	size_t dirnamelen;
>  	int ret;
>  	DIR *d;
>
> -	files_ref_path(refs, &path, dirname);
> +	files_ref_path(refs, &path, "");
>

>  	d = opendir(path.buf);
>  	if (!d) {
> @@ -368,9 +366,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  		return -1;
>  	}
>
> -	strbuf_addstr(&refname, dirname);
> -	dirnamelen = refname.len;
> -

>  	while ((de = readdir(d)) != NULL) {
>  		unsigned char dtype;
>
> @@ -378,6 +373,8 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			continue;
>  		if (ends_with(de->d_name, ".lock"))
>  			continue;
> +
> +		strbuf_reset(&refname);
>  		strbuf_addstr(&refname, de->d_name);
>
>  		dtype = get_dtype(de, &path, 1);
> @@ -386,8 +383,6 @@ static int for_each_root_ref(struct files_ref_store *refs,
>  			if (ret)
>  				goto done;
>  		}
> -
> -		strbuf_setlen(&refname, dirnamelen);

>  	}
>
>  	ret = 0;
>
> --
> 2.52.0.590.g1f87b77810.dirty