Ævar Arnfjörð Bjarmason <avarab@xxxxxxxxx> writes:
> > @@ -1433,6 +1433,7 @@ static int loose_object_info(struct repository *r,
> > {
> > int status = 0;
> > unsigned long mapsize;
> > + const char *path = NULL;
>
> I think the NULL assignment here should either go, or it's incomplete.
[snip]
> So init-ing it didn't help us, but just helps to hide that potential
> (and much worse) bug.
Good catch. I'll remove the assignment.
> I think this change should also remove the existing "const char *path"
> in this function from the "if"'d scope omitted in this context.
>
> The C compiler won't care, but to the human reader it's easier to reason
> about not shadowing the variable now, for as it turns out no reason, as
> they're effectively independent.
Makes sense.
> > git_inflate_end(&stream);
> > cleanup:
> > munmap(map, mapsize);
> > @@ -1611,6 +1616,15 @@ static int do_oid_object_info_extended(struct repository *r,
> > continue;
> > }
> >
> > + if (flags & OBJECT_INFO_DIE_IF_CORRUPT) {
> > + const struct packed_git *p;
>
> Nit: add an extra \n here, between decls and code.
OK.
> > - if (oid_object_info_extended(r, oid, &oi, 0) < 0)
> > + if (oid_object_info_extended(r, oid, &oi,
> > + die_if_corrupt ? OBJECT_INFO_DIE_IF_CORRUPT : 0)
> > + < 0)
> > return NULL;
> > return content;
> > }
>
> This is a very odd coding style/wrapping, to not even end up with a line
> shorter than 79 characters. You can instead do:
>
> if (oid_object_info_extended(r, oid, &oi, die_if_corrupt
> ? OBJECT_INFO_DIE_IF_CORRUPT : 0) < 0)
>
> Which is in line with our usual style, and does wrap before 79 characters...
I didn't want to split the ternary expression, but OK, I'll follow your
wrapping.
> > diff --git a/object-store.h b/object-store.h
> > index b1ec0bde82..98c1d67946 100644
> > --- a/object-store.h
> > +++ b/object-store.h
> > @@ -445,6 +445,9 @@ struct object_info {
> > */
> > #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
> >
> > +/* Die if object corruption (not just an object being missing) was detected. */
> > +#define OBJECT_INFO_DIE_IF_CORRUPT 32
>
> Personally I wouldn't mind a short cleanup step in this series to change
> these to 1<<0, 1<<1 etc., as we do for almost everything els.
>
> I.e. in an earlier step you removed the "16", and changed that "32" to
> "16", now we're adding a "32" again.
>
> I also notice that you didn't just add a "4" here, which is an existing
> gap, which turns out to be a leftover bit from your 9c8a294a1ae
> (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02) ~2 years ago :)
Ah...I didn't notice the 4 missing. But I think the series has gone over
enough iterations now that I'd rather leave this as-is and maybe change
this in a future patch.
