patch, 3 partsSmall fixups for `OBJECT_INFO` flags

> The function `fill_missing_blobs()` receives an array of object IDs and
> verifies for each of them whether the corresponding object exists. If it
> doesn't exist, we add it to a set of objects and then batch-fetch all of
> the objects at once.
> 
> The check for whether or not we already have the object is broken
> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`
> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:
> 
>   - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare
>     in case the object wasn't found. This makes sense, as we'd otherwise
>     reprepare the object database as many times as we have missing
>     objects.
> 
>   - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to
>     not fetch the object in case it's missing. Again, this makes sense,
>     as we want to batch-fetch the objects.
> 
> This shows that we indeed want the equivalent of this flag, but of
> course represented as `HAS_OBJECT_*` flags.
> 
> Luckily, the code is already working correctly. The `OBJECT_INFO` flag
> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`
> flags. And if no flags are passed, `odb_has_object()` ends up calling
> `odb_read_object_info_extended()` with exactly the above two flags that
> we wanted to set in the first place.
> 
> Of course, this is pure luck, and this can break any moment. So let's
> fix this and correct the code to not pass any flags at all.

> On 1/26/2026 7:17 AM, Patrick Steinhardt wrote:
>> The function `fill_missing_blobs()` receives an array of object IDs and
>> verifies for each of them whether the corresponding object exists. If it
>> doesn't exist, we add it to a set of objects and then batch-fetch all of
>> the objects at once.
>> 
>> The check for whether or not we already have the object is broken
>> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`
>> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:
>> 
>>   - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare
>>     in case the object wasn't found. This makes sense, as we'd otherwise
>>     reprepare the object database as many times as we have missing
>>     objects.
>> 
>>   - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to
>>     not fetch the object in case it's missing. Again, this makes sense,
>>     as we want to batch-fetch the objects.
>> 
>> This shows that we indeed want the equivalent of this flag, but of
>> course represented as `HAS_OBJECT_*` flags.
>> 
>> Luckily, the code is already working correctly. The `OBJECT_INFO` flag
>> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`
>> flags. And if no flags are passed, `odb_has_object()` ends up calling
>> `odb_read_object_info_extended()` with exactly the above two flags that
>> we wanted to set in the first place.
>> 
>> Of course, this is pure luck, and this can break any moment. So let's
>> fix this and correct the code to not pass any flags at all.
>
> Absolutely the right fix for this case. Thanks!
>
> -Stolee

> The function `fill_missing_blobs()` receives an array of object IDs and
> verifies for each of them whether the corresponding object exists. If it
> doesn't exist, we add it to a set of objects and then batch-fetch all of
> the objects at once.
> 
> The check for whether or not we already have the object is broken
> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`
> expects us to pass `HAS_OBJECT_*` flags.

> The flag expands to:
> 
>   - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare
>     in case the object wasn't found. This makes sense, as we'd otherwise
>     reprepare the object database as many times as we have missing
>     objects.
> 
>   - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to
>     not fetch the object in case it's missing. Again, this makes sense,
>     as we want to batch-fetch the objects.
> 
> This shows that we indeed want the equivalent of this flag, but of
> course represented as `HAS_OBJECT_*` flags.
> 
> Luckily, the code is already working correctly. The `OBJECT_INFO` flag
> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`
> flags. And if no flags are passed, `odb_has_object()` ends up calling
> `odb_read_object_info_extended()` with exactly the above two flags that
> we wanted to set in the first place.

> Of course, this is pure luck, and this can break any moment. So let's
> fix this and correct the code to not pass any flags at all.
> 
> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  builtin/backfill.c | 3 +--
>  1 file changed, 1 insertion(+), 2 deletions(-)
> 
> diff --git a/builtin/backfill.c b/builtin/backfill.c
> index e80fc1b694..d8cb3b0eba 100644
> --- a/builtin/backfill.c
> +++ b/builtin/backfill.c
> @@ -67,8 +67,7 @@ static int fill_missing_blobs(const char *path UNUSED,
>  		return 0;
>  
>  	for (size_t i = 0; i < list->nr; i++) {
> -		if (!odb_has_object(ctx->repo->objects, &list->oid[i],
> -				    OBJECT_INFO_FOR_PREFETCH))
> +		if (!odb_has_object(ctx->repo->objects, &list->oid[i], 0))

> The function `fill_missing_blobs()` receives an array of object IDs and
> verifies for each of them whether the corresponding object exists. If it
> doesn't exist, we add it to a set of objects and then batch-fetch all of
> the objects at once.
>
> The check for whether or not we already have the object is broken
> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`
> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:
>
>   - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare
>     in case the object wasn't found. This makes sense, as we'd otherwise
>     reprepare the object database as many times as we have missing
>     objects.
>
>   - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to
>     not fetch the object in case it's missing. Again, this makes sense,
>     as we want to batch-fetch the objects.
>
> This shows that we indeed want the equivalent of this flag, but of
> course represented as `HAS_OBJECT_*` flags.
>
> Luckily, the code is already working correctly. The `OBJECT_INFO` flag
> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`
> flags. And if no flags are passed, `odb_has_object()` ends up calling
> `odb_read_object_info_extended()` with exactly the above two flags that
> we wanted to set in the first place.
>
> Of course, this is pure luck, and this can break any moment. So let's
> fix this and correct the code to not pass any flags at all.
>

> Patrick Steinhardt <ps@pks.im> writes:
>
>> The function `fill_missing_blobs()` receives an array of object IDs and
>> verifies for each of them whether the corresponding object exists. If it
>> doesn't exist, we add it to a set of objects and then batch-fetch all of
>> the objects at once.
>>
>> The check for whether or not we already have the object is broken
>> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`
>> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:
>>
>>   - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare
>>     in case the object wasn't found. This makes sense, as we'd otherwise
>>     reprepare the object database as many times as we have missing
>>     objects.
>>
>>   - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to
>>     not fetch the object in case it's missing. Again, this makes sense,
>>     as we want to batch-fetch the objects.
>>
>> This shows that we indeed want the equivalent of this flag, but of
>> course represented as `HAS_OBJECT_*` flags.
>>
>> Luckily, the code is already working correctly. The `OBJECT_INFO` flag
>> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`
>> flags. And if no flags are passed, `odb_has_object()` ends up calling
>> `odb_read_object_info_extended()` with exactly the above two flags that
>> we wanted to set in the first place.
>>
>> Of course, this is pure luck, and this can break any moment. So let's
>> fix this and correct the code to not pass any flags at all.
>>
>
> We do pass the same equivalent, no? I mean `OBJECT_INFO_FOR_PREFETCH`
> does resolve to `OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK` and
> calling `odb_has_object(... , 0)` would also eventually set the same
> flags.
>
> I understand the issue, `odb_has_object()` should only take in
> `HAS_OBJECT_*` flags, even though internally it converts them to
> `OBJECT_INFO_*` flags.
>
> Wouldn't it also be nicer to convert the enum for `HAS_OBJECT_*` to no
> longer be anonymous and use that in `odb_has_object()`?
>
> The patch itself looks good!
>
> - Karthik

> In `mark_object()` we invoke `has_object()` with a value of 1. This is
> somewhat fishy given that the function expects a bitset of flags, so any
> behaviour that this results in is purely coincidental and may break at
> any point in time.
> 
> The call to `has_object()` was originally introduced in 9eb86f41de
> (fsck: do not lazy fetch known non-promisor object, 2020-08-05). The
> intent here was to skip lazy fetches of promisor objects: we have
> already verified that the object is not a promisor object, so if the
> object is missing it indicates a corrupt repository.
> 
> The hardcoded value that we pass maps to `HAS_OBJECT_RECHECK_PACKED`,
> which is probably the intended behaviour: `odb_has_object()` will not
> fetch promisor objects unless `HAS_OBJECT_FETCH_PROMISOR` is passed, but
> we may want to verify that no concurrent process has written the object
> that we're trying to read.

> +enum object_info_flags {
> +	/* Invoke lookup_replace_object() on the given hash. */
> +	OBJECT_INFO_LOOKUP_REPLACE = (1 << 0),
> +
> +	/* Do not reprepare object sources when the first lookup has failed. */
> +	OBJECT_INFO_QUICK = (1 << 1),
> +
> +	/*
> +	 * Do not attempt to fetch the object if missing (even if fetch_is_missing is
> +	 * nonzero).
> +	 */
> +	OBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),
> +
> +	/* Die if object corruption (not just an object being missing) was detected. */
> +	OBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),
>  
> -/* Die if object corruption (not just an object being missing) was detected. */
> -#define OBJECT_INFO_DIE_IF_CORRUPT 32
> +	/*
> +	 * This is meant for bulk prefetching of missing blobs in a partial
> +	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
> +	 */
> +	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),
> +};
>  
>  /*
>   * Read object info from the object database and populate the `object_info`

> Patrick Steinhardt <ps@pks.im> writes:
> 
>> +enum object_info_flags {
>> +	/* Invoke lookup_replace_object() on the given hash. */
>> +	OBJECT_INFO_LOOKUP_REPLACE = (1 << 0),
>> +
>> +	/* Do not reprepare object sources when the first lookup has failed. */
>> +	OBJECT_INFO_QUICK = (1 << 1),
>> +
>> +	/*
>> +	 * Do not attempt to fetch the object if missing (even if fetch_is_missing is
>> +	 * nonzero).
>> +	 */
>> +	OBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),
>> +
>> +	/* Die if object corruption (not just an object being missing) was detected. */
>> +	OBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),
>>  
>> -/* Die if object corruption (not just an object being missing) was detected. */
>> -#define OBJECT_INFO_DIE_IF_CORRUPT 32
>> +	/*
>> +	 * This is meant for bulk prefetching of missing blobs in a partial
>> +	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
>> +	 */
>> +	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),
>> +};
>>  
>>  /*
>>   * Read object info from the object database and populate the `object_info`
> 
> 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.

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

> https://stackoverflow.com/questions/4669454/how-to-make-gcc-warn-about-passing-wrong-enum-to-a-function
> suggests using -Wenum-compare and macros to sneak in a comparison, but
> that doesn't seem to catch more than -Wenum-conversion, which doesn't
> need any macros.
>
> https://godbolt.org/z/Whvc7Mf1n
>
> Perhaps sparse can do that?
>
> René

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

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

> The object info flag values have a two gaps in their definitions, where
> some bits are skipped over. These gaps don't really hurt, but it makes
> one wonder whether anything is going on and whether a subset of flags
> might be defined somewhere else.
> 
> That's not the case though. Instead, this is a case of flags that have
> been dropped in the past:
> 
>   - The value 4 was used by `OBJECT_INFO_SKIP_CACHED`, removed in
>     9c8a294a1a (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02).
> 
>   - The value 8 was used by `OBJECT_INFO_ALLOW_UNKNOWN_TYPE`, removed in
>     ae24b032a0 (object-file: drop OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag,
>     2025-05-16).
> 
> Close those gaps to avoid any more confusion. While at it, convert the
> flags to be declared as an enum and use bit shifts to follow modern best
> practices.

> Signed-off-by: Patrick Steinhardt <ps@pks.im>
> ---
>  odb.h | 38 ++++++++++++++++++++++----------------
>  1 file changed, 22 insertions(+), 16 deletions(-)
> 
> diff --git a/odb.h b/odb.h
> index bab07755f4..1e4326b7f4 100644
> --- a/odb.h
> +++ b/odb.h
> @@ -352,23 +352,29 @@ struct object_info {
>   */
>  #define OBJECT_INFO_INIT { 0 }
>  
> -/* Invoke lookup_replace_object() on the given hash */
> -#define OBJECT_INFO_LOOKUP_REPLACE 1
> -/* Do not retry packed storage after checking packed and loose storage */
> -#define OBJECT_INFO_QUICK 8
> -/*
> - * Do not attempt to fetch the object if missing (even if fetch_is_missing is
> - * nonzero).
> - */
> -#define OBJECT_INFO_SKIP_FETCH_OBJECT 16
> -/*
> - * This is meant for bulk prefetching of missing blobs in a partial
> - * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK
> - */
> -#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)
> +/* Flags that can be passed to `odb_read_object_info_extended()`. */
> +enum object_info_flags {
> +	/* Invoke lookup_replace_object() on the given hash. */
> +	OBJECT_INFO_LOOKUP_REPLACE = (1 << 0),
> +
> +	/* Do not reprepare object sources when the first lookup has failed. */
> +	OBJECT_INFO_QUICK = (1 << 1),
> +
> +	/*
> +	 * Do not attempt to fetch the object if missing (even if fetch_is_missing is
> +	 * nonzero).
> +	 */
> +	OBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),
> +
> +	/* Die if object corruption (not just an object being missing) was detected. */
> +	OBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),
>  
> -/* Die if object corruption (not just an object being missing) was detected. */
> -#define OBJECT_INFO_DIE_IF_CORRUPT 32
> +	/*
> +	 * This is meant for bulk prefetching of missing blobs in a partial
> +	 * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.
> +	 */
> +	OBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),

> Surprisingly though I found out that this breaks tests, which of course
> puzzled me. As it turns out though, we were incorrectly using a couple
> of these flags for `odb_has_object()`, and the changed definitions had
> overlap with the existing meaning of other `HAS_OBJECT_*` flags. There
> isn't really any bug here as far as I can see, but this is only really
> by chance.