Re: [PATCH v2 2/8] fetch-pack: add repairing
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Feb 25, 2022, 06:46 UTC
- Message-ID
- <xmqqwnhjfnpy.fsf@gitster.g>
- In-Reply-To
- <2d817a65db55750d27986c2e2ab5b1723d17081e.1645719218.git.gitgitgadget@gmail.com>
"Robert Coup via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 7 quoted lines
> @@ -694,6 +696,9 @@ static void mark_complete_and_common_ref(struct fetch_negotiator *negotiator, > > save_commit_buffer = 0; > > + if (args->repair) > + return; > +
Reading how the original value of save_commit_buffer is saved away, the variable gets cleared and then gets restored before the function returns in the normal codepath, this new code looks wrong. Hitting this early return after clearing the variable means nobody will restore the saved value of the variable, no?
Show 6 quoted lines
> @@ -1027,9 +1032,6 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args, > struct fetch_negotiator negotiator_alloc; > struct fetch_negotiator *negotiator; > > - negotiator = &negotiator_alloc; > - fetch_negotiator_init(r, negotiator);
I know why you want to force the "noop" negotiator while repairing, but it is unclear why you need to move this down in the function.
Show 13 quoted lines
> sort_ref_list(&ref, ref_compare_name);
> QSORT(sought, nr_sought, cmp_ref_by_name);
>
> @@ -1119,9 +1121,16 @@ static struct ref *do_fetch_pack(struct fetch_pack_args *args,
> if (!server_supports_hash(the_hash_algo->name, NULL))
> die(_("Server does not support this repository's object format"));
>
> + negotiator = &negotiator_alloc;
> + if (args->repair) {
> + fetch_negotiator_init_noop(negotiator);
> + } else {
> + fetch_negotiator_init(r, negotiator);
> + }Hmph. I am debating myself if hardcoding the implementation detail of "when repairing, the noop negitiator is the only useful one" like this code does is a sensible thing to do. If we later need to tweak the choice of negotiator used depending on the caller's needs, perhaps fetch_negotiator_init() should gain a new flags word, i.e.
fetch_negotiator_init(struct repository *, struct fetch_negotiator *, unsigned flags)
where "Use negotiator suitable for the repairing fetch" could be a single bit in the flags word, making this caller more like:
negotiator = &negotiator_alloc; flags = 0; if (args->repair) flags |= FETCH_NEGOTIATOR_REPAIRING; fetch_negotiator_init(r, negotiator, flags);
perhaps. That way, [1/8] becomes unnecessary.
Show 16 quoted lines
> mark_complete_and_common_ref(negotiator, args, &ref);
> filter_refs(args, &ref, sought, nr_sought);
> - if (everything_local(args, &ref)) {
> + if (!args->repair && everything_local(args, &ref)) {
> packet_flush(fd[1]);
> goto all_done;
> }
> @@ -1587,7 +1596,10 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> struct strvec index_pack_args = STRVEC_INIT;
>
> negotiator = &negotiator_alloc;
> - fetch_negotiator_init(r, negotiator);
> + if (args->repair)
> + fetch_negotiator_init_noop(negotiator);
> + else
> + fetch_negotiator_init(r, negotiator);Likewise.
Show 23 quoted lines
> packet_reader_init(&reader, fd[0], NULL, 0,
> PACKET_READ_CHOMP_NEWLINE |
> @@ -1613,7 +1625,7 @@ static struct ref *do_fetch_pack_v2(struct fetch_pack_args *args,
> /* Filter 'ref' by 'sought' and those that aren't local */
> mark_complete_and_common_ref(negotiator, args, &ref);
> filter_refs(args, &ref, sought, nr_sought);
> - if (everything_local(args, &ref))
> + if (!args->repair && everything_local(args, &ref))
> state = FETCH_DONE;
> else
> state = FETCH_SEND_REQUEST;
> diff --git a/fetch-pack.h b/fetch-pack.h
> index 7f94a2a5831..bbb663edda8 100644
> --- a/fetch-pack.h
> +++ b/fetch-pack.h
> @@ -42,6 +42,7 @@ struct fetch_pack_args {
> unsigned update_shallow:1;
> unsigned reject_shallow_remote:1;
> unsigned deepen:1;
> + unsigned repair:1;
>
> /*
> * Indicate that the remote of this request is a promisor remote. The