Re: [PATCH 4/8] bundle-uri: download in creationToken order
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 9, 2023, 03:22 UTC
- Message-ID
- <xmqqtu10wfwt.fsf@gitster.g>
- In-Reply-To
- <57c0174d3752fb61a05e0653de9d3057616ed16a.1673037405.git.gitgitgadget@gmail.com>
"Derrick Stolee via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 12 quoted lines
> +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.
Show 11 quoted lines
> +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.
Show 19 quoted lines
> +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.