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

[Junio C Hamano <gitster@pobox.com>]

Stefan Beller <sbeller@google.com> writes:

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