Re: [PATCH v5 04/16] promisor-remote: implement promisor_remote_get_direct()
- From
Derrick Stolee <stolee@gmail.com>
- Date
- May 30, 2019, 20:54 UTC
- Message-ID
- <2329a4c7-bfb9-10ce-9d1c-8c754d6dee05@gmail.com>
- In-Reply-To
- <nycvar.QRO.7.76.6.1905302235000.44@tvgsbejvaqbjf.bet>
On 5/30/2019 4:46 PM, Johannes Schindelin wrote:
Show 36 quoted lines
> Hi,
>
> On Thu, 30 May 2019, Derrick Stolee wrote:
>
>> On 4/9/2019 12:11 PM, Christian Couder wrote:
>>> From: Christian Couder <christian.couder@gmail.com>
>>>
>>> +{
>>> + int i, missing_nr = 0;
>>> + int *missing = xcalloc(oid_nr, sizeof(*missing));
>>> + struct object_id *old_oids = *oids;
>>> + struct object_id *new_oids;
>>> + int old_fetch_if_missing = fetch_if_missing;
>>> +
>>> + fetch_if_missing = 0;
>>
>> 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?
>
> FWIW I mentioned the very same concern here:
> https://public-inbox.org/git/nycvar.QRO.7.76.6.1903272300020.41@tvgsbejvaqbjf.bet/
>
> The situation is *pretty* bad by now. I see `fetch_if_missing` mentioned
> 25 times in `master`, and all but one are in .c files or in cache.h.
>
> 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.
Thanks, -Stolee