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

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

[Erik Faye-Lund]

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
-- 
1.7.10.msysgit.1

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

[Erik Faye-Lund]

On Tue, May 8, 2012 at 6:05 PM, Erik Faye-Lund <kusmabite@gmail.com> wrote:
> 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:

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

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

[Junio C Hamano]

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

Otherwise the explanation, the patch and the added test makes sense.

Thanks.

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

[Erik Faye-Lund]

On Tue, May 8, 2012 at 6:29 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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 :)

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

[Junio C Hamano]

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

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

[Junio C Hamano]

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.  Perhaps it would make it more palatable if you replaced
"cat .git/HEAD" with "git symbolic-ref HEAD" in the original?

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

[Erik Faye-Lund]

On Tue, May 8, 2012 at 6:41 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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.

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

[Erik Faye-Lund]

On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano <gitster@pobox.com> wrote:
> 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")...

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

[Junio C Hamano]

Erik Faye-Lund <kusmabite@gmail.com> writes:

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

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

[Andreas Schwab]

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