{"thread":{"id":"61755","subject":"[PATCH] builtin/push: call set_refspecs after validating remote","startedAt":"2024-07-08T14:03:54Z","lastAt":"2024-07-11T21:33:19Z","messageCount":20,"participants":["Karthik Nayak","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"498259","messageId":"20240708140350.622986-1-karthik.188@gmail.com","threadId":"61755","inReplyTo":null,"subject":"[PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-08T14:03:50Z","receivedAt":"2024-07-08T14:03:54Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Since 9badf97c4 (remote: allow resetting url list), we reset the remote\nURL if the provided URL is empty. This means any caller of\n`remotes_remote_get()` would now get a NULL remote.\n\nThe 'builtin/push.c' code, calls 'set_refspecs' before validating the\nremote. This worked earlier since we would get a remote, albeit with an\nempty URL. With the new changes, we get a NULL remote and this crashes.\n\nDo a simple fix by doing remote validation first and also add a test to\nvalidate the bug fix.\n\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n\nI noticed that this was breaking on master. We run tests on Git master\nfor Gitaly at GitLab and I noticed a SEFAULT. I could also reproduce the\nbug by simply doing 'git push \"\" refs/heads/master' on master, next and\nseen. \n\n builtin/push.c         | 7 ++++---\n t/t5529-push-errors.sh | 8 ++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8260c6e46a..992f603de7 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -630,10 +630,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (tags)\n \t\trefspec_append(&rs, \"refs/tags/*\");\n \n-\tif (argc > 0) {\n+\tif (argc > 0)\n \t\trepo = argv[0];\n-\t\tset_refspecs(argv + 1, argc - 1, repo);\n-\t}\n \n \tremote = pushremote_get(repo);\n \tif (!remote) {\n@@ -649,6 +647,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t    \"    git push <name>\\n\"));\n \t}\n \n+\tif (argc > 0)\n+\t\tset_refspecs(argv + 1, argc - 1, repo);\n+\n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n \ndiff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh\nindex 0247137cb3..771f5f8ae8 100755\n--- a/t/t5529-push-errors.sh\n+++ b/t/t5529-push-errors.sh\n@@ -2,6 +2,9 @@\n \n test_description='detect some push errors early (before contacting remote)'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -38,6 +41,11 @@ test_expect_success 'detect missing sha1 expressions early' '\n \ttest_cmp expect rp-ran\n '\n \n+test_expect_success 'detect empty remote' '\n+\ttest_must_fail git push \"\" main 2> stderr &&\n+\tgrep \"fatal: bad repository ''\" stderr\n+'\n+\n test_expect_success 'detect ambiguous refs early' '\n \tgit branch foo &&\n \tgit tag foo &&\n-- \n2.45.1\n\n"},{"id":"498307","messageId":"xmqqsewjelrh.fsf@gitster.g","threadId":"61755","inReplyTo":"20240708140350.622986-1-karthik.188@gmail.com","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-08T19:17:38Z","receivedAt":"2024-07-08T19:17:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> Since 9badf97c4 (remote: allow resetting url list),\n\nPlease do not be original in places where it shouldn't matter.  Use\n\"git show -s --format=reference\" that includes the datestamp to help\nreaders judge how old the problem is.\n\n> we reset the remote\n> URL if the provided URL is empty. This means any caller of\n> `remotes_remote_get()` would now get a NULL remote.\n\n\"NULL remote\" meaning?\n\nIf you have this:\n\n [remote \"multi\"]\n\turl = wrong-one\n\turl = wrong-two\n\turl =\n\nand ask \"remotes_remote_get()\" to give you the remote \"multi\", you'd\nget a remote whose URL array has no elements.  Is that what you are\nreferring to?\n\n> The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n> remote.\n\nThere is a comment about \"lazily grab remote\", so it is very\nunderstandable.\n\n> This worked earlier since we would get a remote, albeit with an\n> empty URL. With the new changes, we get a NULL remote and this crashes.\n\nYou'd really really need to clarify what you mean by \"a NULL remote\"\nif you want the proposed log message and the change to be\nunderstood.  The change made by 9badf97c (remote: allow resetting\nurl list, 2024-06-14), as far as I can tell, can make the strvecs\nthat hold URL and pushURL in a remote structure empty, but it does\nnot otherwise destroy the remote structure, or nullify a pointer\nthat points at the remote structure.  So I am completely lost here.\n"},{"id":"498327","messageId":"20240708233220.GA934035@coredump.intra.peff.net","threadId":"61755","inReplyTo":"20240708140350.622986-1-karthik.188@gmail.com","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-08T23:32:20Z","receivedAt":"2024-07-08T23:32:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 08, 2024 at 04:03:50PM +0200, Karthik Nayak wrote:\n\n> Since 9badf97c4 (remote: allow resetting url list), we reset the remote\n> URL if the provided URL is empty. This means any caller of\n> `remotes_remote_get()` would now get a NULL remote.\n> \n> The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n> remote. This worked earlier since we would get a remote, albeit with an\n> empty URL. With the new changes, we get a NULL remote and this crashes.\n\nInteresting. I think this was always a bit buggy, in the sense that the\nsome of the code was prepared for pushremote_get() to return NULL, but\nthe set_refspecs() call was not. So in theory _any_ problem with the\nremote that caused pushremote_get() to bail out would be a problem. But\nin practice, I'm not sure there was a way to do so, since the remote.c\ncode usually falls back on the given name as the url if needed, rather\nthan returning NULL.\n\nAnd 9badf97c4 does something a bit unexpected here, since the fallback\ncalls the same add_url() function that we feed the config values to, and\nso it respects the same \"empty means reset\" logic. Which means that an\nempty-string remote name will no longer fall back in that way.\n\nIt's a little surprising that we hit the \"empty means reset\" logic here.\nI wonder if that fallback path should be avoiding it. OTOH, an empty\nstring URL is nonsense, and it's not going to work, so maybe returning a\nNULL remote is a good thing. The issue here is mostly just calling BUG()\nfor something that is bogus input.\n\n> Do a simple fix by doing remote validation first and also add a test to\n> validate the bug fix.\n\nOK, so we push it further down, past the \"if (!remote)\" check in the\ncaller. I think that's a good fix. It does make me wonder why\nset_refspecs() does not simply take the remote struct in the first\nplace? I.e.:\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 992f603de7..ae787f1f63 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \trefspec_append(refspec, ref);\n }\n \n-static void set_refspecs(const char **refs, int nr, const char *repo)\n+static void set_refspecs(const char **refs, int nr, struct remote *remote)\n {\n-\tstruct remote *remote = NULL;\n \tstruct ref *local_refs = NULL;\n \tint i;\n \n@@ -127,12 +126,6 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n \t\t\t\trefspec_append(&rs, ref);\n \t\t\t} else {\n-\t\t\t\t/* lazily grab remote */\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tremote = remote_get(repo);\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n-\n \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n \t\t\t}\n \t\t} else\n@@ -648,7 +641,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t}\n \n \tif (argc > 0)\n-\t\tset_refspecs(argv + 1, argc - 1, repo);\n+\t\tset_refspecs(argv + 1, argc - 1, remote);\n \n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n\nwhich is now possible after your patch. Note that set_refspecs()\ncurrently calls remote_get(), but the caller will use pushremote_get()\nto get the remote. I think that set_refspecs() is actually wrong here,\nbut it doesn't matter in practice because \"repo\" is always non-NULL at\nthis point.\n\nBut with the patch above, this kind of error would be impossible to\ntrigger (but again, your patch is still necessary to get the ordering\nright in the first place).\n\n> I noticed that this was breaking on master. We run tests on Git master\n> for Gitaly at GitLab and I noticed a SEFAULT. I could also reproduce the\n> bug by simply doing 'git push \"\" refs/heads/master' on master, next and\n> seen. \n\nI don't see a segfault, but rather a BUG() that triggers SIGABRT. Does\nthat match what you see?\n\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n>  TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n> @@ -38,6 +41,11 @@ test_expect_success 'detect missing sha1 expressions early' '\n>  \ttest_cmp expect rp-ran\n>  '\n>  \n> +test_expect_success 'detect empty remote' '\n> +\ttest_must_fail git push \"\" main 2> stderr &&\n> +\tgrep \"fatal: bad repository ''\" stderr\n> +'\n\nThe test makes sense. Your single-quotes are not doing what you expect,\nthough (they are closing and re-opening the outer test body, so end up\nas the empty string). You can use $SQ$SQ instead (I'm also working on\npatches to allow you to specify the body as a here-doc to avoid exactly\nthis kind of situation, but I don't think we should depend on that yet).\n\nI was a little surprised you needed to use the name \"main\" and not just\n\"HEAD\" or something neutral (avoiding the INITIAL_BRANCH_NAME stuff).\nBut we only hit the problematic code path when we match a local name.\n\nAlso, minor style nit: use \"2>stderr\" with no space.\n\n\nAnyway, thanks for catching my bug! I think your fix is the right\napproach, and we just need to adjust the test. I do think we should do\nthe patch I suggested above on top. I'd be happy if you want to add it\nin to your series, but let me know if you want me to write a commit\nmessage or just send it separately afterwards.\n\n-Peff\n"},{"id":"498328","messageId":"20240708233350.GA948906@coredump.intra.peff.net","threadId":"61755","inReplyTo":"xmqqsewjelrh.fsf@gitster.g","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-08T23:33:50Z","receivedAt":"2024-07-08T23:33:51Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Jul 08, 2024 at 12:17:38PM -0700, Junio C Hamano wrote:\n\n> > This worked earlier since we would get a remote, albeit with an\n> > empty URL. With the new changes, we get a NULL remote and this crashes.\n> \n> You'd really really need to clarify what you mean by \"a NULL remote\"\n> if you want the proposed log message and the change to be\n> understood.  The change made by 9badf97c (remote: allow resetting\n> url list, 2024-06-14), as far as I can tell, can make the strvecs\n> that hold URL and pushURL in a remote structure empty, but it does\n> not otherwise destroy the remote structure, or nullify a pointer\n> that points at the remote structure.  So I am completely lost here.\n\nI was somewhat confused how it could happen, too. ;) I left more\ncomments elsewhere in the thread, but the gist of it is that an empty\nremote name confuses the \"when there are no urls, fall back to the\nremote name as a url\" code.\n\n-Peff\n"},{"id":"498350","messageId":"CAOLa=ZTDhXfh3rW8mcv7N51psRZ-k_w+vfFe7Hc4a5OE4O42TQ@mail.gmail.com","threadId":"61755","inReplyTo":"20240708233220.GA934035@coredump.intra.peff.net","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-09T09:05:58Z","receivedAt":"2024-07-09T09:06:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Mon, Jul 08, 2024 at 04:03:50PM +0200, Karthik Nayak wrote:\n>\n>> Since 9badf97c4 (remote: allow resetting url list), we reset the remote\n>> URL if the provided URL is empty. This means any caller of\n>> `remotes_remote_get()` would now get a NULL remote.\n>>\n>> The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n>> remote. This worked earlier since we would get a remote, albeit with an\n>> empty URL. With the new changes, we get a NULL remote and this crashes.\n>\n> Interesting. I think this was always a bit buggy, in the sense that the\n> some of the code was prepared for pushremote_get() to return NULL, but\n> the set_refspecs() call was not. So in theory _any_ problem with the\n> remote that caused pushremote_get() to bail out would be a problem. But\n> in practice, I'm not sure there was a way to do so, since the remote.c\n> code usually falls back on the given name as the url if needed, rather\n> than returning NULL.\n>\n\nYup, that seems right.\n\n> And 9badf97c4 does something a bit unexpected here, since the fallback\n> calls the same add_url() function that we feed the config values to, and\n> so it respects the same \"empty means reset\" logic. Which means that an\n> empty-string remote name will no longer fall back in that way.\n>\n> It's a little surprising that we hit the \"empty means reset\" logic here.\n> I wonder if that fallback path should be avoiding it. OTOH, an empty\n> string URL is nonsense, and it's not going to work, so maybe returning a\n> NULL remote is a good thing. The issue here is mostly just calling BUG()\n> for something that is bogus input.\n>\n\nYup, that's the explanation that I should've added, thanks. This is a\ngood summary.\n\n>> Do a simple fix by doing remote validation first and also add a test to\n>> validate the bug fix.\n>\n> OK, so we push it further down, past the \"if (!remote)\" check in the\n> caller. I think that's a good fix. It does make me wonder why\n> set_refspecs() does not simply take the remote struct in the first\n> place? I.e.:\n>\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 992f603de7..ae787f1f63 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n>  \trefspec_append(refspec, ref);\n>  }\n>\n> -static void set_refspecs(const char **refs, int nr, const char *repo)\n> +static void set_refspecs(const char **refs, int nr, struct remote *remote)\n>  {\n> -\tstruct remote *remote = NULL;\n>  \tstruct ref *local_refs = NULL;\n>  \tint i;\n>\n> @@ -127,12 +126,6 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n>  \t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n>  \t\t\t\trefspec_append(&rs, ref);\n>  \t\t\t} else {\n> -\t\t\t\t/* lazily grab remote */\n> -\t\t\t\tif (!remote)\n> -\t\t\t\t\tremote = remote_get(repo);\n> -\t\t\t\tif (!remote)\n> -\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n> -\n>  \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n>  \t\t\t}\n>  \t\t} else\n> @@ -648,7 +641,7 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \t}\n>\n>  \tif (argc > 0)\n> -\t\tset_refspecs(argv + 1, argc - 1, repo);\n> +\t\tset_refspecs(argv + 1, argc - 1, remote);\n>\n>  \tif (remote->mirror)\n>  \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n>\n> which is now possible after your patch. Note that set_refspecs()\n> currently calls remote_get(), but the caller will use pushremote_get()\n> to get the remote. I think that set_refspecs() is actually wrong here,\n> but it doesn't matter in practice because \"repo\" is always non-NULL at\n> this point.\n>\n> But with the patch above, this kind of error would be impossible to\n> trigger (but again, your patch is still necessary to get the ordering\n> right in the first place).\n>\n\nI think this is a great addition on top of my patch, I will add it in.\n\n>> I noticed that this was breaking on master. We run tests on Git master\n>> for Gitaly at GitLab and I noticed a SEFAULT. I could also reproduce the\n>> bug by simply doing 'git push \"\" refs/heads/master' on master, next and\n>> seen.\n>\n> I don't see a segfault, but rather a BUG() that triggers SIGABRT. Does\n> that match what you see?\n>\n\nYes, it is indeed SIGABRT and not SEGFAULT. Will correct my message.\n\n>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>> +\n>>  TEST_PASSES_SANITIZE_LEAK=true\n>>  . ./test-lib.sh\n>>\n>> @@ -38,6 +41,11 @@ test_expect_success 'detect missing sha1 expressions early' '\n>>  \ttest_cmp expect rp-ran\n>>  '\n>>\n>> +test_expect_success 'detect empty remote' '\n>> +\ttest_must_fail git push \"\" main 2> stderr &&\n>> +\tgrep \"fatal: bad repository ''\" stderr\n>> +'\n>\n> The test makes sense. Your single-quotes are not doing what you expect,\n> though (they are closing and re-opening the outer test body, so end up\n> as the empty string). You can use $SQ$SQ instead (I'm also working on\n> patches to allow you to specify the body as a here-doc to avoid exactly\n> this kind of situation, but I don't think we should depend on that yet).\n>\n\nGood catch. I'm wondering how it worked though, since I wrote the test\nbefore the fix and used the test to validate the failure and the fix.\n\n> I was a little surprised you needed to use the name \"main\" and not just\n> \"HEAD\" or something neutral (avoiding the INITIAL_BRANCH_NAME stuff).\n> But we only hit the problematic code path when we match a local name.\n>\n\nExactly. I'll add a comment though, to make it clearer\n\n> Also, minor style nit: use \"2>stderr\" with no space.\n>\n\nThanks, will change.\n\n>\n> Anyway, thanks for catching my bug! I think your fix is the right\n> approach, and we just need to adjust the test. I do think we should do\n> the patch I suggested above on top. I'd be happy if you want to add it\n> in to your series, but let me know if you want me to write a commit\n> message or just send it separately afterwards.\n>\n> -Peff\n\nI will add it in. Thanks for your help!\n"},{"id":"498351","messageId":"20240709095913.GA966429@coredump.intra.peff.net","threadId":"61755","inReplyTo":"CAOLa=ZTDhXfh3rW8mcv7N51psRZ-k_w+vfFe7Hc4a5OE4O42TQ@mail.gmail.com","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-09T09:59:13Z","receivedAt":"2024-07-09T09:59:21Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2024 at 05:05:58AM -0400, Karthik Nayak wrote:\n\n> >> +test_expect_success 'detect empty remote' '\n> >> +\ttest_must_fail git push \"\" main 2> stderr &&\n> >> +\tgrep \"fatal: bad repository ''\" stderr\n> >> +'\n> >\n> > The test makes sense. Your single-quotes are not doing what you expect,\n> > though (they are closing and re-opening the outer test body, so end up\n> > as the empty string). You can use $SQ$SQ instead (I'm also working on\n> > patches to allow you to specify the body as a here-doc to avoid exactly\n> > this kind of situation, but I don't think we should depend on that yet).\n> \n> Good catch. I'm wondering how it worked though, since I wrote the test\n> before the fix and used the test to validate the failure and the fix.\n\nYou end up grepping for the sub-string \"fatal: bad repository \",\nwhich still matches. It's just not quite as careful as what you\nintended.\n\n-Peff\n"},{"id":"498352","messageId":"CAOLa=ZT6kmP=z1FG_aoZfEnZdqtW3Hmy8D9WsJrM55CUNioCoQ@mail.gmail.com","threadId":"61755","inReplyTo":"xmqqsewjelrh.fsf@gitster.g","subject":"Re: [PATCH] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-09T09:59:55Z","receivedAt":"2024-07-09T09:59:57Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> Since 9badf97c4 (remote: allow resetting url list),\n>\n> Please do not be original in places where it shouldn't matter.  Use\n> \"git show -s --format=reference\" that includes the datestamp to help\n> readers judge how old the problem is.\n>\n\nWill do.\n\n>> we reset the remote\n>> URL if the provided URL is empty. This means any caller of\n>> `remotes_remote_get()` would now get a NULL remote.\n>\n> \"NULL remote\" meaning?\n>\n> If you have this:\n>\n>  [remote \"multi\"]\n> \turl = wrong-one\n> \turl = wrong-two\n> \turl =\n>\n> and ask \"remotes_remote_get()\" to give you the remote \"multi\", you'd\n> get a remote whose URL array has no elements.  Is that what you are\n> referring to?\n\nI was referring to the 'struct remote *' being 'NULL'. I think Jeff\nexplained this in his email reply to this.\n\n>> The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n>> remote.\n>\n> There is a comment about \"lazily grab remote\", so it is very\n> understandable.\n>\n>> This worked earlier since we would get a remote, albeit with an\n>> empty URL. With the new changes, we get a NULL remote and this crashes.\n>\n> You'd really really need to clarify what you mean by \"a NULL remote\"\n> if you want the proposed log message and the change to be\n> understood.  The change made by 9badf97c (remote: allow resetting\n> url list, 2024-06-14), as far as I can tell, can make the strvecs\n> that hold URL and pushURL in a remote structure empty, but it does\n> not otherwise destroy the remote structure, or nullify a pointer\n> that points at the remote structure.  So I am completely lost here.\n\nI will amend the commit to the following:\n\nSince 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\nreset the remote URL if the provided URL is empty. When a user of\n'remotes_remote_get' tries to fetch a remote with an empty repo name,\nthe function initializes the remote via 'make_remote'. But the remote is\nstill not a valid remote, since the URL is empty, so it tries to add the\nURL alias using 'add_url_alias'. This in-turn will call 'add_url', but\nsince the URL is empty we call 'strvec_clear' on the `remote->url`. Back\nin 'remotes_remote_get', we again check if the remote is valid, which\nfails, so we return 'NULL' for the 'struct remote *' value.\n\nThe 'builtin/push.c' code, calls 'set_refspecs' before validating the\nremote. This worked earlier since we would get a remote, albeit with an\nempty URL. With the new changes, we get a NULL remote this causes the\ncheck for remote to fail and raises the BUG.\n\n---\n\nI hope this makes it clearer\n"},{"id":"498367","messageId":"20240709144931.1146528-1-karthik.188@gmail.com","threadId":"61755","inReplyTo":"20240708140350.622986-1-karthik.188@gmail.com","subject":"[PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-09T14:49:31Z","receivedAt":"2024-07-09T14:49:37Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\nreset the remote URL if the provided URL is empty. When a user of\n'remotes_remote_get' tries to fetch a remote with an empty repo name,\nthe function initializes the remote via 'make_remote'. But the remote is\nstill not a valid remote, since the URL is empty, so it tries to add the\nURL alias using 'add_url_alias'. This in-turn will call 'add_url', but\nsince the URL is empty we call 'strvec_clear' on the `remote->url`. Back\nin 'remotes_remote_get', we again check if the remote is valid, which\nfails, so we return 'NULL' for the 'struct remote *' value\n\nThe 'builtin/push.c' code, calls 'set_refspecs' before validating the\nremote. This worked with empty repo names earlier since we would get a\nremote, albeit with an empty URL. With the new changes, we get a 'NULL'\nremote value, this causes the check for remote to fail and raises the\nBUG in 'set_refspecs'.\n\nDo a simple fix by doing remote validation first. Also add a test to\nvalidate the bug fix. With this, we can also now directly pass remote to\n'set_refspecs' instead of it trying to lazily obtain it.\n\nHelped-by: Jeff King <peff@peff.net>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n\nChanges from v1:\n- Added more details to the commit message to clarify the issue at hand.\n- Fixed the quoting in the test.\n- Cleaned up 'set_refspecs' by now accepting a remote instead of trying\n  to obtain one. \n\nRange-diff against v1:\n1:  1bd4dc424d ! 1:  fd9a9387e9 builtin/push: call set_refspecs after validating remote\n    @@ Metadata\n      ## Commit message ##\n         builtin/push: call set_refspecs after validating remote\n     \n    -    Since 9badf97c4 (remote: allow resetting url list), we reset the remote\n    -    URL if the provided URL is empty. This means any caller of\n    -    `remotes_remote_get()` would now get a NULL remote.\n    +    Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\n    +    reset the remote URL if the provided URL is empty. When a user of\n    +    'remotes_remote_get' tries to fetch a remote with an empty repo name,\n    +    the function initializes the remote via 'make_remote'. But the remote is\n    +    still not a valid remote, since the URL is empty, so it tries to add the\n    +    URL alias using 'add_url_alias'. This in-turn will call 'add_url', but\n    +    since the URL is empty we call 'strvec_clear' on the `remote->url`. Back\n    +    in 'remotes_remote_get', we again check if the remote is valid, which\n    +    fails, so we return 'NULL' for the 'struct remote *' value\n     \n         The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n    -    remote. This worked earlier since we would get a remote, albeit with an\n    -    empty URL. With the new changes, we get a NULL remote and this crashes.\n    +    remote. This worked with empty repo names earlier since we would get a\n    +    remote, albeit with an empty URL. With the new changes, we get a 'NULL'\n    +    remote value, this causes the check for remote to fail and raises the\n    +    BUG in 'set_refspecs'.\n     \n    -    Do a simple fix by doing remote validation first and also add a test to\n    -    validate the bug fix.\n    +    Do a simple fix by doing remote validation first. Also add a test to\n    +    validate the bug fix. With this, we can also now directly pass remote to\n    +    'set_refspecs' instead of it trying to lazily obtain it.\n     \n    +    Helped-by: Jeff King <peff@peff.net>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n      ## builtin/push.c ##\n    +@@ builtin/push.c: static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n    + \trefspec_append(refspec, ref);\n    + }\n    + \n    +-static void set_refspecs(const char **refs, int nr, const char *repo)\n    ++static void set_refspecs(const char **refs, int nr, struct remote *remote)\n    + {\n    +-\tstruct remote *remote = NULL;\n    + \tstruct ref *local_refs = NULL;\n    + \tint i;\n    + \n    +@@ builtin/push.c: static void set_refspecs(const char **refs, int nr, const char *repo)\n    + \t\t\t\tlocal_refs = get_local_heads();\n    + \n    + \t\t\t/* Does \"ref\" uniquely name our ref? */\n    +-\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n    ++\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1)\n    + \t\t\t\trefspec_append(&rs, ref);\n    +-\t\t\t} else {\n    +-\t\t\t\t/* lazily grab remote */\n    +-\t\t\t\tif (!remote)\n    +-\t\t\t\t\tremote = remote_get(repo);\n    +-\t\t\t\tif (!remote)\n    +-\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n    +-\n    ++\t\t\telse\n    + \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n    +-\t\t\t}\n    + \t\t} else\n    + \t\t\trefspec_append(&rs, ref);\n    + \t}\n     @@ builtin/push.c: int cmd_push(int argc, const char **argv, const char *prefix)\n      \tif (tags)\n      \t\trefspec_append(&rs, \"refs/tags/*\");\n    @@ builtin/push.c: int cmd_push(int argc, const char **argv, const char *prefix)\n      \t}\n      \n     +\tif (argc > 0)\n    -+\t\tset_refspecs(argv + 1, argc - 1, repo);\n    ++\t\tset_refspecs(argv + 1, argc - 1, remote);\n     +\n      \tif (remote->mirror)\n      \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n    @@ t/t5529-push-errors.sh: test_expect_success 'detect missing sha1 expressions ear\n      \ttest_cmp expect rp-ran\n      '\n      \n    ++# We need to use an existing local_ref so that the remote is mapped to\n    ++# it in 'builtin/push.c:set_refspecs()'.\n     +test_expect_success 'detect empty remote' '\n     +\ttest_must_fail git push \"\" main 2> stderr &&\n    -+\tgrep \"fatal: bad repository ''\" stderr\n    ++\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n     +'\n     +\n      test_expect_success 'detect ambiguous refs early' '\n\n builtin/push.c         | 21 +++++++--------------\n t/t5529-push-errors.sh | 10 ++++++++++\n 2 files changed, 17 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8260c6e46a..7a67398124 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \trefspec_append(refspec, ref);\n }\n \n-static void set_refspecs(const char **refs, int nr, const char *repo)\n+static void set_refspecs(const char **refs, int nr, struct remote *remote)\n {\n-\tstruct remote *remote = NULL;\n \tstruct ref *local_refs = NULL;\n \tint i;\n \n@@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \t\t\t\tlocal_refs = get_local_heads();\n \n \t\t\t/* Does \"ref\" uniquely name our ref? */\n-\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n+\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1)\n \t\t\t\trefspec_append(&rs, ref);\n-\t\t\t} else {\n-\t\t\t\t/* lazily grab remote */\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tremote = remote_get(repo);\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n-\n+\t\t\telse\n \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n-\t\t\t}\n \t\t} else\n \t\t\trefspec_append(&rs, ref);\n \t}\n@@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (tags)\n \t\trefspec_append(&rs, \"refs/tags/*\");\n \n-\tif (argc > 0) {\n+\tif (argc > 0)\n \t\trepo = argv[0];\n-\t\tset_refspecs(argv + 1, argc - 1, repo);\n-\t}\n \n \tremote = pushremote_get(repo);\n \tif (!remote) {\n@@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t    \"    git push <name>\\n\"));\n \t}\n \n+\tif (argc > 0)\n+\t\tset_refspecs(argv + 1, argc - 1, remote);\n+\n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n \ndiff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh\nindex 0247137cb3..54427252a8 100755\n--- a/t/t5529-push-errors.sh\n+++ b/t/t5529-push-errors.sh\n@@ -2,6 +2,9 @@\n \n test_description='detect some push errors early (before contacting remote)'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' '\n \ttest_cmp expect rp-ran\n '\n \n+# We need to use an existing local_ref so that the remote is mapped to\n+# it in 'builtin/push.c:set_refspecs()'.\n+test_expect_success 'detect empty remote' '\n+\ttest_must_fail git push \"\" main 2> stderr &&\n+\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n+'\n+\n test_expect_success 'detect ambiguous refs early' '\n \tgit branch foo &&\n \tgit tag foo &&\n-- \n2.45.1\n\n"},{"id":"498371","messageId":"xmqq4j8yflrq.fsf@gitster.g","threadId":"61755","inReplyTo":"20240709144931.1146528-1-karthik.188@gmail.com","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-09T18:44:25Z","receivedAt":"2024-07-09T18:44:28Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> - Added more details to the commit message to clarify the issue at hand.\n\n> Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\n> reset the remote URL if the provided URL is empty. When a user of\n> 'remotes_remote_get' tries to fetch a remote with an empty repo name,\n> the function initializes the remote via 'make_remote'. But the remote is\n> still not a valid remote, since the URL is empty, so it tries to add the\n> URL alias using 'add_url_alias'. This in-turn will call 'add_url', but\n> since the URL is empty we call 'strvec_clear' on the `remote->url`. Back\n> in 'remotes_remote_get', we again check if the remote is valid, which\n> fails, so we return 'NULL' for the 'struct remote *' value\n\nThat's better, but it still talks only about the implementation and\ncontrol flow description without mentioning what end-user action it\nbreaks.  We see in the new test this:\n\n    $ git push \"\" main\n\nbut is that something we even want to support?  Before 9badf97c\n(remote: allow resetting url list, 2024-06-14), the command would\nhave failed with different way [*1*].\n\nIs it merely \"this is a nonsense request and must fail, but we do\nnot want to hit BUG in general\"?  I think it is the latter, but\nleaving it unsaid is confusing.  How about starting it more like...\n\n    When an end-user runs \"git push\" with an empty string for the\n    remote repository name, e.g.\n\n        $ git push '' main\n\n    \"git push\" fails with a BUG().  This is because ...\n\nor something.\n\ncmd_push() calls set_refspecs() on a repo (whose name is \"\"), which\nthen calls remote.c:remote_get() on it, which calls\nremote.c:make_remote() to obtain a remote structure.\n\nBut during this calling sequence especially down from remote_get(),\nthere are inconsistent decisions made for an empty string as a name.\nIt is not a NULL pointer, so it does not benefit from the default\nrefspecs by calling get_default=remotes_remote_for_branch,\nvalid_remote_nick() considers it is not a valid nick so we do not\nread remotes or branches configuration file, but we still think a\nname was given (even though it is an empty string) and try to do\nsomething useful by calling add_url_alias().\n\nIt is a mess and whoever designed this should be shot [*2*] ;-).\n\nIn any case, an obvious additional fix on top of your change might\nbe to do something like this:\n\n        diff --git i/remote.c w/remote.c\n        index 5fa046c8f8..d7f9ba3571 100644\n        --- i/remote.c\n        +++ w/remote.c\n        @@ -682,7 +682,7 @@ remotes_remote_get_1(\n                struct remote *ret;\n                int name_given = 0;\n\n        -\tif (name)\n        +\tif (name && *name)\n                        name_given = 1;\n                else\n                        name = get_default(remote_state, remote_state->current_branch,\n\nwhich would give us the default remote name, and we would not call\nadd_url_alias() with a bogus empty string to nuke the list.\n\n> - Cleaned up 'set_refspecs' by now accepting a remote instead of trying\n>   to obtain one. \n\nThe last one, which does make sense, is not mentioned in the\nproposed log message.  Lazy remote creation does not help the\nupdated caller because it already has one.\n\n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n>  TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n> @@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' '\n>  \ttest_cmp expect rp-ran\n>  '\n>  \n> +# We need to use an existing local_ref so that the remote is mapped to\n> +# it in 'builtin/push.c:set_refspecs()'.\n\nHmph, it is OK to force 'main' like the above as a workaround, but\nwouldn't it be sufficient to do\n\n    $ git push \"\" HEAD:refs/heads/main\n\nor something to avoid having to know what our current branch is?\n\nThanks.\n\n\n[Footnote }\n\n *1* For example, before Peff's series, here is what I got:\n\n         $ git push \"\" HEAD:master\n         fatal: no path specified; see 'git help pull' for valid url syntax\n\n     It comes from transport_push() -> get_refs_via_connect() ->\n     parse_connect_url() code path.\n\n *2* It probably is me writing in the original in shell script, so I\n     can safely say this without offending anybody.\n\n\n"},{"id":"498373","messageId":"xmqqsewie6mm.fsf@gitster.g","threadId":"61755","inReplyTo":"xmqq4j8yflrq.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-09T18:56:49Z","receivedAt":"2024-07-09T18:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Is it merely \"this is a nonsense request and must fail, but we do\n> not want to hit BUG in general\"?  I think it is the latter, but\n> leaving it unsaid is confusing.  How about starting it more like...\n\nA bit of clarification.  \"it is the latter\" -> \"that is what you\nmeant\".  In any earlier draft I had another possibility but I\nremoved it before sending the message.\n\n>     When an end-user runs \"git push\" with an empty string for the\n>     remote repository name, e.g.\n>\n>         $ git push '' main\n>\n>     \"git push\" fails with a BUG().  This is because ...\n>\n> or something.\n\nAnother clarificaiton.  \n\n    ... with a BUG().\n\n ->\n\n    ... with a BUG().  Even though this is a nonsense request that\n    we want to fail, we shouldn't hit a BUG().  Instead we want to\n    give a sensible error message, e.g., 'bad repository'\".\n\nLet's make a habit of not stopping at saying what is bad, and also\nsaying what we want instead explicitly. \n\nThanks.\n"},{"id":"498398","messageId":"20240709235507.GB1525171@coredump.intra.peff.net","threadId":"61755","inReplyTo":"xmqq4j8yflrq.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-09T23:55:07Z","receivedAt":"2024-07-09T23:55:08Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jul 09, 2024 at 11:44:25AM -0700, Junio C Hamano wrote:\n\n> In any case, an obvious additional fix on top of your change might\n> be to do something like this:\n> \n>         diff --git i/remote.c w/remote.c\n>         index 5fa046c8f8..d7f9ba3571 100644\n>         --- i/remote.c\n>         +++ w/remote.c\n>         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n>                 struct remote *ret;\n>                 int name_given = 0;\n> \n>         -\tif (name)\n>         +\tif (name && *name)\n>                         name_given = 1;\n>                 else\n>                         name = get_default(remote_state, remote_state->current_branch,\n> \n> which would give us the default remote name, and we would not call\n> add_url_alias() with a bogus empty string to nuke the list.\n\nFWIW, I almost suggested something like this earlier. The outcome will\nbe the same (remote_get(), etc, will return NULL), but I think it\nremoves the \"this is surprising\" comment from my earlier email and makes\nthings much more explicit.\n\n(I also agree with everything else you said in your review).\n\n-Peff\n"},{"id":"498405","messageId":"xmqqbk369hxb.fsf@gitster.g","threadId":"61755","inReplyTo":"20240709235507.GB1525171@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-10T01:04:00Z","receivedAt":"2024-07-10T01:04:06Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Tue, Jul 09, 2024 at 11:44:25AM -0700, Junio C Hamano wrote:\n>\n>> In any case, an obvious additional fix on top of your change might\n>> be to do something like this:\n>> \n>>         diff --git i/remote.c w/remote.c\n>>         index 5fa046c8f8..d7f9ba3571 100644\n>>         --- i/remote.c\n>>         +++ w/remote.c\n>>         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n>>                 struct remote *ret;\n>>                 int name_given = 0;\n>> \n>>         -\tif (name)\n>>         +\tif (name && *name)\n>>                         name_given = 1;\n>>                 else\n>>                         name = get_default(remote_state, remote_state->current_branch,\n>> \n>> which would give us the default remote name, and we would not call\n>> add_url_alias() with a bogus empty string to nuke the list.\n>\n> FWIW, I almost suggested something like this earlier. The outcome will\n> be the same (remote_get(), etc, will return NULL), but I think it\n> removes the \"this is surprising\" comment from my earlier email and makes\n> things much more explicit.\n>\n> (I also agree with everything else you said in your review).\n\nHeh, thanks.  I should prepare to shoot myself then ;-)\n"},{"id":"498444","messageId":"CAOLa=ZQXZ6DyE3YjuVU48nQcj0xuW7uPoPvg2yqktk+S6gXwsg@mail.gmail.com","threadId":"61755","inReplyTo":"xmqq4j8yflrq.fsf@gitster.g","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-10T13:12:21Z","receivedAt":"2024-07-10T13:12:23Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Karthik Nayak <karthik.188@gmail.com> writes:\n>\n>> - Added more details to the commit message to clarify the issue at hand.\n>\n>> Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\n>> reset the remote URL if the provided URL is empty. When a user of\n>> 'remotes_remote_get' tries to fetch a remote with an empty repo name,\n>> the function initializes the remote via 'make_remote'. But the remote is\n>> still not a valid remote, since the URL is empty, so it tries to add the\n>> URL alias using 'add_url_alias'. This in-turn will call 'add_url', but\n>> since the URL is empty we call 'strvec_clear' on the `remote->url`. Back\n>> in 'remotes_remote_get', we again check if the remote is valid, which\n>> fails, so we return 'NULL' for the 'struct remote *' value\n>\n> That's better, but it still talks only about the implementation and\n> control flow description without mentioning what end-user action it\n> breaks.  We see in the new test this:\n>\n>     $ git push \"\" main\n>\n> but is that something we even want to support?  Before 9badf97c\n> (remote: allow resetting url list, 2024-06-14), the command would\n> have failed with different way [*1*].\n>\n> Is it merely \"this is a nonsense request and must fail, but we do\n> not want to hit BUG in general\"?  I think it is the latter, but\n> leaving it unsaid is confusing.  How about starting it more like...\n>\n>     When an end-user runs \"git push\" with an empty string for the\n>     remote repository name, e.g.\n>\n>         $ git push '' main\n>\n>     \"git push\" fails with a BUG().  This is because ...\n>\n> or something.\n>\n\nThanks, I was focused on the technical aspect of it that I didn't really\nspill out the user interaction, you're right, it should start this way.\nI will modify accordingly.\n\n> cmd_push() calls set_refspecs() on a repo (whose name is \"\"), which\n> then calls remote.c:remote_get() on it, which calls\n> remote.c:make_remote() to obtain a remote structure.\n>\n> But during this calling sequence especially down from remote_get(),\n> there are inconsistent decisions made for an empty string as a name.\n> It is not a NULL pointer, so it does not benefit from the default\n> refspecs by calling get_default=remotes_remote_for_branch,\n> valid_remote_nick() considers it is not a valid nick so we do not\n> read remotes or branches configuration file, but we still think a\n> name was given (even though it is an empty string) and try to do\n> something useful by calling add_url_alias().\n>\n> It is a mess and whoever designed this should be shot [*2*] ;-).\n>\n> In any case, an obvious additional fix on top of your change might\n> be to do something like this:\n>\n>         diff --git i/remote.c w/remote.c\n>         index 5fa046c8f8..d7f9ba3571 100644\n>         --- i/remote.c\n>         +++ w/remote.c\n>         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n>                 struct remote *ret;\n>                 int name_given = 0;\n>\n>         -\tif (name)\n>         +\tif (name && *name)\n>                         name_given = 1;\n>                 else\n>                         name = get_default(remote_state, remote_state->current_branch,\n>\n> which would give us the default remote name, and we would not call\n> add_url_alias() with a bogus empty string to nuke the list.\n>\n\nI'm a bit skeptical of making this change. Mostly from the user's\nperspective.\n\nWith my patch currently:\n\n    $ git push \"\" refs/heads/master\n    fatal: bad repository ''\n\nBut with this added, we'd be doing\n\n    $ git push \"\" refs/heads/master\n    Everything up-to-date\n\nThis is because we actually obtained the default remote here. Isn't this\nconfusing from a user's perspective? I mean I agree that an empty repo\nname is something we should support, but it also shouldn't be something\nwe simply ignore?\n\n>> - Cleaned up 'set_refspecs' by now accepting a remote instead of trying\n>>   to obtain one.\n>\n> The last one, which does make sense, is not mentioned in the\n> proposed log message.  Lazy remote creation does not help the\n> updated caller because it already has one.\n>\n>> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n>> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n>> +\n>>  TEST_PASSES_SANITIZE_LEAK=true\n>>  . ./test-lib.sh\n>>\n>> @@ -38,6 +41,13 @@ test_expect_success 'detect missing sha1 expressions early' '\n>>  \ttest_cmp expect rp-ran\n>>  '\n>>\n>> +# We need to use an existing local_ref so that the remote is mapped to\n>> +# it in 'builtin/push.c:set_refspecs()'.\n>\n> Hmph, it is OK to force 'main' like the above as a workaround, but\n> wouldn't it be sufficient to do\n>\n>     $ git push \"\" HEAD:refs/heads/main\n>\n> or something to avoid having to know what our current branch is?\n>\n> Thanks.\n>\n\nSo if we're simply testing my patch, this is definitely enough. But I\nwanted to also keep in mind the state before this patch. Wherein the\nonly way the flow would be triggered is if we provide a local_ref,\nproviding ':' follows a different path in 'set_refspecs'.\n\n>\n>\n> [Footnote }\n>\n>  *1* For example, before Peff's series, here is what I got:\n>\n>          $ git push \"\" HEAD:master\n>          fatal: no path specified; see 'git help pull' for valid url syntax\n>\n>      It comes from transport_push() -> get_refs_via_connect() ->\n>      parse_connect_url() code path.\n>\n>  *2* It probably is me writing in the original in shell script, so I\n>      can safely say this without offending anybody.\n"},{"id":"498463","messageId":"xmqqle29z2eo.fsf@gitster.g","threadId":"61755","inReplyTo":"CAOLa=ZQXZ6DyE3YjuVU48nQcj0xuW7uPoPvg2yqktk+S6gXwsg@mail.gmail.com","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-10T15:34:39Z","receivedAt":"2024-07-10T15:34:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n>> In any case, an obvious additional fix on top of your change might\n>> be to do something like this:\n>>\n>>         diff --git i/remote.c w/remote.c\n>>         index 5fa046c8f8..d7f9ba3571 100644\n>>         --- i/remote.c\n>>         +++ w/remote.c\n>>         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n>>                 struct remote *ret;\n>>                 int name_given = 0;\n>>\n>>         -\tif (name)\n>>         +\tif (name && *name)\n>>                         name_given = 1;\n>>                 else\n>>                         name = get_default(remote_state, remote_state->current_branch,\n>>\n>> which would give us the default remote name, and we would not call\n>> add_url_alias() with a bogus empty string to nuke the list.\n>>\n>\n> I'm a bit skeptical of making this change. Mostly from the user's\n> perspective.\n> ...\n> This is because we actually obtained the default remote here. Isn't this\n> confusing from a user's perspective?\n\nI agree with you 100%.  I haven't checked the ramifications of such\na change to other code paths, and callers of remote_get() are many.\nWith your fix, the above is not necessary and it certainly can be\nleft well outside the scope of the current topic.\n\n> I mean I agree that an empty repo\n> name is something we should support, but it also shouldn't be something\n> we simply ignore?\n\nFor the sake of simplicity of the UI design, I think an empty repo\nname (giving an empty string explicitly where a repository nickname\nor URL is expected) should be forbidden.  The above change lets \"\"\nstand for the default remote (which is different from \"simply\"\nignoring), and is confusing, because we never did that historically.\nUnless we advertise it as a new \"feature\" [*], that is (and I do not\nbelieve it is such a great feature myself).\n\n> So if we're simply testing my patch, this is definitely enough. But I\n> wanted to also keep in mind the state before this patch. Wherein the\n> only way the flow would be triggered is if we provide a local_ref,\n> providing ':' follows a different path in 'set_refspecs'.\n\nOK.  If so we may want to have both, so that we won't regress in the\nother code paths?\n\nThanks.\n\n\n[Footnote]\n\n * When you want to interact with the remote you usually work with\n   but want to affect a custom set of refs, with the current UI,\n   you'd need to remember what you call that usual remote (typically\n   \"origin\") and say \"git [push|fetch] origin $refspec\".  The above\n   change may be seen as a feature that allows you to use an empty\n   string in place of the remote that has to be named explicitly on\n   the command line.\n\n"},{"id":"498464","messageId":"20240710154620.GA3097664@coredump.intra.peff.net","threadId":"61755","inReplyTo":"CAOLa=ZQXZ6DyE3YjuVU48nQcj0xuW7uPoPvg2yqktk+S6gXwsg@mail.gmail.com","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-10T15:46:20Z","receivedAt":"2024-07-10T15:46:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote:\n\n> > In any case, an obvious additional fix on top of your change might\n> > be to do something like this:\n> >\n> >         diff --git i/remote.c w/remote.c\n> >         index 5fa046c8f8..d7f9ba3571 100644\n> >         --- i/remote.c\n> >         +++ w/remote.c\n> >         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n> >                 struct remote *ret;\n> >                 int name_given = 0;\n> >\n> >         -\tif (name)\n> >         +\tif (name && *name)\n> >                         name_given = 1;\n> >                 else\n> >                         name = get_default(remote_state, remote_state->current_branch,\n> >\n> > which would give us the default remote name, and we would not call\n> > add_url_alias() with a bogus empty string to nuke the list.\n> >\n> \n> I'm a bit skeptical of making this change. Mostly from the user's\n> perspective.\n> \n> With my patch currently:\n> \n>     $ git push \"\" refs/heads/master\n>     fatal: bad repository ''\n> \n> But with this added, we'd be doing\n> \n>     $ git push \"\" refs/heads/master\n>     Everything up-to-date\n> \n> This is because we actually obtained the default remote here. Isn't this\n> confusing from a user's perspective? I mean I agree that an empty repo\n> name is something we should support, but it also shouldn't be something\n> we simply ignore?\n\nOh, I misread Junio's patch in my earlier response. I was focused on not\nsetting name_given, which I thought would result in a NULL return value,\nand didn't notice that it would also mean using the default remote.\nSomething like:\n\ndiff --git a/remote.c b/remote.c\nindex 7f6406aaa2..883cf6086e 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \t\tif (!valid_remote(ret))\n \t\t\tread_branches_file(remote_state, ret);\n \t}\n-\tif (name_given && !valid_remote(ret))\n+\tif (name_given && *name && !valid_remote(ret))\n \t\tadd_url_alias(remote_state, ret, name);\n \tif (!valid_remote(ret))\n \t\treturn NULL;\n\nwas more what I was thinking. That is, inhibit the empty string\nexplicitly rather than letting the emergent behavior of add_url_alias()\ndo it for us. Or maybe even just:\n\ndiff --git a/remote.c b/remote.c\nindex 7f6406aaa2..a0b166131f 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n \tstruct remote *ret;\n \tint name_given = 0;\n \n-\tif (name)\n+\tif (name) {\n+\t\tif (!name)\n+\t\t\treturn NULL;\n \t\tname_given = 1;\n-\telse\n+\t} else\n \t\tname = get_default(remote_state, remote_state->current_branch,\n \t\t\t\t   &name_given);\n \n\nto bail immediately.\n\nBut all of that would be internal refactoring / cleanup on top of your\npatch. The user-facing behavior would be the same.\n\n-Peff\n"},{"id":"498504","messageId":"CAOLa=ZT-psS1NecoppGOVdxgn+kMqi8qx2vCLMKNXB4n+5hE0A@mail.gmail.com","threadId":"61755","inReplyTo":"20240710154620.GA3097664@coredump.intra.peff.net","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-11T09:35:08Z","receivedAt":"2024-07-11T09:35:10Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> On Wed, Jul 10, 2024 at 06:12:21AM -0700, Karthik Nayak wrote:\n>\n>> > In any case, an obvious additional fix on top of your change might\n>> > be to do something like this:\n>> >\n>> >         diff --git i/remote.c w/remote.c\n>> >         index 5fa046c8f8..d7f9ba3571 100644\n>> >         --- i/remote.c\n>> >         +++ w/remote.c\n>> >         @@ -682,7 +682,7 @@ remotes_remote_get_1(\n>> >                 struct remote *ret;\n>> >                 int name_given = 0;\n>> >\n>> >         -\tif (name)\n>> >         +\tif (name && *name)\n>> >                         name_given = 1;\n>> >                 else\n>> >                         name = get_default(remote_state, remote_state->current_branch,\n>> >\n>> > which would give us the default remote name, and we would not call\n>> > add_url_alias() with a bogus empty string to nuke the list.\n>> >\n>>\n>> I'm a bit skeptical of making this change. Mostly from the user's\n>> perspective.\n>>\n>> With my patch currently:\n>>\n>>     $ git push \"\" refs/heads/master\n>>     fatal: bad repository ''\n>>\n>> But with this added, we'd be doing\n>>\n>>     $ git push \"\" refs/heads/master\n>>     Everything up-to-date\n>>\n>> This is because we actually obtained the default remote here. Isn't this\n>> confusing from a user's perspective? I mean I agree that an empty repo\n>> name is something we should support, but it also shouldn't be something\n>> we simply ignore?\n>\n> Oh, I misread Junio's patch in my earlier response. I was focused on not\n> setting name_given, which I thought would result in a NULL return value,\n> and didn't notice that it would also mean using the default remote.\n> Something like:\n>\n> diff --git a/remote.c b/remote.c\n> index 7f6406aaa2..883cf6086e 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -703,7 +703,7 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n>  \t\tif (!valid_remote(ret))\n>  \t\t\tread_branches_file(remote_state, ret);\n>  \t}\n> -\tif (name_given && !valid_remote(ret))\n> +\tif (name_given && *name && !valid_remote(ret))\n>  \t\tadd_url_alias(remote_state, ret, name);\n>  \tif (!valid_remote(ret))\n>  \t\treturn NULL;\n>\n> was more what I was thinking. That is, inhibit the empty string\n> explicitly rather than letting the emergent behavior of add_url_alias()\n> do it for us. Or maybe even just:\n>\n> diff --git a/remote.c b/remote.c\n> index 7f6406aaa2..a0b166131f 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -690,9 +690,11 @@ remotes_remote_get_1(struct remote_state *remote_state, const char *name,\n>  \tstruct remote *ret;\n>  \tint name_given = 0;\n>\n> -\tif (name)\n> +\tif (name) {\n> +\t\tif (!name)\n> +\t\t\treturn NULL;\n>  \t\tname_given = 1;\n> -\telse\n> +\t} else\n>  \t\tname = get_default(remote_state, remote_state->current_branch,\n>  \t\t\t\t   &name_given);\n>\n>\n> to bail immediately.\n>\n> But all of that would be internal refactoring / cleanup on top of your\n> patch. The user-facing behavior would be the same.\n>\n> -Peff\n\nThese should work as intended on top of my patch. But I will skip doing\nthese changes for now. I do see the merit but I think it is also okay\nthe way it is now.\n"},{"id":"498505","messageId":"20240711093954.20317-1-karthik.188@gmail.com","threadId":"61755","inReplyTo":"20240709144931.1146528-1-karthik.188@gmail.com","subject":"[PATCH v3] builtin/push: call set_refspecs after validating remote","fromName":"Karthik Nayak","fromEmail":"karthik.188@gmail.com","sentAt":"2024-07-11T09:39:54Z","receivedAt":"2024-07-11T09:40:00Z","isPatch":true,"sender":{"key":"karthik.188@gmail.com","avatar":"https://avatars.githubusercontent.com/u/1786334?v=4"},"body":"When an end-user runs \"git push\" with an empty string for the remote\nrepository name, e.g.\n\n    $ git push '' main\n\n\"git push\" fails with a BUG(). Even though this is a nonsense request\nthat we want to fail, we shouldn't hit a BUG().  Instead we want to give\na sensible error message, e.g., 'bad repository'\".\n\nThis is because since 9badf97c42 (remote: allow resetting url list,\n2024-06-14), we reset the remote URL if the provided URL is empty. When\na user of 'remotes_remote_get' tries to fetch a remote with an empty\nrepo name, the function initializes the remote via 'make_remote'. But\nthe remote is still not a valid remote, since the URL is empty, so it\ntries to add the URL alias using 'add_url_alias'. This in-turn will call\n'add_url', but since the URL is empty we call 'strvec_clear' on the\n`remote->url`. Back in 'remotes_remote_get', we again check if the\nremote is valid, which fails, so we return 'NULL' for the 'struct\nremote *' value.\n\nThe 'builtin/push.c' code, calls 'set_refspecs' before validating the\nremote. This worked with empty repo names earlier since we would get a\nremote, albeit with an empty URL. With the new changes, we get a 'NULL'\nremote value, this causes the check for remote to fail and raises the\nBUG in 'set_refspecs'.\n\nDo a simple fix by doing remote validation first. Also add a test to\nvalidate the bug fix. With this, we can also now directly pass remote to\n'set_refspecs' instead of it trying to lazily obtain it.\n\nHelped-by: Jeff King <peff@peff.net>\nHelped-by: Junio C Hamano <gitster@pobox.com>\nSigned-off-by: Karthik Nayak <karthik.188@gmail.com>\n---\n\nChanges from v2:\n- Updated the commit message to talk more about the user\nexperience at the start.\n- Added another test to also check targeted refspec.\n\nRange-diff against v2:\n1:  fd9a9387e9 ! 1:  845be99dd6 builtin/push: call set_refspecs after validating remote\n    @@ Metadata\n      ## Commit message ##\n         builtin/push: call set_refspecs after validating remote\n     \n    -    Since 9badf97c42 (remote: allow resetting url list, 2024-06-14), we\n    -    reset the remote URL if the provided URL is empty. When a user of\n    -    'remotes_remote_get' tries to fetch a remote with an empty repo name,\n    -    the function initializes the remote via 'make_remote'. But the remote is\n    -    still not a valid remote, since the URL is empty, so it tries to add the\n    -    URL alias using 'add_url_alias'. This in-turn will call 'add_url', but\n    -    since the URL is empty we call 'strvec_clear' on the `remote->url`. Back\n    -    in 'remotes_remote_get', we again check if the remote is valid, which\n    -    fails, so we return 'NULL' for the 'struct remote *' value\n    +    When an end-user runs \"git push\" with an empty string for the remote\n    +    repository name, e.g.\n    +\n    +        $ git push '' main\n    +\n    +    \"git push\" fails with a BUG(). Even though this is a nonsense request\n    +    that we want to fail, we shouldn't hit a BUG().  Instead we want to give\n    +    a sensible error message, e.g., 'bad repository'\".\n    +\n    +    This is because since 9badf97c42 (remote: allow resetting url list,\n    +    2024-06-14), we reset the remote URL if the provided URL is empty. When\n    +    a user of 'remotes_remote_get' tries to fetch a remote with an empty\n    +    repo name, the function initializes the remote via 'make_remote'. But\n    +    the remote is still not a valid remote, since the URL is empty, so it\n    +    tries to add the URL alias using 'add_url_alias'. This in-turn will call\n    +    'add_url', but since the URL is empty we call 'strvec_clear' on the\n    +    `remote->url`. Back in 'remotes_remote_get', we again check if the\n    +    remote is valid, which fails, so we return 'NULL' for the 'struct\n    +    remote *' value.\n     \n         The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n         remote. This worked with empty repo names earlier since we would get a\n    @@ Commit message\n         'set_refspecs' instead of it trying to lazily obtain it.\n     \n         Helped-by: Jeff King <peff@peff.net>\n    +    Helped-by: Junio C Hamano <gitster@pobox.com>\n         Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n     \n      ## builtin/push.c ##\n    @@ t/t5529-push-errors.sh: test_expect_success 'detect missing sha1 expressions ear\n      \ttest_cmp expect rp-ran\n      '\n      \n    -+# We need to use an existing local_ref so that the remote is mapped to\n    -+# it in 'builtin/push.c:set_refspecs()'.\n    -+test_expect_success 'detect empty remote' '\n    ++# We use an existing local_ref, since it follows a different flow in\n    ++# 'builtin/push.c:set_refspecs()' and we want to test that regression.\n    ++test_expect_success 'detect empty remote with existing local ref' '\n     +\ttest_must_fail git push \"\" main 2> stderr &&\n     +\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n     +'\n    ++\n    ++# While similar to the previous test, here we want to ensure that\n    ++# even targeted refspecs are handled.\n    ++test_expect_success 'detect empty remote with targeted refspec' '\n    ++\ttest_must_fail git push \"\" HEAD:refs/heads/main 2> stderr &&\n    ++\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n    ++'\n     +\n      test_expect_success 'detect ambiguous refs early' '\n      \tgit branch foo &&\n\n builtin/push.c         | 21 +++++++--------------\n t/t5529-push-errors.sh | 17 +++++++++++++++++\n 2 files changed, 24 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/push.c b/builtin/push.c\nindex 8260c6e46a..7a67398124 100644\n--- a/builtin/push.c\n+++ b/builtin/push.c\n@@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n \trefspec_append(refspec, ref);\n }\n \n-static void set_refspecs(const char **refs, int nr, const char *repo)\n+static void set_refspecs(const char **refs, int nr, struct remote *remote)\n {\n-\tstruct remote *remote = NULL;\n \tstruct ref *local_refs = NULL;\n \tint i;\n \n@@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n \t\t\t\tlocal_refs = get_local_heads();\n \n \t\t\t/* Does \"ref\" uniquely name our ref? */\n-\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n+\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1)\n \t\t\t\trefspec_append(&rs, ref);\n-\t\t\t} else {\n-\t\t\t\t/* lazily grab remote */\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tremote = remote_get(repo);\n-\t\t\t\tif (!remote)\n-\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n-\n+\t\t\telse\n \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n-\t\t\t}\n \t\t} else\n \t\t\trefspec_append(&rs, ref);\n \t}\n@@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \tif (tags)\n \t\trefspec_append(&rs, \"refs/tags/*\");\n \n-\tif (argc > 0) {\n+\tif (argc > 0)\n \t\trepo = argv[0];\n-\t\tset_refspecs(argv + 1, argc - 1, repo);\n-\t}\n \n \tremote = pushremote_get(repo);\n \tif (!remote) {\n@@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n \t\t    \"    git push <name>\\n\"));\n \t}\n \n+\tif (argc > 0)\n+\t\tset_refspecs(argv + 1, argc - 1, remote);\n+\n \tif (remote->mirror)\n \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n \ndiff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh\nindex 0247137cb3..17d7257892 100755\n--- a/t/t5529-push-errors.sh\n+++ b/t/t5529-push-errors.sh\n@@ -2,6 +2,9 @@\n \n test_description='detect some push errors early (before contacting remote)'\n \n+GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n+export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n+\n TEST_PASSES_SANITIZE_LEAK=true\n . ./test-lib.sh\n \n@@ -38,6 +41,20 @@ test_expect_success 'detect missing sha1 expressions early' '\n \ttest_cmp expect rp-ran\n '\n \n+# We use an existing local_ref, since it follows a different flow in\n+# 'builtin/push.c:set_refspecs()' and we want to test that regression.\n+test_expect_success 'detect empty remote with existing local ref' '\n+\ttest_must_fail git push \"\" main 2> stderr &&\n+\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n+'\n+\n+# While similar to the previous test, here we want to ensure that\n+# even targeted refspecs are handled.\n+test_expect_success 'detect empty remote with targeted refspec' '\n+\ttest_must_fail git push \"\" HEAD:refs/heads/main 2> stderr &&\n+\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n+'\n+\n test_expect_success 'detect ambiguous refs early' '\n \tgit branch foo &&\n \tgit tag foo &&\n-- \n2.45.2\n\n"},{"id":"498517","messageId":"xmqqo774neyj.fsf@gitster.g","threadId":"61755","inReplyTo":"20240711093954.20317-1-karthik.188@gmail.com","subject":"Re: [PATCH v3] builtin/push: call set_refspecs after validating remote","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2024-07-11T15:08:52Z","receivedAt":"2024-07-11T15:08:55Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Karthik Nayak <karthik.188@gmail.com> writes:\n\n> When an end-user runs \"git push\" with an empty string for the remote\n> repository name, e.g.\n>\n>     $ git push '' main\n>\n> \"git push\" fails with a BUG(). Even though this is a nonsense request\n> that we want to fail, we shouldn't hit a BUG().  Instead we want to give\n> a sensible error message, e.g., 'bad repository'\".\n>\n> This is because since 9badf97c42 (remote: allow resetting url list,\n> 2024-06-14), we reset the remote URL if the provided URL is empty. When\n> a user of 'remotes_remote_get' tries to fetch a remote with an empty\n> repo name, the function initializes the remote via 'make_remote'. But\n> the remote is still not a valid remote, since the URL is empty, so it\n> tries to add the URL alias using 'add_url_alias'. This in-turn will call\n> 'add_url', but since the URL is empty we call 'strvec_clear' on the\n> `remote->url`. Back in 'remotes_remote_get', we again check if the\n> remote is valid, which fails, so we return 'NULL' for the 'struct\n> remote *' value.\n>\n> The 'builtin/push.c' code, calls 'set_refspecs' before validating the\n> remote. This worked with empty repo names earlier since we would get a\n> remote, albeit with an empty URL. With the new changes, we get a 'NULL'\n> remote value, this causes the check for remote to fail and raises the\n> BUG in 'set_refspecs'.\n>\n> Do a simple fix by doing remote validation first. Also add a test to\n> validate the bug fix. With this, we can also now directly pass remote to\n> 'set_refspecs' instead of it trying to lazily obtain it.\n>\n> Helped-by: Jeff King <peff@peff.net>\n> Helped-by: Junio C Hamano <gitster@pobox.com>\n> Signed-off-by: Karthik Nayak <karthik.188@gmail.com>\n> ---\n> ...\n>  builtin/push.c         | 21 +++++++--------------\n>  t/t5529-push-errors.sh | 17 +++++++++++++++++\n>  2 files changed, 24 insertions(+), 14 deletions(-)\n\nEverything makes sense to me.  Thanks.\n\n> diff --git a/builtin/push.c b/builtin/push.c\n> index 8260c6e46a..7a67398124 100644\n> --- a/builtin/push.c\n> +++ b/builtin/push.c\n> @@ -96,9 +96,8 @@ static void refspec_append_mapped(struct refspec *refspec, const char *ref,\n>  \trefspec_append(refspec, ref);\n>  }\n>  \n> -static void set_refspecs(const char **refs, int nr, const char *repo)\n> +static void set_refspecs(const char **refs, int nr, struct remote *remote)\n>  {\n> -\tstruct remote *remote = NULL;\n>  \tstruct ref *local_refs = NULL;\n>  \tint i;\n>  \n> @@ -124,17 +123,10 @@ static void set_refspecs(const char **refs, int nr, const char *repo)\n>  \t\t\t\tlocal_refs = get_local_heads();\n>  \n>  \t\t\t/* Does \"ref\" uniquely name our ref? */\n> -\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1) {\n> +\t\t\tif (count_refspec_match(ref, local_refs, &matched) != 1)\n>  \t\t\t\trefspec_append(&rs, ref);\n> -\t\t\t} else {\n> -\t\t\t\t/* lazily grab remote */\n> -\t\t\t\tif (!remote)\n> -\t\t\t\t\tremote = remote_get(repo);\n> -\t\t\t\tif (!remote)\n> -\t\t\t\t\tBUG(\"must get a remote for repo '%s'\", repo);\n> -\n> +\t\t\telse\n>  \t\t\t\trefspec_append_mapped(&rs, ref, remote, matched);\n> -\t\t\t}\n>  \t\t} else\n>  \t\t\trefspec_append(&rs, ref);\n>  \t}\n> @@ -630,10 +622,8 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \tif (tags)\n>  \t\trefspec_append(&rs, \"refs/tags/*\");\n>  \n> -\tif (argc > 0) {\n> +\tif (argc > 0)\n>  \t\trepo = argv[0];\n> -\t\tset_refspecs(argv + 1, argc - 1, repo);\n> -\t}\n>  \n>  \tremote = pushremote_get(repo);\n>  \tif (!remote) {\n> @@ -649,6 +639,9 @@ int cmd_push(int argc, const char **argv, const char *prefix)\n>  \t\t    \"    git push <name>\\n\"));\n>  \t}\n>  \n> +\tif (argc > 0)\n> +\t\tset_refspecs(argv + 1, argc - 1, remote);\n> +\n>  \tif (remote->mirror)\n>  \t\tflags |= (TRANSPORT_PUSH_MIRROR|TRANSPORT_PUSH_FORCE);\n>  \n> diff --git a/t/t5529-push-errors.sh b/t/t5529-push-errors.sh\n> index 0247137cb3..17d7257892 100755\n> --- a/t/t5529-push-errors.sh\n> +++ b/t/t5529-push-errors.sh\n> @@ -2,6 +2,9 @@\n>  \n>  test_description='detect some push errors early (before contacting remote)'\n>  \n> +GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main\n> +export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME\n> +\n>  TEST_PASSES_SANITIZE_LEAK=true\n>  . ./test-lib.sh\n>  \n> @@ -38,6 +41,20 @@ test_expect_success 'detect missing sha1 expressions early' '\n>  \ttest_cmp expect rp-ran\n>  '\n>  \n> +# We use an existing local_ref, since it follows a different flow in\n> +# 'builtin/push.c:set_refspecs()' and we want to test that regression.\n> +test_expect_success 'detect empty remote with existing local ref' '\n> +\ttest_must_fail git push \"\" main 2> stderr &&\n> +\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n> +'\n> +\n> +# While similar to the previous test, here we want to ensure that\n> +# even targeted refspecs are handled.\n> +test_expect_success 'detect empty remote with targeted refspec' '\n> +\ttest_must_fail git push \"\" HEAD:refs/heads/main 2> stderr &&\n> +\tgrep \"fatal: bad repository ${SQ}${SQ}\" stderr\n> +'\n> +\n>  test_expect_success 'detect ambiguous refs early' '\n>  \tgit branch foo &&\n>  \tgit tag foo &&\n"},{"id":"498541","messageId":"20240711213233.GC3648684@coredump.intra.peff.net","threadId":"61755","inReplyTo":"CAOLa=ZT-psS1NecoppGOVdxgn+kMqi8qx2vCLMKNXB4n+5hE0A@mail.gmail.com","subject":"Re: [PATCH v2] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-11T21:32:33Z","receivedAt":"2024-07-11T21:32:34Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 11, 2024 at 02:35:08AM -0700, Karthik Nayak wrote:\n\n> > But all of that would be internal refactoring / cleanup on top of your\n> > patch. The user-facing behavior would be the same.\n> \n> These should work as intended on top of my patch. But I will skip doing\n> these changes for now. I do see the merit but I think it is also okay\n> the way it is now.\n\nYep, that is perfectly fine with me. Thanks for working on this!\n\n-Peff\n"},{"id":"498542","messageId":"20240711213318.GD3648684@coredump.intra.peff.net","threadId":"61755","inReplyTo":"xmqqo774neyj.fsf@gitster.g","subject":"Re: [PATCH v3] builtin/push: call set_refspecs after validating remote","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2024-07-11T21:33:18Z","receivedAt":"2024-07-11T21:33:19Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Jul 11, 2024 at 08:08:52AM -0700, Junio C Hamano wrote:\n\n> > Do a simple fix by doing remote validation first. Also add a test to\n> > validate the bug fix. With this, we can also now directly pass remote to\n> > 'set_refspecs' instead of it trying to lazily obtain it.\n> [..]\n> \n> Everything makes sense to me.  Thanks.\n\nLikewise for me.\n\n-Peff\n"}]}