Jonathan Tan <jonathantanmy@xxxxxxxxxx> writes: > Thanks everyone for your comments. I don't think that there is a test > for when a lazy fetch fails, for some reason, so I added one. > > Jonathan Tan (2): > promisor-remote: remove a return value > promisor-remote: die upon failing fetch > > object-file.c | 4 ---- > promisor-remote.c | 23 ++++++++++++++--------- > promisor-remote.h | 11 +++++------ > t/t0410-partial-clone.sh | 14 ++++++++++++++ > 4 files changed, 33 insertions(+), 19 deletions(-) Thanks. Will replace. I think the C/H code was good already in the first round, so hopefully this can go 'next' early in this cycle. > > Range-diff against v1: > 1: fdb35bc9a6 ! 1: 207332c2df promisor-remote: remove a return value > @@ Commit message > No caller of promisor_remote_get_direct() is checking its return value, > so remove it. > > + Not checking the return value means that the user would not know > + whether the failure of reading an object is due to the promisor remote > + not supplying the object or because of local repository corruption, but > + this will be fixed in a subsequent patch. > + > Signed-off-by: Jonathan Tan <jonathantanmy@xxxxxxxxxx> > Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> > > 2: 06a649b01a ! 2: 9bbf130c2c promisor-remote: die upon failing fetch > @@ Commit message > When this batch prefetch fails, these commands fall back to the > sequential fetches. But at $DAYJOB we have noticed that this results in > a bad user experience: a command would take unexpectedly long to finish > - if the batch prefetch would fail for some intermittent reason, but all > - subsequent fetches would work. It would be a better user experience for > - such a command would just fail. > + (and possibly use up a lot of bandwidth) if the batch prefetch would > + fail for some intermittent reason, but all subsequent fetches would > + work. It would be a better user experience for such a command would > + just fail. > > Therefore, make it a fatal error if the prefetch fails and at least one > object being fetched is known to be a promisor object. (The latter > @@ promisor-remote.c: void promisor_remote_get_direct(struct repository *repo, > if (to_free) > free(remaining_oids); > } > + > + ## t/t0410-partial-clone.sh ## > +@@ t/t0410-partial-clone.sh: test_expect_success 'fetching of missing objects' ' > + grep "$HASH" out > + ' > + > ++test_expect_success 'fetching of a promised object that promisor remote no longer has' ' > ++ rm -f err && > ++ test_create_repo unreliable-server && > ++ git -C unreliable-server config uploadpack.allowanysha1inwant 1 && > ++ git -C unreliable-server config uploadpack.allowfilter 1 && > ++ test_commit -C unreliable-server foo && > ++ > ++ git clone --filter=blob:none --no-checkout "file://$(pwd)/unreliable-server" unreliable-client && > ++ > ++ rm -rf unreliable-server/.git/objects/* && > ++ test_must_fail git -C unreliable-client checkout HEAD 2>err && > ++ grep "could not fetch.*from promisor remote" err > ++' > ++ > + test_expect_success 'fetching of missing objects works with ref-in-want enabled' ' > + # ref-in-want requires protocol version 2 > + git -C server config protocol.version 2 &&
