{"thread":{"id":"56051","subject":"git pull --set-upstream segfaults on branchless repo.","startedAt":"2021-07-05T15:47:12Z","lastAt":"2021-09-01T17:44:50Z","messageCount":13,"participants":["Clemens Fruhwirth","Jan Pokorný","Ævar Arnfjörð Bjarmason","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"429247","messageId":"CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com","threadId":"56051","inReplyTo":null,"subject":"git pull --set-upstream segfaults on branchless repo.","fromName":"Clemens Fruhwirth","fromEmail":"clemens@endorphin.org","sentAt":"2021-07-05T15:46:58Z","receivedAt":"2021-07-05T15:47:12Z","isPatch":false,"sender":{"key":"clemens@endorphin.org","avatar":null},"body":"Thank you for filling out a Git bug report!\nPlease answer the following questions to help us understand your issue.\n\nWhat did you do before the bug happened? (Steps to reproduce your issue)\n\nRun \"git pull origin nixos-unstable --rebase --set-upstream\"\non a repo that had no branch set, e.g. when running \"git branch\" gave\n\"* (no branch)\"\n\nWhat did you expect to happen? (Expected behavior)\nPull from upstream and set upstream.\n\nWhat happened instead? (Actual behavior)\nSegfault with the following trace:\n(gdb) bt\n#0  0x000000000044a8c9 in do_fetch (transport=0x2557920,\nrs=rs@entry=0x7ffd42335c00) at builtin/fetch.c:1568\n#1  0x000000000044ac61 in fetch_one (remote=<optimized out>,\nargc=1110662144, argv=0x7ffd42335fc8,\n    prune_tags_ok=<optimized out>, use_stdin_refspecs=0) at builtin/fetch.c:1892\n#2  0x000000000044af15 in cmd_fetch (argc=0, argv=0x7ffd42335fc8,\nprefix=0x0) at builtin/fetch.c:1992\n#3  0x0000000000406354 in run_builtin (p=0x70d3e0 <commands+960>,\nargc=argc@entry=5, argv=argv@entry=0x7ffd42335fc0)\n    at git.c:453\n#4  0x00000000004065c3 in handle_builtin (argc=5, argv=0x7ffd42335fc0)\nat git.c:704\n#5  0x0000000000407c4a in run_argv (argcp=argcp@entry=0x7ffd42335e8c,\nargv=argv@entry=0x7ffd42335e80) at git.c:771\n#6  0x00000000004080a4 in cmd_main (argc=<optimized out>,\nargc@entry=6, argv=<optimized out>,\n    argv@entry=0x7ffd42335fb8) at git.c:902\n#7  0x00000000004c614c in main (argc=6, argv=0x7ffd42335fb8) at common-main.c:52\n(gdb) p branch\n$1 = (struct branch *) 0x0\n\nWhat's different between what you expected and what actually happened?\nNot segfault\n\nAnything else you want to add:\n\nDropping the \"--set-upstream\" makes the segfault go away. Looking at\nbuiltin/fetch.c around L1568 I see,\n\n        if (set_upstream) {\n                struct branch *branch = branch_get(\"HEAD\");\n                [..]\n                if (source_ref) {\n                        if (!strcmp(source_ref->name, \"HEAD\") ||\n                            starts_with(source_ref->name, \"refs/heads/\"))\n                                install_branch_config(0,\n                                                      branch->name, //\n<- SEGFAULT HERE\n                                                      transport->remote->name,\n                                                      source_ref->name);\n                        [..]\n              }\n        }\n\nIt's rather clear that branch is just NULL from the gdb session above,\nand the branch->name dereference fails.\nIt might be useful to catch branch == NULL.\n\nPlease review the rest of the bug report below.\nYou can delete any lines you don't wish to share.\n\n[System Info]\ngit version:\ngit version 2.31.1\ncpu: x86_64\nno commit associated with this build\nsizeof-long: 8\nsizeof-size_t: 8\nshell-path: /nix/store/kxj6cblcsd1qcbbxlmbswwrn89zcmgd6-bash-4.4-p23/bin/bash\nuname: Linux 5.12.12 #1-NixOS SMP Fri Jun 18 08:02:52 UTC 2021 x86_64\ncompiler info: gnuc: 10.3\nlibc info: glibc: 2.32\n$SHELL (typically, interactive shell): /var/run/current-system/sw/bin/zsh\n\n\n[Enabled Hooks]\n\n(Please cc me on replies, not subscribe to the mailing list).\n-- \nFruhwirth Clemens http://clemens.endorphin.org\n"},{"id":"430485","messageId":"8636b96be256b47d207e543995abbecde9ca5055.camel@fnusa.cz","threadId":"56051","inReplyTo":"CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com","subject":"Re: git pull --set-upstream segfaults on branchless repo.","fromName":"Jan Pokorný","fromEmail":"poki@fnusa.cz","sentAt":"2021-07-19T10:04:55Z","receivedAt":"2021-07-19T10:12:06Z","isPatch":false,"sender":{"key":"poki@fnusa.cz","avatar":null},"body":"Hello,\n\nClemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:\n> What did you do before the bug happened? (Steps to reproduce your\n> issue)\n> \n> Run \"git pull origin nixos-unstable --rebase --set-upstream\"\n> on a repo that had no branch set, e.g. when running \"git branch\" gave\n> \"* (no branch)\"\n> \n> What did you expect to happen? (Expected behavior)\n> Pull from upstream and set upstream.\n> \n> What happened instead? (Actual behavior)\n> Segfault with the following trace:\n> (gdb) bt\n> #0  0x000000000044a8c9 in do_fetch (transport=0x2557920, rs=rs@entry=0x7ffd42335c00) at builtin/fetch.c:1568\n> #1  0x000000000044ac61 in fetch_one (remote=<optimized out>, argc=1110662144, argv=0x7ffd42335fc8, prune_tags_ok=<optimized out>, use_stdin_refspecs=0) at builtin/fetch.c:1892\n> #2  0x000000000044af15 in cmd_fetch (argc=0, argv=0x7ffd42335fc8, prefix=0x0) at builtin/fetch.c:1992\n> #3  0x0000000000406354 in run_builtin (p=0x70d3e0 <commands+960>, argc=argc@entry=5, argv=argv@entry=0x7ffd42335fc0) at git.c:453\n> #4  0x00000000004065c3 in handle_builtin (argc=5, argv=0x7ffd42335fc0) at git.c:704\n> #5  0x0000000000407c4a in run_argv (argcp=argcp@entry=0x7ffd42335e8c, argv=argv@entry=0x7ffd42335e80) at git.c:771\n> #6  0x00000000004080a4 in cmd_main (argc=<optimized out>, argc@entry=6, argv=<optimized out>, argv@entry=0x7ffd42335fb8) at git.c:902\n> #7  0x00000000004c614c in main (argc=6, argv=0x7ffd42335fb8) at common-main.c:52\n> (gdb) p branch\n> $1 = (struct branch *) 0x0\n> \n> What's different between what you expected and what actually happened?\n> Not segfault\n> \n> Anything else you want to add:\n> \n> Dropping the \"--set-upstream\" makes the segfault go away. Looking at\n> builtin/fetch.c around L1568 I see,\n> \n>         if (set_upstream) {\n>                 struct branch *branch = branch_get(\"HEAD\");\n>                 [..]\n>                 if (source_ref) {\n>                         if (!strcmp(source_ref->name, \"HEAD\") ||\n>                             starts_with(source_ref->name, \"refs/heads/\"))\n>                                 install_branch_config(0,\n>                                                       branch->name, // <- SEGFAULT HERE\n>                                                       transport->remote->name,\n>                                                       source_ref->name);\n>                         [..]\n>               }\n>         }\n> \n> It's rather clear that branch is just NULL from the gdb session above,\n> and the branch->name dereference fails.\n> It might be useful to catch branch == NULL.\n> \n> Please review the rest of the bug report below.\n> You can delete any lines you don't wish to share.\n> \n> [System Info]\n> git version:\n> git version 2.31.1\n> [...]\n\nobserved something pretty similar on Fedora, with both\ngit-2.32.0-0.3.rc2.fc35.x86_64 and git-2.32.0-1.fc35.x86_64, e.g.:\n\n$ git clone https://github.com/git/git.git\n[...]\n$ cd git\n$ git checkout --detach v2.1.0\nHEAD is now at 6c4ab27f23 Git 2.1\n$ git pull --set-upstream origin master\nFrom https://github.com/git/git\n * branch                  master     -> FETCH_HEAD\nerror: fetch died of signal 11\n\n  1625\t\t\tif (source_ref) {\n  1626\t\t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n  1627\t\t\t\t   starts_with(source_ref->name, \"refs/heads/\"))\n=>1628\t\t\t\t\tinstall_branch_config(0,\n  1629\t\t\t\t\t\t\t     branch->name,\n  1630\t\t\t\t\t\t\t     transport->remote->name,\n  1631\t\t\t\t\t\t\t     source_ref->name);\n\n(gdb) p branch\n$1 = <optimized out>\n(gdb) p branch->name\nvalue has been optimized out\n\n   0x000055b5bceba921 <+8705>:\tnopl   0x0(%rax)\n   0x000055b5bceba928 <+8712>:\tmov    (%rsp),%rax\n=> 0x000055b5bceba92c <+8716>:\tmov    (%r8),%rsi\n   0x000055b5bceba92f <+8719>:\txor    %edi,%edi\n   0x000055b5bceba931 <+8721>:\tmov    0x8(%rax),%rax\n   0x000055b5bceba935 <+8725>:\tmov    0x10(%rax),%rdx\n   0x000055b5bceba939 <+8729>:\tcall   0x55b5bcf75d40 <install_branch_config>\n\n(gdb) info registers r8 rsi\nr8             0x0                 0\nrsi            0x55b5bdbe430b      94239060804363\n\nThis is just a generic reproducer; original use case revolved around\nthe use of submodules (also happened to be in a detached state within,\n\"--set-upstream\" attempt was then my rushed response to \"You are\nnot currently on a branch.\" error message on a plain pull there).\n\n> (Please cc me on replies, not subscribe to the mailing list).\n\n(ditto, please)\n\n-- poki\n"},{"id":"430506","messageId":"patch-1.1-2d8f3e59e1f-20210719T142808Z-avarab@gmail.com","threadId":"56051","inReplyTo":"8636b96be256b47d207e543995abbecde9ca5055.camel@fnusa.cz","subject":"[PATCH] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-07-19T14:30:51Z","receivedAt":"2021-07-19T14:30:57Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --set-upstream option added in\n24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added\nin v2.24.0.\n\nThe code added there did not do the same checking we do for \"git\nbranch\" itself since 8efb8899cfe (branch: segfault fixes and\nvalidation, 2013-02-23), which in turn fixed the same sort of segfault\nI'm fixing now in \"git branch --set-upstream-to\", see\n6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n\nThe error message I'm adding here is an amalgamation of the error\nadded for \"git branch\" in 8efb8899cfe, and the error output\ninstall_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\nthe name and says \"branch X on remote\", not \"branch refs/heads/X on\nremote\".\n\nReported-by: Clemens Fruhwirth <clemens@endorphin.org>\nReported-by: Jan Pokorný <poki@fnusa.cz>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nOn Mon, Jul 19 2021, Jan Pokorný wrote:\n\n> Hello,\n>\n> Clemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:\n>> What did you do before the bug happened? (Steps to reproduce your\n>> issue)\n>> \n>> Run \"git pull origin nixos-unstable --rebase --set-upstream\"\n>> on a repo that had no branch set, e.g. when running \"git branch\" gave\n>> \"* (no branch)\"\n>> \n\nThanks for the report both & sorry that this fell through the cracks\nfor so long.\n\nThe bug itself seems rather obvious given the benefit of that\nreproduction scenario & backtrace, as noted above we've been playing\nwhack-a-mole with a related segfault since 2012.\n\nIt would be nice if you could test this patch and confirm, but given\nthe trivality of the segfault it shouldn't be necessary, I'm confident\nthat this fixes the bug you two reported. Thanks both!\n\n builtin/fetch.c         | 11 +++++++++++\n t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 9191620e50c..c2eac8f15e5 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,\n \t\t\t}\n \t\t}\n \t\tif (source_ref) {\n+\t\t\tif (!branch) {\n+\t\t\t\tconst char *shortname = NULL;\n+\t\t\t\tif (!skip_prefix(source_ref->name,\n+\t\t\t\t\t\t \"refs/heads/\", &shortname))\n+\t\t\t\t\tshortname = source_ref->name;\n+\t\t\t\t    \n+\t\t\t\tdie(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n+\t\t\t\t      \"it does not point to any branch.\"),\n+\t\t\t\t    shortname, transport->remote->name);\n+\t\t\t}\n+\n \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n \t\t\t    starts_with(source_ref->name, \"refs/heads/\"))\n \t\t\t\tinstall_branch_config(0,\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex b1d614ce18c..ae90554645e 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'fetch --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\ttest_must_fail git fetch --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^fatal: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # tests for pull --set-upstream\n \n test_expect_success 'setup bare parent pull' '\n@@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'pull --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\ttest_must_fail git pull --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^fatal: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.32.0.874.ge7a9d58bfcf\n\n"},{"id":"430507","messageId":"xmqqsg0anxix.fsf@gitster.g","threadId":"56051","inReplyTo":"patch-1.1-2d8f3e59e1f-20210719T142808Z-avarab@gmail.com","subject":"Re: [PATCH] pull, fetch: fix segfault in --set-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-07-19T15:17:26Z","receivedAt":"2021-07-19T15:19:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> Fix a segfault in the --set-upstream option added in\n> 24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added\n> in v2.24.0.\n>\n> The code added there did not do the same checking we do for \"git\n> branch\" itself since 8efb8899cfe (branch: segfault fixes and\n> validation, 2013-02-23), which in turn fixed the same sort of segfault\n> I'm fixing now in \"git branch --set-upstream-to\", see\n> 6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n>\n> The error message I'm adding here is an amalgamation of the error\n> added for \"git branch\" in 8efb8899cfe, and the error output\n> install_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\n> the name and says \"branch X on remote\", not \"branch refs/heads/X on\n> remote\".\n>\n> Reported-by: Clemens Fruhwirth <clemens@endorphin.org>\n> Reported-by: Jan Pokorný <poki@fnusa.cz>\n> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n\nHmph, there is 5e0fa8cb (fetch: fix segfault on --set-upstream while\non a detached HEAD, 2021-07-06) from Clemens, which warns and skips\ninstead of dies.  Given that a handful of other conditions in this\nfunction results in warnings \"not setting upstream due to X\", it\nmight be more in line with the spirit of the current system (it is\ndebatable if we should stress the fact that we did not set the\nconfiguration with hard dying, though).\n\n\n> ---\n>\n> On Mon, Jul 19 2021, Jan Pokorný wrote:\n>\n>> Hello,\n>>\n>> Clemens Fruhwirth píše v Po 05. 07. 2021 v 17:46 +0200:\n>>> What did you do before the bug happened? (Steps to reproduce your\n>>> issue)\n>>> \n>>> Run \"git pull origin nixos-unstable --rebase --set-upstream\"\n>>> on a repo that had no branch set, e.g. when running \"git branch\" gave\n>>> \"* (no branch)\"\n>>> \n>\n> Thanks for the report both & sorry that this fell through the cracks\n> for so long.\n>\n> The bug itself seems rather obvious given the benefit of that\n> reproduction scenario & backtrace, as noted above we've been playing\n> whack-a-mole with a related segfault since 2012.\n>\n> It would be nice if you could test this patch and confirm, but given\n> the trivality of the segfault it shouldn't be necessary, I'm confident\n> that this fixes the bug you two reported. Thanks both!\n>\n>  builtin/fetch.c         | 11 +++++++++++\n>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n>  2 files changed, 33 insertions(+)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 9191620e50c..c2eac8f15e5 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,\n>  \t\t\t}\n>  \t\t}\n>  \t\tif (source_ref) {\n> +\t\t\tif (!branch) {\n> +\t\t\t\tconst char *shortname = NULL;\n> +\t\t\t\tif (!skip_prefix(source_ref->name,\n> +\t\t\t\t\t\t \"refs/heads/\", &shortname))\n> +\t\t\t\t\tshortname = source_ref->name;\n> +\t\t\t\t    \n> +\t\t\t\tdie(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n> +\t\t\t\t      \"it does not point to any branch.\"),\n> +\t\t\t\t    shortname, transport->remote->name);\n> +\t\t\t}\n> +\n>  \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n>  \t\t\t    starts_with(source_ref->name, \"refs/heads/\"))\n>  \t\t\t\tinstall_branch_config(0,\n> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\n> index b1d614ce18c..ae90554645e 100755\n> --- a/t/t5553-set-upstream.sh\n> +++ b/t/t5553-set-upstream.sh\n> @@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n>  \tcheck_config_missing other2\n>  '\n>  \n> +test_expect_success 'fetch --set-upstream with a detached HEAD' '\n> +\tgit checkout HEAD^0 &&\n> +\ttest_when_finished \"git checkout -\" &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n> +\tEOF\n> +\ttest_must_fail git fetch --set-upstream upstream main 2>actual.raw &&\n> +\tgrep ^fatal: actual.raw >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  # tests for pull --set-upstream\n>  \n>  test_expect_success 'setup bare parent pull' '\n> @@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'\n>  \tcheck_config_missing other2\n>  '\n>  \n> +test_expect_success 'pull --set-upstream with a detached HEAD' '\n> +\tgit checkout HEAD^0 &&\n> +\ttest_when_finished \"git checkout -\" &&\n> +\tcat >expect <<-\\EOF &&\n> +\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n> +\tEOF\n> +\ttest_must_fail git pull --set-upstream upstream main 2>actual.raw &&\n> +\tgrep ^fatal: actual.raw >actual &&\n> +\ttest_cmp expect actual\n> +'\n> +\n>  test_done\n"},{"id":"433434","messageId":"patch-v2-1.1-9e846b76959-20210823T125434Z-avarab@gmail.com","threadId":"56051","inReplyTo":"patch-1.1-2d8f3e59e1f-20210719T142808Z-avarab@gmail.com","subject":"[PATCH v2] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-23T12:56:32Z","receivedAt":"2021-08-23T12:56:42Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --set-upstream option added in\n24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added\nin v2.24.0.\n\nThe code added there did not do the same checking we do for \"git\nbranch\" itself since 8efb8899cfe (branch: segfault fixes and\nvalidation, 2013-02-23), which in turn fixed the same sort of segfault\nI'm fixing now in \"git branch --set-upstream-to\", see\n6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n\nThe warning message I'm adding here is an amalgamation of the error\nadded for \"git branch\" in 8efb8899cfe, and the error output\ninstall_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\nthe name and says \"branch X on remote\", not \"branch refs/heads/X on\nremote\".\n\nI think it would make more sense to simply die() here, but in the\nother checks for --set-upstream added in 24bc1a12926 we issue a\nwarning() instead. Let's do the same here for consistency for now.\n\nThere was an earlier submitted alternate way of fixing this in [1],\ndue to that patch breaking threading with the original report at [2] I\ndidn't notice it before authoring this version. I think the more\ndetailed warning message here is better, and we should also have tests\nfor this behavior.\n\n1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/\n2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/\n\nReported-by: Clemens Fruhwirth <clemens@endorphin.org>\nReported-by: Jan Pokorný <poki@fnusa.cz>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nA proposed replacement of cf/fetch-set-upstream-while-detached which\nas noted in What's Cooking has been languishing for a while. I changed\nthe die() in my version to a warning() as suggested by Junio & updated\nthe test and commit message accordingly.\n\nRange-diff against v1:\n1:  2d8f3e59e1f ! 1:  9e846b76959 pull, fetch: fix segfault in --set-upstream option\n    @@ Commit message\n         I'm fixing now in \"git branch --set-upstream-to\", see\n         6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n     \n    -    The error message I'm adding here is an amalgamation of the error\n    +    The warning message I'm adding here is an amalgamation of the error\n         added for \"git branch\" in 8efb8899cfe, and the error output\n         install_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\n         the name and says \"branch X on remote\", not \"branch refs/heads/X on\n         remote\".\n     \n    +    I think it would make more sense to simply die() here, but in the\n    +    other checks for --set-upstream added in 24bc1a12926 we issue a\n    +    warning() instead. Let's do the same here for consistency for now.\n    +\n    +    There was an earlier submitted alternate way of fixing this in [1],\n    +    due to that patch breaking threading with the original report at [2] I\n    +    didn't notice it before authoring this version. I think the more\n    +    detailed warning message here is better, and we should also have tests\n    +    for this behavior.\n    +\n    +    1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/\n    +    2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/\n    +\n         Reported-by: Clemens Fruhwirth <clemens@endorphin.org>\n         Reported-by: Jan Pokorný <poki@fnusa.cz>\n         Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n     +\t\t\t\tif (!skip_prefix(source_ref->name,\n     +\t\t\t\t\t\t \"refs/heads/\", &shortname))\n     +\t\t\t\t\tshortname = source_ref->name;\n    -+\t\t\t\t    \n    -+\t\t\t\tdie(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n    -+\t\t\t\t      \"it does not point to any branch.\"),\n    -+\t\t\t\t    shortname, transport->remote->name);\n    ++\t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n    ++\t\t\t\t\t  \"it does not point to any branch.\"),\n    ++\t\t\t\t\tshortname, transport->remote->name);\n    ++\t\t\t\tgoto skip;\n     +\t\t\t}\n     +\n      \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n    @@ t/t5553-set-upstream.sh: test_expect_success 'fetch --set-upstream with valid UR\n     +\tgit checkout HEAD^0 &&\n     +\ttest_when_finished \"git checkout -\" &&\n     +\tcat >expect <<-\\EOF &&\n    -+\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n    ++\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n     +\tEOF\n    -+\ttest_must_fail git fetch --set-upstream upstream main 2>actual.raw &&\n    -+\tgrep ^fatal: actual.raw >actual &&\n    ++\tgit fetch --set-upstream upstream main 2>actual.raw &&\n    ++\tgrep ^warning: actual.raw >actual &&\n     +\ttest_cmp expect actual\n     +'\n     +\n    @@ t/t5553-set-upstream.sh: test_expect_success 'pull --set-upstream with valid URL\n     +\tgit checkout HEAD^0 &&\n     +\ttest_when_finished \"git checkout -\" &&\n     +\tcat >expect <<-\\EOF &&\n    -+\tfatal: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n    ++\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n     +\tEOF\n    -+\ttest_must_fail git pull --set-upstream upstream main 2>actual.raw &&\n    -+\tgrep ^fatal: actual.raw >actual &&\n    ++\tgit pull --set-upstream upstream main 2>actual.raw &&\n    ++\tgrep ^warning: actual.raw >actual &&\n     +\ttest_cmp expect actual\n     +'\n     +\n\n builtin/fetch.c         | 11 +++++++++++\n t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n 2 files changed, 33 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex 25740c13df1..ca487edd805 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,\n \t\t\t}\n \t\t}\n \t\tif (source_ref) {\n+\t\t\tif (!branch) {\n+\t\t\t\tconst char *shortname = NULL;\n+\t\t\t\tif (!skip_prefix(source_ref->name,\n+\t\t\t\t\t\t \"refs/heads/\", &shortname))\n+\t\t\t\t\tshortname = source_ref->name;\n+\t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n+\t\t\t\t\t  \"it does not point to any branch.\"),\n+\t\t\t\t\tshortname, transport->remote->name);\n+\t\t\t\tgoto skip;\n+\t\t\t}\n+\n \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n \t\t\t    starts_with(source_ref->name, \"refs/heads/\"))\n \t\t\t\tinstall_branch_config(0,\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex b1d614ce18c..7d12ceff702 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'fetch --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit fetch --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # tests for pull --set-upstream\n \n test_expect_success 'setup bare parent pull' '\n@@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'pull --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit pull --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.662.gbc81f8cbdca\n\n"},{"id":"433509","messageId":"CAG6gW_s6FHG6muJ6Hai-Sb2ntmygLymQZr8YrN212_SV1VhBAw@mail.gmail.com","threadId":"56051","inReplyTo":"patch-v2-1.1-9e846b76959-20210823T125434Z-avarab@gmail.com","subject":"Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option","fromName":"Clemens Fruhwirth","fromEmail":"clemens@endorphin.org","sentAt":"2021-08-24T07:30:59Z","receivedAt":"2021-08-24T07:31:13Z","isPatch":true,"sender":{"key":"clemens@endorphin.org","avatar":null},"body":"On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>\n> A proposed replacement of cf/fetch-set-upstream-while-detached which\n> as noted in What's Cooking has been languishing for a while. I changed\n> the die() in my version to a warning() as suggested by Junio & updated\n> the test and commit message accordingly.\n\nThank you Ævar, for taking care of that and adding tests and my\napologizes for not responding in a timely manner.\n\n>  builtin/fetch.c         | 11 +++++++++++\n>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n>  2 files changed, 33 insertions(+)\n>\n> diff --git a/builtin/fetch.c b/builtin/fetch.c\n> index 25740c13df1..ca487edd805 100644\n> --- a/builtin/fetch.c\n> +++ b/builtin/fetch.c\n> @@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,\n>                         }\n>                 }\n>                 if (source_ref) {\n> +                       if (!branch) {\n> +                               const char *shortname = NULL;\n> +                               if (!skip_prefix(source_ref->name,\n> +                                                \"refs/heads/\", &shortname))\n> +                                       shortname = source_ref->name;\n\nIs skip_prefix ever able to fail?\n\n- If yes, then shortname will be left pointing to NULL when we print\nthe warning below. Warning probably won't die because of NULL, but it\nstill will print something weird.\n- If no, then an assert(!skip_prefix(source_ref->name, \"refs/heads/\",\n&shortname)) might be more idiomatic.\n\n> diff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\n> index b1d614ce18c..7d12ceff702 100755\n> --- a/t/t5553-set-upstream.sh\n> +++ b/t/t5553-set-upstream.sh\n\nThank for you adding tests. LGTM.\n\nRegards,\n-- \nFruhwirth Clemens http://clemens.endorphin.org\n"},{"id":"433513","messageId":"87bl5nfcof.fsf@evledraar.gmail.com","threadId":"56051","inReplyTo":"CAG6gW_s6FHG6muJ6Hai-Sb2ntmygLymQZr8YrN212_SV1VhBAw@mail.gmail.com","subject":"Re: [PATCH v2] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-24T08:49:26Z","receivedAt":"2021-08-24T08:51:40Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 24 2021, Clemens Fruhwirth wrote:\n\n> On Mon, 23 Aug 2021 at 14:56, Ævar Arnfjörð Bjarmason <avarab@gmail.com> wrote:\n>>\n>> A proposed replacement of cf/fetch-set-upstream-while-detached which\n>> as noted in What's Cooking has been languishing for a while. I changed\n>> the die() in my version to a warning() as suggested by Junio & updated\n>> the test and commit message accordingly.\n>\n> Thank you Ævar, for taking care of that and adding tests and my\n> apologizes for not responding in a timely manner.\n\nNo worries.\n\n>>  builtin/fetch.c         | 11 +++++++++++\n>>  t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n>>  2 files changed, 33 insertions(+)\n>>\n>> diff --git a/builtin/fetch.c b/builtin/fetch.c\n>> index 25740c13df1..ca487edd805 100644\n>> --- a/builtin/fetch.c\n>> +++ b/builtin/fetch.c\n>> @@ -1623,6 +1623,17 @@ static int do_fetch(struct transport *transport,\n>>                         }\n>>                 }\n>>                 if (source_ref) {\n>> +                       if (!branch) {\n>> +                               const char *shortname = NULL;\n>> +                               if (!skip_prefix(source_ref->name,\n>> +                                                \"refs/heads/\", &shortname))\n>> +                                       shortname = source_ref->name;\n>\n> Is skip_prefix ever able to fail?\n>\n> - If yes, then shortname will be left pointing to NULL when we print\n> the warning below. Warning probably won't die because of NULL, but it\n> still will print something weird.\n\nIf it fails we'll use source_ref->name, so it won't be NULL...\n\n> - If no, then an assert(!skip_prefix(source_ref->name, \"refs/heads/\",\n> &shortname)) might be more idiomatic.\n\nWhich means that this would be a bug, since it's not handling the case\nwhere the source doesn't start with refs/heads/*.\n\nI.e. it's the same as doing it this way (on top of this v2), which\nperhaps is a clearer way to express the same idea. What do you think?\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex ca487edd805..2bc9159690d 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1624,10 +1624,9 @@ static int do_fetch(struct transport *transport,\n \t\t}\n \t\tif (source_ref) {\n \t\t\tif (!branch) {\n-\t\t\t\tconst char *shortname = NULL;\n-\t\t\t\tif (!skip_prefix(source_ref->name,\n-\t\t\t\t\t\t \"refs/heads/\", &shortname))\n-\t\t\t\t\tshortname = source_ref->name;\n+\t\t\t\tconst char *shortname = source_ref->name;\n+\t\t\t\tskip_prefix(shortname, \"refs/heads/\", &shortname);\n+\n \t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n \t\t\t\t\t  \"it does not point to any branch.\"),\n \t\t\t\t\tshortname, transport->remote->name);\n"},{"id":"434084","messageId":"patch-v3-1.1-68899471206-20210830T144020Z-avarab@gmail.com","threadId":"56051","inReplyTo":"patch-v2-1.1-9e846b76959-20210823T125434Z-avarab@gmail.com","subject":"[PATCH v3] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-30T14:41:18Z","receivedAt":"2021-08-30T14:41:25Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --set-upstream option added in\n24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added\nin v2.24.0.\n\nThe code added there did not do the same checking we do for \"git\nbranch\" itself since 8efb8899cfe (branch: segfault fixes and\nvalidation, 2013-02-23), which in turn fixed the same sort of segfault\nI'm fixing now in \"git branch --set-upstream-to\", see\n6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n\nThe warning message I'm adding here is an amalgamation of the error\nadded for \"git branch\" in 8efb8899cfe, and the error output\ninstall_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\nthe name and says \"branch X on remote\", not \"branch refs/heads/X on\nremote\".\n\nI think it would make more sense to simply die() here, but in the\nother checks for --set-upstream added in 24bc1a12926 we issue a\nwarning() instead. Let's do the same here for consistency for now.\n\nThere was an earlier submitted alternate way of fixing this in [1],\ndue to that patch breaking threading with the original report at [2] I\ndidn't notice it before authoring this version. I think the more\ndetailed warning message here is better, and we should also have tests\nfor this behavior.\n\n1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/\n2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/\n\nReported-by: Clemens Fruhwirth <clemens@endorphin.org>\nReported-by: Jan Pokorný <poki@fnusa.cz>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nA v3 that functionally behaves the same way, but uses a more idiomatic\nway of calling skip_prefix() to strip the \"refs/heads/*\" prefix, if\npresent.\n\nRange-diff against v2:\n1:  9e846b76959 ! 1:  68899471206 pull, fetch: fix segfault in --set-upstream option\n    @@ builtin/fetch.c: static int do_fetch(struct transport *transport,\n      \t\t}\n      \t\tif (source_ref) {\n     +\t\t\tif (!branch) {\n    -+\t\t\t\tconst char *shortname = NULL;\n    -+\t\t\t\tif (!skip_prefix(source_ref->name,\n    -+\t\t\t\t\t\t \"refs/heads/\", &shortname))\n    -+\t\t\t\t\tshortname = source_ref->name;\n    ++\t\t\t\tconst char *shortname = source_ref->name;\n    ++\t\t\t\tskip_prefix(shortname, \"refs/heads/\", &shortname);\n    ++\n     +\t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n     +\t\t\t\t\t  \"it does not point to any branch.\"),\n     +\t\t\t\t\tshortname, transport->remote->name);\n\n builtin/fetch.c         | 10 ++++++++++\n t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbdc..28fa168133a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1625,6 +1625,16 @@ static int do_fetch(struct transport *transport,\n \t\t\t}\n \t\t}\n \t\tif (source_ref) {\n+\t\t\tif (!branch) {\n+\t\t\t\tconst char *shortname = source_ref->name;\n+\t\t\t\tskip_prefix(shortname, \"refs/heads/\", &shortname);\n+\n+\t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n+\t\t\t\t\t  \"it does not point to any branch.\"),\n+\t\t\t\t\tshortname, transport->remote->name);\n+\t\t\t\tgoto skip;\n+\t\t\t}\n+\n \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n \t\t\t    starts_with(source_ref->name, \"refs/heads/\"))\n \t\t\t\tinstall_branch_config(0,\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex b1d614ce18c..7d12ceff702 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'fetch --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit fetch --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # tests for pull --set-upstream\n \n test_expect_success 'setup bare parent pull' '\n@@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'pull --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit pull --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.741.g4db85f1eb27\n\n"},{"id":"434131","messageId":"xmqqilzm4ygn.fsf@gitster.g","threadId":"56051","inReplyTo":"patch-v3-1.1-68899471206-20210830T144020Z-avarab@gmail.com","subject":"Re: [PATCH v3] pull, fetch: fix segfault in --set-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-30T17:46:48Z","receivedAt":"2021-08-30T17:46:52Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n> There was an earlier submitted alternate way of fixing this in [1],\n> due to that patch breaking threading with the original report at [2] I\n> didn't notice it before authoring this version. I think the more\n> detailed warning message here is better, and we should also have tests\n> for this behavior.\n\nI do not think it is necessarily an improvement to give more info,\nif it is irrelevant to explain what the error is.  And the point of\nthe error message here is that we cannot set the upstream of\ndetached HEAD, no matter what the value of old source ref or new\nsource ref are.\n\nThe original from Clemens gives a warning message that omits the\npiece of information that does not contribute to the error.\n\nTesting the new behaviour is a good idea.  I also agree with you\nthat die() would be more appropriate and does not risk regression,\nif the original behaviour was to segfault.\n\nThanks.\n"},{"id":"434281","messageId":"patch-v4-1.1-0caa9a89a86-20210831T135212Z-avarab@gmail.com","threadId":"56051","inReplyTo":"patch-v3-1.1-68899471206-20210830T144020Z-avarab@gmail.com","subject":"[PATCH v4] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-31T13:58:17Z","receivedAt":"2021-08-31T13:59:27Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"Fix a segfault in the --set-upstream option added in\n24bc1a12926 (pull, fetch: add --set-upstream option, 2019-08-19) added\nin v2.24.0.\n\nThe code added there did not do the same checking we do for \"git\nbranch\" itself since 8efb8899cfe (branch: segfault fixes and\nvalidation, 2013-02-23), which in turn fixed the same sort of segfault\nI'm fixing now in \"git branch --set-upstream-to\", see\n6183d826ba6 (branch: introduce --set-upstream-to, 2012-08-20).\n\nThe warning message I'm adding here is an amalgamation of the error\nadded for \"git branch\" in 8efb8899cfe, and the error output\ninstall_branch_config() itself emits, i.e. it trims \"refs/heads/\" from\nthe name and says \"branch X on remote\", not \"branch refs/heads/X on\nremote\".\n\nI think it would make more sense to simply die() here, but in the\nother checks for --set-upstream added in 24bc1a12926 we issue a\nwarning() instead. Let's do the same here for consistency for now.\n\nThere was an earlier submitted alternate way of fixing this in [1],\ndue to that patch breaking threading with the original report at [2] I\ndidn't notice it before authoring this version. I think the more\ndetailed warning message here is better, and we should also have tests\nfor this behavior.\n\nThe --no-rebase option to \"git pull\" is needed as of the recently\nmerged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',\n2021-08-30).\n\n1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/\n2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/\n\nReported-by: Clemens Fruhwirth <clemens@endorphin.org>\nReported-by: Jan Pokorný <poki@fnusa.cz>\nSigned-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>\n---\n\nA re-submission for a semantic conflict with the now-merged\n7d0daf3f12f on master.\n\nOn Mon, Aug 30 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>> There was an earlier submitted alternate way of fixing this in [1],\n>> due to that patch breaking threading with the original report at [2] I\n>> didn't notice it before authoring this version. I think the more\n>> detailed warning message here is better, and we should also have tests\n>> for this behavior.\n>\n> I do not think it is necessarily an improvement to give more info,\n> if it is irrelevant to explain what the error is.  And the point of\n> the error message here is that we cannot set the upstream of\n> detached HEAD, no matter what the value of old source ref or new\n> source ref are.\n>\n> The original from Clemens gives a warning message that omits the\n> piece of information that does not contribute to the error.\n\nI see your point, but looking at it again I decided to keep it as-is\nin this re-roll. As noted in the commit message this is for\nconsistency with the output we emit when running a commind like \"git\nbranch --set-upstream-to <upstream> <branchname>\", which as you'll see\nif you \"git checkout HEAD^0\" we'll quote the wanted upstream back at\nyou.\n\nSo yeah, arguably we should just punt on that whole thing and tell the\nuser \"you're on a detached HEAD, this will never work\" or something\nlike that, but let's consider that UI change separately, and then do\nit for all of \"branch\", \"fetch\" and \"pull\", rather than leave the\nlatter two inconsistent with \"branch\" with this fix.\n\n> Testing the new behaviour is a good idea.  I also agree with you\n> that die() would be more appropriate and does not risk regression,\n> if the original behaviour was to segfault.\n\nIndeed. I changed it due to your upthread\n<xmqqsg0anxix.fsf@gitster.g>.\n\nI think doing s/warning/die/ here makes sense, but similarly to the\nabove comment: Let's punt on that and do it separately from this\nnarrow segfault fix. If and when we change that we should change\nvarious other \"warning()\" around this codepath to \"die()\" as well.\n\nRange-diff against v3:\n1:  68899471206 ! 1:  0caa9a89a86 pull, fetch: fix segfault in --set-upstream option\n    @@ Commit message\n         detailed warning message here is better, and we should also have tests\n         for this behavior.\n     \n    +    The --no-rebase option to \"git pull\" is needed as of the recently\n    +    merged 7d0daf3f12f (Merge branch 'en/pull-conflicting-options',\n    +    2021-08-30).\n    +\n         1. https://lore.kernel.org/git/20210706162238.575988-1-clemens@endorphin.org/\n         2. https://lore.kernel.org/git/CAG6gW_uHhfNiHGQDgGmb1byMqBA7xa8kuH1mP-wAPEe5Tmi2Ew@mail.gmail.com/\n     \n    @@ t/t5553-set-upstream.sh: test_expect_success 'pull --set-upstream with valid URL\n     +\tcat >expect <<-\\EOF &&\n     +\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n     +\tEOF\n    -+\tgit pull --set-upstream upstream main 2>actual.raw &&\n    ++\tgit pull --no-rebase --set-upstream upstream main 2>actual.raw &&\n     +\tgrep ^warning: actual.raw >actual &&\n     +\ttest_cmp expect actual\n     +'\n\n builtin/fetch.c         | 10 ++++++++++\n t/t5553-set-upstream.sh | 22 ++++++++++++++++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/builtin/fetch.c b/builtin/fetch.c\nindex e064687dbdc..28fa168133a 100644\n--- a/builtin/fetch.c\n+++ b/builtin/fetch.c\n@@ -1625,6 +1625,16 @@ static int do_fetch(struct transport *transport,\n \t\t\t}\n \t\t}\n \t\tif (source_ref) {\n+\t\t\tif (!branch) {\n+\t\t\t\tconst char *shortname = source_ref->name;\n+\t\t\t\tskip_prefix(shortname, \"refs/heads/\", &shortname);\n+\n+\t\t\t\twarning(_(\"could not set upstream of HEAD to '%s' from '%s' when \"\n+\t\t\t\t\t  \"it does not point to any branch.\"),\n+\t\t\t\t\tshortname, transport->remote->name);\n+\t\t\t\tgoto skip;\n+\t\t\t}\n+\n \t\t\tif (!strcmp(source_ref->name, \"HEAD\") ||\n \t\t\t    starts_with(source_ref->name, \"refs/heads/\"))\n \t\t\t\tinstall_branch_config(0,\ndiff --git a/t/t5553-set-upstream.sh b/t/t5553-set-upstream.sh\nindex 9c12c0f8c32..48050162c27 100755\n--- a/t/t5553-set-upstream.sh\n+++ b/t/t5553-set-upstream.sh\n@@ -91,6 +91,17 @@ test_expect_success 'fetch --set-upstream with valid URL sets upstream to URL' '\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'fetch --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit fetch --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n # tests for pull --set-upstream\n \n test_expect_success 'setup bare parent pull' '\n@@ -178,4 +189,15 @@ test_expect_success 'pull --set-upstream with valid URL and branch sets branch'\n \tcheck_config_missing other2\n '\n \n+test_expect_success 'pull --set-upstream with a detached HEAD' '\n+\tgit checkout HEAD^0 &&\n+\ttest_when_finished \"git checkout -\" &&\n+\tcat >expect <<-\\EOF &&\n+\twarning: could not set upstream of HEAD to '\"'\"'main'\"'\"' from '\"'\"'upstream'\"'\"' when it does not point to any branch.\n+\tEOF\n+\tgit pull --no-rebase --set-upstream upstream main 2>actual.raw &&\n+\tgrep ^warning: actual.raw >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.33.0.805.g739b16c2189\n\n"},{"id":"434302","messageId":"xmqqfsupwose.fsf@gitster.g","threadId":"56051","inReplyTo":"patch-v4-1.1-0caa9a89a86-20210831T135212Z-avarab@gmail.com","subject":"Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-08-31T16:40:33Z","receivedAt":"2021-08-31T16:40:37Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n\n>> Testing the new behaviour is a good idea.  I also agree with you\n>> that die() would be more appropriate and does not risk regression,\n>> if the original behaviour was to segfault.\n>\n> Indeed. I changed it due to your upthread\n> <xmqqsg0anxix.fsf@gitster.g>.\n>\n> I think doing s/warning/die/ here makes sense, but similarly to the\n> above comment: Let's punt on that and do it separately from this\n> narrow segfault fix. If and when we change that we should change\n> various other \"warning()\" around this codepath to \"die()\" as well.\n\nI do not think that can be thrown into the same bin as \"should UI\ngive irrelevant details?\" issue.  If you make it not to segfault and\ngive just a warning(), it becomes impossible to make it die() later.\n\nIf we all agree that die() is a better action, that must be done\nnow, or it will become never once the change is released to the\nfield.\n\n\n"},{"id":"434334","messageId":"87h7f5we6q.fsf@evledraar.gmail.com","threadId":"56051","inReplyTo":"xmqqfsupwose.fsf@gitster.g","subject":"Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option","fromName":"Ævar Arnfjörð Bjarmason","fromEmail":"avarab@gmail.com","sentAt":"2021-08-31T20:20:46Z","receivedAt":"2021-08-31T20:29:37Z","isPatch":true,"sender":{"key":"avarab@gmail.com","avatar":"https://avatars.githubusercontent.com/u/45301?v=4"},"body":"\nOn Tue, Aug 31 2021, Junio C Hamano wrote:\n\n> Ævar Arnfjörð Bjarmason  <avarab@gmail.com> writes:\n>\n>>> Testing the new behaviour is a good idea.  I also agree with you\n>>> that die() would be more appropriate and does not risk regression,\n>>> if the original behaviour was to segfault.\n>>\n>> Indeed. I changed it due to your upthread\n>> <xmqqsg0anxix.fsf@gitster.g>.\n>>\n>> I think doing s/warning/die/ here makes sense, but similarly to the\n>> above comment: Let's punt on that and do it separately from this\n>> narrow segfault fix. If and when we change that we should change\n>> various other \"warning()\" around this codepath to \"die()\" as well.\n>\n> I do not think that can be thrown into the same bin as \"should UI\n> give irrelevant details?\" issue.  If you make it not to segfault and\n> give just a warning(), it becomes impossible to make it die() later.\n>\n> If we all agree that die() is a better action, that must be done\n> now, or it will become never once the change is released to the\n> field.\n\nBecause someone might have been relying on the current behavior of\nsegfaulting to stop their script? And a mere warning() would break\nthings for them by having the script \"work\" if this patch were to make\nit into a release?\n\nI think it's unlikely that anyone's running into this in the wild as\nanything but a one-off, and in any case whether or not we segfault, warn\nor die the behavior of fetch at this point is to have already finished\nthe fetch itself.\n\nWe're merely doing some post-fetch work of setting config etc. Both\nbefore and after this patch we won't be setting the upstream config. But\nyes, the exit code will change from a segfault to successful exit.\n\nI think the first priority should be to just narrowly fix the segfault &\nleave refactoring of e.g. having fetch do sanity checking on all options\nbefore doing work for later, especially as we're almost 2 months into no\nfix for the segfault landing on \"master\" after the first working patch\nto fix it, so if we have that all wait on agreeing on the perfect\nbehavior for fetch error handling...\n\n"},{"id":"434460","messageId":"xmqqfsuoqjg1.fsf@gitster.g","threadId":"56051","inReplyTo":"87h7f5we6q.fsf@evledraar.gmail.com","subject":"Re: [PATCH v4] pull, fetch: fix segfault in --set-upstream option","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2021-09-01T17:44:46Z","receivedAt":"2021-09-01T17:44:50Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Ævar Arnfjörð Bjarmason <avarab@gmail.com> writes:\n\n>> If we all agree that die() is a better action, that must be done\n>> now, or it will become never once the change is released to the\n>> field.\n>\n> Because someone might have been relying on the current behavior of\n> segfaulting to stop their script? And a mere warning() would break\n> things for them by having the script \"work\" if this patch were to make\n> it into a release?\n\nNo, because someone WILL start rely on the warning() behaviour,\nexpecting that the \"fixed\" command will now run to completion\nwithout exiting with non-zero status.  Once that happens, it will\nbecome impossible to flip it to die().\n\n"}]}