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

> Junio C Hamano <gitster@pobox.com> writes:
>
>> 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.
>
> ... because having to strcmp() their names would be much more
> expensive than the pointer comparison.
>
> However, I think you could use a pointer into a single array as
> an element of prio_queue.  I notice here:
>
>         for (; suc->current < suc->list.nr; suc->current++) {
> -               const struct cache_entry *ce = suc->list.entries[suc->current];
> +               ce = suc->list.entries[suc->current];
>                 if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
> +                       *ce_task_cb = (struct cache_entry *) ce;
>                         suc->current++;
>                         return 1;
>                 }
>         }
>
> that list.entries[] can serve as such an array.  If you pass around
> the pointer to its element instead, i.e.
>
> -               ce = suc->list.entries[suc->current];
> +               ceP = &suc->list.entries[suc->current];
> -               if (prepare_to_clone_next_submodule(ce, child, suc, err)) {
> +               if (prepare_to_clone_next_submodule(*ceP, child, suc, err)) {
> +                       *ce_task_cb = (struct cache_entry *) ce;
> -                       *ce_task_cb = ceP;
>                 ...
>         }
>         /*
>          * The loop above tried cloning each submodule once,
>          * now try the stragglers again.
>          */
> -       ce = (struct cache_entry *) prio_queue_get(&suc->failed_queue);
> +       ceP = (struct cache_entry **) prio_queue_get(&suc->failed_queue);
>
> then the elements you are pushing into prio-queue would not be ce
> (pointer to a cache entry), but would be a pointer of an array that
> holds many pointers to cache entries, so it becomes kosher to
> compare them for ordering.
>
> That would probably not add too much overhead at runtime; it may
> have to involve a bit of code restructuring, so I do not know if it
> is worth it, though.
>