On Fri, Mar 14, 2025 at 7:59 PM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > +test_expect_success "clone with 'KnownName' and missing URL in the config" ' > > + git -C server config promisor.advertise true && > > + > > + # Clone from server to create a client > > + # Lazy fetching by the client from the LOP will fail because of the > > + # missing URL in the client config, so the server will have to lazy > > + # fetch from the LOP. > > + GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ > > + -c promisor.acceptfromserver=KnownName \ > > + --no-local --filter="blob:limit=5k" server client && > > + test_when_finished "rm -rf client" && > > These are the other way around. When 'clone' fails, test_when_finished > is not run, so nobody arranges the new directory 'client' to be removed. > "git clone" does try to remove in such a case, but we are protecting > against a failing "clone", so swapping them around, i.e. arrange to > remove it and then try to create it, would make more sense. Yeah, right. I made this change in the next version. I also think it would make more sense for all the tests in this test script to arrange to remove it before cloning in case the clone fails in weird ways. So I am adding a preparatory patch to do that in the next version then. > > +test_expect_success "clone with 'KnownUrl' and url not configured on the server" ' > > + git -C server config promisor.advertise true && > > + > > + git -C server config unset remote.lop.url && > > + test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" && > > Probably the same principle applies here, but the case where "git > config" fails, it is likely that the file is not touched at all, or > it gets so corrupt beyond salvaging with another "config set", so it > matters much less than the previous one. I agree it would be a bit better, so, in the next version, I moved the "test_when_finished ..." line above the other one. I did that for the "clone with 'KnownUrl' and empty url, so not advertised" test below too. While at it, I think it's also a bit better to add `test_when_finished "rm -rf client"` to these 2 tests, to protect the following test in case the clone actually succeeds. So I have added that in the next version. Thanks.
