[PATCH v2] builtin/push: call set_refspecs after validating 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 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(-)
Show changes to 2 files +17 −14
builtin/push.c, t/t5529-push-errors.sh
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
Karthik Nayak
