From 757c6ee7a3f6e1266bba0bf47545471c027c8340 Mon Sep 17 00:00:00 2001 From: Karthik Nayak Date: Thu, 11 Jul 2024 11:39:54 +0200 Subject: builtin/push: call set_refspecs after validating remote When an end-user runs "git push" with an empty string for the remote repository name, e.g. $ git push '' main "git push" fails with a BUG(). Even though this is a nonsense request that we want to fail, we shouldn't hit a BUG(). Instead we want to give a sensible error message, e.g., 'bad repository'". This is because since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we reset the remote URL if the provided URL is empty. When a user of 'remotes_remote_get' tries to fetch a remote with an empty repo name, the function initializes the remote via 'make_remote'. But the remote is still not a valid remote, since the URL is empty, so it tries to add the URL alias using 'add_url_alias'. This in-turn will call 'add_url', but since the URL is empty we call 'strvec_clear' on the `remote->url`. Back in 'remotes_remote_get', we again check if the remote is valid, which fails, so we return 'NULL' for the 'struct remote *' value. The 'builtin/push.c' code, calls 'set_refspecs' before validating the remote. This worked with empty repo names earlier since we would get a remote, albeit with an empty URL. With the new changes, we get a 'NULL' remote value, this causes the check for remote to fail and raises the BUG in 'set_refspecs'. Do a simple fix by doing remote validation first. Also add a test to validate the bug fix. With this, we can also now directly pass remote to 'set_refspecs' instead of it trying to lazily obtain it. Helped-by: Jeff King Helped-by: Junio C Hamano Signed-off-by: Karthik Nayak Signed-off-by: Junio C Hamano --- t/t5529-push-errors.sh | 17 +++++++++++++++++ 1 file changed, 17 insertions(+) (limited to 't') diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh index 0247137cb3..17d7257892 100755 --- a/t/t5529-push-errors.sh +++ b/t/t5529-push-errors.sh @@ -2,6 +2,9 @@ test_description='detect some push errors early (before contacting remote)' +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME + TEST_PASSES_SANITIZE_LEAK=true . ./test-lib.sh @@ -38,6 +41,20 @@ test_expect_success 'detect missing sha1 expressions early' ' test_cmp expect rp-ran ' +# We use an existing local_ref, since it follows a different flow in +# 'builtin/push.c:set_refspecs()' and we want to test that regression. +test_expect_success 'detect empty remote with existing local ref' ' + test_must_fail git push "" main 2> stderr && + grep "fatal: bad repository ${SQ}${SQ}" stderr +' + +# While similar to the previous test, here we want to ensure that +# even targeted refspecs are handled. +test_expect_success 'detect empty remote with targeted refspec' ' + test_must_fail git push "" HEAD:refs/heads/main 2> stderr && + grep "fatal: bad repository ${SQ}${SQ}" stderr +' + test_expect_success 'detect ambiguous refs early' ' git branch foo && git tag foo && -- cgit v1.3