{"thread":{"id":"30472","subject":"[PATCH] checkout: do not corrupt HEAD on empty repo","startedAt":"2012-05-08T16:05:14Z","lastAt":"2012-05-08T17:33:41Z","messageCount":10,"participants":["Erik Faye-Lund","Junio C Hamano","Andreas Schwab"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"191106","messageId":"1336493114-4984-1-git-send-email-kusmabite@gmail.com","threadId":"30472","inReplyTo":null,"subject":"[PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-05-08T16:05:14Z","receivedAt":"2012-05-08T16:05:14Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"In abe1998 (\"git checkout -b: allow switching out of an unborn\nbranch\"), a code-path overly-optimisticly assumed that a\nbranch-name was specified. This is not always the case, and as\na result a NULL-pointer was attempted printed to .git/HEAD.\n\nThis could lead to at least two different failure modes:\n 1) The CRT formated the NULL-string as something useful (e.g\n \"(null)\")\n 2) The CRT crasheed.\n\nNeither were very convenient for formatting a new HEAD-reference.\n\nTo fix this, reintroduce some strictness so we only take this\nnew codepath if a banch-name was specified.\n\nSigned-off-by: Erik Faye-Lund \n---\n builtin/checkout.c | 2 +-\n t/t2015-checkout-unborn.sh | 11 +++++++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex 23fc56d..35924d4 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -1091,7 +1091,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n \tif (opts.writeout_stage)\n \t\tdie(_(\"--ours/--theirs is incompatible with switching branches.\"));\n \n-\tif (!new.commit) {\n+\tif (!new.commit && opts.new_branch) {\n \t\tunsigned char rev[20];\n \t\tint flag;\n \ndiff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\nindex 6352b74..2fa9458 100755\n--- a/t/t2015-checkout-unborn.sh\n+++ b/t/t2015-checkout-unborn.sh\n@@ -46,4 +46,15 @@ test_expect_success 'checking out another branch from unborn state' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'checking out in a newly created repo' '\n+\ttest_create_repo empty &&\n+\t(\n+\t\tcd empty &&\n+\t\tcat .git/HEAD >expect &&\n+\t\ttest_must_fail git checkout &&\n+\t\tcat .git/HEAD >actual &&\n+\t\ttest_cmp expect actual\n+\t)\n+'\n+\n test_done\n-- \n1.7.10.msysgit.1\n"},{"id":"191111","messageId":"CABPQNSau5snkNLqsnBnsLVBVNKHXnvb8rsE8DbNfpo9__5uijA@mail.gmail.com","threadId":"30472","inReplyTo":"1336493114-4984-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-05-08T16:25:29Z","receivedAt":"2012-05-08T16:25:29Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, May 8, 2012 at 6:05 PM, Erik Faye-Lund wrote:\n> In abe1998 (\"git checkout -b: allow switching out of an unborn\n> branch\"), a code-path overly-optimisticly assumed that a\n> branch-name was specified. This is not always the case, and as\n> a result a NULL-pointer was attempted printed to .git/HEAD.\n>\n> This could lead to at least two different failure modes:\n>  1) The CRT formated the NULL-string as something useful (e.g\n>    \"(null)\")\n>  2) The CRT crasheed.\n>\n> Neither were very convenient for formatting a new HEAD-reference.\n>\n> To fix this, reintroduce some strictness so we only take this\n> new codepath if a banch-name was specified.\n>\n> Signed-off-by: Erik Faye-Lund \n> ---\n>  builtin/checkout.c         |    2 +-\n>  t/t2015-checkout-unborn.sh |   11 +++++++++++\n>  2 files changed, 12 insertions(+), 1 deletion(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index 23fc56d..35924d4 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -1091,7 +1091,7 @@ int cmd_checkout(int argc, const char **argv, const char *prefix)\n>        if (opts.writeout_stage)\n>                die(_(\"--ours/--theirs is incompatible with switching branches.\"));\n>\n> -       if (!new.commit) {\n> +       if (!new.commit && opts.new_branch) {\n>                unsigned char rev[20];\n>                int flag;\n>\n> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\n> index 6352b74..2fa9458 100755\n> --- a/t/t2015-checkout-unborn.sh\n> +++ b/t/t2015-checkout-unborn.sh\n> @@ -46,4 +46,15 @@ test_expect_success 'checking out another branch from unborn state' '\n>        test_cmp expect actual\n>  '\n>\n> +test_expect_success 'checking out in a newly created repo' '\n> +       test_create_repo empty &&\n> +       (\n> +               cd empty &&\n> +               cat .git/HEAD >expect &&\n> +               test_must_fail git checkout &&\n> +               cat .git/HEAD >actual &&\n> +               test_cmp expect actual\n> +       )\n> +'\n> +\n>  test_done\n\nHmh, this is needlessly cluttery. The following should cover it:\n\n---\ndiff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\nindex 2fa9458..b49fedf 100755\n--- a/t/t2015-checkout-unborn.sh\n+++ b/t/t2015-checkout-unborn.sh\n@@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '\n \ttest_create_repo empty &&\n \t(\n \t\tcd empty &&\n-\t\tcat .git/HEAD >expect &&\n-\t\ttest_must_fail git checkout &&\n-\t\tcat .git/HEAD >actual &&\n-\t\ttest_cmp expect actual\n+\t\ttest_must_fail git checkout\n \t)\n '\n"},{"id":"191113","messageId":"7v4nrqy6ur.fsf@alter.siamese.dyndns.org","threadId":"30472","inReplyTo":"1336493114-4984-1-git-send-email-kusmabite@gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-08T16:29:00Z","receivedAt":"2012-05-08T16:29:00Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> In abe1998 (\"git checkout -b: allow switching out of an unborn\n> branch\"), a code-path overly-optimisticly assumed that a\n> branch-name was specified. This is not always the case, and as\n> a result a NULL-pointer was attempted printed to .git/HEAD.\n>\n> This could lead to at least two different failure modes:\n> 1) The CRT formated the NULL-string as something useful (e.g\n> \"(null)\")\n> 2) The CRT crasheed.\n\nJust a quick question. What does a cathode ray tube have to do with this\n(or \"are people expected to know what CRT you are talking about?\")?\n\nOtherwise the explanation, the patch and the added test makes sense.\n\nThanks.\n"},{"id":"191114","messageId":"CABPQNSb-YPAa6qPtxeo1k-p-0REnk3soOTuPgABj9+s61o80VA@mail.gmail.com","threadId":"30472","inReplyTo":"7v4nrqy6ur.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-05-08T16:35:10Z","receivedAt":"2012-05-08T16:35:10Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, May 8, 2012 at 6:29 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> In abe1998 (\"git checkout -b: allow switching out of an unborn\n>> branch\"), a code-path overly-optimisticly assumed that a\n>> branch-name was specified. This is not always the case, and as\n>> a result a NULL-pointer was attempted printed to .git/HEAD.\n>>\n>> This could lead to at least two different failure modes:\n>>  1) The CRT formated the NULL-string as something useful (e.g\n>>     \"(null)\")\n>>  2) The CRT crasheed.\n>\n> Just a quick question.  What does a cathode ray tube have to do with this\n> (or \"are people expected to know what CRT you are talking about?\")?\n>\n\nFeel free to change it to \"the C runtime\", or even \"the vsnprintf\nimplementation\" if you feel like it :)\n"},{"id":"191115","messageId":"7vwr4mwrq6.fsf@alter.siamese.dyndns.org","threadId":"30472","inReplyTo":"CABPQNSb-YPAa6qPtxeo1k-p-0REnk3soOTuPgABj9+s61o80VA@mail.gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-08T16:41:05Z","receivedAt":"2012-05-08T16:41:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n>> (or \"are people expected to know what CRT you are talking about?\")?\n>\n> Feel free to change it to \"the C runtime\", or even \"the vsnprintf\n> implementation\" if you feel like it :)\n\nAhh, \"C RunTime\" was what you meant. OK, it somehow immediately did not\n\"click\".\n"},{"id":"191117","messageId":"7vsjfawrmw.fsf@alter.siamese.dyndns.org","threadId":"30472","inReplyTo":"CABPQNSau5snkNLqsnBnsLVBVNKHXnvb8rsE8DbNfpo9__5uijA@mail.gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-08T16:43:03Z","receivedAt":"2012-05-08T16:43:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> Hmh, this is needlessly cluttery. The following should cover it:\n>\n> ---\n> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\n> index 2fa9458..b49fedf 100755\n> --- a/t/t2015-checkout-unborn.sh\n> +++ b/t/t2015-checkout-unborn.sh\n> @@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '\n> \ttest_create_repo empty &&\n> \t(\n> \t\tcd empty &&\n> -\t\tcat .git/HEAD >expect &&\n> -\t\ttest_must_fail git checkout &&\n> -\t\tcat .git/HEAD >actual &&\n> -\t\ttest_cmp expect actual\n> +\t\ttest_must_fail git checkout\n> \t)\n> '\n\nHrm, I am of two minds. Yes, we may want checkout to fail, but at the\nsame time, we would want to make sure that a failed checkout does not\ncorrupt the HEAD. Perhaps it would make it more palatable if you replaced\n\"cat .git/HEAD\" with \"git symbolic-ref HEAD\" in the original?\n"},{"id":"191118","messageId":"CABPQNSYLLSsfAumjk0_RGBPbbV4=vFWVFAn13ZLwq_vm01+M4Q@mail.gmail.com","threadId":"30472","inReplyTo":"7vwr4mwrq6.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-05-08T16:43:07Z","receivedAt":"2012-05-08T16:43:07Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, May 8, 2012 at 6:41 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>>> (or \"are people expected to know what CRT you are talking about?\")?\n>>\n>> Feel free to change it to \"the C runtime\", or even \"the vsnprintf\n>> implementation\" if you feel like it :)\n>\n> Ahh, \"C RunTime\" was what you meant.  OK, it somehow immediately did not\n> \"click\".\n\nIt's the name we Windows users tend to use for what you unix-guys tend\nto call \"libc\" ;)\n\nProbably because Microsoft Visual C names their MSVCRT.dll.\n"},{"id":"191120","messageId":"CABPQNSbyh8oMfz8GbDDn_bdLc8RpEt5sq7wxtVFh61ekD_j8yQ@mail.gmail.com","threadId":"30472","inReplyTo":"7vsjfawrmw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Erik Faye-Lund","fromEmail":"kusmabite@gmail.com","sentAt":"2012-05-08T16:52:11Z","receivedAt":"2012-05-08T16:52:11Z","isPatch":true,"sender":{"key":"kusmabite@gmail.com","avatar":"https://avatars.githubusercontent.com/u/47073?v=4"},"body":"On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano wrote:\n> Erik Faye-Lund writes:\n>\n>> Hmh, this is needlessly cluttery. The following should cover it:\n>>\n>> ---\n>> diff --git a/t/t2015-checkout-unborn.sh b/t/t2015-checkout-unborn.sh\n>> index 2fa9458..b49fedf 100755\n>> --- a/t/t2015-checkout-unborn.sh\n>> +++ b/t/t2015-checkout-unborn.sh\n>> @@ -50,10 +50,7 @@ test_expect_success 'checking out in a newly created repo' '\n>>       test_create_repo empty &&\n>>       (\n>>               cd empty &&\n>> -             cat .git/HEAD >expect &&\n>> -             test_must_fail git checkout &&\n>> -             cat .git/HEAD >actual &&\n>> -             test_cmp expect actual\n>> +             test_must_fail git checkout\n>>       )\n>>  '\n>\n> Hrm, I am of two minds.  Yes, we may want checkout to fail, but at the\n> same time, we would want to make sure that a failed checkout does not\n> corrupt the HEAD.\n\nGood point.\n\n>  Perhaps it would make it more palatable if you replaced\n> \"cat .git/HEAD\" with \"git symbolic-ref HEAD\" in the original?\n\nAh, yes. That's much better. Do you want me to resend (improving the\ntest and replacing \"CRT\" with \"vsnprintf\")? I also spotted a typo in\nthe commit message (\"crasheed\" vs \"crashed\")...\n"},{"id":"191123","messageId":"7v8vh2wq8u.fsf@alter.siamese.dyndns.org","threadId":"30472","inReplyTo":"CABPQNSbyh8oMfz8GbDDn_bdLc8RpEt5sq7wxtVFh61ekD_j8yQ@mail.gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-05-08T17:13:05Z","receivedAt":"2012-05-08T17:13:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Erik Faye-Lund writes:\n\n> On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano wrote:\n> ...\n>> Hrm, I am of two minds.  Yes, we may want checkout to fail, but at the\n>> same time, we would want to make sure that a failed checkout does not\n>> corrupt the HEAD.\n>\n> Good point.\n>\n>>  Perhaps it would make it more palatable if you replaced\n>> \"cat .git/HEAD\" with \"git symbolic-ref HEAD\" in the original?\n>\n> Ah, yes. That's much better. Do you want me to resend (improving the\n> test and replacing \"CRT\" with \"vsnprintf\")? I also spotted a typo in\n> the commit message (\"crasheed\" vs \"crashed\")...\n\nSurely.\n\nBy the way, notice I said \"we *may* want checkout to fail\"? With the\ndiscussion \"why is it wrong to allow 'git checkout' to be no-op in a\nfreshly created repository\" stemming from your other \"I do not claim it is\ncorrect but it makes the test pass\" message, we may actually want to make\nthat 'checkout' in the test pass, and then 'test_must_fail git checkout'\nin the test would have to go when that happens.\n"},{"id":"191126","messageId":"m2aa1ih91m.fsf@igel.home","threadId":"30472","inReplyTo":"CABPQNSYLLSsfAumjk0_RGBPbbV4=vFWVFAn13ZLwq_vm01+M4Q@mail.gmail.com","subject":"Re: [PATCH] checkout: do not corrupt HEAD on empty repo","fromName":"Andreas Schwab","fromEmail":"schwab@linux-m68k.org","sentAt":"2012-05-08T17:33:41Z","receivedAt":"2012-05-08T17:33:41Z","isPatch":true,"sender":{"key":"schwab@linux-m68k.org","avatar":"https://avatars.githubusercontent.com/u/2175493?v=4"},"body":"Erik Faye-Lund writes:\n\n> It's the name we Windows users tend to use for what you unix-guys tend\n> to call \"libc\" ;)\n\nI think it originated in Unix, viz /usr/lib/crt0.o.\n\nAndreas.\n\n-- \nAndreas Schwab, schwab@linux-m68k.org\nGPG Key fingerprint = 58CA 54C7 6D53 942B 1756 01D3 44D5 214B 8276 4ED5\n\"And now for something completely different.\"\n"}]}