Re: [PATCH 3/5] object-file: move empty_tree struct into find_cached_object()
- From
Jeff King <peff@peff.net>
- Date
- Nov 18, 2024, 09:17 UTC
- Message-ID
- <20241118091742.GB3984843@coredump.intra.peff.net>
- In-Reply-To
- <ZzrvciXuOfw_V6ox@pks.im>
On Mon, Nov 18, 2024 at 08:40:34AM +0100, Patrick Steinhardt wrote:
Show 14 quoted lines
> > static struct cached_object *find_cached_object(const struct object_id *oid)
> > {
> > + static struct cached_object empty_tree = {
> > + /* no oid needed; we'll look it up manually based on the_hash_algo */
> > + .type = OBJ_TREE,
> > + .buf = "",
> > + };
> > int i;
> > struct cached_object *co = cached_objects;
>
> I was wondering whether we want to also mark this as `const` so that no
> caller ever gets the idea of modifying the struct. Something like the
> below patch (which applies on "master", so it of course would have to
> adapt to your changes).This seems like a fairly unlikely bug to me, just because it would be weird for somebody to want to write to the response (whereas the other future-proofing was against somebody reading a private-ish value). Still, I agree that "const" is the right thing, and it's not hard to add it in to my series.
Show 13 quoted lines
> diff --git a/object-file.c b/object-file.c
> index b1a3463852..f15a3f6a5f 100644
> --- a/object-file.c
> +++ b/object-file.c
> @@ -321,7 +321,7 @@ static struct cached_object {
> } *cached_objects;
> static int cached_object_nr, cached_object_alloc;
>
> -static struct cached_object empty_tree = {
> +static const struct cached_object empty_tree = {
> .oid = {
> .hash = EMPTY_TREE_SHA1_BIN_LITERAL,
> },This hunk is technically not needed since we can implicitly cast from non-const to const when returning. I included it, though, along with making the iteration pointer in find_cached_object() const since that better represents the intent.
-Peff