Re: [PATCH 1/3] builtin/backfill: fix flags passed to `odb_has_object()`

Karthik Nayak [off-list ref] writes:

Patrick Steinhardt [off-list ref] writes:

quoted

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

Just noticed that there is a similar discussion on the other patch in
this series. So will drop the discussion here.