Subject: Re: [PATCH 4/8] bundle-uri: download in creationToken order

From: Derrick Stolee <[email-masked]>
Date: Mon, 9 Jan 2023 09:58:54 -0500

On 1/8/2023 10:22 PM, Junio C Hamano wrote:
> "Derrick Stolee via GitGitGadget" <[email-masked]> 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