[PATCH] checkout: Don't crash when switching away from an invalid branch.

[PATCH] checkout: Don't crash when switching away from an invalid branch.

[Alexandre Julliard]

I have a tree where for some reason HEAD was pointing to an invalid
commit. I'm not sure how this happened, but git checkout should be
able to recover from that situation without crashing.

Signed-off-by: Alexandre Julliard <julliard@winehq.org>
---
 builtin-checkout.c               |   14 +++++++++-----
 t/t2011-checkout-invalid-head.sh |   18 ++++++++++++++++++
 2 files changed, 27 insertions(+), 5 deletions(-)
 create mode 100755 t/t2011-checkout-invalid-head.sh

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 57b94d2..7c1b8cd 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
 
 	memset(&proc, 0, sizeof(proc));
 	argv[0] = name;
-	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
+	argv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))
+		      : "0000000000000000000000000000000000000000";
 	argv[2] = xstrdup(sha1_to_hex(new->object.sha1));
 	argv[3] = changed ? "1" : "0";
 	argv[4] = NULL;
@@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	}
 
 	old_desc = old->name;
-	if (!old_desc)
+	if (!old_desc && old->commit)
 		old_desc = sha1_to_hex(old->commit->object.sha1);
-	strbuf_addf(&msg, "checkout: moving from %s to %s",
-		    old_desc, new->name);
+	if (old_desc)
+		strbuf_addf(&msg, "checkout: moving from %s to %s",
+			    old_desc, new->name);
+	else
+		strbuf_addf(&msg, "checkout: moving to %s", new->name);
 
 	if (new->path) {
 		create_symref("HEAD", new->path, msg.buf);
@@ -551,7 +555,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	 * a new commit, we want to mention the old commit once more
 	 * to remind the user that it might be lost.
 	 */
-	if (!opts->quiet && !old.path && new->commit != old.commit)
+	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
 		describe_detached_head("Previous HEAD position was", old.commit);
 
 	if (!old.commit) {
diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh
new file mode 100755
index 0000000..764bb0a
--- /dev/null
+++ b/t/t2011-checkout-invalid-head.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='checkout switching away from an invalid branch'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >world &&
+	git add world &&
+	git commit -m initial
+'
+
+test_expect_success 'checkout master from invalid HEAD' '
+	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
+	git checkout master --
+'
+
+test_done
-- 
1.6.0.3.669.g76740

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.

[Johannes Schindelin]

Hi,

On Fri, 7 Nov 2008, Alexandre Julliard wrote:

> I have a tree where for some reason HEAD was pointing to an invalid 
> commit. I'm not sure how this happened, but git checkout should be able 
> to recover from that situation without crashing.

Agree.

> diff --git a/builtin-checkout.c b/builtin-checkout.c
> index 57b94d2..7c1b8cd 100644
> --- a/builtin-checkout.c
> +++ b/builtin-checkout.c
> @@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
>  
>  	memset(&proc, 0, sizeof(proc));
>  	argv[0] = name;
> -	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
> +	argv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))
> +		      : "0000000000000000000000000000000000000000";

I guess you want to use the variable null_sha1 here.

> @@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>  	}
>  
>  	old_desc = old->name;
> -	if (!old_desc)
> +	if (!old_desc && old->commit)
>  		old_desc = sha1_to_hex(old->commit->object.sha1);
> -	strbuf_addf(&msg, "checkout: moving from %s to %s",
> -		    old_desc, new->name);
> +	if (old_desc)
> +		strbuf_addf(&msg, "checkout: moving from %s to %s",
> +			    old_desc, new->name);
> +	else
> +		strbuf_addf(&msg, "checkout: moving to %s", new->name);

Why not
		old_desc ? old_desc : "(invalid)"
?

> diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh
> new file mode 100755

Nice!

Thank you,
Dscho

Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.

[Alexandre Julliard]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

>> diff --git a/builtin-checkout.c b/builtin-checkout.c
>> index 57b94d2..7c1b8cd 100644
>> --- a/builtin-checkout.c
>> +++ b/builtin-checkout.c
>> @@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
>>  
>>  	memset(&proc, 0, sizeof(proc));
>>  	argv[0] = name;
>> -	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
>> +	argv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))
>> +		      : "0000000000000000000000000000000000000000";
>
> I guess you want to use the variable null_sha1 here.

I could, though it seemed to me a bit silly to format and strdup a
string that is a known constant. But I'm happy to change it if needed.

>> @@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,
>>  	}
>>  
>>  	old_desc = old->name;
>> -	if (!old_desc)
>> +	if (!old_desc && old->commit)
>>  		old_desc = sha1_to_hex(old->commit->object.sha1);
>> -	strbuf_addf(&msg, "checkout: moving from %s to %s",
>> -		    old_desc, new->name);
>> +	if (old_desc)
>> +		strbuf_addf(&msg, "checkout: moving from %s to %s",
>> +			    old_desc, new->name);
>> +	else
>> +		strbuf_addf(&msg, "checkout: moving to %s", new->name);
>
> Why not
> 		old_desc ? old_desc : "(invalid)"
> ?

IMO it looks more friendly to not display a branch that doesn't exist,
rather than printing something like (invalid) or (null).

-- 
Alexandre Julliard
julliard@winehq.org

Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.

[Junio C Hamano]

Alexandre Julliard <julliard@winehq.org> writes:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> ...
>> Why not
>> 		old_desc ? old_desc : "(invalid)"
>> ?
>
> IMO it looks more friendly to not display a branch that doesn't exist,
> rather than printing something like (invalid) or (null).

Actually I think it is a good idea to remind that you were in a funny
state.

For that matter, dying without removing the trace of that funny state
might be even preferrable if you need to do postmortem to figure out why
you got into such a funny state to begin with, but not everybody uses git
to debug git.  I think Dscho's suggestion is a reasonable middle ground.

Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.

[Alexandre Julliard]

Junio C Hamano <gitster@pobox.com> writes:

> For that matter, dying without removing the trace of that funny state
> might be even preferrable if you need to do postmortem to figure out why
> you got into such a funny state to begin with, but not everybody uses git
> to debug git.

It turns out to be user error, that was a tree I hadn't used in a long
time and I didn't realize it was using alternates, so HEAD was pointing
to a commit that had been rebased and garbage-collected in the source
repository.

Most other commands die with a "bad object HEAD" in that situation, and
checkout could certainly do that too, but I think it's nicer to provide
an easy way of getting out of that broken state.  I'll resend an updated
patch.

-- 
Alexandre Julliard
julliard@winehq.org

[PATCH] checkout: Don't crash when switching away from an invalid branch.

[Alexandre Julliard]

When using alternates, it is possible for HEAD to end up pointing to
an invalid commit. git checkout should be able to recover from that
situation without crashing.

Signed-off-by: Alexandre Julliard <julliard@winehq.org>
---
 builtin-checkout.c               |    8 ++++----
 t/t2011-checkout-invalid-head.sh |   18 ++++++++++++++++++
 2 files changed, 22 insertions(+), 4 deletions(-)
 create mode 100755 t/t2011-checkout-invalid-head.sh

diff --git a/builtin-checkout.c b/builtin-checkout.c
index 57b94d2..06904c3 100644
--- a/builtin-checkout.c
+++ b/builtin-checkout.c
@@ -47,7 +47,7 @@ static int post_checkout_hook(struct commit *old, struct commit *new,
 
 	memset(&proc, 0, sizeof(proc));
 	argv[0] = name;
-	argv[1] = xstrdup(sha1_to_hex(old->object.sha1));
+	argv[1] = xstrdup(sha1_to_hex(old ? old->object.sha1 : null_sha1));
 	argv[2] = xstrdup(sha1_to_hex(new->object.sha1));
 	argv[3] = changed ? "1" : "0";
 	argv[4] = NULL;
@@ -492,10 +492,10 @@ static void update_refs_for_switch(struct checkout_opts *opts,
 	}
 
 	old_desc = old->name;
-	if (!old_desc)
+	if (!old_desc && old->commit)
 		old_desc = sha1_to_hex(old->commit->object.sha1);
 	strbuf_addf(&msg, "checkout: moving from %s to %s",
-		    old_desc, new->name);
+		    old_desc ? old_desc : "(invalid)", new->name);
 
 	if (new->path) {
 		create_symref("HEAD", new->path, msg.buf);
@@ -551,7 +551,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)
 	 * a new commit, we want to mention the old commit once more
 	 * to remind the user that it might be lost.
 	 */
-	if (!opts->quiet && !old.path && new->commit != old.commit)
+	if (!opts->quiet && !old.path && old.commit && new->commit != old.commit)
 		describe_detached_head("Previous HEAD position was", old.commit);
 
 	if (!old.commit) {
diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh
new file mode 100755
index 0000000..764bb0a
--- /dev/null
+++ b/t/t2011-checkout-invalid-head.sh
@@ -0,0 +1,18 @@
+#!/bin/sh
+
+test_description='checkout switching away from an invalid branch'
+
+. ./test-lib.sh
+
+test_expect_success 'setup' '
+	echo hello >world &&
+	git add world &&
+	git commit -m initial
+'
+
+test_expect_success 'checkout master from invalid HEAD' '
+	echo 0000000000000000000000000000000000000000 >.git/HEAD &&
+	git checkout master --
+'
+
+test_done
-- 
1.6.0.3.669.g76740

-- 
Alexandre Julliard
julliard@winehq.org