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

From: Junio C Hamano <gitster@pobox.com>
To: "Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, vdye@github.com,
	avarab@gmail.com, steadmon@google.com, chooglen@google.com,
	Derrick Stolee <derrickstolee@github.com>
Subject: Re: [PATCH 4/8] bundle-uri: download in creationToken order
Date: Mon, 09 Jan 2023 12:22:26 +0900	[thread overview]
Message-ID: <xmqqtu10wfwt.fsf@gitster.g> (raw)
In-Reply-To: <57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com> (Derrick Stolee via GitGitGadget's message of "Fri, 06 Jan 2023 20:36:41 +0000")

"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> 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.

> +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.

> +static int fetch_bundles_by_token(struct repository *r,
> +				  struct bundle_list *list)
> +{
> +	int cur;
> +	int pop_or_push = 0;
> +	struct bundle_list_context ctx = {
> +		.r = r,
> +		.list = list,
> +		.mode = list->mode,
> +	};
> +	struct sorted_bundle_list sorted = {
> +		.alloc = hashmap_get_size(&list->bundles),
> +	};
> +
> +	ALLOC_ARRAY(sorted.items, sorted.alloc);
> +
> +	for_all_bundles_in_list(list, insert_bundle, &sorted);
> +
> +	QSORT(sorted.items, sorted.nr, compare_creation_token);

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.