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

[Justin Tobler <jltobler@gmail.com>]

On 26/01/27 07:29AM, Patrick Steinhardt wrote:
> 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.

I guess we could have enum values to each of the combinations that get
used together, but that problably isn't a great idea if we use many
different combinations and may not be good in the long term.

> 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.

I agree with the sentiment. Passing a combination of enum values as
unsigned flags is a bit fragile and in some ways feels like it defeats
the point of using enums to begin with. Also when using a function that
accepts flags, it is not always immediately obvious which set of flags
are expected.

> 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.

This would certainly be the most safe option, but I also agree that it
not particually ergonomic. I'm not sure it's worth going this far as
long as it's documented/easy-to-find the corresponding set of flags.

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

I think this version of series is good and doesn't need a reroll.

Thanks,
-Justin