threads / patch / 30472

patchcheckout: do not corrupt HEAD on empty repo

Subject: [PATCH] checkout: do not corrupt HEAD on empty repo

## tl;dr

10 messages between May 8, 2012 and May 8, 2012. Diffs are folded; open one to read it.

replies: 9people: 3as markdown or json

Erik Faye-Lund· May 8, 2012, 16:05 UTC · lore

In abe1998 ("git checkout -b: allow switching out of an unborn branch"), a code-path overly-optimisticly assumed that a branch-name was specified. This is not always the case, and as a result a NULL-pointer was attempted printed to .git/HEAD.

This could lead to at least two different failure modes:
 1) The CRT formated the NULL-string as something useful (e.g
    "(null)")
 2) The CRT crasheed.
Neither were very convenient for formatting a new HEAD-reference.

To fix this, reintroduce some strictness so we only take this new codepath if a banch-name was specified.

Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
---
 builtin/checkout.c         |    2 +-
 t/t2015-checkout-unborn.sh |   11 +++++++++++
 2 files changed, 12 insertions(+), 1 deletion(-)
Show changes to 2 files +12 −1

builtin/checkout.c, t/t2015-checkout-unborn.sh

diff --git a/builtin/checkout.c b/builtin/checkout.c
index 23fc56d..35924d4 100644
--- a/builtin/checkout.c
+++ b/builtin/checkout.c
@@ -1091,7 +1091,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
 	if (opts.writeout_stage)
 		die(_("--ours/--theirs is incompatible with switching branches."));
 
-	if (!new.commit) {
+	if (!new.commit && opts.new_branch) {
 		unsigned char rev[20];
 		int flag;
 
diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
index 6352b74..2fa9458 100755
--- a/t/t2015-checkout-unborn.sh
+++ b/t/t2015-checkout-unborn.sh
@@ -46,4 +46,15 @@ test_expect_success 'checking out another branch from unborn state' '
 	test_cmp expect actual
 '
 
+test_expect_success 'checking out in a newly created repo' '
+	test_create_repo empty &&
+	(
+		cd empty &&
+		cat .git/HEAD >expect &&
+		test_must_fail git checkout &&
+		cat .git/HEAD >actual &&
+		test_cmp expect actual
+	)
+'
+
 test_done
-- 
1.7.10.msysgit.1
Erik Faye-Lund· May 8, 2012, 16:25 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

On Tue, May 8, 2012 at 6:05 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
Show 54 quoted lines
> In abe1998 ("git checkout -b: allow switching out of an unborn
> branch"), a code-path overly-optimisticly assumed that a
> branch-name was specified. This is not always the case, and as
> a result a NULL-pointer was attempted printed to .git/HEAD.
>
> This could lead to at least two different failure modes:
>  1) The CRT formated the NULL-string as something useful (e.g
>    "(null)")
>  2) The CRT crasheed.
>
> Neither were very convenient for formatting a new HEAD-reference.
>
> To fix this, reintroduce some strictness so we only take this
> new codepath if a banch-name was specified.
>
> Signed-off-by: Erik Faye-Lund <kusmabite@gmail.com>
> ---
>  builtin/checkout.c         |    2 +-
>  t/t2015-checkout-unborn.sh |   11 +++++++++++
>  2 files changed, 12 insertions(+), 1 deletion(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 23fc56d..35924d4 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -1091,7 +1091,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)
>        if (opts.writeout_stage)
>                die(_("--ours/--theirs is incompatible with switching branches."));
>
> -       if (!new.commit) {
> +       if (!new.commit && opts.new_branch) {
>                unsigned char rev[20];
>                int flag;
>
> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
> index 6352b74..2fa9458 100755
> --- a/t/t2015-checkout-unborn.sh
> +++ b/t/t2015-checkout-unborn.sh
> @@ -46,4 +46,15 @@ test_expect_success 'checking out another branch from unborn state' '
>        test_cmp expect actual
>  '
>
> +test_expect_success 'checking out in a newly created repo' '
> +       test_create_repo empty &&
> +       (
> +               cd empty &&
> +               cat .git/HEAD >expect &&
> +               test_must_fail git checkout &&
> +               cat .git/HEAD >actual &&
> +               test_cmp expect actual
> +       )
> +'
> +
>  test_done
Hmh, this is needlessly cluttery. The following should cover it:
---
Show changes to t/t2015-checkout-unborn.sh +1 −4
diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
index 2fa9458..b49fedf 100755
--- a/t/t2015-checkout-unborn.sh
+++ b/t/t2015-checkout-unborn.sh
@@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '
 	test_create_repo empty &&
 	(
 		cd empty &&
-		cat .git/HEAD >expect &&
-		test_must_fail git checkout &&
-		cat .git/HEAD >actual &&
-		test_cmp expect actual
+		test_must_fail git checkout
 	)
 '
Junio C Hamano· May 8, 2012, 16:43 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 18 quoted lines
> Hmh, this is needlessly cluttery. The following should cover it:
>
> ---
> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
> index 2fa9458..b49fedf 100755
> --- a/t/t2015-checkout-unborn.sh
> +++ b/t/t2015-checkout-unborn.sh
> @@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '
>  	test_create_repo empty &&
>  	(
>  		cd empty &&
> -		cat .git/HEAD >expect &&
> -		test_must_fail git checkout &&
> -		cat .git/HEAD >actual &&
> -		test_cmp expect actual
> +		test_must_fail git checkout
>  	)
>  '

Hrm, I am of two minds. Yes, we may want checkout to fail, but at the same time, we would want to make sure that a failed checkout does not corrupt the HEAD. Perhaps it would make it more palatable if you replaced "cat .git/HEAD" with "git symbolic-ref HEAD" in the original?

Erik Faye-Lund· May 8, 2012, 16:52 UTC · re: Junio C Hamano · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 24 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> Hmh, this is needlessly cluttery. The following should cover it:
>>
>> ---
>> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh
>> index 2fa9458..b49fedf 100755
>> --- a/t/t2015-checkout-unborn.sh
>> +++ b/t/t2015-checkout-unborn.sh
>> @@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '
>>       test_create_repo empty &&
>>       (
>>               cd empty &&
>> -             cat .git/HEAD >expect &&
>> -             test_must_fail git checkout &&
>> -             cat .git/HEAD >actual &&
>> -             test_cmp expect actual
>> +             test_must_fail git checkout
>>       )
>>  '
>
> Hrm, I am of two minds.  Yes, we may want checkout to fail, but at the
> same time, we would want to make sure that a failed checkout does not
> corrupt the HEAD.
Good point.
>  Perhaps it would make it more palatable if you replaced
> "cat .git/HEAD" with "git symbolic-ref HEAD" in the original?

Ah, yes. That's much better. Do you want me to resend (improving the test and replacing "CRT" with "vsnprintf")? I also spotted a typo in the commit message ("crasheed" vs "crashed")...

Junio C Hamano· May 8, 2012, 17:13 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 14 quoted lines
> On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> Hrm, I am of two minds.  Yes, we may want checkout to fail, but at the
>> same time, we would want to make sure that a failed checkout does not
>> corrupt the HEAD.
>
> Good point.
>
>>  Perhaps it would make it more palatable if you replaced
>> "cat .git/HEAD" with "git symbolic-ref HEAD" in the original?
>
> Ah, yes. That's much better. Do you want me to resend (improving the
> test and replacing "CRT" with "vsnprintf")? I also spotted a typo in
> the commit message ("crasheed" vs "crashed")...
Surely.

By the way, notice I said "we *may* want checkout to fail"? With the discussion "why is it wrong to allow 'git checkout' to be no-op in a freshly created repository" stemming from your other "I do not claim it is correct but it makes the test pass" message, we may actually want to make that 'checkout' in the test pass, and then 'test_must_fail git checkout' in the test would have to go when that happens.

Junio C Hamano· May 8, 2012, 16:29 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

Erik Faye-Lund <kusmabite@gmail.com> writes:
Show 9 quoted lines
> In abe1998 ("git checkout -b: allow switching out of an unborn
> branch"), a code-path overly-optimisticly assumed that a
> branch-name was specified. This is not always the case, and as
> a result a NULL-pointer was attempted printed to .git/HEAD.
>
> This could lead to at least two different failure modes:
>  1) The CRT formated the NULL-string as something useful (e.g
>     "(null)")
>  2) The CRT crasheed.

Just a quick question. What does a cathode ray tube have to do with this (or "are people expected to know what CRT you are talking about?")?

Otherwise the explanation, the patch and the added test makes sense.
Thanks.
Erik Faye-Lund· May 8, 2012, 16:35 UTC · re: Junio C Hamano · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

On Tue, May 8, 2012 at 6:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 15 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>> In abe1998 ("git checkout -b: allow switching out of an unborn
>> branch"), a code-path overly-optimisticly assumed that a
>> branch-name was specified. This is not always the case, and as
>> a result a NULL-pointer was attempted printed to .git/HEAD.
>>
>> This could lead to at least two different failure modes:
>>  1) The CRT formated the NULL-string as something useful (e.g
>>     "(null)")
>>  2) The CRT crasheed.
>
> Just a quick question.  What does a cathode ray tube have to do with this
> (or "are people expected to know what CRT you are talking about?")?
>

Feel free to change it to "the C runtime", or even "the vsnprintf implementation" if you feel like it :)

Junio C Hamano· May 8, 2012, 16:41 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

Erik Faye-Lund <kusmabite@gmail.com> writes:
>> (or "are people expected to know what CRT you are talking about?")?
>
> Feel free to change it to "the C runtime", or even "the vsnprintf
> implementation" if you feel like it :)

Ahh, "C RunTime" was what you meant. OK, it somehow immediately did not "click".

Erik Faye-Lund· May 8, 2012, 16:43 UTC · re: Junio C Hamano · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

On Tue, May 8, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
Show 9 quoted lines
> Erik Faye-Lund <kusmabite@gmail.com> writes:
>
>>> (or "are people expected to know what CRT you are talking about?")?
>>
>> Feel free to change it to "the C runtime", or even "the vsnprintf
>> implementation" if you feel like it :)
>
> Ahh, "C RunTime" was what you meant.  OK, it somehow immediately did not
> "click".

It's the name we Windows users tend to use for what you unix-guys tend to call "libc" ;)

Probably because Microsoft Visual C names their MSVCRT.dll.
Andreas Schwab· May 8, 2012, 17:33 UTC · re: Erik Faye-Lund · lore

Re: [PATCH] checkout: do not corrupt HEAD on empty repo

Erik Faye-Lund <kusmabite@gmail.com> writes:
> It's the name we Windows users tend to use for what you unix-guys tend
> to call "libc" ;)
I think it originated in Unix, viz /usr/lib/crt0.o.
Andreas.
-- 
Andreas Schwab, schwab@linux-m68k.org
GPG Key fingerprint = 58CA 54C7 6D53 942B 1756  01D3 44D5 214B 8276 4ED5
"And now for something completely different."

← back to recent threads