Re: [PATCH v5 00/12] refs: ref storage migrations

From: Jeff King <peff@peff.net>
Date: Jun 6, 2024, 07:06 UTC
Message-ID: <20240606070619.GD646308@coredump.intra.peff.net>
In-Reply-To: <cover.1717649802.git.ps@pks.im>

On Thu, Jun 06, 2024 at 07:28:52AM +0200, Patrick Steinhardt wrote:

Show 15 quoted lines

> the ref storage migration was merged to `next`, but got reverted due to
> some additional findings by Peff and/or Coverity.
> 
> Changes compared to v4:
> 
>   - Adapt comment of `ref_store_init()` to the new parameter.
> 
>   - Fix use of an uninitialized return value in `for_each_root_ref()`.
> 
>   - Fix overwrite of ret code in `files_ref_store_remove_on_disk()`.
> 
>   - Adapt an error message to more clearly point out that deletion of
>     "refs/" directory failed in `reftable_be_remove_on_disk()`.
> 
>   - Fix a leak when `mkdtemp()` fails.

These all looked good to me (though I did not carefully read the original topic, so was just looking at the parts I mentioned earlier).

Show 10 quoted lines

>  6:  f7577a0ab3 !  6:  86cf0c84b1 refs/files: extract function to iterate through root refs
>     @@ refs/files-backend.c: static void add_root_refs(struct files_ref_store *refs,
>       		strbuf_setlen(&refname, dirnamelen);
>       	}
>      +
>     ++	ret = 0;
>     ++
>      +done:
>       	strbuf_release(&refname);
>       	strbuf_release(&path);

Since the context doesn't show much, I wondered whether there was any case where we'd overwrite an earlier "ret" here. But nope, we always jump to "done" after finding "ret" contains a non-zero value. So setting it to zero here is the right thing.

-Peff