From: Junio C Hamano <gitster@pobox•com>
To: Karthik Nayak <karthik.188@gmail•com>
Cc: git@vger•kernel.org, peff@peff•net
Subject: Re: [PATCH v3] builtin/push: call set_refspecs after validating remote
Date: Thu, 11 Jul 2024 08:08:52 -0700 [thread overview]
Message-ID: <xmqqo774neyj.fsf@gitster.g> (raw)
In-Reply-To: <20240711093954.20317-1-karthik.188@gmail.com> (Karthik Nayak's message of "Thu, 11 Jul 2024 11:39:54 +0200")
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 &&
next prev parent reply other threads:[~2024-07-11 15:08 UTC|newest]
Thread overview: 20+ messages / expand[flat|nested] mbox.gz Atom feed top
2024-07-08 14:03 [PATCH] builtin/push: call set_refspecs after validating remote Karthik Nayak
2024-07-08 19:17 ` Junio C Hamano
2024-07-08 23:33 ` Jeff King
2024-07-09 9:59 ` Karthik Nayak
2024-07-08 23:32 ` Jeff King
2024-07-09 9:05 ` Karthik Nayak
2024-07-09 9:59 ` Jeff King
2024-07-09 14:49 ` [PATCH v2] " Karthik Nayak
2024-07-09 18:44 ` Junio C Hamano
2024-07-09 18:56 ` Junio C Hamano
2024-07-09 23:55 ` Jeff King
2024-07-10 1:04 ` Junio C Hamano
2024-07-10 13:12 ` Karthik Nayak
2024-07-10 15:34 ` Junio C Hamano
2024-07-10 15:46 ` Jeff King
2024-07-11 9:35 ` Karthik Nayak
2024-07-11 21:32 ` Jeff King
2024-07-11 9:39 ` [PATCH v3] " Karthik Nayak
2024-07-11 15:08 ` Junio C Hamano [this message]
2024-07-11 21:33 ` Jeff King
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqo774neyj.fsf@gitster.g \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=karthik.188@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox