checkout -p: handle tree arguments correctly again

Commit Message

Johannes Schindelin Dec. 19, 2020, 2:55 p.m. UTC

From: Johannes Schindelin <[e-mail]>

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.

Signed-off-by: Johannes Schindelin <[e-mail]>
---
    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(-)


base-commit: 6d3ef5b467eccd2769f1aa1c555d317d3c8dc707

Comments

Denton Liu Dec. 19, 2020, 3:02 p.m. UTC | #1

Hi Dscho,

On Sat, Dec 19, 2020 at 02:55:59PM +0000, Johannes Schindelin via GitGitGadget wrote:
> From: Johannes Schindelin <[e-mail]>
> 
> 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.
> 
> Signed-off-by: Johannes Schindelin <[e-mail]>

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

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

	Acked-by: Denton Liu <[e-mail]>

Junio C Hamano Dec. 19, 2020, 5:34 p.m. UTC | #2

Denton Liu <[e-mail]> writes:

> Hi Dscho,
>
> On Sat, Dec 19, 2020 at 02:55:59PM +0000, Johannes Schindelin via GitGitGadget wrote:
>> From: Johannes Schindelin <[e-mail]>
>> 
>> 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.
>> 
>> Signed-off-by: Johannes Schindelin <[e-mail]>
>
> Argh, thanks for catching this. The patch looks obviously good.
>
> Since I'm the one who introduced the bug in the first place:
>
> 	Acked-by: Denton Liu <[e-mail]>

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

Will queue for fast-tracking.

Junio C Hamano Dec. 19, 2020, 5:40 p.m. UTC | #3

"Johannes Schindelin via GitGitGadget" <[e-mail]>
writes:

>     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.

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 ;-)

> @@ -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"))

The comment above talks about why "&& strcmp()" is there.  We now
have two.

I'd squash this in while queuing.  Thanks.


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

diff --git c/builtin/checkout.c w/builtin/checkout.c
index 6f24b381c5..728c67e8ca 100644
--- c/builtin/checkout.c
+++ w/builtin/checkout.c
@@ -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);

Patch

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 9b82119129a..8b567b0424d 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -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)
diff --git a/t/t2016-checkout-patch.sh b/t/t2016-checkout-patch.sh
index d91a329eb31..abfd586c32b 100755
--- a/t/t2016-checkout-patch.sh
+++ b/t/t2016-checkout-patch.sh
@@ -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