Re: [PATCH 1/2] submodule--helper: initial clone learns retry logic

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 9, 2016, 19:59 UTC
Message-ID: <xmqqtwh2gmlv.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAGZ79kbpurbwAB4H5_JPMsaGHmbeZA=EWjMOfQGB6apLhEXVeA@mail.gmail.com>

Stefan Beller <sbeller@google.com> writes:

Show 7 quoted lines

> instead. But that is still unspecified, so we rather go with
>
> static int compare_ce(const void *one, const void *two, void *cb_data)
> {
>     const struct cache_entry *ce_one = one, *ce_two = two;
>     return strcmp(ce_one->name, ce_two->name);
> }

As I said below, I do not think it is worth addressing by making the code's behaviour on real systems worse. As long as what you have as the key into priority queue is a pointer to cache_entry, you cannot make it better from that point of view.

>> I think we have one or two other instances of such fishy pointer
>> comparison already in the codebase, so it is not a show-stopper, but
>> it would be better if this can be avoided and be replaced with
>> something that I do not have to raise eyebrows at ;-)

If you are using priority queue, it probably would make more sense to use the "priority" part properly---the paths whose ce happens to be lower in the address space are not inherently more important and deserve to be processed sooner. From "let's retry failed ones after finishing", I expected that the queue was either fifo (simplest), or a prio queue whose priority function would give lower priority for entries with least number of previous attempts (more involved but probably more fair).

So a proper "fix" probably is either (1) not to use prio queue as you are not doing anything with priority anyway, or (2) use something other than ce itself as entries in prio queue, so that the priority function can be computation that is more meaningful than "happens to sit in the lower part of the address space".