From: Karthik Nayak Date: Mon, 12 Jan 2026 09:56:03 GMT Subject: Re: [PATCH v2 01/17] refs/files: simplify iterating through root refs Message-ID: In-Reply-To: <20260112-pks-refs-verify-fixes-v2-1-2e9e453bd6c3@pks.im> Patrick Steinhardt writes: > 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 > --- > 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, ""); > Since refs->loose->root->name is always `""`, we directly pass that instead. Makes sense. > 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; > - This too is unnecessary since the len here is 0. > 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); Earlier we were setting the length to 0, but thats the same as strbuf_reset(), so we do that now. This gets rid of the `dirnamelen` varible. Looks good. > } > > ret = 0; > > -- > 2.52.0.590.g1f87b77810.dirty