{"thread":{"id":"54859","subject":"[PATCH] negative-refspec: fix segfault on : refspec","startedAt":"2020-12-19T17:24:37Z","lastAt":"2021-02-19T09:33:23Z","messageCount":33,"participants":["Nipunn Koorapati via GitGitGadget","Junio C Hamano","Eric Sunshine","Nipunn Koorapati","Jacob Keller"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"412654","messageId":"pull.820.git.1608398598893.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":null,"subject":"[PATCH] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T17:23:18Z","receivedAt":"2020-12-19T17:24:37Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nPreviously, if remote.origin.push was set to \":\",\ngit would segfault during a push operation, due to bad\nparsing logic in query_matches_negative_refspec. Per\nbisect, the bug was introduced in:\nc0192df630 (refspec: add support for negative refspecs, 2020-09-30)\n\nAdded testing for this case in fetch-negative-refspec\n\nSigned-off-by: Nipunn Koorapati \n---\n negative-refspec: fix segfault on : refspec\n \n Previously, if remote.origin.push was set to \":\", git would segfault\n during a push operation, due to bad parsing logic in\n query_matches_negative_refspec. Per bisect, the bug was introduced in:\n c0192df630 (refspec: add support for negative refspecs, 2020-09-30)\n \n We found this issue when rolling out git 2.29 at Dropbox - as several\n folks had \"push = :\" in their configuration. I based my diff off the\n master branch, but also confirmed that it patches cleanly onto maint -\n if the maintainers would like to also fix the segfault on 2.29\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/820\n\n remote.c | 5 ++---\n t/t5582-fetch-negative-refspec.sh | 10 ++++++++++\n 2 files changed, 12 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9f2450cb51b..8ab8d25294c 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -751,9 +751,8 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \n \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n-\t\t} else {\n-\t\t\tif (!strcmp(needle, refspec->src))\n-\t\t\t\tstring_list_append(&reversed, refspec->src);\n+\t\t} else if (refspec->src != NULL && !strcmp(needle, refspec->src)) {\n+\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 8c61e28fec8..4960378e0b7 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -186,4 +186,14 @@ test_expect_success \"fetch --prune with negative refspec\" '\n \t)\n '\n \n+test_expect_success \"push with empty refspec\" '\n+\t(\n+\t\tcd two &&\n+\t\tgit config remote.one.push : &&\n+\t\t# Fails w/ tip behind counterpart - but should not segfault\n+\t\ttest_must_fail git push one master &&\n+\t\tgit config --unset remote.one.push\n+\t)\n+'\n+\n test_done\n\nbase-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n-- \ngitgitgadget\n"},{"id":"412659","messageId":"xmqqy2htoen9.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"pull.820.git.1608398598893.gitgitgadget@gmail.com","subject":"Re: [PATCH] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T18:05:14Z","receivedAt":"2020-12-19T18:06:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" writes:\n\n> From: Nipunn Koorapati \n>\n> Previously, if remote.origin.push was set to \":\",\n> git would segfault during a push operation, due to bad\n> parsing logic in query_matches_negative_refspec. Per\n> bisect, the bug was introduced in:\n> c0192df630 (refspec: add support for negative refspecs, 2020-09-30)\n>\n> Added testing for this case in fetch-negative-refspec\n\nThanks.\n\nOur local convention in this project is to write about the\nstatus-quo without the patch under discussion in the present tense,\nand describe the fix as if we are giving orders to the codebase to\nbecome like so (or giving orders to the monkeys sitting in front of\nthe keyboard to update the code). I'd explain the \"problem\ndescription\" part of the above perhaps like so:\n\n\tThe logic added to check for negative pathspec match by\n\tc0192df630 (refspec: add support for negative refspecs,\n\t2020-09-30) looks at refspec->src assuming it never is NULL,\n\tbut when remote.origin.push is set to \":\" (i.e. \"matching\"),\n\trefspec->src is NULL, causing a segfauilt.\n\t\nBut stepping back a bit, a \"matching\" push is saying \"if we have\nbranch 'hello', and they also have branch 'hello', push ours to\ntheirs\". So if the query is asking about 'hello' (e.g. needle is\n'hello'), shouldn't a refspec \":\" have the same effect as a refspec\n\"hello:hello\", instead of getting ignored like this patch does?\n\nOriginal author of the feature (Jacob) cc'ed for insight.\n\n - Can we have refspec->src==NULL in cases other than where\n refspec->matching is true? If not, then perhaps the patch should\n insert, before the problematic \"else if\" clause, something like\n\n\t\tif (match_name_with_pattern(...))\n\t\t\tstring_list_append_nodup(...);\n +\t} else if (refspec->matching) {\n +\t\t... behaviour for the matching case ...\n +\t} else if (refspec->src == NULL) {\n +\t\tBUG(\"refspec->src cannot be null here\");\n\t} else {\n\t\tif (!strcmp(needle, refspec->src))\n\n - We'd need to decide if ignoring is the right behaviour for the\n matching refspec. I do not recall what we decided the logic of\n the function should be offhand.\n\n> We found this issue when rolling out git 2.29 at Dropbox - as several\n> folks had \"push = :\" in their configuration. I based my diff off the\n> master branch, but also confirmed that it patches cleanly onto maint -\n> if the maintainers would like to also fix the segfault on 2.29\n\nYes, it is very much appreciated you were considerate to base the\npatch on the maintenance track. We want the code to do with the\nright thing with \":\" matching refspec.\n\n> diff --git a/remote.c b/remote.c\n> index 9f2450cb51b..8ab8d25294c 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -751,9 +751,8 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n> \n> \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n> \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n> -\t\t} else {\n> -\t\t\tif (!strcmp(needle, refspec->src))\n> -\t\t\t\tstring_list_append(&reversed, refspec->src);\n> +\t\t} else if (refspec->src != NULL && !strcmp(needle, refspec->src)) {\n> +\t\t\tstring_list_append(&reversed, refspec->src);\n> \t\t}\n> \t}\n> \n> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\n> index 8c61e28fec8..4960378e0b7 100755\n> --- a/t/t5582-fetch-negative-refspec.sh\n> +++ b/t/t5582-fetch-negative-refspec.sh\n> @@ -186,4 +186,14 @@ test_expect_success \"fetch --prune with negative refspec\" '\n> \t)\n> '\n> \n> +test_expect_success \"push with empty refspec\" '\n\ns/empty/matching/ (see \"git push --help\" and look for \"The special\nrefspec :\").\n\n> +\t(\n> +\t\tcd two &&\n> +\t\tgit config remote.one.push : &&\n> +\t\t# Fails w/ tip behind counterpart - but should not segfault\n> +\t\ttest_must_fail git push one master &&\n> +\t\tgit config --unset remote.one.push\n> +\t)\n> +'\n> +\n> test_done\n>\n> base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n"},{"id":"412671","messageId":"pull.820.v2.git.1608415117.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.git.1608398598893.gitgitgadget@gmail.com","subject":"[PATCH v2 0/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T21:58:35Z","receivedAt":"2020-12-19T21:59:37Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Previously, if remote.origin.push was set to \":\", git would segfault during\na push operation, due to bad parsing logic in\nquery_matches_negative_refspec. Per bisect, the bug was introduced in:\nc0192df630 (refspec: add support for negative refspecs, 2020-09-30)\n\nWe found this issue when rolling out git 2.29 at Dropbox - as several folks\nhad \"push = :\" in their configuration. I based my diff off the master\nbranch, but also confirmed that it patches cleanly onto maint - if the\nmaintainers would like to also fix the segfault on 2.29\n\nUpdate since Patch series V1:\n\n * Handled matching refspec explicitly\n * Added testing for \"+:\" case\n * Added comment explaining how the two loops work together\n\nIt may be wise to add additional testing for a case with a matching refspec\n+ negative refspec with expected behavior\n\nNipunn Koorapati (2):\n negative-refspec: fix segfault on : refspec\n negative-refspec: improve comment on query_matches_negative_refspec\n\n remote.c | 16 +++++++++++++---\n t/t5582-fetch-negative-refspec.sh | 15 +++++++++++++++\n 2 files changed, 28 insertions(+), 3 deletions(-)\n\n\nbase-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v2\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v2\nPull-Request: https://github.com/gitgitgadget/git/pull/820\n\nRange-diff vs v1:\n\n 1: 743e848653f ! 1: e42200b644a negative-refspec: fix segfault on : refspec\n @@ Metadata\n ## Commit message ##\n negative-refspec: fix segfault on : refspec\n \n - Previously, if remote.origin.push was set to \":\",\n - git would segfault during a push operation, due to bad\n - parsing logic in query_matches_negative_refspec. Per\n - bisect, the bug was introduced in:\n - c0192df630 (refspec: add support for negative refspecs, 2020-09-30)\n + The logic added to check for negative pathspec match by c0192df630\n + (refspec: add support for negative refspecs, 2020-09-30) looks at\n + refspec->src assuming it is never NULL, however when\n + remote.origin.push is set to \":\", then refspec->src is NULL,\n + causing a segfault within strcmp\n \n Added testing for this case in fetch-negative-refspec\n \n @@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct r\n -\t\t} else {\n -\t\t\tif (!strcmp(needle, refspec->src))\n -\t\t\t\tstring_list_append(&reversed, refspec->src);\n -+\t\t} else if (refspec->src != NULL && !strcmp(needle, refspec->src)) {\n ++\t\t} else if (refspec->matching) {\n ++\t\t\t/* For the special matching refspec, any query should match */\n ++\t\t\tstring_list_append(&reversed, needle);\n ++\t\t} else if (refspec->src == NULL) {\n ++\t\t\tBUG(\"refspec->src should not be null here\");\n ++\t\t} else if (!strcmp(needle, refspec->src)) {\n +\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n @@ t/t5582-fetch-negative-refspec.sh: test_expect_success \"fetch --prune with negat\n \t)\n '\n \n -+test_expect_success \"push with empty refspec\" '\n ++test_expect_success \"push with matching ':' refspec\" '\n +\t(\n +\t\tcd two &&\n +\t\tgit config remote.one.push : &&\n +\t\t# Fails w/ tip behind counterpart - but should not segfault\n +\t\ttest_must_fail git push one master &&\n ++\n ++\t\tgit config remote.one.push +: &&\n ++\t\t# Fails w/ tip behind counterpart - but should not segfault\n ++\t\ttest_must_fail git push one master &&\n ++\n +\t\tgit config --unset remote.one.push\n +\t)\n +'\n -: ----------- > 2: 8da8d9cd1c5 negative-refspec: improve comment on query_matches_negative_refspec\n\n-- \ngitgitgadget\n"},{"id":"412669","messageId":"e42200b644adab3ad78bf23e0258466287dbae70.1608415117.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v2.git.1608415117.gitgitgadget@gmail.com","subject":"[PATCH v2 1/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T21:58:36Z","receivedAt":"2020-12-19T21:59:38Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe logic added to check for negative pathspec match by c0192df630\n(refspec: add support for negative refspecs, 2020-09-30) looks at\nrefspec->src assuming it is never NULL, however when\nremote.origin.push is set to \":\", then refspec->src is NULL,\ncausing a segfault within strcmp\n\nAdded testing for this case in fetch-negative-refspec\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 10 +++++++---\n t/t5582-fetch-negative-refspec.sh | 15 +++++++++++++++\n 2 files changed, 22 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9f2450cb51b..cbb3113b105 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \n \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n-\t\t} else {\n-\t\t\tif (!strcmp(needle, refspec->src))\n-\t\t\t\tstring_list_append(&reversed, refspec->src);\n+\t\t} else if (refspec->matching) {\n+\t\t\t/* For the special matching refspec, any query should match */\n+\t\t\tstring_list_append(&reversed, needle);\n+\t\t} else if (refspec->src == NULL) {\n+\t\t\tBUG(\"refspec->src should not be null here\");\n+\t\t} else if (!strcmp(needle, refspec->src)) {\n+\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 8c61e28fec8..58b42fabd97 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -186,4 +186,19 @@ test_expect_success \"fetch --prune with negative refspec\" '\n \t)\n '\n \n+test_expect_success \"push with matching ':' refspec\" '\n+\t(\n+\t\tcd two &&\n+\t\tgit config remote.one.push : &&\n+\t\t# Fails w/ tip behind counterpart - but should not segfault\n+\t\ttest_must_fail git push one master &&\n+\n+\t\tgit config remote.one.push +: &&\n+\t\t# Fails w/ tip behind counterpart - but should not segfault\n+\t\ttest_must_fail git push one master &&\n+\n+\t\tgit config --unset remote.one.push\n+\t)\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"412670","messageId":"8da8d9cd1c5230aa3a62f1d339c5006d33630edd.1608415117.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v2.git.1608415117.gitgitgadget@gmail.com","subject":"[PATCH v2 2/2] negative-refspec: improve comment on query_matches_negative_refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T21:58:37Z","receivedAt":"2020-12-19T21:59:38Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nComment did not adequately explain how the two loops work\ntogether to achieve the goal of querying for matching of any\nnegative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex cbb3113b105..6cdaa8da75a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \t * item uses the destination. To handle this, we apply pattern\n \t * refspecs in reverse to figure out if the query source matches any\n \t * of the negative refspecs.\n+\t *\n+\t * The first loop finds and expands all positive refspecs\n+\t * matched by the queried ref.\n+\t *\n+\t * The second loop checks if any of the results of the first loop\n+\t * match any negative refspec.\n \t */\n \tfor (i = 0; i < rs->nr; i++) {\n \t\tstruct refspec_item *refspec = &rs->items[i];\n-- \ngitgitgadget\n"},{"id":"412674","messageId":"CAPig+cTBn6fPgkjaf=fYXi4XrHqc5Kf-ZJiMhxvdCDsMBuLTDQ@mail.gmail.com","threadId":"54859","inReplyTo":"e42200b644adab3ad78bf23e0258466287dbae70.1608415117.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v2 1/2] negative-refspec: fix segfault on : refspec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-20T02:57:35Z","receivedAt":"2020-12-20T02:59:06Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Dec 19, 2020 at 5:00 PM Nipunn Koorapati via GitGitGadget\n wrote:\n> The logic added to check for negative pathspec match by c0192df630\n> (refspec: add support for negative refspecs, 2020-09-30) looks at\n> refspec->src assuming it is never NULL, however when\n> remote.origin.push is set to \":\", then refspec->src is NULL,\n> causing a segfault within strcmp\n>\n> Added testing for this case in fetch-negative-refspec\n\nA couple minor comments below...\n\n> Signed-off-by: Nipunn Koorapati \n> ---\n> diff --git a/remote.c b/remote.c\n> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n> + } else if (refspec->matching) {\n> + /* For the special matching refspec, any query should match */\n> + string_list_append(&reversed, needle);\n> + } else if (refspec->src == NULL) {\n> + BUG(\"refspec->src should not be null here\");\n\nI realize that you copied Junio's example, but style on this project\nis to write this as:\n\n } else if (!refspec->src) {\n ...\n\n> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\n> @@ -186,4 +186,19 @@ test_expect_success \"fetch --prune with negative refspec\" '\n> +test_expect_success \"push with matching ':' refspec\" '\n> + (\n> + cd two &&\n> + git config remote.one.push : &&\n> + # Fails w/ tip behind counterpart - but should not segfault\n> + test_must_fail git push one master &&\n> +\n> + git config remote.one.push +: &&\n> + # Fails w/ tip behind counterpart - but should not segfault\n> + test_must_fail git push one master &&\n> +\n> + git config --unset remote.one.push\n> + )\n> +'\n\nIf anything in this test fails prior to the final `git config\n--unset`, then that cleanup command won't be executed, which might\nnegatively impact tests which follow. To ensure cleanup whether the\ntest succeeds or fails, use test_config(). Unfortunately,\ntest_config() has the limitation that it can't be used in subshells,\nso you may have to restructure the test a bit, perhaps like this:\n\n test_config remote.one.push : &&\n (\n cd two &&\n test_must_fail git push one master &&\n\n git config remote.one.push +: &&\n test_must_fail git push one master\n )\n\nDriving the test with a for-loop and taking advantage of -C to avoid\nthe subshell is also an option:\n\n for v in : +:\n do\n test_config -C two remote.one.push $v &&\n test_must_fail git -C two push one master || return 1\n done\n"},{"id":"412688","messageId":"0fd4e9f7459901e6e93bb21c41d04759b40b60c3.1608516320.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v3.git.1608516320.gitgitgadget@gmail.com","subject":"[PATCH v3 3/3] negative-refspec: improve comment on query_matches_negative_refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-21T02:05:20Z","receivedAt":"2020-12-21T04:47:19Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nComment did not adequately explain how the two loops work\ntogether to achieve the goal of querying for matching of any\nnegative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 7323694b163..c3f85c17ca7 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \t * item uses the destination. To handle this, we apply pattern\n \t * refspecs in reverse to figure out if the query source matches any\n \t * of the negative refspecs.\n+\t *\n+\t * The first loop finds and expands all positive refspecs\n+\t * matched by the queried ref.\n+\t *\n+\t * The second loop checks if any of the results of the first loop\n+\t * match any negative refspec.\n \t */\n \tfor (i = 0; i < rs->nr; i++) {\n \t\tstruct refspec_item *refspec = &rs->items[i];\n-- \ngitgitgadget\n"},{"id":"412689","messageId":"20cff2f5c59adb1076c845657aabed7ebbf0a6b5.1608516320.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v3.git.1608516320.gitgitgadget@gmail.com","subject":"[PATCH v3 2/3] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-21T02:05:19Z","receivedAt":"2020-12-21T04:50:01Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe logic added to check for negative pathspec match by c0192df630\n(refspec: add support for negative refspecs, 2020-09-30) looks at\nrefspec->src assuming it is never NULL, however when\nremote.origin.push is set to \":\", then refspec->src is NULL,\ncausing a segfault within strcmp\n\nTell git to handle matching refspec by adding the needle to the\nset of positively matched refspecs, since matching \":\" refspecs\nmatch anything as src.\n\nAdded testing for matching refspec pushes fetch-negative-refspec\nboth individually and in combination with a negative refspec\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 10 +++++++---\n t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++++++\n 2 files changed, 29 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 9f2450cb51b..7323694b163 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \n \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n-\t\t} else {\n-\t\t\tif (!strcmp(needle, refspec->src))\n-\t\t\t\tstring_list_append(&reversed, refspec->src);\n+\t\t} else if (refspec->matching) {\n+\t\t\t/* For the special matching refspec, any query should match */\n+\t\t\tstring_list_append(&reversed, needle);\n+\t\t} else if (!refspec->src) {\n+\t\t\tBUG(\"refspec->src should not be null here\");\n+\t\t} else if (!strcmp(needle, refspec->src)) {\n+\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 8c61e28fec8..30209e98a62 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -186,4 +186,26 @@ test_expect_success \"fetch --prune with negative refspec\" '\n \t)\n '\n \n+test_expect_success \"push with matching ':' refspec\" '\n+\ttest_config -C two remote.one.push : &&\n+\t# Fails w/ tip behind counterpart - but should not segfault\n+\ttest_must_fail git -C two push one\n+'\n+\n+test_expect_success \"push with matching '+:' refspec\" '\n+\ttest_config -C two remote.one.push +: &&\n+\t# Fails w/ tip behind counterpart - but should not segfault\n+\ttest_must_fail git -C two push one\n+'\n+\n+test_expect_success \"push with matching and negative refspec\" '\n+\ttest_config -C two --add remote.one.push : &&\n+\t# Fails to push master w/ tip behind counterpart\n+\ttest_must_fail git -C two push one &&\n+\n+\t# If master is in negative refspec, then the command will succeed\n+\ttest_config -C two --add remote.one.push ^refs/heads/master &&\n+\tgit -C two push one\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"412692","messageId":"pull.820.v3.git.1608516320.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v2.git.1608415117.gitgitgadget@gmail.com","subject":"[PATCH v3 0/3] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-21T02:05:17Z","receivedAt":"2020-12-21T05:00:42Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"If remote.origin.push was set to \":\", git segfaults during a push operation,\ndue to bad parsing logic in query_matches_negative_refspec. Per bisect, the\nbug was introduced in: c0192df630 (refspec: add support for negative\nrefspecs, 2020-09-30)\n\nWe found this issue when rolling out git 2.29 at Dropbox - as several folks\nhad \"push = :\" in their configuration. I based my diff off the master\nbranch, but also confirmed that it patches cleanly onto maint - if the\nmaintainers would like to also fix the segfault on 2.29\n\nUpdate since Patch series V1:\n\n * Handled matching refspec explicitly\n * Added testing for \"+:\" case\n * Added comment explaining how the two loops work together\n\nUpdate since Patch series V2\n\n * style suggestion in remote.c\n * Use test_config\n * Add test for a case with a matching refspec + negative refspec\n * Fix test_config to work with --add\n * Updated commit message to describe what git is told to do instead of\n segfaulting\n\nNipunn Koorapati (3):\n test-lib-functions: handle --add in test_config\n negative-refspec: fix segfault on : refspec\n negative-refspec: improve comment on query_matches_negative_refspec\n\n remote.c | 16 +++++++++++++---\n t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++++++\n t/test-lib-functions.sh | 9 ++++++++-\n 3 files changed, 43 insertions(+), 4 deletions(-)\n\n\nbase-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v3\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v3\nPull-Request: https://github.com/gitgitgadget/git/pull/820\n\nRange-diff vs v2:\n\n -: ----------- > 1: 733c674bd19 test-lib-functions: handle --add in test_config\n 1: e42200b644a ! 2: 20cff2f5c59 negative-refspec: fix segfault on : refspec\n @@ Commit message\n remote.origin.push is set to \":\", then refspec->src is NULL,\n causing a segfault within strcmp\n \n - Added testing for this case in fetch-negative-refspec\n + Tell git to handle matching refspec by adding the needle to the\n + set of positively matched refspecs, since matching \":\" refspecs\n + match anything as src.\n +\n + Added testing for matching refspec pushes fetch-negative-refspec\n + both individually and in combination with a negative refspec\n \n Signed-off-by: Nipunn Koorapati \n \n @@ remote.c: static int query_matches_negative_refspec(struct refspec *rs, struct r\n +\t\t} else if (refspec->matching) {\n +\t\t\t/* For the special matching refspec, any query should match */\n +\t\t\tstring_list_append(&reversed, needle);\n -+\t\t} else if (refspec->src == NULL) {\n ++\t\t} else if (!refspec->src) {\n +\t\t\tBUG(\"refspec->src should not be null here\");\n +\t\t} else if (!strcmp(needle, refspec->src)) {\n +\t\t\tstring_list_append(&reversed, refspec->src);\n @@ t/t5582-fetch-negative-refspec.sh: test_expect_success \"fetch --prune with negat\n '\n \n +test_expect_success \"push with matching ':' refspec\" '\n -+\t(\n -+\t\tcd two &&\n -+\t\tgit config remote.one.push : &&\n -+\t\t# Fails w/ tip behind counterpart - but should not segfault\n -+\t\ttest_must_fail git push one master &&\n ++\ttest_config -C two remote.one.push : &&\n ++\t# Fails w/ tip behind counterpart - but should not segfault\n ++\ttest_must_fail git -C two push one\n ++'\n ++\n ++test_expect_success \"push with matching '+:' refspec\" '\n ++\ttest_config -C two remote.one.push +: &&\n ++\t# Fails w/ tip behind counterpart - but should not segfault\n ++\ttest_must_fail git -C two push one\n ++'\n +\n -+\t\tgit config remote.one.push +: &&\n -+\t\t# Fails w/ tip behind counterpart - but should not segfault\n -+\t\ttest_must_fail git push one master &&\n ++test_expect_success \"push with matching and negative refspec\" '\n ++\ttest_config -C two --add remote.one.push : &&\n ++\t# Fails to push master w/ tip behind counterpart\n ++\ttest_must_fail git -C two push one &&\n +\n -+\t\tgit config --unset remote.one.push\n -+\t)\n ++\t# If master is in negative refspec, then the command will succeed\n ++\ttest_config -C two --add remote.one.push ^refs/heads/master &&\n ++\tgit -C two push one\n +'\n +\n test_done\n 2: 8da8d9cd1c5 = 3: 0fd4e9f7459 negative-refspec: improve comment on query_matches_negative_refspec\n\n-- \ngitgitgadget\n"},{"id":"412697","messageId":"733c674bd1901c931a8917045eb72f661872f462.1608516320.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v3.git.1608516320.gitgitgadget@gmail.com","subject":"[PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-21T02:05:18Z","receivedAt":"2020-12-21T05:21:34Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\ntest_config fails to unset the configuration variable when\nusing --add, as it tries to run git config --unset-all --add\n\nTell test_config to invoke test_unconfig with the arg $2 when\nthe arg $1 is --add\n\nSigned-off-by: Nipunn Koorapati \n---\n t/test-lib-functions.sh | 9 ++++++++-\n 1 file changed, 8 insertions(+), 1 deletion(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex 999982fe4a9..1fdd7129d51 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -381,6 +381,7 @@ test_unconfig () {\n \t\tconfig_dir=$1\n \t\tshift\n \tfi\n+\techo git ${config_dir:+-C \"$config_dir\"} config --unset-all \"$@\"\n \tgit ${config_dir:+-C \"$config_dir\"} config --unset-all \"$@\"\n \tconfig_status=$?\n \tcase \"$config_status\" in\n@@ -400,7 +401,13 @@ test_config () {\n \t\tconfig_dir=$1\n \t\tshift\n \tfi\n-\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n+\n+\tfirst_arg=$1\n+\tif test \"$1\" = --add; then\n+\t\tfirst_arg=$2\n+\tfi\n+\n+\ttest_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'\" &&\n \tgit ${config_dir:+-C \"$config_dir\"} config \"$@\"\n }\n \n-- \ngitgitgadget\n\n"},{"id":"412703","messageId":"CAPig+cSaq4vTK7CtvxB2bd0=WTW+d=s0H2RMquyCEf+q0YVn2w@mail.gmail.com","threadId":"54859","inReplyTo":"733c674bd1901c931a8917045eb72f661872f462.1608516320.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T07:07:31Z","receivedAt":"2020-12-21T07:08:25Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget\n wrote:\n> test_config fails to unset the configuration variable when\n> using --add, as it tries to run git config --unset-all --add\n>\n> Tell test_config to invoke test_unconfig with the arg $2 when\n> the arg $1 is --add\n>\n> Signed-off-by: Nipunn Koorapati \n> ---\n> diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\n> @@ -381,6 +381,7 @@ test_unconfig () {\n> config_dir=$1\n> shift\n> fi\n> + echo git ${config_dir:+-C \"$config_dir\"} config --unset-all \"$@\"\n\nStray debugging gunk?\n\n> @@ -400,7 +401,13 @@ test_config () {\n> - test_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$1'\" &&\n> +\n> + first_arg=$1\n> + if test \"$1\" = --add; then\n> + first_arg=$2\n> + fi\n> +\n> + test_when_finished \"test_unconfig ${config_dir:+-C '$config_dir'} '$first_arg'\" &&\n\nSeveral comments...\n\nStyle on this project is to place `then` on its own line (as seen a\nfew lines above this change):\n\n if test \"$1\" = --add\n then\n ...\n\nThis logic would be easier to understand if the variable was named\n`varname` or `cfgvar` (or something), which better conveys intention,\nrather than `first_arg`.\n\nIt feels odd to single out `--add` when there are other similar\noptions, such as `--replace-all`, `--fixed-value`, or even `--type`\nwhich people might try using in the future.\n\nThis new option parsing is somewhat brittle. If a caller uses\n`test_config --add -C ...`, it won't work as expected. Perhaps\nthat's not likely to happen, but it would be easy enough to fix by\nunifying and generalizing option parsing a bit. Doing so would also\nmake it easy for the other options mentioned above to be added later\nif ever needed. For instance:\n\n options=\n while test $# != 0\n do\n case \"$1\" in\n -C)\n config_dir=$2\n shift\n ;;\n --add)\n options=\"$options $1\"\n ;;\n *)\n break\n ;;\n esac\n shift\n done\n\nFinally, as this is a one-off case, it might be simpler just to drop\nthis patch altogether and open-code the cleanup in the test itself in\npatch [2/3] rather than bothering with test_config() in that\nparticular case. For example:\n\n test_when_finished \"test_unconfig -C two remote.one.push\" &&\n git config -C two --add remote.one.push : &&\n test_must_fail git -C two push one &&\n git config -C two --add remote.one.push ^refs/heads/master &&\n git -C two push one\n"},{"id":"412704","messageId":"CAPig+cStb8a7QW-TDit2mfodEQMcPQTsCB0eJX=BMZJzo-TmUQ@mail.gmail.com","threadId":"54859","inReplyTo":"20cff2f5c59adb1076c845657aabed7ebbf0a6b5.1608516320.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v3 2/3] negative-refspec: fix segfault on : refspec","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T07:20:40Z","receivedAt":"2020-12-21T07:22:29Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Dec 20, 2020 at 9:05 PM Nipunn Koorapati via GitGitGadget\n wrote:\n> The logic added to check for negative pathspec match by c0192df630\n> (refspec: add support for negative refspecs, 2020-09-30) looks at\n> refspec->src assuming it is never NULL, however when\n> remote.origin.push is set to \":\", then refspec->src is NULL,\n> causing a segfault within strcmp\n>\n> Tell git to handle matching refspec by adding the needle to the\n> set of positively matched refspecs, since matching \":\" refspecs\n> match anything as src.\n>\n> Added testing for matching refspec pushes fetch-negative-refspec\n\ns/Added testing/Add test/\n\n> both individually and in combination with a negative refspec\n>\n> Signed-off-by: Nipunn Koorapati \n> ---\n> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\n> @@ -186,4 +186,26 @@ test_expect_success \"fetch --prune with negative refspec\" '\n> +test_expect_success \"push with matching ':' refspec\" '\n> + test_config -C two remote.one.push : &&\n> + # Fails w/ tip behind counterpart - but should not segfault\n> + test_must_fail git -C two push one\n> +'\n\nNit: It is understood implicitly that Git should not segfault (or\nindeed any software). That's also implied by use of test_must_fail()\nwhich explicitly distinguishes expected failures from unexpected\nfailures (where segfault falls in the category of unexpected failure).\nTherefore, it doesn't really add value to say \"but should not\nsegfault\" in the comment.\n\nSame observation applies to the other similarly-worded comments in\nthis patch. Not alone worth a re-roll, but perhaps worth changing if\nyou do re-roll.\n"},{"id":"412755","messageId":"xmqqa6u7m1bu.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"CAPig+cSaq4vTK7CtvxB2bd0=WTW+d=s0H2RMquyCEf+q0YVn2w@mail.gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-21T19:00:21Z","receivedAt":"2020-12-21T19:01:11Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> Finally, as this is a one-off case, it might be simpler just to drop\n> this patch altogether and open-code the cleanup in the test itself in\n> patch [2/3] rather than bothering with test_config() in that\n> particular case. For example:\n>\n> test_when_finished \"test_unconfig -C two remote.one.push\" &&\n> git config -C two --add remote.one.push : &&\n> test_must_fail git -C two push one &&\n> git config -C two --add remote.one.push ^refs/heads/master &&\n> git -C two push one\n\nThat would be my preference, too. Thanks for carefully and\npatiently reviewing.\n\n"},{"id":"412769","messageId":"CAPig+cRqa9Y4mEdktdP3d2+PHWanKZ6q6tXfJXEAW9sqcVwHOw@mail.gmail.com","threadId":"54859","inReplyTo":"xmqqa6u7m1bu.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-21T20:08:52Z","receivedAt":"2020-12-21T20:10:01Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 21, 2020 at 2:00 PM Junio C Hamano wrote:\n> Eric Sunshine writes:\n> > Finally, as this is a one-off case, it might be simpler just to drop\n> > this patch altogether and open-code the cleanup in the test itself in\n> > patch [2/3] rather than bothering with test_config() in that\n> > particular case. For example:\n> >\n> > test_when_finished \"test_unconfig -C two remote.one.push\" &&\n> > git config -C two --add remote.one.push : &&\n> > test_must_fail git -C two push one &&\n> > git config -C two --add remote.one.push ^refs/heads/master &&\n> > git -C two push one\n>\n> That would be my preference, too. Thanks for carefully and\n> patiently reviewing.\n\nI forgot to mention that it likely would be a good idea to at least\nmention in the commit message why test_config() is not being used for\nthat particular case. Perhaps saying something along the lines of \"one\ntest handles config cleanup manually since test_config() is not\nprepared to take arbitrary options such as --add\" -- or something\nalong those lines -- would be sufficient. Alternatively, an in-code\ncomment within the test explaining the open-coding might be more\nhelpful to people reading the code in the future.\n"},{"id":"412791","messageId":"CAN8Z4-UG-watOnJMYUe3KU4JHnmJTxvwKSZ3s2DtBg104PACaA@mail.gmail.com","threadId":"54859","inReplyTo":"CAPig+cRqa9Y4mEdktdP3d2+PHWanKZ6q6tXfJXEAW9sqcVwHOw@mail.gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-12-22T00:00:29Z","receivedAt":"2020-12-22T00:01:24Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> I forgot to mention that it likely would be a good idea to at least\n> mention in the commit message why test_config() is not being used for\n> that particular case. Perhaps saying something along the lines of \"one\n> test handles config cleanup manually since test_config() is not\n> prepared to take arbitrary options such as --add\" -- or something\n> along those lines -- would be sufficient. Alternatively, an in-code\n> comment within the test explaining the open-coding might be more\n> helpful to people reading the code in the future.\n\nI found that since test_unconfig uses --unset-all, I can write a test as such\n\n test_config -C two remote.one.push +: &&\n test_must_fail git -C two push one &&\n git -C two config --add remote.one.push ^refs/heads/master &&\n git -C two push one\n\nThe unconfig of the test_config will --unset-all remote.one.push. I can\nuse this technique and add a comment to that extent.\n\n--Nipunn\n"},{"id":"412829","messageId":"CAPig+cS4F5fhu-ej5ZVpzLR17AUhxzLRKVZxLuKaMCCk937C1A@mail.gmail.com","threadId":"54859","inReplyTo":"CAN8Z4-UG-watOnJMYUe3KU4JHnmJTxvwKSZ3s2DtBg104PACaA@mail.gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-22T00:13:08Z","receivedAt":"2020-12-22T00:14:02Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 21, 2020 at 7:00 PM Nipunn Koorapati wrote:\n> I found that since test_unconfig uses --unset-all, I can write a test as such\n>\n> test_config -C two remote.one.push +: &&\n> test_must_fail git -C two push one &&\n> git -C two config --add remote.one.push ^refs/heads/master &&\n> git -C two push one\n>\n> The unconfig of the test_config will --unset-all remote.one.push. I can\n> use this technique and add a comment to that extent.\n\nYes, you could do that, though it is somewhat subtle and increases\ncognitive load since the reader has to reason about it a bit more --\nand perhaps study the internal implementation of test_config() -- to\nconvince himself or herself that the different methods of setting\nconfiguration (test_config() vs. `git config`) used in the same test\nis intentional and works as intended.\n\nThe example presented earlier, on the other hand, in which cleanup is\nexplicit via `test_when_finished \"test_unconfig ...\"` does not suffer\nfrom such increased cognitive load since it uses `git config`\nconsistently to set configuration rather than a mix of `git config`\nand test_config(). This sort of consideration is important not just\nfor reviewers, but for people who need to understand the code down the\nroad. For this reason, I think I favor the version in which the\ncleanup is explicit. (But that's just my opinion...)\n"},{"id":"412831","messageId":"20575407cc0d60a1c0a0f8251b45b2e5a317d465.1608599513.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v4.git.1608599513.gitgitgadget@gmail.com","subject":"[PATCH v4 2/2] negative-refspec: improve comment on query_matches_negative_refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T01:11:53Z","receivedAt":"2020-12-22T01:12:53Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nComment did not adequately explain how the two loops work\ntogether to achieve the goal of querying for matching of any\nnegative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 4f1a4099f1a..4d150a316ed 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \t * item uses the destination. To handle this, we apply pattern\n \t * refspecs in reverse to figure out if the query source matches any\n \t * of the negative refspecs.\n+\t *\n+\t * The first loop finds and expands all positive refspecs\n+\t * matched by the queried ref.\n+\t *\n+\t * The second loop checks if any of the results of the first loop\n+\t * match any negative refspec.\n \t */\n \tfor (i = 0; i < rs->nr; i++) {\n \t\tstruct refspec_item *refspec = &rs->items[i];\n-- \ngitgitgadget\n"},{"id":"412832","messageId":"pull.820.v4.git.1608599513.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v3.git.1608516320.gitgitgadget@gmail.com","subject":"[PATCH v4 0/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T01:11:51Z","receivedAt":"2020-12-22T01:12:53Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"If remote.origin.push was set to \":\", git segfaults during a push operation,\ndue to bad parsing logic in query_matches_negative_refspec. Per bisect, the\nbug was introduced in: c0192df630 (refspec: add support for negative\nrefspecs, 2020-09-30)\n\nWe found this issue when rolling out git 2.29 at Dropbox - as several folks\nhad \"push = :\" in their configuration. I based my diff off the master\nbranch, but also confirmed that it patches cleanly onto maint - if the\nmaintainers would like to also fix the segfault on 2.29\n\nUpdate since Patch series V1:\n\n * Handled matching refspec explicitly\n * Added testing for \"+:\" case\n * Added comment explaining how the two loops work together\n\nUpdate since Patch series V2\n\n * style suggestion in remote.c\n * Use test_config\n * Add test for a case with a matching refspec + negative refspec\n * Fix test_config to work with --add\n * Updated commit message to describe what git is told to do instead of\n segfaulting\n\nUpdate since Patch series V3\n\n * Removed commit modifying test_config\n * Remove segfault-related comments in test\n * Consolidate the three tests to two tests (1st and 3rd test overlapped in\n functionality)\n * Base the patch series on the maint branch - since the bug affects 2.29.2\n\nAppreciate the reviews from Junio and Eric! Happy Holidays!\n\nNipunn Koorapati (2):\n negative-refspec: fix segfault on : refspec\n negative-refspec: improve comment on query_matches_negative_refspec\n\n remote.c | 16 +++++++++++++---\n t/t5582-fetch-negative-refspec.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\n\nbase-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v4\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v4\nPull-Request: https://github.com/gitgitgadget/git/pull/820\n\nRange-diff vs v3:\n\n 1: 733c674bd19 < -: ----------- test-lib-functions: handle --add in test_config\n 2: 20cff2f5c59 ! 1: e59ff29bdef negative-refspec: fix segfault on : refspec\n @@ Commit message\n (refspec: add support for negative refspecs, 2020-09-30) looks at\n refspec->src assuming it is never NULL, however when\n remote.origin.push is set to \":\", then refspec->src is NULL,\n - causing a segfault within strcmp\n + causing a segfault within strcmp.\n \n Tell git to handle matching refspec by adding the needle to the\n set of positively matched refspecs, since matching \":\" refspecs\n match anything as src.\n \n - Added testing for matching refspec pushes fetch-negative-refspec\n - both individually and in combination with a negative refspec\n + Add test for matching refspec pushes fetch-negative-refspec\n + both individually and in combination with a negative refspec.\n \n Signed-off-by: Nipunn Koorapati \n \n @@ t/t5582-fetch-negative-refspec.sh: test_expect_success \"fetch --prune with negat\n \t)\n '\n \n -+test_expect_success \"push with matching ':' refspec\" '\n ++test_expect_success \"push with matching : and negative refspec\" '\n +\ttest_config -C two remote.one.push : &&\n -+\t# Fails w/ tip behind counterpart - but should not segfault\n -+\ttest_must_fail git -C two push one\n -+'\n ++\t# Fails to push master w/ tip behind counterpart\n ++\ttest_must_fail git -C two push one &&\n +\n -+test_expect_success \"push with matching '+:' refspec\" '\n -+\ttest_config -C two remote.one.push +: &&\n -+\t# Fails w/ tip behind counterpart - but should not segfault\n -+\ttest_must_fail git -C two push one\n ++\t# If master is in negative refspec, then the command will not attempt\n ++\t# to push and succeed.\n ++\t# We do not need test_config here as we are updating remote.one.push\n ++\t# again. The teardown of the first test_config will do --unset-all\n ++\tgit -C two config --add remote.one.push ^refs/heads/master &&\n ++\tgit -C two push one\n +'\n +\n -+test_expect_success \"push with matching and negative refspec\" '\n -+\ttest_config -C two --add remote.one.push : &&\n ++test_expect_success \"push with matching +: and negative refspec\" '\n ++\ttest_config -C two remote.one.push +: &&\n +\t# Fails to push master w/ tip behind counterpart\n +\ttest_must_fail git -C two push one &&\n +\n -+\t# If master is in negative refspec, then the command will succeed\n -+\ttest_config -C two --add remote.one.push ^refs/heads/master &&\n ++\t# If master is in negative refspec, then the command will not attempt\n ++\t# to push and succeed\n ++\tgit -C two config --add remote.one.push ^refs/heads/master &&\n +\tgit -C two push one\n +'\n +\n 3: 0fd4e9f7459 = 2: 20575407cc0 negative-refspec: improve comment on query_matches_negative_refspec\n\n-- \ngitgitgadget\n"},{"id":"412833","messageId":"e59ff29bdef9ce6bbdf8fbab307178e3e983cf2c.1608599513.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v4.git.1608599513.gitgitgadget@gmail.com","subject":"[PATCH v4 1/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T01:11:52Z","receivedAt":"2020-12-22T01:12:54Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe logic added to check for negative pathspec match by c0192df630\n(refspec: add support for negative refspecs, 2020-09-30) looks at\nrefspec->src assuming it is never NULL, however when\nremote.origin.push is set to \":\", then refspec->src is NULL,\ncausing a segfault within strcmp.\n\nTell git to handle matching refspec by adding the needle to the\nset of positively matched refspecs, since matching \":\" refspecs\nmatch anything as src.\n\nAdd test for matching refspec pushes fetch-negative-refspec\nboth individually and in combination with a negative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 10 +++++++---\n t/t5582-fetch-negative-refspec.sh | 24 ++++++++++++++++++++++++\n 2 files changed, 31 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 8be67f0892b..4f1a4099f1a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \n \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n-\t\t} else {\n-\t\t\tif (!strcmp(needle, refspec->src))\n-\t\t\t\tstring_list_append(&reversed, refspec->src);\n+\t\t} else if (refspec->matching) {\n+\t\t\t/* For the special matching refspec, any query should match */\n+\t\t\tstring_list_append(&reversed, needle);\n+\t\t} else if (!refspec->src) {\n+\t\t\tBUG(\"refspec->src should not be null here\");\n+\t\t} else if (!strcmp(needle, refspec->src)) {\n+\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 8c61e28fec8..a4960c586b1 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -186,4 +186,28 @@ test_expect_success \"fetch --prune with negative refspec\" '\n \t)\n '\n \n+test_expect_success \"push with matching : and negative refspec\" '\n+\ttest_config -C two remote.one.push : &&\n+\t# Fails to push master w/ tip behind counterpart\n+\ttest_must_fail git -C two push one &&\n+\n+\t# If master is in negative refspec, then the command will not attempt\n+\t# to push and succeed.\n+\t# We do not need test_config here as we are updating remote.one.push\n+\t# again. The teardown of the first test_config will do --unset-all\n+\tgit -C two config --add remote.one.push ^refs/heads/master &&\n+\tgit -C two push one\n+'\n+\n+test_expect_success \"push with matching +: and negative refspec\" '\n+\ttest_config -C two remote.one.push +: &&\n+\t# Fails to push master w/ tip behind counterpart\n+\ttest_must_fail git -C two push one &&\n+\n+\t# If master is in negative refspec, then the command will not attempt\n+\t# to push and succeed\n+\tgit -C two config --add remote.one.push ^refs/heads/master &&\n+\tgit -C two push one\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"412834","messageId":"xmqqk0tak2ym.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"e59ff29bdef9ce6bbdf8fbab307178e3e983cf2c.1608599513.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v4 1/2] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T02:08:01Z","receivedAt":"2020-12-22T02:08:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" writes:\n\n> +test_expect_success \"push with matching : and negative refspec\" '\n> +\ttest_config -C two remote.one.push : &&\n> +\t# Fails to push master w/ tip behind counterpart\n> +\ttest_must_fail git -C two push one &&\n\nI offhand do not know where the master branch of two and one\nrepositories are, but I presume that one's master is not an ancestor\nof two's master here, and the reason why this fails is because we\nwould prevent such a non-ff push unless forced? Are there other\nmatching refs between one and two that are subject to the push\noperation here, or is the 'master' the only thing that exists?\n\n> +\t# If master is in negative refspec, then the command will not attempt\n> +\t# to push and succeed.\n> +\t# We do not need test_config here as we are updating remote.one.push\n> +\t# again. The teardown of the first test_config will do --unset-all\n> +\tgit -C two config --add remote.one.push ^refs/heads/master &&\n> +\tgit -C two push one\n\n... and the idea of the test is that by adding a \"we do not want to\npush out our master\" configuration, we no longer attempt to push out\nthe 'master' branch from two that is not a descendant of the master\nbranch of one, so \"push\" would \"succeed\". Is there other branches\ninvolved, or this is essentially a no-op as there is only 'master'\nbranch involved in the operation?\n\n> +'\n> +\n> +test_expect_success \"push with matching +: and negative refspec\" '\n> +\ttest_config -C two remote.one.push +: &&\n> +\t# Fails to push master w/ tip behind counterpart\n> +\ttest_must_fail git -C two push one &&\n\nAssuming that the successful case from the previous test was a\nno-op, we start from the same condition from the previous one. THe\nonly difference is that the matching push is now configured to force.\n\nSo, how would this one fail, exactly? Aren't we forcing? Shouldn't\nwe succeed in such a case?\n\nI think the test still fails to push but for a different reason. It\nis not because the tip being pushed is not ahead of the counterpart\nat the receiving repository. +: (i.e. force-push matching refs)\ntakes care of the \"must fast-forward\" requirement that causes the\nprevious one to fail.\n\nIt is because the receiving repository is not a bare repository, and\nthe push attempts to update its current branch. It cannot be forced\nwith + prefix, and that is why it fails.\n\nSo, the comment above is wrong. Perhaps\n\n\t# Fail to update the branch currently checked out.\n\nor something.\n\n> +\t# If master is in negative refspec, then the command will not attempt\n> +\t# to push and succeed\n> +\tgit -C two config --add remote.one.push ^refs/heads/master &&\n> +\tgit -C two push one\n\nAnd this succeeds for the same reason, i.e. it becomes no-op because\nthere is no other branches involved?\n\n> +'\n> +\n> test_done\n\nIdeally, we should make sure that the next person who reads \"git\nshow\" output of the commit that would result from the patch would\nnot have to ask any of the \"?\" asked in the review above. Let me\nsee if I can come up with a suggestion to get us closer to that\ngoal.\n\n\t... goes and hacks ...\n\nPerhaps squash the following into this step?\n\nThanks.\n\n\n t/t5582-fetch-negative-refspec.sh | 22 ++++++++++++++++++----\n 1 file changed, 18 insertions(+), 4 deletions(-)\n\ndiff --git c/t/t5582-fetch-negative-refspec.sh w/t/t5582-fetch-negative-refspec.sh\nindex a4960c586b..bed67cf92d 100755\n--- c/t/t5582-fetch-negative-refspec.sh\n+++ w/t/t5582-fetch-negative-refspec.sh\n@@ -187,8 +187,13 @@ test_expect_success \"fetch --prune with negative refspec\" '\n '\n \n test_expect_success \"push with matching : and negative refspec\" '\n+\t# Repositories two and one have branches other than master\"\n+\t# but they have no overlap---\"master\" is the only one that\n+\t# is shared between them. And the master branch at two is\n+\t# behind the master branch at one by one commit.\n \ttest_config -C two remote.one.push : &&\n-\t# Fails to push master w/ tip behind counterpart\n+\n+\t# A matching push tries to update master, fails due to non-ff\n \ttest_must_fail git -C two push one &&\n \n \t# If master is in negative refspec, then the command will not attempt\n@@ -196,18 +201,27 @@ test_expect_success \"push with matching : and negative refspec\" '\n \t# We do not need test_config here as we are updating remote.one.push\n \t# again. The teardown of the first test_config will do --unset-all\n \tgit -C two config --add remote.one.push ^refs/heads/master &&\n-\tgit -C two push one\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n '\n \n test_expect_success \"push with matching +: and negative refspec\" '\n+\t# The same set-up as above, whose side-effect was a no-op.\n \ttest_config -C two remote.one.push +: &&\n-\t# Fails to push master w/ tip behind counterpart\n+\n+\t# The push refuses to update the \"master\" branch that is checked\n+\t# out in the \"one\" repository, even when it is forced with +:\n \ttest_must_fail git -C two push one &&\n \n \t# If master is in negative refspec, then the command will not attempt\n \t# to push and succeed\n \tgit -C two config --add remote.one.push ^refs/heads/master &&\n-\tgit -C two push one\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n '\n \n test_done\n"},{"id":"412836","messageId":"CAN8Z4-WpNuqN=HxL4AQU_+zi4hGhkC18d3ZSOJGzbdkMPkYAMQ@mail.gmail.com","threadId":"54859","inReplyTo":"CAPig+cS4F5fhu-ej5ZVpzLR17AUhxzLRKVZxLuKaMCCk937C1A@mail.gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-12-22T02:25:06Z","receivedAt":"2020-12-22T02:25:59Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> Yes, you could do that, though it is somewhat subtle and increases\n> cognitive load since the reader has to reason about it a bit more --\n> and perhaps study the internal implementation of test_config() -- to\n> convince himself or herself that the different methods of setting\n> configuration (test_config() vs. `git config`) used in the same test\n> is intentional and works as intended.\n>\n> The example presented earlier, on the other hand, in which cleanup is\n> explicit via `test_when_finished \"test_unconfig ...\"` does not suffer\n> from such increased cognitive load since it uses `git config`\n> consistently to set configuration rather than a mix of `git config`\n> and test_config(). This sort of consideration is important not just\n> for reviewers, but for people who need to understand the code down the\n> road. For this reason, I think I favor the version in which the\n> cleanup is explicit. (But that's just my opinion...)\n\nTotally sympathize with wanting to reduce the cognitive load of\nreading the test suite.\nAs a reader of the test, I found both implementation choices\nnonobvious - and requiring\nsome diving into test_config and test_unconfig (namely the --unset-all\nbehavior).\nA comment on the `git config` stating that it's there because\n`test_config` doesn't yet support\n`--add` seems equally clarifying as inserting a comment next to the\ntest_unconfig usage.\nThat being said, I suspect anyone in the future poking around with\nthis will have to sourcedive\nthrough test_config and test_unconfig to make sense of it.\n\nI'll switch it over to the test_unconfig option on the next reroll if requested.\n\n--Nipunn\n"},{"id":"412837","messageId":"xmqqa6u6k1zt.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"xmqqk0tak2ym.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v4 1/2] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T02:28:54Z","receivedAt":"2020-12-22T02:29:42Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> @@ -196,18 +201,27 @@ test_expect_success \"push with matching : and negative refspec\" '\n> \t# We do not need test_config here as we are updating remote.one.push\n> \t# again. The teardown of the first test_config will do --unset-all\n> \tgit -C two config --add remote.one.push ^refs/heads/master &&\n> -\tgit -C two push one\n> +\n> +\t# With \"master\" excluded, this push is a no-op. Nothing gets\n> +\t# pushed and it succeeds.\n> +\tgit -C two push -v one\n> '\n\nAnother obvious thing is that these tests will not work without\ntweaking when merged to 'seen', as over there the name given by\ndefault to the initial branch might not be 'master'. The negative\nrefspec specification must be written in a way not to depend on\na particular name, I think.\n\nHere is another try (disregard the previous one and squash this one\non top of your 1/2).\n\nThanks.\n\n t/t5582-fetch-negative-refspec.sh | 35 +++++++++++++++++++++++++++++------\n 1 file changed, 29 insertions(+), 6 deletions(-)\n\ndiff --git c/t/t5582-fetch-negative-refspec.sh w/t/t5582-fetch-negative-refspec.sh\nindex a4960c586b..83a3c58c0c 100755\n--- c/t/t5582-fetch-negative-refspec.sh\n+++ w/t/t5582-fetch-negative-refspec.sh\n@@ -187,27 +187,50 @@ test_expect_success \"fetch --prune with negative refspec\" '\n '\n \n test_expect_success \"push with matching : and negative refspec\" '\n+\t# For convenience, we use \"master\" to refer to the name of\n+\t# the branch created by default in the following.\n+\t#\n+\t# Repositories two and one have branches other than \"master\"\n+\t# but they have no overlap---\"master\" is the only one that\n+\t# is shared between them. And the master branch at two is\n+\t# behind the master branch at one by one commit.\n \ttest_config -C two remote.one.push : &&\n-\t# Fails to push master w/ tip behind counterpart\n+\n+\t# A matching push tries to update master, fails due to non-ff\n \ttest_must_fail git -C two push one &&\n \n+\t# \"master\" may actually not be \"master\"---find it out.\n+\tcurrent=$(git symbolic-ref HEAD) &&\n+\n \t# If master is in negative refspec, then the command will not attempt\n \t# to push and succeed.\n \t# We do not need test_config here as we are updating remote.one.push\n \t# again. The teardown of the first test_config will do --unset-all\n-\tgit -C two config --add remote.one.push ^refs/heads/master &&\n-\tgit -C two push one\n+\tgit -C two config --add remote.one.push \"^$current\" &&\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n '\n \n test_expect_success \"push with matching +: and negative refspec\" '\n+\t# The same set-up as above, whose side-effect was a no-op.\n \ttest_config -C two remote.one.push +: &&\n-\t# Fails to push master w/ tip behind counterpart\n+\n+\t# The push refuses to update the \"master\" branch that is checked\n+\t# out in the \"one\" repository, even when it is forced with +:\n \ttest_must_fail git -C two push one &&\n \n+\t# \"master\" may actually not be \"master\"---find it out.\n+\tcurrent=$(git symbolic-ref HEAD) &&\n+\n \t# If master is in negative refspec, then the command will not attempt\n \t# to push and succeed\n-\tgit -C two config --add remote.one.push ^refs/heads/master &&\n-\tgit -C two push one\n+\tgit -C two config --add remote.one.push \"^$current\" &&\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n '\n \n test_done\n"},{"id":"412838","messageId":"pull.820.v5.git.1608609498.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v4.git.1608599513.gitgitgadget@gmail.com","subject":"[PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T03:58:15Z","receivedAt":"2020-12-22T03:59:02Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"If remote.origin.push was set to \":\", git segfaults during a push operation,\ndue to bad parsing logic in query_matches_negative_refspec. Per bisect, the\nbug was introduced in: c0192df630 (refspec: add support for negative\nrefspecs, 2020-09-30)\n\nWe found this issue when rolling out git 2.29 at Dropbox - as several folks\nhad \"push = :\" in their configuration. I based my diff off the master\nbranch, but also confirmed that it patches cleanly onto maint - if the\nmaintainers would like to also fix the segfault on 2.29\n\nUpdate since Patch series V1:\n\n * Handled matching refspec explicitly\n * Added testing for \"+:\" case\n * Added comment explaining how the two loops work together\n\nUpdate since Patch series V2\n\n * style suggestion in remote.c\n * Use test_config\n * Add test for a case with a matching refspec + negative refspec\n * Fix test_config to work with --add\n * Updated commit message to describe what git is told to do instead of\n segfaulting\n\nUpdate since Patch series V3\n\n * Removed commit modifying test_config\n * Remove segfault-related comments in test\n * Consolidate the three tests to two tests (1st and 3rd test overlapped in\n functionality)\n * Base the patch series on the maint branch - since the bug affects 2.29.2\n\nUpdate since Patch series V4\n\n * Squashed in Junio's patch to handle non-master named branches\n * Explicitly use test_unconfig\n\nAppreciate the reviews from Junio and Eric! Happy Holidays!\n\nNipunn Koorapati (2):\n negative-refspec: fix segfault on : refspec\n negative-refspec: improve comment on query_matches_negative_refspec\n\n remote.c | 16 ++++++++--\n t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++\n 2 files changed, 64 insertions(+), 3 deletions(-)\n\n\nbase-commit: 898f80736c75878acc02dc55672317fcc0e0a5a6\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-820%2Fnipunn1313%2Fnk%2Fpush-refspec-segfault-v5\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-820/nipunn1313/nk/push-refspec-segfault-v5\nPull-Request: https://github.com/gitgitgadget/git/pull/820\n\nRange-diff vs v4:\n\n 1: e59ff29bdef ! 1: 48c79dc3d84 negative-refspec: fix segfault on : refspec\n @@ t/t5582-fetch-negative-refspec.sh: test_expect_success \"fetch --prune with negat\n '\n \n +test_expect_success \"push with matching : and negative refspec\" '\n -+\ttest_config -C two remote.one.push : &&\n -+\t# Fails to push master w/ tip behind counterpart\n ++\t# Manually handle cleanup, since test_config is not\n ++\t# prepared to take arbitrary options like --add\n ++\ttest_when_finished \"test_unconfig -C two remote.one.push\" &&\n ++\n ++\t# For convenience, we use \"master\" to refer to the name of\n ++\t# the branch created by default in the following.\n ++\t#\n ++\t# Repositories two and one have branches other than \"master\"\n ++\t# but they have no overlap---\"master\" is the only one that\n ++\t# is shared between them. And the master branch at two is\n ++\t# behind the master branch at one by one commit.\n ++\tgit -C two config --add remote.one.push : &&\n ++\n ++\t# A matching push tries to update master, fails due to non-ff\n +\ttest_must_fail git -C two push one &&\n +\n ++\t# \"master\" may actually not be \"master\"---find it out.\n ++\tcurrent=$(git symbolic-ref HEAD) &&\n ++\n +\t# If master is in negative refspec, then the command will not attempt\n +\t# to push and succeed.\n -+\t# We do not need test_config here as we are updating remote.one.push\n -+\t# again. The teardown of the first test_config will do --unset-all\n -+\tgit -C two config --add remote.one.push ^refs/heads/master &&\n -+\tgit -C two push one\n ++\tgit -C two config --add remote.one.push \"^$current\" &&\n ++\n ++\t# With \"master\" excluded, this push is a no-op. Nothing gets\n ++\t# pushed and it succeeds.\n ++\tgit -C two push -v one\n +'\n +\n +test_expect_success \"push with matching +: and negative refspec\" '\n -+\ttest_config -C two remote.one.push +: &&\n -+\t# Fails to push master w/ tip behind counterpart\n ++\ttest_when_finished \"test_unconfig -C two remote.one.push\" &&\n ++\n ++\t# The same set-up as above, whose side-effect was a no-op.\n ++\tgit -C two config --add remote.one.push +: &&\n ++\n ++\t# The push refuses to update the \"master\" branch that is checked\n ++\t# out in the \"one\" repository, even when it is forced with +:\n +\ttest_must_fail git -C two push one &&\n +\n ++\t# \"master\" may actually not be \"master\"---find it out.\n ++\tcurrent=$(git symbolic-ref HEAD) &&\n ++\n +\t# If master is in negative refspec, then the command will not attempt\n +\t# to push and succeed\n -+\tgit -C two config --add remote.one.push ^refs/heads/master &&\n -+\tgit -C two push one\n ++\tgit -C two config --add remote.one.push \"^$current\" &&\n ++\n ++\t# With \"master\" excluded, this push is a no-op. Nothing gets\n ++\t# pushed and it succeeds.\n ++\tgit -C two push -v one\n +'\n +\n test_done\n 2: 20575407cc0 = 2: 1f9af0e991c negative-refspec: improve comment on query_matches_negative_refspec\n\n-- \ngitgitgadget\n"},{"id":"412839","messageId":"1f9af0e991c0b84cd00641322f2d76bdc8fbeb29.1608609498.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v5.git.1608609498.gitgitgadget@gmail.com","subject":"[PATCH v5 2/2] negative-refspec: improve comment on query_matches_negative_refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T03:58:17Z","receivedAt":"2020-12-22T03:59:19Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nComment did not adequately explain how the two loops work\ntogether to achieve the goal of querying for matching of any\nnegative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 6 ++++++\n 1 file changed, 6 insertions(+)\n\ndiff --git a/remote.c b/remote.c\nindex 4f1a4099f1a..4d150a316ed 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -736,6 +736,12 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \t * item uses the destination. To handle this, we apply pattern\n \t * refspecs in reverse to figure out if the query source matches any\n \t * of the negative refspecs.\n+\t *\n+\t * The first loop finds and expands all positive refspecs\n+\t * matched by the queried ref.\n+\t *\n+\t * The second loop checks if any of the results of the first loop\n+\t * match any negative refspec.\n \t */\n \tfor (i = 0; i < rs->nr; i++) {\n \t\tstruct refspec_item *refspec = &rs->items[i];\n-- \ngitgitgadget\n"},{"id":"412840","messageId":"48c79dc3d84f55dec4cd2199cc4152e146bee0ba.1608609498.git.gitgitgadget@gmail.com","threadId":"54859","inReplyTo":"pull.820.v5.git.1608609498.gitgitgadget@gmail.com","subject":"[PATCH v5 1/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-22T03:58:16Z","receivedAt":"2020-12-22T03:59:19Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"From: Nipunn Koorapati \n\nThe logic added to check for negative pathspec match by c0192df630\n(refspec: add support for negative refspecs, 2020-09-30) looks at\nrefspec->src assuming it is never NULL, however when\nremote.origin.push is set to \":\", then refspec->src is NULL,\ncausing a segfault within strcmp.\n\nTell git to handle matching refspec by adding the needle to the\nset of positively matched refspecs, since matching \":\" refspecs\nmatch anything as src.\n\nAdd test for matching refspec pushes fetch-negative-refspec\nboth individually and in combination with a negative refspec.\n\nSigned-off-by: Nipunn Koorapati \n---\n remote.c | 10 ++++--\n t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++\n 2 files changed, 58 insertions(+), 3 deletions(-)\n\ndiff --git a/remote.c b/remote.c\nindex 8be67f0892b..4f1a4099f1a 100644\n--- a/remote.c\n+++ b/remote.c\n@@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n \n \t\t\tif (match_name_with_pattern(key, needle, value, &expn_name))\n \t\t\t\tstring_list_append_nodup(&reversed, expn_name);\n-\t\t} else {\n-\t\t\tif (!strcmp(needle, refspec->src))\n-\t\t\t\tstring_list_append(&reversed, refspec->src);\n+\t\t} else if (refspec->matching) {\n+\t\t\t/* For the special matching refspec, any query should match */\n+\t\t\tstring_list_append(&reversed, needle);\n+\t\t} else if (!refspec->src) {\n+\t\t\tBUG(\"refspec->src should not be null here\");\n+\t\t} else if (!strcmp(needle, refspec->src)) {\n+\t\t\tstring_list_append(&reversed, refspec->src);\n \t\t}\n \t}\n \ndiff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\nindex 8c61e28fec8..2f3b064d0e7 100755\n--- a/t/t5582-fetch-negative-refspec.sh\n+++ b/t/t5582-fetch-negative-refspec.sh\n@@ -186,4 +186,55 @@ test_expect_success \"fetch --prune with negative refspec\" '\n \t)\n '\n \n+test_expect_success \"push with matching : and negative refspec\" '\n+\t# Manually handle cleanup, since test_config is not\n+\t# prepared to take arbitrary options like --add\n+\ttest_when_finished \"test_unconfig -C two remote.one.push\" &&\n+\n+\t# For convenience, we use \"master\" to refer to the name of\n+\t# the branch created by default in the following.\n+\t#\n+\t# Repositories two and one have branches other than \"master\"\n+\t# but they have no overlap---\"master\" is the only one that\n+\t# is shared between them. And the master branch at two is\n+\t# behind the master branch at one by one commit.\n+\tgit -C two config --add remote.one.push : &&\n+\n+\t# A matching push tries to update master, fails due to non-ff\n+\ttest_must_fail git -C two push one &&\n+\n+\t# \"master\" may actually not be \"master\"---find it out.\n+\tcurrent=$(git symbolic-ref HEAD) &&\n+\n+\t# If master is in negative refspec, then the command will not attempt\n+\t# to push and succeed.\n+\tgit -C two config --add remote.one.push \"^$current\" &&\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n+'\n+\n+test_expect_success \"push with matching +: and negative refspec\" '\n+\ttest_when_finished \"test_unconfig -C two remote.one.push\" &&\n+\n+\t# The same set-up as above, whose side-effect was a no-op.\n+\tgit -C two config --add remote.one.push +: &&\n+\n+\t# The push refuses to update the \"master\" branch that is checked\n+\t# out in the \"one\" repository, even when it is forced with +:\n+\ttest_must_fail git -C two push one &&\n+\n+\t# \"master\" may actually not be \"master\"---find it out.\n+\tcurrent=$(git symbolic-ref HEAD) &&\n+\n+\t# If master is in negative refspec, then the command will not attempt\n+\t# to push and succeed\n+\tgit -C two config --add remote.one.push \"^$current\" &&\n+\n+\t# With \"master\" excluded, this push is a no-op. Nothing gets\n+\t# pushed and it succeeds.\n+\tgit -C two push -v one\n+'\n+\n test_done\n-- \ngitgitgadget\n\n"},{"id":"412841","messageId":"CAPig+cR1AUH6w8QBaumFwcbz+0LmzYdP61WtaENGHhSgts277A@mail.gmail.com","threadId":"54859","inReplyTo":"CAN8Z4-WpNuqN=HxL4AQU_+zi4hGhkC18d3ZSOJGzbdkMPkYAMQ@mail.gmail.com","subject":"Re: [PATCH v3 1/3] test-lib-functions: handle --add in test_config","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-12-22T05:19:21Z","receivedAt":"2020-12-22T05:21:54Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Dec 21, 2020 at 9:25 PM Nipunn Koorapati wrote:\n> That being said, I suspect anyone in the future poking around with\n> this will have to sourcedive\n> through test_config and test_unconfig to make sense of it.\n>\n> I'll switch it over to the test_unconfig option on the next reroll if requested.\n\nIndeed, it's not entirely uncommon to have to dig into the test\nfunctions when writing tests. My bigger concern was someone coming\nalong and thinking that the mixed use of test_config() and manual `git\nconfig` in the same test was a mistake, and want to fix that mistake,\nwhich would lead the person down the same rabbit hole. Explicit\ncleanup via test_unconfig() and consistent use of `git config` within\nthe test, on the other hand, does not look accidental, so the reader\nwould be less likely to want to \"fix the mistake\". The comment you\nadded in the re-roll above the manual cleanup saves the reader the\ntrouble of having to dive into the implementation of test_config(),\nwhich is a nice bonus.\n"},{"id":"412844","messageId":"xmqq1rfijpyf.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"pull.820.v5.git.1608609498.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-22T06:48:56Z","receivedAt":"2020-12-22T06:49:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Nipunn Koorapati via GitGitGadget\" writes:\n\n> Update since Patch series V4\n>\n> * Squashed in Junio's patch to handle non-master named branches\n> * Explicitly use test_unconfig\n>\n> Appreciate the reviews from Junio and Eric! Happy Holidays!\n>\n> Nipunn Koorapati (2):\n> negative-refspec: fix segfault on : refspec\n> negative-refspec: improve comment on query_matches_negative_refspec\n\nThanks, will replace. Happy holidays to you, too.\n"},{"id":"412964","messageId":"CAN8Z4-VQJsXWmJPNg0Fdu98csK7ZQ0yDNzxPqRhsbuw9CUJjnw@mail.gmail.com","threadId":"54859","inReplyTo":"xmqq1rfijpyf.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2020-12-23T23:56:01Z","receivedAt":"2020-12-23T23:57:02Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"Is this something we want to merge into the 2.29 maint branch?\nIt is a segfault for anyone pushing with matching refspecs on 2.29\nAt what point does git stop patching bugs in the maint branch?\n\n--Nipunn\n\n\n--Nipunn\n\nOn Tue, Dec 22, 2020 at 6:49 AM Junio C Hamano wrote:\n>\n> \"Nipunn Koorapati via GitGitGadget\" writes:\n>\n> > Update since Patch series V4\n> >\n> > * Squashed in Junio's patch to handle non-master named branches\n> > * Explicitly use test_unconfig\n> >\n> > Appreciate the reviews from Junio and Eric! Happy Holidays!\n> >\n> > Nipunn Koorapati (2):\n> > negative-refspec: fix segfault on : refspec\n> > negative-refspec: improve comment on query_matches_negative_refspec\n>\n> Thanks, will replace. Happy holidays to you, too.\n"},{"id":"412966","messageId":"xmqq8s9o5aza.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"CAN8Z4-VQJsXWmJPNg0Fdu98csK7ZQ0yDNzxPqRhsbuw9CUJjnw@mail.gmail.com","subject":"Re: [PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-24T00:00:41Z","receivedAt":"2020-12-24T00:01:41Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nipunn Koorapati writes:\n\n> Is this something we want to merge into the 2.29 maint branch?\n\nEventually by backporting, but a fix typically goes to the current\ndevelopment track first so it would happen after 2.30 is finished, I\nwould think.\n"},{"id":"414075","messageId":"CAN8Z4-UpQzvQguhEqwCJHVZ_0phOXtGouHFNFNwa8jwSpugxSw@mail.gmail.com","threadId":"54859","inReplyTo":"xmqq8s9o5aza.fsf@gitster.c.googlers.com","subject":"Re: [PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Nipunn Koorapati","fromEmail":"nipunn1313@gmail.com","sentAt":"2021-01-11T20:22:51Z","receivedAt":"2021-01-11T20:24:00Z","isPatch":true,"sender":{"key":"nipunn1313@gmail.com","avatar":"https://gravatar.com/avatar/d0b19cc6499ffcae349d237d7166f5fa0fc29942783a61894ccdb5ee5246ac97?d=mp&s=160"},"body":"> Eventually by backporting, but a fix typically goes to the current\n> development track first so it would happen after 2.30 is finished, I\n> would think.\n\nI wanted to bump this idea - now that it appears that 2.30 is complete\nand the new maint branch. Given that this patch makes matching-refspec\nunusable in 2.29, would it make sense to backport a fix to the 2.29\nrelease? If that seems risky/unwanted, is there some practice of\ndocumenting known (serious) bugs in past releases?\n\nThanks\n--Nipunn\n"},{"id":"414086","messageId":"xmqqk0si6hgp.fsf@gitster.c.googlers.com","threadId":"54859","inReplyTo":"CAN8Z4-UpQzvQguhEqwCJHVZ_0phOXtGouHFNFNwa8jwSpugxSw@mail.gmail.com","subject":"Re: [PATCH v5 0/2] negative-refspec: fix segfault on : refspec","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-01-12T02:01:58Z","receivedAt":"2021-01-12T02:02:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Nipunn Koorapati writes:\n\n>> Eventually by backporting, but a fix typically goes to the current\n>> development track first so it would happen after 2.30 is finished, I\n>> would think.\n>\n> I wanted to bump this idea - now that it appears that 2.30 is complete\n> and the new maint branch. Given that this patch makes matching-refspec\n> unusable in 2.29, would it make sense to backport a fix to the 2.29\n> release?\n\nYes, it does make sense.\n\nIf we were to spend engineering effort to cut a 2.29.1, however,\nwe'd better make sure not just this fix but all the other fixes\nrelevant to the 2.29 track that are already well tested are included\nin it. We just issued 2.30 and not many people are using it to\nexercise a relatively new negative pathspec feature yet, so it\nprobably is a good idea to spend a weeks or two to enumerate what\nother things we want to be in the 2.29 maintenance track.\n\nI personally do not have time for doing that myself right now, but\nluckily it is something contributors like you can step in to help\n;-)\n\nThanks.\n\n"},{"id":"417390","messageId":"CA+P7+xrA1kCfJF1B13-yPKFgOKLQhcjQ+zJYpnqJuS5wOXk3wQ@mail.gmail.com","threadId":"54859","inReplyTo":"xmqqy2htoen9.fsf@gitster.c.googlers.com","subject":"Re: [PATCH] negative-refspec: fix segfault on : refspec","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2021-02-19T09:28:11Z","receivedAt":"2021-02-19T09:29:19Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Sat, Dec 19, 2020 at 10:05 AM Junio C Hamano wrote:\n>\n> Original author of the feature (Jacob) cc'ed for insight.\n>\n\nHi,\n\nSorry I missed this thread last couple months.\n\n> - Can we have refspec->src==NULL in cases other than where\n> refspec->matching is true? If not, then perhaps the patch should\n> insert, before the problematic \"else if\" clause, something like\n>\n> if (match_name_with_pattern(...))\n> string_list_append_nodup(...);\n> + } else if (refspec->matching) {\n> + ... behaviour for the matching case ...\n> + } else if (refspec->src == NULL) {\n> + BUG(\"refspec->src cannot be null here\");\n> } else {\n> if (!strcmp(needle, refspec->src))\n>\n> - We'd need to decide if ignoring is the right behaviour for the\n> matching refspec. I do not recall what we decided the logic of\n> the function should be offhand.\n>\n\nIsn't this patch about how we somehow broke \":\" on its own, not as a\nnegative refspec?\n"},{"id":"417391","messageId":"CA+P7+xr7nZBdTgknjM_H34=q5qr3sfT=dJJkasm87h8=qkd9PQ@mail.gmail.com","threadId":"54859","inReplyTo":"48c79dc3d84f55dec4cd2199cc4152e146bee0ba.1608609498.git.gitgitgadget@gmail.com","subject":"Re: [PATCH v5 1/2] negative-refspec: fix segfault on : refspec","fromName":"Jacob Keller","fromEmail":"jacob.keller@gmail.com","sentAt":"2021-02-19T09:32:13Z","receivedAt":"2021-02-19T09:33:23Z","isPatch":true,"sender":{"key":"jacob.keller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/874719?v=4"},"body":"On Mon, Dec 21, 2020 at 8:01 PM Nipunn Koorapati via GitGitGadget\n wrote:\n>\n> From: Nipunn Koorapati \n>\n> The logic added to check for negative pathspec match by c0192df630\n> (refspec: add support for negative refspecs, 2020-09-30) looks at\n> refspec->src assuming it is never NULL, however when\n> remote.origin.push is set to \":\", then refspec->src is NULL,\n> causing a segfault within strcmp.\n>\n> Tell git to handle matching refspec by adding the needle to the\n> set of positively matched refspecs, since matching \":\" refspecs\n> match anything as src.\n>\n\nThis seems like the right approach to me. Thanks for the fix, and the\ntests so we don't break it on accident again in the future.\n\nbelated, but....\n\nReviewed-by: Jacob Keller \n\n> Add test for matching refspec pushes fetch-negative-refspec\n> both individually and in combination with a negative refspec.\n>\n> Signed-off-by: Nipunn Koorapati \n> ---\n> remote.c | 10 ++++--\n> t/t5582-fetch-negative-refspec.sh | 51 +++++++++++++++++++++++++++++++\n> 2 files changed, 58 insertions(+), 3 deletions(-)\n>\n> diff --git a/remote.c b/remote.c\n> index 8be67f0892b..4f1a4099f1a 100644\n> --- a/remote.c\n> +++ b/remote.c\n> @@ -751,9 +751,13 @@ static int query_matches_negative_refspec(struct refspec *rs, struct refspec_ite\n>\n> if (match_name_with_pattern(key, needle, value, &expn_name))\n> string_list_append_nodup(&reversed, expn_name);\n> - } else {\n> - if (!strcmp(needle, refspec->src))\n> - string_list_append(&reversed, refspec->src);\n> + } else if (refspec->matching) {\n> + /* For the special matching refspec, any query should match */\n> + string_list_append(&reversed, needle);\n\nRight, so we explicitly handle matching first...\n\n> + } else if (!refspec->src) {\n> + BUG(\"refspec->src should not be null here\");\n\nand then carefully check to make sure we don't end up with a NULL src\nfor some other reason, and at least BUG() instead of just crashing.\n\nThis shouldn't be possible because when we build the refspec, src is\nalways not NULL unless in the case of matching. Ok.\n\n> + } else if (!strcmp(needle, refspec->src)) {\n> + string_list_append(&reversed, refspec->src);\n> }\n> }\n\nYep, this looks like the best approach to solving this.\n\n>\n> diff --git a/t/t5582-fetch-negative-refspec.sh b/t/t5582-fetch-negative-refspec.sh\n> index 8c61e28fec8..2f3b064d0e7 100755\n> --- a/t/t5582-fetch-negative-refspec.sh\n> +++ b/t/t5582-fetch-negative-refspec.sh\n> @@ -186,4 +186,55 @@ test_expect_success \"fetch --prune with negative refspec\" '\n> )\n> '\n>\n> +test_expect_success \"push with matching : and negative refspec\" '\n> + # Manually handle cleanup, since test_config is not\n> + # prepared to take arbitrary options like --add\n> + test_when_finished \"test_unconfig -C two remote.one.push\" &&\n> +\n> + # For convenience, we use \"master\" to refer to the name of\n> + # the branch created by default in the following.\n> + #\n> + # Repositories two and one have branches other than \"master\"\n> + # but they have no overlap---\"master\" is the only one that\n> + # is shared between them. And the master branch at two is\n> + # behind the master branch at one by one commit.\n> + git -C two config --add remote.one.push : &&\n> +\n> + # A matching push tries to update master, fails due to non-ff\n> + test_must_fail git -C two push one &&\n> +\n> + # \"master\" may actually not be \"master\"---find it out.\n> + current=$(git symbolic-ref HEAD) &&\n> +\n> + # If master is in negative refspec, then the command will not attempt\n> + # to push and succeed.\n> + git -C two config --add remote.one.push \"^$current\" &&\n> +\n> + # With \"master\" excluded, this push is a no-op. Nothing gets\n> + # pushed and it succeeds.\n> + git -C two push -v one\n> +'\n> +\n> +test_expect_success \"push with matching +: and negative refspec\" '\n> + test_when_finished \"test_unconfig -C two remote.one.push\" &&\n> +\n> + # The same set-up as above, whose side-effect was a no-op.\n> + git -C two config --add remote.one.push +: &&\n> +\n> + # The push refuses to update the \"master\" branch that is checked\n> + # out in the \"one\" repository, even when it is forced with +:\n> + test_must_fail git -C two push one &&\n> +\n> + # \"master\" may actually not be \"master\"---find it out.\n> + current=$(git symbolic-ref HEAD) &&\n> +\n> + # If master is in negative refspec, then the command will not attempt\n> + # to push and succeed\n> + git -C two config --add remote.one.push \"^$current\" &&\n> +\n> + # With \"master\" excluded, this push is a no-op. Nothing gets\n> + # pushed and it succeeds.\n> + git -C two push -v one\n> +'\n> +\n> test_done\n> --\n> gitgitgadget\n>\n"}]}