[PATCH v2] builtin/push: call set_refspecs after validating remote

[Karthik Nayak <karthik.188@gmail.com>]

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>
Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
---

Changes from v1:
- Added more details to the commit message to clarify the issue at hand.
- Fixed the quoting in the test.
- Cleaned up 'set_refspecs' by now accepting a remote instead of trying
  to obtain one. 

Range-diff against v1:
1:  1bd4dc424d ! 1:  fd9a9387e9 builtin/push: call set_refspecs after validating remote
    @@ Metadata
      ## Commit message ##
         builtin/push: call set_refspecs after validating remote
     
    -    Since 9badf97c4 (remote: allow resetting url list), we reset the remote
    -    URL if the provided URL is empty. This means any caller of
    -    `remotes_remote_get()` would now get a NULL remote.
    +    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 earlier since we would get a remote, albeit with an
    -    empty URL. With the new changes, we get a NULL remote and this crashes.
    +    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 and also add a test to
    -    validate the bug fix.
    +    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>
         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>
     
      ## builtin/push.c ##
    +@@ builtin/push.c: 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;
    + 
    +@@ builtin/push.c: 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);
    + 	}
     @@ builtin/push.c: int cmd_push(int argc, const char **argv, const char *prefix)
      	if (tags)
      		refspec_append(&rs, "refs/tags/*");
    @@ builtin/push.c: int cmd_push(int argc, const char **argv, const char *prefix)
      	}
      
     +	if (argc > 0)
    -+		set_refspecs(argv + 1, argc - 1, repo);
    ++		set_refspecs(argv + 1, argc - 1, remote);
     +
      	if (remote->mirror)
      		flags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);
    @@ t/t5529-push-errors.sh: test_expect_success 'detect missing sha1 expressions ear
      	test_cmp expect rp-ran
      '
      
    ++# We need to use an existing local_ref so that the remote is mapped to
    ++# it in 'builtin/push.c:set_refspecs()'.
     +test_expect_success 'detect empty remote' '
     +	test_must_fail git push "" main 2> stderr &&
    -+	grep "fatal: bad repository ''" stderr
    ++	grep "fatal: bad repository ${SQ}${SQ}" stderr
     +'
     +
      test_expect_success 'detect ambiguous refs early' '

 builtin/push.c         | 21 +++++++--------------
 t/t5529-push-errors.sh | 10 ++++++++++
 2 files changed, 17 insertions(+), 14 deletions(-)

diff --git a/builtin/push.c b/builtin/push.c
index 8260c6e46a..7a67398124 100644
--- 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 0247137cb3..54427252a8 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,13 @@ test_expect_success 'detect missing sha1 expressions early' '
 	test_cmp expect rp-ran
 '
 
+# We need to use an existing local_ref so that the remote is mapped to
+# it in 'builtin/push.c:set_refspecs()'.
+test_expect_success 'detect empty remote' '
+	test_must_fail git push "" main 2> stderr &&
+	grep "fatal: bad repository ${SQ}${SQ}" stderr
+'
+
 test_expect_success 'detect ambiguous refs early' '
 	git branch foo &&
 	git tag foo &&
-- 
2.45.1