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

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

Hi Stolee,

On Thu, 30 May 2019, Derrick Stolee wrote:

> On 5/30/2019 4:46 PM, Johannes Schindelin wrote:
> >
> > 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.

That is a good idea. I fear that it will still take a Herculean effort to
get there, as some of the call paths strike me as rather deep...

> > 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.

Okay, then, I added https://github.com/gitgitgadget/git/issues/251 so we
won't forget.

BTW I am rather happy about the way the GitGitGadget issues turn out: I
added a couple of left-over bits, and could already close two tickets
after other developers pointed out that they had already been addressed,
something an unsuspecting GSoC student, for example, could not otherwise
have found out very easily (or for that matter, I myself...).

Ciao,
Dscho