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 <peff@peff.net> Helped-by: Junio C Hamano <gitster@pobox.com> Signed-off-by: Karthik Nayak <karthik.188@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>

Karthik Nayak committed Jul 11, 2024 at 11:39 UTC 757c6ee7a3f6e1266bba0bf47545471c027c8340
2 files changed +24 -14
builtin/push.c
+7 -14
@@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
96 refspec_append(refspec, ref);
97 }
98
99 -static void set_refspecs(const char **refs, int nr, const char *repo)
99 +static void set_refspecs(const char **refs, int nr, struct remote *remote)
100 {
101 - struct remote *remote = NULL;
101 struct ref *local_refs = NULL;
102 int i;
103
@@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
123 local_refs = get_local_heads();
124
125 /* Does "ref" uniquely name our ref? */
127 - if (count_refspec_match(ref, local_refs, &matched) != 1) {
126 + if (count_refspec_match(ref, local_refs, &matched) != 1)
127 refspec_append(&rs, ref);
129 - } else {
130 - /* lazily grab remote */
131 - if (!remote)
132 - remote = remote_get(repo);
133 - if (!remote)
134 - BUG("must get a remote for repo '%s'", repo);
135 -
128 + else
129 refspec_append_mapped(&rs, ref, remote, matched);
137 - }
130 } else
131 refspec_append(&rs, ref);
132 }
@@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
622 if (tags)
623 refspec_append(&rs, "refs/tags/*");
624
633 - if (argc > 0) {
625 + if (argc > 0)
626 repo = argv[0];
635 - set_refspecs(argv + 1, argc - 1, repo);
636 - }
627
628 remote = pushremote_get(repo);
629 if (!remote) {
@@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
639 " git push <name>\n"));
640 }
641
642 + if (argc > 0)
643 + set_refspecs(argv + 1, argc - 1, remote);
644 +
645 if (remote->mirror)
646 flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
647
t/t5529-push-errors.sh
+17
@@ -2,6 +2,9 @@
2
3 test_description='detect some push errors early (before contacting remote)'
4
5 +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
6 +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
7 +
8 TEST_PASSES_SANITIZE_LEAK=true
9 . ./test-lib.sh
10
@@ -38,6 +41,20 @@ test_expect_success 'detect missing sha1 expressions early' '
41 test_cmp expect rp-ran
42 '
43
44 +# We use an existing local_ref, since it follows a different flow in
45 +# 'builtin/push.c:set_refspecs()' and we want to test that regression.
46 +test_expect_success 'detect empty remote with existing local ref' '
47 + test_must_fail git push "" main 2> stderr &&
48 + grep "fatal: bad repository ${SQ}${SQ}" stderr
49 +'
50 +
51 +# While similar to the previous test, here we want to ensure that
52 +# even targeted refspecs are handled.
53 +test_expect_success 'detect empty remote with targeted refspec' '
54 + test_must_fail git push "" HEAD:refs/heads/main 2> stderr &&
55 + grep "fatal: bad repository ${SQ}${SQ}" stderr
56 +'
57 +
58 test_expect_success 'detect ambiguous refs early' '
59 git branch foo &&
60 git tag foo &&