{"thread":{"id":"54856","subject":"[PATCH] checkout -p: handle tree arguments correctly again","startedAt":"2020-12-19T14:57:00Z","lastAt":"2020-12-19T17:41:47Z","messageCount":4,"participants":["Johannes Schindelin via GitGitGadget","Denton Liu","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"412644","messageId":"pull.819.git.1608389760050.gitgitgadget@gmail.com","threadId":"54856","inReplyTo":null,"subject":"[PATCH] checkout -p: handle tree arguments correctly again","fromName":"Johannes Schindelin via GitGitGadget","fromEmail":"gitgitgadget@gmail.com","sentAt":"2020-12-19T14:55:59Z","receivedAt":"2020-12-19T14:57:00Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"From: Johannes Schindelin \n\nThis fixes a segmentation fault.\n\nThe bug is caused by dereferencing `new_branch_info->commit` when it is\n`NULL`, which is the case when the tree-ish argument is actually a tree,\nnot a commit-ish. This was introduced in 5602b500c3c (builtin/checkout:\nfix `git checkout -p HEAD...` bug, 2020-10-07), where we tried to ensure\nthat the special tree-ish `HEAD...` is handled correctly.\n\nSigned-off-by: Johannes Schindelin \n---\n checkout -p: handle tree arguments correctly again\n \n I literally just ran into this segmentation fault after rebasing Git for\n Windows onto -rc1, and did not really think that the regression was\n introduced in the v2.30.0 cycle, but was proven wrong by my\n investigation: it was introduced by v2.30.0-rc0151^23.\n\nPublished-As: https://github.com/gitgitgadget/git/releases/tag/pr-819%2Fdscho%2Fcheckout-p-empty-tree-v1\nFetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-819/dscho/checkout-p-empty-tree-v1\nPull-Request: https://github.com/gitgitgadget/git/pull/819\n\n builtin/checkout.c | 2 +-\n t/t2016-checkout-patch.sh | 5 +++++\n 2 files changed, 6 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 9b82119129a..8b567b0424d 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -482,7 +482,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * properly. However, there is special logic for the HEAD case\n \t\t * so we mustn't replace that.\n \t\t */\n-\t\tif (rev && strcmp(rev, \"HEAD\"))\n+\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n \n \t\tif (opts->checkout_index && opts->checkout_worktree)\ndiff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh\nindex d91a329eb31..abfd586c32b 100755\n--- a/t/t2016-checkout-patch.sh\n+++ b/t/t2016-checkout-patch.sh\n@@ -123,4 +123,9 @@ test_expect_success PERL 'none of this moved HEAD' '\n \tverify_saved_head\n '\n \n+test_expect_success PERL 'empty tree can be handled' '\n+\ttest_when_finished \"git reset --hard\" &&\n+\tgit checkout -p $(test_oid empty_tree) --\n+'\n+\n test_done\n\nbase-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707\n-- \ngitgitgadget\n"},{"id":"412647","messageId":"X94WHmY3Cd30wqan@generichostname","threadId":"54856","inReplyTo":"pull.819.git.1608389760050.gitgitgadget@gmail.com","subject":"Re: [PATCH] checkout -p: handle tree arguments correctly again","fromName":"Denton Liu","fromEmail":"liu.denton@gmail.com","sentAt":"2020-12-19T15:02:54Z","receivedAt":"2020-12-19T15:03:54Z","isPatch":true,"sender":{"key":"liu.denton@gmail.com","avatar":"https://avatars.githubusercontent.com/u/9620836?v=4"},"body":"Hi Dscho,\n\nOn Sat, Dec 19, 2020 at 02:55:59PM +0000, Johannes Schindelin via GitGitGadget wrote:\n> From: Johannes Schindelin \n> \n> This fixes a segmentation fault.\n> \n> The bug is caused by dereferencing `new_branch_info->commit` when it is\n> `NULL`, which is the case when the tree-ish argument is actually a tree,\n> not a commit-ish. This was introduced in 5602b500c3c (builtin/checkout:\n> fix `git checkout -p HEAD...` bug, 2020-10-07), where we tried to ensure\n> that the special tree-ish `HEAD...` is handled correctly.\n> \n> Signed-off-by: Johannes Schindelin \n\nArgh, thanks for catching this. The patch looks obviously good.\n\nSince I'm the one who introduced the bug in the first place:\n\n\tAcked-by: Denton Liu \n"},{"id":"412656","messageId":"xmqqczz5punk.fsf@gitster.c.googlers.com","threadId":"54856","inReplyTo":"X94WHmY3Cd30wqan@generichostname","subject":"Re: [PATCH] checkout -p: handle tree arguments correctly again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T17:34:07Z","receivedAt":"2020-12-19T17:35:07Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Denton Liu writes:\n\n> Hi Dscho,\n>\n> On Sat, Dec 19, 2020 at 02:55:59PM +0000, Johannes Schindelin via GitGitGadget wrote:\n>> From: Johannes Schindelin \n>> \n>> This fixes a segmentation fault.\n>> \n>> The bug is caused by dereferencing `new_branch_info->commit` when it is\n>> `NULL`, which is the case when the tree-ish argument is actually a tree,\n>> not a commit-ish. This was introduced in 5602b500c3c (builtin/checkout:\n>> fix `git checkout -p HEAD...` bug, 2020-10-07), where we tried to ensure\n>> that the special tree-ish `HEAD...` is handled correctly.\n>> \n>> Signed-off-by: Johannes Schindelin \n>\n> Argh, thanks for catching this. The patch looks obviously good.\n>\n> Since I'm the one who introduced the bug in the first place:\n>\n> \tAcked-by: Denton Liu \n\nThe fix looks obviously correct to me. Thanks, both.\n\nWill queue for fast-tracking.\n\n"},{"id":"412657","messageId":"xmqq5z4xpuck.fsf@gitster.c.googlers.com","threadId":"54856","inReplyTo":"pull.819.git.1608389760050.gitgitgadget@gmail.com","subject":"Re: [PATCH] checkout -p: handle tree arguments correctly again","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-12-19T17:40:43Z","receivedAt":"2020-12-19T17:41:47Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"\"Johannes Schindelin via GitGitGadget\" \nwrites:\n\n> I literally just ran into this segmentation fault after rebasing Git for\n> Windows onto -rc1, and did not really think that the regression was\n> introduced in the v2.30.0 cycle, but was proven wrong by my\n> investigation: it was introduced by v2.30.0-rc0151^23.\n\nAs \"checkout -p branch~4\" is easier than \"checkout -p branch~4:\", I\nthink you have to work harder to give a true tree object rather than\nthe containing commit object.\n\nIt still is curious how you stumbled onto this, but I am glad you\ndid ;-)\n\n> @@ -482,7 +482,7 @@ static int checkout_paths(const struct checkout_opts *opts,\n> \t\t * properly. However, there is special logic for the HEAD case\n> \t\t * so we mustn't replace that.\n> \t\t */\n> -\t\tif (rev && strcmp(rev, \"HEAD\"))\n> +\t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n\nThe comment above talks about why \"&& strcmp()\" is there. We now\nhave two.\n\nI'd squash this in while queuing. Thanks.\n\n\n builtin/checkout.c | 4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git c/builtin/checkout.c w/builtin/checkout.c\nindex 6f24b381c5..728c67e8ca 100644\n--- c/builtin/checkout.c\n+++ w/builtin/checkout.c\n@@ -479,7 +479,9 @@ static int checkout_paths(const struct checkout_opts *opts,\n \t\t * with the hex of the commit (whether it's in `...` form or\n \t\t * not) for the run_add_interactive() machinery to work\n \t\t * properly. However, there is special logic for the HEAD case\n-\t\t * so we mustn't replace that.\n+\t\t * so we mustn't replace that. Also, when we were given a\n+\t\t * tree-object, new_branch_info->commit would be NULL, but we\n+\t\t * do not have to do any replacement, either.\n \t\t */\n \t\tif (rev && new_branch_info->commit && strcmp(rev, \"HEAD\"))\n \t\t\trev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);\n"}]}