Hi,

On Thu, 30 May 2019, Derrick Stolee wrote:

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.

An alternative that strikes me as inelegant, still, but nevertheless
better would be to move `fetch_if_missing` into `struct repository`.

Ciao,
Dscho