{"thread":{"id":"64869","subject":"[PATCH 0/3] Small fixups for `OBJECT_INFO` flags","startedAt":"2026-01-26T12:17:52Z","lastAt":"2026-02-12T07:00:04Z","messageCount":23,"participants":["Patrick Steinhardt","Junio C Hamano","René Scharfe","Derrick Stolee","Justin Tobler","Karthik Nayak"],"isPatch":true,"patchVersion":1,"patchTotal":3},"messages":[{"id":"534660","messageId":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","threadId":"64869","inReplyTo":null,"subject":"[PATCH 0/3] Small fixups for `OBJECT_INFO` flags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-26T12:17:40Z","receivedAt":"2026-01-26T12:17:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI was kind of curious why there were gaps in the `OBJECT_INFO_*` flags,\nbut eventually found out that these gaps are of historic nature: there\nused to be more flags, but their respective values got removed at one\npoint in time. So naturally, I wanted to clean this up a bit so that the\nnext reader wouldn't have the same question.\n\nSurprisingly though I found out that this breaks tests, which of course\npuzzled me. As it turns out though, we were incorrectly using a couple\nof these flags for `odb_has_object()`, and the changed definitions had\noverlap with the existing meaning of other `HAS_OBJECT_*` flags. There\nisn't really any bug here as far as I can see, but this is only really\nby chance.\n\nIn any case, the first two commits fix calls to `odb_has_object()` that\nused invalid flags. The last commit then removes the gaps and converts\nthe flags to use an enum instead.\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (3):\n builtin/backfill: fix flags passed to `odb_has_object()`\n builtin/fsck: fix flags passed to `odb_has_object()`\n odb: drop gaps in object info flag values\n\n builtin/backfill.c | 3 +--\n builtin/fsck.c | 3 ++-\n odb.h | 38 ++++++++++++++++++++++----------------\n 3 files changed, 25 insertions(+), 19 deletions(-)\n\n\n---\nbase-commit: ea24e2c55433012a0a6c4ae947a87bc66404e484\nchange-id: 20260126-b4-pks-read-object-info-flags-236c4437cfc5\n\n"},{"id":"534661","messageId":"20260126-b4-pks-read-object-info-flags-v1-1-e682a003b17c@pks.im","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","subject":"[PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-26T12:17:41Z","receivedAt":"2026-01-26T12:17:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `fill_missing_blobs()` receives an array of object IDs and\nverifies for each of them whether the corresponding object exists. If it\ndoesn't exist, we add it to a set of objects and then batch-fetch all of\nthe objects at once.\n\nThe check for whether or not we already have the object is broken\nthough: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\nexpects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n\n - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n in case the object wasn't found. This makes sense, as we'd otherwise\n reprepare the object database as many times as we have missing\n objects.\n\n - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n not fetch the object in case it's missing. Again, this makes sense,\n as we want to batch-fetch the objects.\n\nThis shows that we indeed want the equivalent of this flag, but of\ncourse represented as `HAS_OBJECT_*` flags.\n\nLuckily, the code is already working correctly. The `OBJECT_INFO` flag\nexpands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\nflags. And if no flags are passed, `odb_has_object()` ends up calling\n`odb_read_object_info_extended()` with exactly the above two flags that\nwe wanted to set in the first place.\n\nOf course, this is pure luck, and this can break any moment. So let's\nfix this and correct the code to not pass any flags at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/backfill.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex e80fc1b694..d8cb3b0eba 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -67,8 +67,7 @@ static int fill_missing_blobs(const char *path UNUSED,\n \t\treturn 0;\n \n \tfor (size_t i = 0; i < list->nr; i++) {\n-\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i],\n-\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i], 0))\n \t\t\toid_array_append(&ctx->current_batch, &list->oid[i]);\n \t}\n \n\n-- \n2.53.0.rc1.267.g6e3a78c723.dirty\n\n"},{"id":"534662","messageId":"20260126-b4-pks-read-object-info-flags-v1-2-e682a003b17c@pks.im","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","subject":"[PATCH 2/3] builtin/fsck: fix flags passed to `odb_has_object()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-26T12:17:42Z","receivedAt":"2026-01-26T12:17:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `mark_object()` we invoke `has_object()` with a value of 1. This is\nsomewhat fishy given that the function expects a bitset of flags, so any\nbehaviour that this results in is purely coincidental and may break at\nany point in time.\n\nThe call to `has_object()` was originally introduced in 9eb86f41de\n(fsck: do not lazy fetch known non-promisor object, 2020-08-05). The\nintent here was to skip lazy fetches of promisor objects: we have\nalready verified that the object is not a promisor object, so if the\nobject is missing it indicates a corrupt repository.\n\nThe hardcoded value that we pass maps to `HAS_OBJECT_RECHECK_PACKED`,\nwhich is probably the intended behaviour: `odb_has_object()` will not\nfetch promisor objects unless `HAS_OBJECT_FETCH_PROMISOR` is passed, but\nwe may want to verify that no concurrent process has written the object\nthat we're trying to read.\n\nConvert the code to use the named flag instead of the the hardcoded\nvalue.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fsck.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 0512f78a87..1d059dd6c2 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -162,7 +162,8 @@ static int mark_object(struct object *obj, enum object_type type,\n \t\treturn 0;\n \n \tif (!(obj->flags & HAS_OBJ)) {\n-\t\tif (parent && !odb_has_object(the_repository->objects, &obj->oid, 1)) {\n+\t\tif (parent && !odb_has_object(the_repository->objects, &obj->oid,\n+\t\t\t\t\t HAS_OBJECT_RECHECK_PACKED)) {\n \t\t\tprintf_ln(_(\"broken link from %7s %s\\n\"\n \t\t\t\t \" to %7s %s\"),\n \t\t\t\t printable_type(&parent->oid, parent->type),\n\n-- \n2.53.0.rc1.267.g6e3a78c723.dirty\n\n"},{"id":"534663","messageId":"20260126-b4-pks-read-object-info-flags-v1-3-e682a003b17c@pks.im","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","subject":"[PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-26T12:17:43Z","receivedAt":"2026-01-26T12:18:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The object info flag values have a two gaps in their definitions, where\nsome bits are skipped over. These gaps don't really hurt, but it makes\none wonder whether anything is going on and whether a subset of flags\nmight be defined somewhere else.\n\nThat's not the case though. Instead, this is a case of flags that have\nbeen dropped in the past:\n\n - The value 4 was used by `OBJECT_INFO_SKIP_CACHED`, removed in\n 9c8a294a1a (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02).\n\n - The value 8 was used by `OBJECT_INFO_ALLOW_UNKNOWN_TYPE`, removed in\n ae24b032a0 (object-file: drop OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag,\n 2025-05-16).\n\nClose those gaps to avoid any more confusion. While at it, convert the\nflags to be declared as an enum and use bit shifts to follow modern best\npractices.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.h | 38 ++++++++++++++++++++++----------------\n 1 file changed, 22 insertions(+), 16 deletions(-)\n\ndiff --git a/odb.h b/odb.h\nindex bab07755f4..1e4326b7f4 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -352,23 +352,29 @@ struct object_info {\n */\n #define OBJECT_INFO_INIT { 0 }\n \n-/* Invoke lookup_replace_object() on the given hash */\n-#define OBJECT_INFO_LOOKUP_REPLACE 1\n-/* Do not retry packed storage after checking packed and loose storage */\n-#define OBJECT_INFO_QUICK 8\n-/*\n- * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n- * nonzero).\n- */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n-/*\n- * This is meant for bulk prefetching of missing blobs in a partial\n- * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n- */\n-#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n+/* Flags that can be passed to `odb_read_object_info_extended()`. */\n+enum object_info_flags {\n+\t/* Invoke lookup_replace_object() on the given hash. */\n+\tOBJECT_INFO_LOOKUP_REPLACE = (1 << 0),\n+\n+\t/* Do not reprepare object sources when the first lookup has failed. */\n+\tOBJECT_INFO_QUICK = (1 << 1),\n+\n+\t/*\n+\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n+\t * nonzero).\n+\t */\n+\tOBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),\n+\n+\t/* Die if object corruption (not just an object being missing) was detected. */\n+\tOBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),\n \n-/* Die if object corruption (not just an object being missing) was detected. */\n-#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+\t/*\n+\t * This is meant for bulk prefetching of missing blobs in a partial\n+\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.\n+\t */\n+\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n+};\n \n /*\n * Read object info from the object database and populate the `object_info`\n\n-- \n2.53.0.rc1.267.g6e3a78c723.dirty\n\n"},{"id":"534673","messageId":"xmqqecncjq37.fsf@gitster.g","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","subject":"Re: [PATCH 0/3] Small fixups for `OBJECT_INFO` flags","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-26T16:28:28Z","receivedAt":"2026-01-26T16:28:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> Surprisingly though I found out that this breaks tests, which of course\n> puzzled me. As it turns out though, we were incorrectly using a couple\n> of these flags for `odb_has_object()`, and the changed definitions had\n> overlap with the existing meaning of other `HAS_OBJECT_*` flags. There\n> isn't really any bug here as far as I can see, but this is only really\n> by chance.\n\nGreat findings.\n\n> In any case, the first two commits fix calls to `odb_has_object()` that\n> used invalid flags. The last commit then removes the gaps and converts\n> the flags to use an enum instead.\n\nNice.\n"},{"id":"534675","messageId":"xmqqa4y0jop7.fsf@gitster.g","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-3-e682a003b17c@pks.im","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-26T16:58:28Z","receivedAt":"2026-01-26T16:58:31Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Patrick Steinhardt writes:\n\n> +enum object_info_flags {\n> +\t/* Invoke lookup_replace_object() on the given hash. */\n> +\tOBJECT_INFO_LOOKUP_REPLACE = (1 << 0),\n> +\n> +\t/* Do not reprepare object sources when the first lookup has failed. */\n> +\tOBJECT_INFO_QUICK = (1 << 1),\n> +\n> +\t/*\n> +\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> +\t * nonzero).\n> +\t */\n> +\tOBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),\n> +\n> +\t/* Die if object corruption (not just an object being missing) was detected. */\n> +\tOBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),\n> \n> -/* Die if object corruption (not just an object being missing) was detected. */\n> -#define OBJECT_INFO_DIE_IF_CORRUPT 32\n> +\t/*\n> +\t * This is meant for bulk prefetching of missing blobs in a partial\n> +\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.\n> +\t */\n> +\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n> +};\n> \n> /*\n> * Read object info from the object database and populate the `object_info`\n\nI wonder if this series can be restructured a bit to demonstrate the\nbenefit of moving to enum a bit more prominently. For example, even\nat the end of the three patches, odb_read_object_info_extended()\nstill takes an \"unsigned flags\" parameter, but it is meant to take\nthis new enum, isn't it? If we do the \"#define to enum\" conversion\n(without renumbering) first, then \"unsigned to enum\", would it, with\nappropriate compiler warning flags, already reveal the existing bugs\nthat happened to be working OK as potential problems? And with that,\nfixes in 1/3 and 2/3 would demonstrate why #define to enum\" is worth\ndoing very well. And after all that, we can renumber the enums in a\nseparate and final step.\n\nExactly the same comment applies to odb_has_object() that still\ntakes \"unsigned flags\", even though HAS_OBJECT_* constants have\nalready gone through the \"#define to enum\" conversion with an earier\nf8fc4cac (object-store: allow fetching objects via `has_object()`,\n2025-04-29).\n\nIn any case, well spotted and nicely done.\nThanks.\n"},{"id":"534680","messageId":"add7c86f-9d5e-4136-8c3d-a04df523487b@web.de","threadId":"64869","inReplyTo":"xmqqa4y0jop7.fsf@gitster.g","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2026-01-26T18:02:15Z","receivedAt":"2026-01-26T18:02:20Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"On 1/26/26 5:58 PM, Junio C Hamano wrote:\n> Patrick Steinhardt writes:\n> \n>> +enum object_info_flags {\n>> +\t/* Invoke lookup_replace_object() on the given hash. */\n>> +\tOBJECT_INFO_LOOKUP_REPLACE = (1 << 0),\n>> +\n>> +\t/* Do not reprepare object sources when the first lookup has failed. */\n>> +\tOBJECT_INFO_QUICK = (1 << 1),\n>> +\n>> +\t/*\n>> +\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n>> +\t * nonzero).\n>> +\t */\n>> +\tOBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),\n>> +\n>> +\t/* Die if object corruption (not just an object being missing) was detected. */\n>> +\tOBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),\n>> \n>> -/* Die if object corruption (not just an object being missing) was detected. */\n>> -#define OBJECT_INFO_DIE_IF_CORRUPT 32\n>> +\t/*\n>> +\t * This is meant for bulk prefetching of missing blobs in a partial\n>> +\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.\n>> +\t */\n>> +\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n>> +};\n>> \n>> /*\n>> * Read object info from the object database and populate the `object_info`\n> \n> I wonder if this series can be restructured a bit to demonstrate the\n> benefit of moving to enum a bit more prominently. For example, even\n> at the end of the three patches, odb_read_object_info_extended()\n> still takes an \"unsigned flags\" parameter, but it is meant to take\n> this new enum, isn't it? If we do the \"#define to enum\" conversion\n> (without renumbering) first, then \"unsigned to enum\", would it, with\n> appropriate compiler warning flags, already reveal the existing bugs\n> that happened to be working OK as potential problems? And with that,\n> fixes in 1/3 and 2/3 would demonstrate why #define to enum\" is worth\n> doing very well. And after all that, we can renumber the enums in a\n> separate and final step.\nWith -Wenum-conversion you can get GCC to report implicit conversions\nbetween different enum types (like in the backfill case), but I don't\nsee a way to warn about conversions from int (the fsck case).\n\nhttps://stackoverflow.com/questions/4669454/how-to-make-gcc-warn-about-passing-wrong-enum-to-a-function\nsuggests using -Wenum-compare and macros to sneak in a comparison, but\nthat doesn't seem to catch more than -Wenum-conversion, which doesn't\nneed any macros.\n\nhttps://godbolt.org/z/Whvc7Mf1n\n\nPerhaps sparse can do that?\n\nRené\n\n"},{"id":"534681","messageId":"xmqqpl6wi6n4.fsf@gitster.g","threadId":"64869","inReplyTo":"add7c86f-9d5e-4136-8c3d-a04df523487b@web.de","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-26T18:13:51Z","receivedAt":"2026-01-26T18:13:54Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe writes:\n\n>> I wonder if this series can be restructured a bit to demonstrate the\n>> benefit of moving to enum a bit more prominently. For example, even\n>> at the end of the three patches, odb_read_object_info_extended()\n>> still takes an \"unsigned flags\" parameter, but it is meant to take\n>> this new enum, isn't it? If we do the \"#define to enum\" conversion\n>> (without renumbering) first, then \"unsigned to enum\", would it, with\n>> appropriate compiler warning flags, already reveal the existing bugs\n>> that happened to be working OK as potential problems? And with that,\n>> fixes in 1/3 and 2/3 would demonstrate why #define to enum\" is worth\n>> doing very well. And after all that, we can renumber the enums in a\n>> separate and final step.\n> With -Wenum-conversion you can get GCC to report implicit conversions\n> between different enum types (like in the backfill case), but I don't\n> see a way to warn about conversions from int (the fsck case).\n\nYes, that is why I suggested \"unsigned to enum\" change after doing\n\"#define to enum\" conversion. If a caller passes an enum with\nHAS_OBJECT_* to odb_read_object_info_extended() that expects\n\"unsigned flags\", it would not be warned, but if the callee expects\n\"enum object_info_flags\", passing HAS_OBJECT_* enum to it would be\nflagged, right? We may need to give the currently-unnamed enum with\nHAS_OBJECT_* a name first.\n\n> https://stackoverflow.com/questions/4669454/how-to-make-gcc-warn-about-passing-wrong-enum-to-a-function\n> suggests using -Wenum-compare and macros to sneak in a comparison, but\n> that doesn't seem to catch more than -Wenum-conversion, which doesn't\n> need any macros.\n>\n> https://godbolt.org/z/Whvc7Mf1n\n>\n> Perhaps sparse can do that?\n>\n> René\n"},{"id":"534688","messageId":"a5b456dc-2158-4f9a-addd-12fb9f408edf@gmail.com","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-1-e682a003b17c@pks.im","subject":"Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2026-01-26T20:17:45Z","receivedAt":"2026-01-26T20:17:47Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 1/26/2026 7:17 AM, Patrick Steinhardt wrote:\n> The function `fill_missing_blobs()` receives an array of object IDs and\n> verifies for each of them whether the corresponding object exists. If it\n> doesn't exist, we add it to a set of objects and then batch-fetch all of\n> the objects at once.\n> \n> The check for whether or not we already have the object is broken\n> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\n> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n> \n> - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n> in case the object wasn't found. This makes sense, as we'd otherwise\n> reprepare the object database as many times as we have missing\n> objects.\n> \n> - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n> not fetch the object in case it's missing. Again, this makes sense,\n> as we want to batch-fetch the objects.\n> \n> This shows that we indeed want the equivalent of this flag, but of\n> course represented as `HAS_OBJECT_*` flags.\n> \n> Luckily, the code is already working correctly. The `OBJECT_INFO` flag\n> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\n> flags. And if no flags are passed, `odb_has_object()` ends up calling\n> `odb_read_object_info_extended()` with exactly the above two flags that\n> we wanted to set in the first place.\n> \n> Of course, this is pure luck, and this can break any moment. So let's\n> fix this and correct the code to not pass any flags at all.\n\nAbsolutely the right fix for this case. Thanks!\n\n-Stolee\n"},{"id":"534689","messageId":"xmqqwm14gk10.fsf@gitster.g","threadId":"64869","inReplyTo":"a5b456dc-2158-4f9a-addd-12fb9f408edf@gmail.com","subject":"Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2026-01-26T21:07:39Z","receivedAt":"2026-01-26T21:07:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Derrick Stolee writes:\n\n> On 1/26/2026 7:17 AM, Patrick Steinhardt wrote:\n>> The function `fill_missing_blobs()` receives an array of object IDs and\n>> verifies for each of them whether the corresponding object exists. If it\n>> doesn't exist, we add it to a set of objects and then batch-fetch all of\n>> the objects at once.\n>> \n>> The check for whether or not we already have the object is broken\n>> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\n>> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n>> \n>> - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n>> in case the object wasn't found. This makes sense, as we'd otherwise\n>> reprepare the object database as many times as we have missing\n>> objects.\n>> \n>> - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n>> not fetch the object in case it's missing. Again, this makes sense,\n>> as we want to batch-fetch the objects.\n>> \n>> This shows that we indeed want the equivalent of this flag, but of\n>> course represented as `HAS_OBJECT_*` flags.\n>> \n>> Luckily, the code is already working correctly. The `OBJECT_INFO` flag\n>> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\n>> flags. And if no flags are passed, `odb_has_object()` ends up calling\n>> `odb_read_object_info_extended()` with exactly the above two flags that\n>> we wanted to set in the first place.\n>> \n>> Of course, this is pure luck, and this can break any moment. So let's\n>> fix this and correct the code to not pass any flags at all.\n>\n> Absolutely the right fix for this case. Thanks!\n>\n> -Stolee\n\nThanks, both.\n"},{"id":"534700","messageId":"aXhbXQo6taM33m-1@pks.im","threadId":"64869","inReplyTo":"xmqqpl6wi6n4.fsf@gitster.g","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-01-27T06:29:49Z","receivedAt":"2026-01-27T06:29:55Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"On Mon, Jan 26, 2026 at 10:13:51AM -0800, Junio C Hamano wrote:\n> René Scharfe writes:\n> \n> >> I wonder if this series can be restructured a bit to demonstrate the\n> >> benefit of moving to enum a bit more prominently. For example, even\n> >> at the end of the three patches, odb_read_object_info_extended()\n> >> still takes an \"unsigned flags\" parameter, but it is meant to take\n> >> this new enum, isn't it? If we do the \"#define to enum\" conversion\n> >> (without renumbering) first, then \"unsigned to enum\", would it, with\n> >> appropriate compiler warning flags, already reveal the existing bugs\n> >> that happened to be working OK as potential problems? And with that,\n> >> fixes in 1/3 and 2/3 would demonstrate why #define to enum\" is worth\n> >> doing very well. And after all that, we can renumber the enums in a\n> >> separate and final step.\n> > With -Wenum-conversion you can get GCC to report implicit conversions\n> > between different enum types (like in the backfill case), but I don't\n> > see a way to warn about conversions from int (the fsck case).\n> \n> Yes, that is why I suggested \"unsigned to enum\" change after doing\n> \"#define to enum\" conversion. If a caller passes an enum with\n> HAS_OBJECT_* to odb_read_object_info_extended() that expects\n> \"unsigned flags\", it would not be warned, but if the callee expects\n> \"enum object_info_flags\", passing HAS_OBJECT_* enum to it would be\n> flagged, right? We may need to give the currently-unnamed enum with\n> HAS_OBJECT_* a name first.\n\nYou can get it to generate a warning for one of the callsites:\n\n ../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]\n 70 | if (!odb_has_object(ctx->repo->objects, &list->oid[i],\n | ~~~~~~~~~~~~~~\n 71 | OBJECT_INFO_FOR_PREFETCH))\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n 1 error generated.\n\nUnfortunately, the other callsite wouldn't see a warning because we pass\nan integer constant, and the compiler doesn't complain about that at\nall. It also falls apart once you start to OR multiple flags together.\n\nIt would be great if there was a way to tell the compiler that a given\nflags field expects only enum values so that it could always warn about\nmisuse. But I'm not aware of any way to do this.\n\nWe could of course start to take a more heavy-handed approach and always\naccept an options struct instead. E.g.\n\n struct odb_read_object_info_options {\n unsigned lookup_replace : 1,\n quick : 1,\n skip_fetch_object : 1,\n for_prefetch : 1,\n die_if_corrupt : 1;\n };\n\nThat would give us full type safety, and it would be impossible to\nmisuse without getting a compiler warning. Furthermore, with designated\ninitializers it wouldn't be _that_ awful to use:\n\n\tif (!odb_read_object_extended(ctx->repo->objects, &list->oid[i],\n\t\t\t\t (struct odb_read_object_info_options) {\n\t\t.skip_fetch_object = 1,\n\t}) < 0) {\n\t\tdie(\"...\");\n\t}\n\nBut I wouldn't exactly call it ergonomic, either.\n\nSo I'm not sure whether this partial protection would be worth it, but\nif you think it is I'm happy to reroll.\n\nThanks!\n\nPatrick\n"},{"id":"535599","messageId":"aYo5M7YLqroH4fab@denethor","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-1-e682a003b17c@pks.im","subject":"Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-02-09T19:57:44Z","receivedAt":"2026-02-09T19:57:50Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/01/26 01:17PM, Patrick Steinhardt wrote:\n> The function `fill_missing_blobs()` receives an array of object IDs and\n> verifies for each of them whether the corresponding object exists. If it\n> doesn't exist, we add it to a set of objects and then batch-fetch all of\n> the objects at once.\n> \n> The check for whether or not we already have the object is broken\n> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\n> expects us to pass `HAS_OBJECT_*` flags.\n\nOk so the flag we are passing to `odb_has_object()` here is not from the\nexpected set.\n\n> The flag expands to:\n> \n> - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n> in case the object wasn't found. This makes sense, as we'd otherwise\n> reprepare the object database as many times as we have missing\n> objects.\n> \n> - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n> not fetch the object in case it's missing. Again, this makes sense,\n> as we want to batch-fetch the objects.\n> \n> This shows that we indeed want the equivalent of this flag, but of\n> course represented as `HAS_OBJECT_*` flags.\n> \n> Luckily, the code is already working correctly. The `OBJECT_INFO` flag\n> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\n> flags. And if no flags are passed, `odb_has_object()` ends up calling\n> `odb_read_object_info_extended()` with exactly the above two flags that\n> we wanted to set in the first place.\n\nLucky indeed.\n\n> Of course, this is pure luck, and this can break any moment. So let's\n> fix this and correct the code to not pass any flags at all.\n> \n> Signed-off-by: Patrick Steinhardt \n> ---\n> builtin/backfill.c | 3 +--\n> 1 file changed, 1 insertion(+), 2 deletions(-)\n> \n> diff --git a/builtin/backfill.c b/builtin/backfill.c\n> index e80fc1b694..d8cb3b0eba 100644\n> --- a/builtin/backfill.c\n> +++ b/builtin/backfill.c\n> @@ -67,8 +67,7 @@ static int fill_missing_blobs(const char *path UNUSED,\n> \t\treturn 0;\n> \n> \tfor (size_t i = 0; i < list->nr; i++) {\n> -\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i],\n> -\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n> +\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i], 0))\n\nBy passing 0 as the flag value here, the underlying\n`odb_read_object_info_extended()` gets both the `OBJECT_INFO_QUICK` and\n`OBJECT_INFO_SKIP_FETCH_OBJECT` flags set. This is exactly what we want\nso there is no need to add an additional `HAS_OBJECT_*` flag. Looks\ngood.\n\n-Justin\n"},{"id":"535601","messageId":"aYo8QoT2y8s_0itJ@denethor","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-2-e682a003b17c@pks.im","subject":"Re: [PATCH 2/3] builtin/fsck: fix flags passed to `odb_has_object()`","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-02-09T20:04:29Z","receivedAt":"2026-02-09T20:04:31Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/01/26 01:17PM, Patrick Steinhardt wrote:\n> In `mark_object()` we invoke `has_object()` with a value of 1. This is\n> somewhat fishy given that the function expects a bitset of flags, so any\n> behaviour that this results in is purely coincidental and may break at\n> any point in time.\n> \n> The call to `has_object()` was originally introduced in 9eb86f41de\n> (fsck: do not lazy fetch known non-promisor object, 2020-08-05). The\n> intent here was to skip lazy fetches of promisor objects: we have\n> already verified that the object is not a promisor object, so if the\n> object is missing it indicates a corrupt repository.\n> \n> The hardcoded value that we pass maps to `HAS_OBJECT_RECHECK_PACKED`,\n> which is probably the intended behaviour: `odb_has_object()` will not\n> fetch promisor objects unless `HAS_OBJECT_FETCH_PROMISOR` is passed, but\n> we may want to verify that no concurrent process has written the object\n> that we're trying to read.\n\nAs you mentioned, promisor objects are not fetched unless\n`HAS_OBJECT_FETCH_PROMISOR` is passed and in this case a flag value of 1\nmaps only to the `HAS_OBJECT_RECHECK_PACKED` flag. This certainly seems\nlike the intended option.\n\n> Convert the code to use the named flag instead of the the hardcoded\n> value.\n\nMakes sense, this patch looks good to me.\n\n-Justin\n"},{"id":"535602","messageId":"aYpAzVSUN9NcngFi@denethor","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-3-e682a003b17c@pks.im","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-02-09T20:18:26Z","receivedAt":"2026-02-09T20:18:31Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/01/26 01:17PM, Patrick Steinhardt wrote:\n> The object info flag values have a two gaps in their definitions, where\n> some bits are skipped over. These gaps don't really hurt, but it makes\n> one wonder whether anything is going on and whether a subset of flags\n> might be defined somewhere else.\n> \n> That's not the case though. Instead, this is a case of flags that have\n> been dropped in the past:\n> \n> - The value 4 was used by `OBJECT_INFO_SKIP_CACHED`, removed in\n> 9c8a294a1a (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02).\n> \n> - The value 8 was used by `OBJECT_INFO_ALLOW_UNKNOWN_TYPE`, removed in\n> ae24b032a0 (object-file: drop OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag,\n> 2025-05-16).\n> \n> Close those gaps to avoid any more confusion. While at it, convert the\n> flags to be declared as an enum and use bit shifts to follow modern best\n> practices.\n\nMakes sense.\n\n> Signed-off-by: Patrick Steinhardt \n> ---\n> odb.h | 38 ++++++++++++++++++++++----------------\n> 1 file changed, 22 insertions(+), 16 deletions(-)\n> \n> diff --git a/odb.h b/odb.h\n> index bab07755f4..1e4326b7f4 100644\n> --- a/odb.h\n> +++ b/odb.h\n> @@ -352,23 +352,29 @@ struct object_info {\n> */\n> #define OBJECT_INFO_INIT { 0 }\n> \n> -/* Invoke lookup_replace_object() on the given hash */\n> -#define OBJECT_INFO_LOOKUP_REPLACE 1\n> -/* Do not retry packed storage after checking packed and loose storage */\n> -#define OBJECT_INFO_QUICK 8\n> -/*\n> - * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> - * nonzero).\n> - */\n> -#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n> -/*\n> - * This is meant for bulk prefetching of missing blobs in a partial\n> - * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n> - */\n> -#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n> +/* Flags that can be passed to `odb_read_object_info_extended()`. */\n> +enum object_info_flags {\n> +\t/* Invoke lookup_replace_object() on the given hash. */\n> +\tOBJECT_INFO_LOOKUP_REPLACE = (1 << 0),\n> +\n> +\t/* Do not reprepare object sources when the first lookup has failed. */\n> +\tOBJECT_INFO_QUICK = (1 << 1),\n> +\n> +\t/*\n> +\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n> +\t * nonzero).\n> +\t */\n> +\tOBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),\n> +\n> +\t/* Die if object corruption (not just an object being missing) was detected. */\n> +\tOBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),\n> \n> -/* Die if object corruption (not just an object being missing) was detected. */\n> -#define OBJECT_INFO_DIE_IF_CORRUPT 32\n> +\t/*\n> +\t * This is meant for bulk prefetching of missing blobs in a partial\n> +\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.\n> +\t */\n> +\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n\nThe changes here all look obviously correct to me. Looks good.\n\n-Justin\n"},{"id":"535603","messageId":"aYpBH2eSjArsM_To@denethor","threadId":"64869","inReplyTo":"aXhbXQo6taM33m-1@pks.im","subject":"Re: [PATCH 3/3] odb: drop gaps in object info flag values","fromName":"Justin Tobler","fromEmail":"jltobler@gmail.com","sentAt":"2026-02-09T20:32:33Z","receivedAt":"2026-02-09T20:32:37Z","isPatch":true,"sender":{"key":"jltobler@gmail.com","avatar":"https://avatars.githubusercontent.com/u/53454972?v=4"},"body":"On 26/01/27 07:29AM, Patrick Steinhardt wrote:\n> Unfortunately, the other callsite wouldn't see a warning because we pass\n> an integer constant, and the compiler doesn't complain about that at\n> all. It also falls apart once you start to OR multiple flags together.\n\nI guess we could have enum values to each of the combinations that get\nused together, but that problably isn't a great idea if we use many\ndifferent combinations and may not be good in the long term.\n\n> It would be great if there was a way to tell the compiler that a given\n> flags field expects only enum values so that it could always warn about\n> misuse. But I'm not aware of any way to do this.\n\nI agree with the sentiment. Passing a combination of enum values as\nunsigned flags is a bit fragile and in some ways feels like it defeats\nthe point of using enums to begin with. Also when using a function that\naccepts flags, it is not always immediately obvious which set of flags\nare expected.\n\n> We could of course start to take a more heavy-handed approach and always\n> accept an options struct instead. E.g.\n> \n> struct odb_read_object_info_options {\n> unsigned lookup_replace : 1,\n> quick : 1,\n> skip_fetch_object : 1,\n> for_prefetch : 1,\n> die_if_corrupt : 1;\n> };\n> \n> That would give us full type safety, and it would be impossible to\n> misuse without getting a compiler warning. Furthermore, with designated\n> initializers it wouldn't be _that_ awful to use:\n> \n> \tif (!odb_read_object_extended(ctx->repo->objects, &list->oid[i],\n> \t\t\t\t (struct odb_read_object_info_options) {\n> \t\t.skip_fetch_object = 1,\n> \t}) < 0) {\n> \t\tdie(\"...\");\n> \t}\n> \n> But I wouldn't exactly call it ergonomic, either.\n\nThis would certainly be the most safe option, but I also agree that it\nnot particually ergonomic. I'm not sure it's worth going this far as\nlong as it's documented/easy-to-find the corresponding set of flags.\n\n> So I'm not sure whether this partial protection would be worth it, but\n> if you think it is I'm happy to reroll.\n\nI think this version of series is good and doesn't need a reroll.\n\nThanks,\n-Justin\n"},{"id":"535659","messageId":"CAOLa=ZQeDTFkVjJcmY8VOeR_F1E8c6dcc+fcMbUdcWcw2DPcGQ@mail.gmail.com","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-1-e682a003b17c@pks.im","subject":"Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-10T09:24:41Z","receivedAt":"2026-02-10T09:24:44Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Patrick Steinhardt writes:\n\n> The function `fill_missing_blobs()` receives an array of object IDs and\n> verifies for each of them whether the corresponding object exists. If it\n> doesn't exist, we add it to a set of objects and then batch-fetch all of\n> the objects at once.\n>\n> The check for whether or not we already have the object is broken\n> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\n> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n>\n> - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n> in case the object wasn't found. This makes sense, as we'd otherwise\n> reprepare the object database as many times as we have missing\n> objects.\n>\n> - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n> not fetch the object in case it's missing. Again, this makes sense,\n> as we want to batch-fetch the objects.\n>\n> This shows that we indeed want the equivalent of this flag, but of\n> course represented as `HAS_OBJECT_*` flags.\n>\n> Luckily, the code is already working correctly. The `OBJECT_INFO` flag\n> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\n> flags. And if no flags are passed, `odb_has_object()` ends up calling\n> `odb_read_object_info_extended()` with exactly the above two flags that\n> we wanted to set in the first place.\n>\n> Of course, this is pure luck, and this can break any moment. So let's\n> fix this and correct the code to not pass any flags at all.\n>\n\nWe do pass the same equivalent, no? I mean `OBJECT_INFO_FOR_PREFETCH`\ndoes resolve to `OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK` and\ncalling `odb_has_object(... , 0)` would also eventually set the same\nflags.\n\nI understand the issue, `odb_has_object()` should only take in\n`HAS_OBJECT_*` flags, even though internally it converts them to\n`OBJECT_INFO_*` flags.\n\nWouldn't it also be nicer to convert the enum for `HAS_OBJECT_*` to no\nlonger be anonymous and use that in `odb_has_object()`?\n\nThe patch itself looks good!\n\n- Karthik\n"},{"id":"535660","messageId":"CAOLa=ZQR4FryFF0NvX5TYZMWFDw_h8SL+aesv5S2Li=jgVEBew@mail.gmail.com","threadId":"64869","inReplyTo":"CAOLa=ZQeDTFkVjJcmY8VOeR_F1E8c6dcc+fcMbUdcWcw2DPcGQ@mail.gmail.com","subject":"Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2026-02-10T09:32:18Z","receivedAt":"2026-02-10T09:32:20Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Karthik Nayak writes:\n\n> Patrick Steinhardt writes:\n>\n>> The function `fill_missing_blobs()` receives an array of object IDs and\n>> verifies for each of them whether the corresponding object exists. If it\n>> doesn't exist, we add it to a set of objects and then batch-fetch all of\n>> the objects at once.\n>>\n>> The check for whether or not we already have the object is broken\n>> though: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\n>> expects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n>>\n>> - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n>> in case the object wasn't found. This makes sense, as we'd otherwise\n>> reprepare the object database as many times as we have missing\n>> objects.\n>>\n>> - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n>> not fetch the object in case it's missing. Again, this makes sense,\n>> as we want to batch-fetch the objects.\n>>\n>> This shows that we indeed want the equivalent of this flag, but of\n>> course represented as `HAS_OBJECT_*` flags.\n>>\n>> Luckily, the code is already working correctly. The `OBJECT_INFO` flag\n>> expands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\n>> flags. And if no flags are passed, `odb_has_object()` ends up calling\n>> `odb_read_object_info_extended()` with exactly the above two flags that\n>> we wanted to set in the first place.\n>>\n>> Of course, this is pure luck, and this can break any moment. So let's\n>> fix this and correct the code to not pass any flags at all.\n>>\n>\n> We do pass the same equivalent, no? I mean `OBJECT_INFO_FOR_PREFETCH`\n> does resolve to `OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK` and\n> calling `odb_has_object(... , 0)` would also eventually set the same\n> flags.\n>\n> I understand the issue, `odb_has_object()` should only take in\n> `HAS_OBJECT_*` flags, even though internally it converts them to\n> `OBJECT_INFO_*` flags.\n>\n> Wouldn't it also be nicer to convert the enum for `HAS_OBJECT_*` to no\n> longer be anonymous and use that in `odb_has_object()`?\n>\n> The patch itself looks good!\n>\n> - Karthik\n\nJust noticed that there is a similar discussion on the other patch in\nthis series. So will drop the discussion here.\n"},{"id":"535825","messageId":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im","subject":"[PATCH v2 0/5] Small fixups for `OBJECT_INFO` flags","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:36Z","receivedAt":"2026-02-12T06:59:50Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Hi,\n\nI was kind of curious why there were gaps in the `OBJECT_INFO_*` flags,\nbut eventually found out that these gaps are of historic nature: there\nused to be more flags, but their respective values got removed at one\npoint in time. So naturally, I wanted to clean this up a bit so that the\nnext reader wouldn't have the same question.\n\nSurprisingly though I found out that this breaks tests, which of course\npuzzled me. As it turns out though, we were incorrectly using a couple\nof these flags for `odb_has_object()`, and the changed definitions had\noverlap with the existing meaning of other `HAS_OBJECT_*` flags. There\nisn't really any bug here as far as I can see, but this is only really\nby chance.\n\nIn any case, the first two commits fix calls to `odb_has_object()` that\nused invalid flags. The last commit then removes the gaps and converts\nthe flags to use an enum instead.\n\nChanges in v2:\n - Add two patches on top that convert the object info and\n `odb_has_object()` flags into enums.\n - Link to v1: https://lore.kernel.org/r/20260126-b4-pks-read-object-info-flags-v1-0-e682a003b17c@pks.im\n\nThanks!\n\nPatrick\n\n---\nPatrick Steinhardt (5):\n builtin/backfill: fix flags passed to `odb_has_object()`\n builtin/fsck: fix flags passed to `odb_has_object()`\n odb: drop gaps in object info flag values\n odb: convert object info flags into an enum\n odb: convert `odb_has_object()` flags into an enum\n\n builtin/backfill.c | 3 +--\n builtin/fsck.c | 3 ++-\n object-file.c | 3 ++-\n object-file.h | 3 ++-\n odb.c | 4 ++--\n odb.h | 44 +++++++++++++++++++++++++-------------------\n packfile.c | 2 +-\n packfile.h | 2 +-\n 8 files changed, 36 insertions(+), 28 deletions(-)\n\nRange-diff versus v1:\n\n1: eec721a55b = 1: 308963f244 builtin/backfill: fix flags passed to `odb_has_object()`\n2: 317893853b = 2: 7a69a648bb builtin/fsck: fix flags passed to `odb_has_object()`\n3: c785043a72 < -: ---------- odb: drop gaps in object info flag values\n-: ---------- > 3: 0cea7f03f3 odb: drop gaps in object info flag values\n-: ---------- > 4: ab98547370 odb: convert object info flags into an enum\n-: ---------- > 5: 414dd30e14 odb: convert `odb_has_object()` flags into an enum\n\n---\nbase-commit: ea24e2c55433012a0a6c4ae947a87bc66404e484\nchange-id: 20260126-b4-pks-read-object-info-flags-236c4437cfc5\n\n"},{"id":"535826","messageId":"20260212-b4-pks-read-object-info-flags-v2-1-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","subject":"[PATCH v2 1/5] builtin/backfill: fix flags passed to `odb_has_object()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:37Z","receivedAt":"2026-02-12T06:59:52Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The function `fill_missing_blobs()` receives an array of object IDs and\nverifies for each of them whether the corresponding object exists. If it\ndoesn't exist, we add it to a set of objects and then batch-fetch all of\nthe objects at once.\n\nThe check for whether or not we already have the object is broken\nthough: we pass `OBJECT_INFO_FOR_PREFETCH`, but `odb_has_object()`\nexpects us to pass `HAS_OBJECT_*` flags. The flag expands to:\n\n - `OBJECT_INFO_QUICK`, which asks the object database to not reprepare\n in case the object wasn't found. This makes sense, as we'd otherwise\n reprepare the object database as many times as we have missing\n objects.\n\n - `OBJECT_INFO_SKIP_FETCH_OBJECT`, which asks the object database to\n not fetch the object in case it's missing. Again, this makes sense,\n as we want to batch-fetch the objects.\n\nThis shows that we indeed want the equivalent of this flag, but of\ncourse represented as `HAS_OBJECT_*` flags.\n\nLuckily, the code is already working correctly. The `OBJECT_INFO` flag\nexpands to `(1 << 3) | (1 << 4)`, none of which are valid `HAS_OBJECT`\nflags. And if no flags are passed, `odb_has_object()` ends up calling\n`odb_read_object_info_extended()` with exactly the above two flags that\nwe wanted to set in the first place.\n\nOf course, this is pure luck, and this can break any moment. So let's\nfix this and correct the code to not pass any flags at all.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/backfill.c | 3 +--\n 1 file changed, 1 insertion(+), 2 deletions(-)\n\ndiff --git a/builtin/backfill.c b/builtin/backfill.c\nindex e80fc1b694..d8cb3b0eba 100644\n--- a/builtin/backfill.c\n+++ b/builtin/backfill.c\n@@ -67,8 +67,7 @@ static int fill_missing_blobs(const char *path UNUSED,\n \t\treturn 0;\n \n \tfor (size_t i = 0; i < list->nr; i++) {\n-\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i],\n-\t\t\t\t OBJECT_INFO_FOR_PREFETCH))\n+\t\tif (!odb_has_object(ctx->repo->objects, &list->oid[i], 0))\n \t\t\toid_array_append(&ctx->current_batch, &list->oid[i]);\n \t}\n \n\n-- \n2.53.0.295.g64333814d3.dirty\n\n"},{"id":"535827","messageId":"20260212-b4-pks-read-object-info-flags-v2-2-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","subject":"[PATCH v2 2/5] builtin/fsck: fix flags passed to `odb_has_object()`","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:38Z","receivedAt":"2026-02-12T06:59:54Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"In `mark_object()` we invoke `has_object()` with a value of 1. This is\nsomewhat fishy given that the function expects a bitset of flags, so any\nbehaviour that this results in is purely coincidental and may break at\nany point in time.\n\nThe call to `has_object()` was originally introduced in 9eb86f41de\n(fsck: do not lazy fetch known non-promisor object, 2020-08-05). The\nintent here was to skip lazy fetches of promisor objects: we have\nalready verified that the object is not a promisor object, so if the\nobject is missing it indicates a corrupt repository.\n\nThe hardcoded value that we pass maps to `HAS_OBJECT_RECHECK_PACKED`,\nwhich is probably the intended behaviour: `odb_has_object()` will not\nfetch promisor objects unless `HAS_OBJECT_FETCH_PROMISOR` is passed, but\nwe may want to verify that no concurrent process has written the object\nthat we're trying to read.\n\nConvert the code to use the named flag instead of the the hardcoded\nvalue.\n\nSigned-off-by: Patrick Steinhardt \n---\n builtin/fsck.c | 3 ++-\n 1 file changed, 2 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/fsck.c b/builtin/fsck.c\nindex 0512f78a87..1d059dd6c2 100644\n--- a/builtin/fsck.c\n+++ b/builtin/fsck.c\n@@ -162,7 +162,8 @@ static int mark_object(struct object *obj, enum object_type type,\n \t\treturn 0;\n \n \tif (!(obj->flags & HAS_OBJ)) {\n-\t\tif (parent && !odb_has_object(the_repository->objects, &obj->oid, 1)) {\n+\t\tif (parent && !odb_has_object(the_repository->objects, &obj->oid,\n+\t\t\t\t\t HAS_OBJECT_RECHECK_PACKED)) {\n \t\t\tprintf_ln(_(\"broken link from %7s %s\\n\"\n \t\t\t\t \" to %7s %s\"),\n \t\t\t\t printable_type(&parent->oid, parent->type),\n\n-- \n2.53.0.295.g64333814d3.dirty\n\n"},{"id":"535828","messageId":"20260212-b4-pks-read-object-info-flags-v2-3-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","subject":"[PATCH v2 3/5] odb: drop gaps in object info flag values","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:39Z","receivedAt":"2026-02-12T06:59:57Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"The object info flag values have a two gaps in their definitions, where\nsome bits are skipped over. These gaps don't really hurt, but it makes\none wonder whether anything is going on and whether a subset of flags\nmight be defined somewhere else.\n\nThat's not the case though. Instead, this is a case of flags that have\nbeen dropped in the past:\n\n - The value 4 was used by `OBJECT_INFO_SKIP_CACHED`, removed in\n 9c8a294a1a (sha1-file: remove OBJECT_INFO_SKIP_CACHED, 2020-01-02).\n\n - The value 8 was used by `OBJECT_INFO_ALLOW_UNKNOWN_TYPE`, removed in\n ae24b032a0 (object-file: drop OBJECT_INFO_ALLOW_UNKNOWN_TYPE flag,\n 2025-05-16).\n\nClose those gaps to avoid any more confusion.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.h | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/odb.h b/odb.h\nindex bab07755f4..8e1fca7755 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -353,14 +353,14 @@ struct object_info {\n #define OBJECT_INFO_INIT { 0 }\n \n /* Invoke lookup_replace_object() on the given hash */\n-#define OBJECT_INFO_LOOKUP_REPLACE 1\n+#define OBJECT_INFO_LOOKUP_REPLACE (1 << 0)\n /* Do not retry packed storage after checking packed and loose storage */\n-#define OBJECT_INFO_QUICK 8\n+#define OBJECT_INFO_QUICK (1 << 1)\n /*\n * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n * nonzero).\n */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT 16\n+#define OBJECT_INFO_SKIP_FETCH_OBJECT (1 << 2)\n /*\n * This is meant for bulk prefetching of missing blobs in a partial\n * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n@@ -368,7 +368,7 @@ struct object_info {\n #define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n \n /* Die if object corruption (not just an object being missing) was detected. */\n-#define OBJECT_INFO_DIE_IF_CORRUPT 32\n+#define OBJECT_INFO_DIE_IF_CORRUPT (1 << 3)\n \n /*\n * Read object info from the object database and populate the `object_info`\n\n-- \n2.53.0.295.g64333814d3.dirty\n\n"},{"id":"535829","messageId":"20260212-b4-pks-read-object-info-flags-v2-4-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","subject":"[PATCH v2 4/5] odb: convert object info flags into an enum","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:40Z","receivedAt":"2026-02-12T07:00:00Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Convert the object info flags into an enum and adapt all functions that\nreceive these flags as parameters to use the enum instead of an integer.\nThis serves two purposes:\n\n - The function signatures become more self-documenting, as callers\n don't have to wonder which flags they expect.\n\n - The compiler can warn when a wrong flag type is passed.\n\nNote that the second benefit is somewhat limited. For example, when\nor-ing multiple enum flags together the result will be an integer, and\nthe compiler will not warn about such use cases. But where it does help\nis when a single flag of the wrong type is passed, as the compiler would\ngenerate a warning in that case.\n\nSigned-off-by: Patrick Steinhardt \n---\n object-file.c | 3 ++-\n object-file.h | 3 ++-\n odb.c | 2 +-\n odb.h | 40 +++++++++++++++++++++++-----------------\n packfile.c | 2 +-\n packfile.h | 2 +-\n 6 files changed, 30 insertions(+), 22 deletions(-)\n\ndiff --git a/object-file.c b/object-file.c\nindex e7e4c3348f..0ab6c4d4f3 100644\n--- a/object-file.c\n+++ b/object-file.c\n@@ -414,7 +414,8 @@ static int parse_loose_header(const char *hdr, struct object_info *oi)\n \n int odb_source_loose_read_object_info(struct odb_source *source,\n \t\t\t\t const struct object_id *oid,\n-\t\t\t\t struct object_info *oi, int flags)\n+\t\t\t\t struct object_info *oi,\n+\t\t\t\t enum object_info_flags flags)\n {\n \tint ret;\n \tint fd;\ndiff --git a/object-file.h b/object-file.h\nindex 1229d5f675..cdb54b5218 100644\n--- a/object-file.h\n+++ b/object-file.h\n@@ -47,7 +47,8 @@ void odb_source_loose_reprepare(struct odb_source *source);\n \n int odb_source_loose_read_object_info(struct odb_source *source,\n \t\t\t\t const struct object_id *oid,\n-\t\t\t\t struct object_info *oi, int flags);\n+\t\t\t\t struct object_info *oi,\n+\t\t\t\t enum object_info_flags flags);\n \n int odb_source_loose_read_object_stream(struct odb_read_stream **out,\n \t\t\t\t\tstruct odb_source *source,\ndiff --git a/odb.c b/odb.c\nindex ac70b6a099..d437aa8b06 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -842,7 +842,7 @@ static int oid_object_info_convert(struct repository *r,\n int odb_read_object_info_extended(struct object_database *odb,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t struct object_info *oi,\n-\t\t\t\t unsigned flags)\n+\t\t\t\t enum object_info_flags flags)\n {\n \tint ret;\n \ndiff --git a/odb.h b/odb.h\nindex 8e1fca7755..e94cdc3665 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -352,23 +352,29 @@ struct object_info {\n */\n #define OBJECT_INFO_INIT { 0 }\n \n-/* Invoke lookup_replace_object() on the given hash */\n-#define OBJECT_INFO_LOOKUP_REPLACE (1 << 0)\n-/* Do not retry packed storage after checking packed and loose storage */\n-#define OBJECT_INFO_QUICK (1 << 1)\n-/*\n- * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n- * nonzero).\n- */\n-#define OBJECT_INFO_SKIP_FETCH_OBJECT (1 << 2)\n-/*\n- * This is meant for bulk prefetching of missing blobs in a partial\n- * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK\n- */\n-#define OBJECT_INFO_FOR_PREFETCH (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK)\n+/* Flags that can be passed to `odb_read_object_info_extended()`. */\n+enum object_info_flags {\n+\t/* Invoke lookup_replace_object() on the given hash. */\n+\tOBJECT_INFO_LOOKUP_REPLACE = (1 << 0),\n+\n+\t/* Do not reprepare object sources when the first lookup has failed. */\n+\tOBJECT_INFO_QUICK = (1 << 1),\n+\n+\t/*\n+\t * Do not attempt to fetch the object if missing (even if fetch_is_missing is\n+\t * nonzero).\n+\t */\n+\tOBJECT_INFO_SKIP_FETCH_OBJECT = (1 << 2),\n+\n+\t/* Die if object corruption (not just an object being missing) was detected. */\n+\tOBJECT_INFO_DIE_IF_CORRUPT = (1 << 3),\n \n-/* Die if object corruption (not just an object being missing) was detected. */\n-#define OBJECT_INFO_DIE_IF_CORRUPT (1 << 3)\n+\t/*\n+\t * This is meant for bulk prefetching of missing blobs in a partial\n+\t * clone. Implies OBJECT_INFO_SKIP_FETCH_OBJECT and OBJECT_INFO_QUICK.\n+\t */\n+\tOBJECT_INFO_FOR_PREFETCH = (OBJECT_INFO_SKIP_FETCH_OBJECT | OBJECT_INFO_QUICK),\n+};\n \n /*\n * Read object info from the object database and populate the `object_info`\n@@ -377,7 +383,7 @@ struct object_info {\n int odb_read_object_info_extended(struct object_database *odb,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t struct object_info *oi,\n-\t\t\t\t unsigned flags);\n+\t\t\t\t enum object_info_flags flags);\n \n /*\n * Read a subset of object info for the given object ID. Returns an `enum\ndiff --git a/packfile.c b/packfile.c\nindex 402c3b5dc7..cb418846ae 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -2149,7 +2149,7 @@ int packfile_store_freshen_object(struct packfile_store *store,\n int packfile_store_read_object_info(struct packfile_store *store,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t struct object_info *oi,\n-\t\t\t\t unsigned flags UNUSED)\n+\t\t\t\t enum object_info_flags flags UNUSED)\n {\n \tstruct pack_entry e;\n \tint ret;\ndiff --git a/packfile.h b/packfile.h\nindex acc5c55ad5..989fd10cb6 100644\n--- a/packfile.h\n+++ b/packfile.h\n@@ -247,7 +247,7 @@ int packfile_store_read_object_stream(struct odb_read_stream **out,\n int packfile_store_read_object_info(struct packfile_store *store,\n \t\t\t\t const struct object_id *oid,\n \t\t\t\t struct object_info *oi,\n-\t\t\t\t unsigned flags);\n+\t\t\t\t enum object_info_flags flags);\n \n /*\n * Open the packfile and add it to the store if it isn't yet known. Returns\n\n-- \n2.53.0.295.g64333814d3.dirty\n\n"},{"id":"535830","messageId":"20260212-b4-pks-read-object-info-flags-v2-5-3bfa9bb149ef@pks.im","threadId":"64869","inReplyTo":"20260212-b4-pks-read-object-info-flags-v2-0-3bfa9bb149ef@pks.im","subject":"[PATCH v2 5/5] odb: convert `odb_has_object()` flags into an enum","fromName":"Patrick Steinhardt","fromEmail":"ps@pks.im","sentAt":"2026-02-12T06:59:41Z","receivedAt":"2026-02-12T07:00:04Z","isPatch":true,"sender":{"key":"ps@pks.im","avatar":"https://avatars.githubusercontent.com/u/4056630?v=4"},"body":"Following the reason in the preceding commit, convert the\n`odb_has_object()` flags into an enum.\n\nWith this change, we would have catched the misuse of `odb_has_object()`\nthat was fixed in a preceding commit as the compiler would have\ngenerated a warning:\n\n ../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]\n 70 | if (!odb_has_object(ctx->repo->objects, &list->oid[i],\n | ~~~~~~~~~~~~~~\n 71 | OBJECT_INFO_FOR_PREFETCH))\n | ^~~~~~~~~~~~~~~~~~~~~~~~\n 1 error generated.\n\nSigned-off-by: Patrick Steinhardt \n---\n odb.c | 2 +-\n odb.h | 4 ++--\n 2 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/odb.c b/odb.c\nindex d437aa8b06..2bbbfb344a 100644\n--- a/odb.c\n+++ b/odb.c\n@@ -964,7 +964,7 @@ void *odb_read_object_peeled(struct object_database *odb,\n }\n \n int odb_has_object(struct object_database *odb, const struct object_id *oid,\n-\t unsigned flags)\n+\t\t enum has_object_flags flags)\n {\n \tunsigned object_info_flags = 0;\n \ndiff --git a/odb.h b/odb.h\nindex e94cdc3665..f7368827ac 100644\n--- a/odb.h\n+++ b/odb.h\n@@ -395,7 +395,7 @@ int odb_read_object_info(struct object_database *odb,\n \t\t\t const struct object_id *oid,\n \t\t\t unsigned long *sizep);\n \n-enum {\n+enum has_object_flags {\n \t/* Retry packed storage after checking packed and loose storage */\n \tHAS_OBJECT_RECHECK_PACKED = (1 << 0),\n \t/* Allow fetching the object in case the repository has a promisor remote. */\n@@ -408,7 +408,7 @@ enum {\n */\n int odb_has_object(struct object_database *odb,\n \t\t const struct object_id *oid,\n-\t\t unsigned flags);\n+\t\t enum has_object_flags flags);\n \n int odb_freshen_object(struct object_database *odb,\n \t\t const struct object_id *oid);\n\n-- \n2.53.0.295.g64333814d3.dirty\n\n"}]}