On Thu, Jun 9, 2016 at 1:40 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Junio C Hamano <gitster@xxxxxxxxx> writes:
>
>> Stefan Beller <sbeller@xxxxxxxxxx> 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.
>
I realized the patch above was bogus for another reason:
In update_clone_task_finished we never distinguish between first-failing
submodules and repeated fails, such that we may end up in an infinite loop.
So I think the easiest way forward would be if we would pass around the index
and then we can properly determine if we tried that already. (Though casting
pointer to int is not possible, so we'll pass around a pointer to an int)
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html