Re: [PATCH 3/3] odb: drop gaps in object info flag values

[Patrick Steinhardt <ps@pks.im>]

On Mon, Jan 26, 2026 at 10:13:51AM -0800, Junio C Hamano wrote:
> René Scharfe <l.s.r@web.de> writes:
> 
> >> I wonder if this series can be restructured a bit to demonstrate the
> >> benefit of moving to enum a bit more prominently.  For example, even
> >> at the end of the three patches, odb_read_object_info_extended()
> >> still takes an "unsigned flags" parameter, but it is meant to take
> >> this new enum, isn't it?  If we do the "#define to enum" conversion
> >> (without renumbering) first, then "unsigned to enum", would it, with
> >> appropriate compiler warning flags, already reveal the existing bugs
> >> that happened to be working OK as potential problems?  And with that,
> >> fixes in 1/3 and 2/3 would demonstrate why #define to enum" is worth
> >> doing very well.  And after all that, we can renumber the enums in a
> >> separate and final step.
> > With -Wenum-conversion you can get GCC to report implicit conversions
> > between different enum types (like in the backfill case), but I don't
> > see a way to warn about conversions from int (the fsck case).
> 
> Yes, that is why I suggested "unsigned to enum" change after doing
> "#define to enum" conversion.  If a caller passes an enum with
> HAS_OBJECT_* to odb_read_object_info_extended() that expects
> "unsigned flags", it would not be warned, but if the callee expects
> "enum object_info_flags", passing HAS_OBJECT_* enum to it would be
> flagged, right?  We may need to give the currently-unnamed enum with
> HAS_OBJECT_* a name first.

You can get it to generate a warning for one of the callsites:

    ../builtin/backfill.c:71:9: error: implicit conversion from enumeration type 'enum odb_object_info_flag' to different enumeration type 'enum odb_has_object_flag' [-Werror,-Wenum-conversion]
       70 |                 if (!odb_has_object(ctx->repo->objects, &list->oid[i],
          |                      ~~~~~~~~~~~~~~
       71 |                                     OBJECT_INFO_FOR_PREFETCH))
          |                                     ^~~~~~~~~~~~~~~~~~~~~~~~
    1 error generated.

Unfortunately, the other callsite wouldn't see a warning because we pass
an integer constant, and the compiler doesn't complain about that at
all. It also falls apart once you start to OR multiple flags together.

It would be great if there was a way to tell the compiler that a given
flags field expects only enum values so that it could always warn about
misuse. But I'm not aware of any way to do this.

We could of course start to take a more heavy-handed approach and always
accept an options struct instead. E.g.

    struct odb_read_object_info_options {
            unsigned lookup_replace : 1,
                     quick : 1,
                     skip_fetch_object : 1,
                     for_prefetch : 1,
                     die_if_corrupt : 1;
    };

That would give us full type safety, and it would be impossible to
misuse without getting a compiler warning. Furthermore, with designated
initializers it wouldn't be _that_ awful to use:

	if (!odb_read_object_extended(ctx->repo->objects, &list->oid[i],
				      (struct odb_read_object_info_options) {
		.skip_fetch_object = 1,
	}) < 0) {
		die("...");
	}

But I wouldn't exactly call it ergonomic, either.

So I'm not sure whether this partial protection would be worth it, but
if you think it is I'm happy to reroll.

Thanks!

Patrick