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>

diff --git a/builtin/push.c b/builtin/push.c
index 8260c6e46a72c9af42358f65a4b4ec8658913bde..7a67398124f93121a85f5722d3e06413dcc31ff5 100644 (file)
--- a/builtin/push.c
+++ b/builtin/push.c
@@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,
        refspec_append(refspec, ref);
 }
 
-static void set_refspecs(const char **refs, int nr, const char *repo)
+static void set_refspecs(const char **refs, int nr, struct remote *remote)
 {
-       struct remote *remote = NULL;
        struct ref *local_refs = NULL;
        int i;
 
@@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo)
                                local_refs = get_local_heads();
 
                        /* Does "ref" uniquely name our ref? */
-                       if (count_refspec_match(ref, local_refs, &matched) != 1) {
+                       if (count_refspec_match(ref, local_refs, &matched) != 1)
                                refspec_append(&rs, ref);
-                       } else {
-                               /* lazily grab remote */
-                               if (!remote)
-                                       remote = remote_get(repo);
-                               if (!remote)
-                                       BUG("must get a remote for repo '%s'", repo);
-
+                       else
                                refspec_append_mapped(&rs, ref, remote, matched);
-                       }
                } else
                        refspec_append(&rs, ref);
        }
@@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)
        if (tags)
                refspec_append(&rs, "refs/tags/*");
 
-       if (argc > 0) {
+       if (argc > 0)
                repo = argv[0];
-               set_refspecs(argv + 1, argc - 1, repo);
-       }
 
        remote = pushremote_get(repo);
        if (!remote) {
@@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)
                    "    git push <name>\n"));
        }
 
+       if (argc > 0)
+               set_refspecs(argv + 1, argc - 1, remote);
+
        if (remote->mirror)
                flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
 

diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh
index 0247137cb36474c22f6ce5ed045379fa43f1a3ea..17d72578926acc0eb3823712deb79fa72aa470bd 100755 (executable)
--- 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 &&