# [PATCH] checkout: do not corrupt HEAD on empty repo 10 messages from 2012-05-08 to 2012-05-08. Participants: Erik Faye-Lund, Junio C Hamano, Andreas Schwab. Thread: https://gitlist.dev/t/30472 ## Erik Faye-Lund, 2012-05-08 16:05 Subject: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: <1336493114-4984-1-git-send-email-kusmabite@gmail.com> URL: https://gitlist.dev/e/1336493114-4984-1-git-send-email-kusmabite%40gmail.com ``` 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 --- 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 ``` ## Erik Faye-Lund, 2012-05-08 16:25 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: URL: https://gitlist.dev/e/CABPQNSau5snkNLqsnBnsLVBVNKHXnvb8rsE8DbNfpo9__5uijA%40mail.gmail.com In-Reply-To: <1336493114-4984-1-git-send-email-kusmabite@gmail.com> ``` On Tue, May 8, 2012 at 6:05 PM, Erik Faye-Lund 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 > --- >  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 ) ' ``` ## Junio C Hamano, 2012-05-08 16:29 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: <7v4nrqy6ur.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v4nrqy6ur.fsf%40alter.siamese.dyndns.org In-Reply-To: <1336493114-4984-1-git-send-email-kusmabite@gmail.com> ``` Erik Faye-Lund 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. ``` ## Erik Faye-Lund, 2012-05-08 16:35 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: URL: https://gitlist.dev/e/CABPQNSb-YPAa6qPtxeo1k-p-0REnk3soOTuPgABj9%2Bs61o80VA%40mail.gmail.com In-Reply-To: <7v4nrqy6ur.fsf@alter.siamese.dyndns.org> ``` On Tue, May 8, 2012 at 6:29 PM, Junio C Hamano wrote: > Erik Faye-Lund 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, 2012-05-08 16:41 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: <7vwr4mwrq6.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vwr4mwrq6.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Erik Faye-Lund 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". ``` ## Junio C Hamano, 2012-05-08 16:43 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: <7vsjfawrmw.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7vsjfawrmw.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Erik Faye-Lund 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? ``` ## Erik Faye-Lund, 2012-05-08 16:43 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: URL: https://gitlist.dev/e/CABPQNSYLLSsfAumjk0_RGBPbbV4%3DvFWVFAn13ZLwq_vm01%2BM4Q%40mail.gmail.com In-Reply-To: <7vwr4mwrq6.fsf@alter.siamese.dyndns.org> ``` On Tue, May 8, 2012 at 6:41 PM, Junio C Hamano wrote: > Erik Faye-Lund 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. ``` ## Erik Faye-Lund, 2012-05-08 16:52 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: URL: https://gitlist.dev/e/CABPQNSbyh8oMfz8GbDDn_bdLc8RpEt5sq7wxtVFh61ekD_j8yQ%40mail.gmail.com In-Reply-To: <7vsjfawrmw.fsf@alter.siamese.dyndns.org> ``` On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano wrote: > Erik Faye-Lund 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, 2012-05-08 17:13 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: <7v8vh2wq8u.fsf@alter.siamese.dyndns.org> URL: https://gitlist.dev/e/7v8vh2wq8u.fsf%40alter.siamese.dyndns.org In-Reply-To: ``` Erik Faye-Lund writes: > On Tue, May 8, 2012 at 6:43 PM, Junio C Hamano 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. ``` ## Andreas Schwab, 2012-05-08 17:33 Subject: Re: [PATCH] checkout: do not corrupt HEAD on empty repo Message-ID: URL: https://gitlist.dev/e/m2aa1ih91m.fsf%40igel.home In-Reply-To: ``` Erik Faye-Lund 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." ```