| Message ID | 20241117090842.GC3409496@coredump.intra.peff.net (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | b2a95dfd63e812dc4abe5750371f2f0596d2d063 |
| Headers | show |
| Series | [1/5] object-file: prefer array-of-bytes initializer for hash literals | expand |
On Sun, Nov 17, 2024 at 04:08:42AM -0500, Jeff King wrote: > diff --git a/object-file.c b/object-file.c > index b7c4fdcabd..5fadd470c1 100644 > --- a/object-file.c > +++ b/object-file.c > @@ -325,14 +325,13 @@ static struct cached_object { > } *cached_objects; > static int cached_object_nr, cached_object_alloc; > > -static struct cached_object empty_tree = { > - /* no oid needed; we'll look it up manually based on the_hash_algo */ > - .type = OBJ_TREE, > - .buf = "", > -}; > - > 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). Patrick 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, }, @@ -329,7 +329,7 @@ static struct cached_object empty_tree = { .buf = "", }; -static struct cached_object *find_cached_object(const struct object_id *oid) +static const struct cached_object *find_cached_object(const struct object_id *oid) { int i; struct cached_object *co = cached_objects; @@ -1627,7 +1627,7 @@ static int do_oid_object_info_extended(struct repository *r, struct object_info *oi, unsigned flags) { static struct object_info blank_oi = OBJECT_INFO_INIT; - struct cached_object *co; + const struct cached_object *co; struct pack_entry e; int rtype; const struct object_id *real = oid;
On Mon, Nov 18, 2024 at 08:40:34AM +0100, Patrick Steinhardt wrote: > > 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. > 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
diff --git a/object-file.c b/object-file.c index b7c4fdcabd..5fadd470c1 100644 --- a/object-file.c +++ b/object-file.c @@ -325,14 +325,13 @@ static struct cached_object { } *cached_objects; static int cached_object_nr, cached_object_alloc; -static struct cached_object empty_tree = { - /* no oid needed; we'll look it up manually based on the_hash_algo */ - .type = OBJ_TREE, - .buf = "", -}; - 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;
The fake empty_tree struct is a static global, but the only code that looks at it is find_cached_object(). The struct itself is a little odd, with an invalid "oid" field that is handled specially by that function. Since it's really just an implementation detail, let's move it to a static within the function. That future-proofs against other code trying to use it and seeing the weird oid value. Signed-off-by: Jeff King <peff@peff.net> --- object-file.c | 11 +++++------ 1 file changed, 5 insertions(+), 6 deletions(-)