On 5/30/2019 4:46 PM, Johannes Schindelin wrote:

Hi,

On Thu, 30 May 2019, Derrick Stolee wrote:

quoted

On 4/9/2019 12:11 PM, Christian Couder wrote:

quoted

From: Christian Couder <redacted>

+{
+	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