checkout -p: handle tree arguments correctly again

From: Johannes Schindelin <[email protected]>

This fixes a segmentation fault.

The bug is caused by dereferencing new_branch_info->commit when it is NULL, which is the case when the tree-ish argument is actually a tree, not a commit-ish. This was introduced in 5602b500c3c (builtin/checkout:
fix git checkout -p HEAD... bug, 2020-10-07), where we tried to ensure that the special tree-ish HEAD... is handled correctly.

---
checkout -p: handle tree arguments correctly again

I literally just ran into this segmentation fault after rebasing Git for Windows onto -rc1, and did not really think that the regression was introduced in the v2.30.0 cycle, but was proven wrong by my
investigation: it was introduced by v2.30.0-rc0151^23.

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-819%2Fdscho%2Fcheckout-p-empty-tree-v1
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-819/dscho/checkout-p-empty-tree-v1
Pull-Request: https://github.com/gitgitgadget/git/pull/819

 builtin/checkout.c        | 2 +-
 t/t2016-checkout-patch.sh | 5 +++++
 2 files changed, 6 insertions(+), 1 deletion(-)

index 9b82119129a..8b567b0424d 100644
@@ -482,7 +482,7 @@ static int checkout_paths(const struct checkout_opts *opts,
 		 * properly. However, there is special logic for the HEAD case
 		 * so we mustn't replace that.
 		 */
-		if (rev && strcmp(rev, "HEAD"))
+		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
 			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);
 
 		if (opts->checkout_index && opts->checkout_worktree)

index d91a329eb31..abfd586c32b 100755
@@ -123,4 +123,9 @@ test_expect_success PERL 'none of this moved HEAD' '
 	verify_saved_head
 '
 
+test_expect_success PERL 'empty tree can be handled' '
+	test_when_finished "git reset --hard" &&
+	git checkout -p $(test_oid empty_tree) --
+'
+
 test_done

base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707

gitgitgadget

Hi Dscho,

Argh, thanks for catching this. The patch looks obviously good.

Since I'm the one who introduced the bug in the first place:

The fix looks obviously correct to me. Thanks, both.

Will queue for fast-tracking.

"Johannes Schindelin via GitGitGadget" <[email protected]>

As "checkout -p branch~4" is easier than "checkout -p branch~4:", I think you have to work harder to give a true tree object rather than the containing commit object.

It still is curious how you stumbled onto this, but I am glad you did ;-)

 builtin/checkout.c | 4 +++-
 1 file changed, 3 insertions(+), 1 deletion(-)

index 6f24b381c5..728c67e8ca 100644
@@ -479,7 +479,9 @@ static int checkout_paths(const struct checkout_opts *opts,
 		 * with the hex of the commit (whether it's in `...` form or
 		 * not) for the run_add_interactive() machinery to work
 		 * properly. However, there is special logic for the HEAD case
-		 * so we mustn't replace that.
+		 * so we mustn't replace that.  Also, when we were given a
+		 * tree-object, new_branch_info->commit would be NULL, but we
+		 * do not have to do any replacement, either.
 		 */
 		if (rev && new_branch_info->commit && strcmp(rev, "HEAD"))
 			rev = oid_to_hex_r(rev_oid, &new_branch_info->commit->object.oid);