On 1/8/2023 10:22 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
>
>> +struct sorted_bundle_list {
>> + struct remote_bundle_info **items;
>> + size_t alloc;
>> + size_t nr;
>> +};
>> +
>> +static int insert_bundle(struct remote_bundle_info *bundle, void *data)
>> +{
>> + struct sorted_bundle_list *list = data;
>> + list->items[list->nr++] = bundle;
>> + return 0;
>> +}
>
> Especially given that the type of the list claims it to be "sorted",
> insert_bundle() is a misleading name for a helper that merely
> appends to it to make the list (tentatively) unsorted.
>
> I am not opposed to "append all to make an unsorted list, then sort
> the list at the end" strategy.
...
> If I were doing this patch, I would call the type of the list of
> bundles "struct bundle_list" (without "sorted_" in its name), and
> name the variable of that type used here "sorted". That would make
> it more clear that this particular bundle list starts its life as
> unsorted (with "append_bundle" function adding more elements) and
> then gets sorted in the end, from the above several lines.
Since "struct bundle_list" is taken, how about "bundles_for_sorting"
since that's the purpose of this struct (to be passed as data to
the for_all_bundles_in_list() and then to QSORT()).
Renaming insert_bundle() to append_bundle() is clearly better.
>> +static int compare_creation_token(const void *va, const void *vb)
>> +{
>> + const struct remote_bundle_info * const *a = va;
>> + const struct remote_bundle_info * const *b = vb;
>> +
>> + if ((*a)->creationToken > (*b)->creationToken)
>> + return -1;
>> + if ((*a)->creationToken < (*b)->creationToken)
>> + return 1;
>> + return 0;
>> +}
>
> Usually compare(a,b) returns the sign of (a-b), but the returned
> value from the above is the opposite. This is because we want the
> list sorted from newer to older? It may help developers to name
> such a (reverse) "compare" function differently.
Would renaming this to "compare_creation_token_decreasing" be clear
enough? (Plus a doc comment.)
Thanks,
-Stolee
