| Message ID | 20250314141203.2548803-2-christian.couder@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | "promisor-remote" capability fixes | expand |
Christian Couder <christian.couder@gmail.com> writes: > While at it, let's also use git_config_get_string_tmp() instead of > git_config_get_string() to simplify memory management. > ... > - strvec_push(names, r->name); > - strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url); > + /* Only add remotes with a non empty URL */ > + if (!git_config_get_string_tmp(url_key, &url) && *url) { > + strvec_push(names, r->name); > + strvec_push(urls, url); > + } > > - free(url); Nice. > +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. > +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. > + # Clone from server to create a client > + # It should fail because the client will reject the LOP as URLs are > + # different, and the server cannot lazy fetch as the LOP URL is > + # missing, so the remote name will be used instead which will fail. > + test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ > + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ > + -c remote.lop.url="file://$(pwd)/lop" \ > + -c promisor.acceptfromserver=KnownUrl \ > + --no-local --filter="blob:limit=5k" server client && > + > + # Check that the largest object is still missing on the server > + check_missing_objects server 1 "$oid" > +' > + > +test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" ' > + git -C server config promisor.advertise true && > + > + git -C server config set remote.lop.url "" && > + test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" && > + > + # Clone from server to create a client > + # It should fail because the client will reject the LOP as an empty URL is > + # not advertised, and the server cannot lazy fetch as the LOP URL is empty, > + # so the remote name will be used instead which will fail. > + test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ > + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ > + -c remote.lop.url="file://$(pwd)/lop" \ > + -c promisor.acceptfromserver=KnownUrl \ > + --no-local --filter="blob:limit=5k" server client && > + > + # Check that the largest object is still missing on the server > + check_missing_objects server 1 "$oid" > +' The test clone is identical to the previous one except for the four-line comment in the middle. The set-up on the other side is different (the server has remote.lop.url set in the previous one to empty, and unset in this one, which should amount to the same thing). Makes sense. > test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" ' > git -C server config promisor.advertise true &&
On Fri, Mar 14, 2025 at 7:59 PM Junio C Hamano <gitster@pobox.com> 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.
diff --git a/promisor-remote.c b/promisor-remote.c index 6a0a61382f..ba80240f12 100644 --- a/promisor-remote.c +++ b/promisor-remote.c @@ -323,13 +323,15 @@ static void promisor_info_vecs(struct repository *repo, promisor_remote_init(repo); for (r = repo->promisor_remote_config->promisors; r; r = r->next) { - char *url; + const char *url; char *url_key = xstrfmt("remote.%s.url", r->name); - strvec_push(names, r->name); - strvec_push(urls, git_config_get_string(url_key, &url) ? NULL : url); + /* Only add remotes with a non empty URL */ + if (!git_config_get_string_tmp(url_key, &url) && *url) { + strvec_push(names, r->name); + strvec_push(urls, url); + } - free(url); free(url_key); } } @@ -356,10 +358,8 @@ char *promisor_remote_info(struct repository *repo) strbuf_addch(&sb, ';'); strbuf_addstr(&sb, "name="); strbuf_addstr_urlencode(&sb, names.v[i], allow_unsanitized); - if (urls.v[i]) { - strbuf_addstr(&sb, ",url="); - strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized); - } + strbuf_addstr(&sb, ",url="); + strbuf_addstr_urlencode(&sb, urls.v[i], allow_unsanitized); } strvec_clear(&names); diff --git a/t/t5710-promisor-remote-capability.sh b/t/t5710-promisor-remote-capability.sh index d2cc69a17e..05ae96d1f7 100755 --- a/t/t5710-promisor-remote-capability.sh +++ b/t/t5710-promisor-remote-capability.sh @@ -193,6 +193,25 @@ test_expect_success "clone with 'KnownName' and different remote names" ' initialize_server 1 "$oid" ' +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" && + + # Check that the largest object is not missing on the server + check_missing_objects server 0 "" && + + # Reinitialize server so that the largest object is missing again + initialize_server 1 "$oid" +' + test_expect_success "clone with promisor.acceptfromserver set to 'KnownUrl'" ' git -C server config promisor.advertise true && @@ -228,6 +247,46 @@ test_expect_success "clone with 'KnownUrl' and different remote urls" ' initialize_server 1 "$oid" ' +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\"" && + + # Clone from server to create a client + # It should fail because the client will reject the LOP as URLs are + # different, and the server cannot lazy fetch as the LOP URL is + # missing, so the remote name will be used instead which will fail. + test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ + -c remote.lop.url="file://$(pwd)/lop" \ + -c promisor.acceptfromserver=KnownUrl \ + --no-local --filter="blob:limit=5k" server client && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + +test_expect_success "clone with 'KnownUrl' and empty url, so not advertised" ' + git -C server config promisor.advertise true && + + git -C server config set remote.lop.url "" && + test_when_finished "git -C server config set remote.lop.url \"file://$(pwd)/lop\"" && + + # Clone from server to create a client + # It should fail because the client will reject the LOP as an empty URL is + # not advertised, and the server cannot lazy fetch as the LOP URL is empty, + # so the remote name will be used instead which will fail. + test_must_fail env GIT_NO_LAZY_FETCH=0 git clone -c remote.lop.promisor=true \ + -c remote.lop.fetch="+refs/heads/*:refs/remotes/lop/*" \ + -c remote.lop.url="file://$(pwd)/lop" \ + -c promisor.acceptfromserver=KnownUrl \ + --no-local --filter="blob:limit=5k" server client && + + # Check that the largest object is still missing on the server + check_missing_objects server 1 "$oid" +' + test_expect_success "clone with promisor.advertise set to 'true' but don't delete the client" ' git -C server config promisor.advertise true &&
Using strvec_push() to push `NULL` into a 'strvec' results in a segfault, because `xstrdup(NULL)` crashes. So when an URL is missing from the config, let's not push the remote name and URL into the 'strvec's. While at it, let's also not push them in case the URL is empty. It's just not worth the trouble and it's consistent with how Git otherwise treats missing and empty URLs in the same way. Note that in case of missing or empty URL, Git uses the remote name to fetch, which can work if the remote is on the same filesystem. So configurations where the client, server and remote are all on the same filesystem may need URLs to be configured even if they are the same as the remote names. But this is a rare case, and the work around is easy enough. We leave improving the strvec API and/or xstrdup() for a future separate effort. While at it, let's also use git_config_get_string_tmp() instead of git_config_get_string() to simplify memory management. Helped-by: Jeff King <peff@peff.net> Signed-off-by: Christian Couder <chriscool@tuxfamily.org> --- promisor-remote.c | 16 ++++---- t/t5710-promisor-remote-capability.sh | 59 +++++++++++++++++++++++++++ 2 files changed, 67 insertions(+), 8 deletions(-)