From: René Scharfe <l.s.r@web.de>
Date: Mon, 26 Jan 2026 18:02:15 GMT
Subject: Re: [PATCH 3/3] odb: drop gaps in object info flag values
Message-ID: <add7c86f-9d5e-4136-8c3d-a04df523487b@web.de>
In-Reply-To: <xmqqa4y0jop7.fsf@gitster.g>

On 1/26/26 5:58 PM, Junio C Hamano wrote:
> 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.
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é