| Message ID | 20240711093954.20317-1-karthik.188@gmail.com (mailing list archive) |
|---|---|
| State | Accepted |
| Commit | 757c6ee7a3f6e1266bba0bf47545471c027c8340 |
| Headers | show |
| Series | [v3] builtin/push: call set_refspecs after validating remote | expand |
Karthik Nayak <karthik.188@gmail.com> writes: > 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> > --- > ... > builtin/push.c | 21 +++++++-------------- > t/t5529-push-errors.sh | 17 +++++++++++++++++ > 2 files changed, 24 insertions(+), 14 deletions(-) Everything makes sense to me. Thanks. > 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..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 &&
On Thu, Jul 11, 2024 at 08:08:52AM -0700, Junio C Hamano wrote: > > 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. > [..] > > Everything makes sense to me. Thanks. Likewise for me. -Peff
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..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 &&
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> --- Changes from v2: - Updated the commit message to talk more about the user experience at the start. - Added another test to also check targeted refspec. Range-diff against v2: 1: fd9a9387e9 ! 1: 845be99dd6 builtin/push: call set_refspecs after validating remote @@ Metadata ## Commit message ## 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 + 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 @@ Commit message '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> ## builtin/push.c ## @@ 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' ' ++# 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 && builtin/push.c | 21 +++++++-------------- t/t5529-push-errors.sh | 17 +++++++++++++++++ 2 files changed, 24 insertions(+), 14 deletions(-)