Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()

[Junio C Hamano <gitster@pobox.com>]

Derrick Stolee <stolee@gmail.com> writes:

>>> This global 'fetch_if_missing' swap seems very fragile. I'm guessing you
>>> are using it to prevent a loop when calling oid_object_info_extended()
>>> below. Can you instead pass a flag to the method that disables the
>>> fetch_if_missing behavior?
>>  ...
>> The flag is actually used only in `oid_object_info_extended()`, and that
>> function accepts an `unsigned flags`, so one might think that it could be
>> extended to accept also a `OBJECT_INFO_LOOKUP_FETCH_IF_MISSING`. But then,
>> there are many callers of that function, some of them also pretty low in
>> the food chain. For example, `oid_object_info()` (does not accept `flags`)
>> or `read_object()` (does not accept flags either).
>>
>> So it looks as if the idea to pass this flag down the call chain entailed
>> a pretty serious avalanche effect.
>
> It could be approached in small bits.
>
> First, add an OBJECT_INFO_NEVER_FETCH_IF_MISSING flag that overrides fetch_if_missing,
> and then use the flag in small places like this one. Then, build up to the other
> methods as appropriate.
>
>> An alternative that strikes me as inelegant, still, but nevertheless
>> better would be to move `fetch_if_missing` into `struct repository`.
>
> This is literally the _least_ we should do to reduce our dependence on
> globals. Maybe this happens first, then the flag idea could be done bits
> at a time.

The bit is not an attribute of a repository instance, and I agree it
is an ugly hack to take advantage of an unrelated fact that a repo
is getting passed throughout the codechain.  It is better than
nothing if we stop there and will not do anything more to the topic,
but in the longer term, it is not that better than a global, I am
afraid.  We may not be doing the save-flip-and-restore-the-bit dance
on the global anymore, but instead would be doing the same for the
field in the repository object, no?

In any case, thanks for taking a look at the topic; what it wants to
achieve is worthwhile, but its execution does look like it needs
quite a lot more polishing, which is helped by review comments like
these.