On 10/04/2021 13:48, René Scharfe wrote:
Am 10.04.21 um 10:30 schrieb Andrzej Hunt via GitGitGadget:[...] diff --git a/merge-ort.c b/merge-ort.c index 5e118a85ee04..4da4b4688336 100644 --- a/merge-ort.c +++ b/merge-ort.c @@ -2504,8 +2504,10 @@ static void write_tree(struct object_id *result_oid, * We won't use relevant_entries again and will let it just pop off the * stack, so there won't be allocation worries or anything. */ - relevant_entries.items = versions->items + offset; - relevant_entries.nr = versions->nr - offset; + if (versions->nr) { + relevant_entries.items = versions->items + offset; + relevant_entries.nr = versions->nr - offset; + } QSORT(relevant_entries.items, relevant_entries.nr, tree_entry_order);Reading the diff I was wondering if QSORT now gets handed uninitialized values if version-nr is 0. The answer is no -- relevant_entries is initialized at declaration. Otherwise the compiler would have probably warned, but sometimes it gets confused. I wonder why relevant_entries is introduced at all, though. It's not referenced later. So how about this instead? if (versions->nr) QSORT(versions->items + offset, nr, tree_entry_order); The intent to sort the last versions->nr-offset entries of versions, but only if it's not empty, is easier to see like this, I think.
That is much more elegant, I will follow this approach. Thank you for the suggestion!
An alternative might be to keep relevant_entries, and replace all later usages of versions->items[offset+i] with relevant_entries.items[i], but that's more invasive and I don't see any good reason for doing so given that the existing pattern works fine.
