{"thread":{"id":"16211","subject":"[PATCH] checkout: Don't crash when switching away from an invalid branch.","startedAt":"2008-11-07T17:02:10Z","lastAt":"2008-11-08T10:07:25Z","messageCount":5,"participants":["Alexandre Julliard","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"95131","messageId":"871vxnbhbh.fsf@wine.dyndns.org","threadId":"16211","inReplyTo":null,"subject":"[PATCH] checkout: Don't crash when switching away from an invalid branch.","fromName":"Alexandre Julliard","fromEmail":"julliard@winehq.org","sentAt":"2008-11-07T17:02:10Z","receivedAt":"2008-11-07T17:02:10Z","isPatch":true,"sender":{"key":"julliard@winehq.org","avatar":null},"body":"I have a tree where for some reason HEAD was pointing to an invalid\ncommit. I'm not sure how this happened, but git checkout should be\nable to recover from that situation without crashing.\n\nSigned-off-by: Alexandre Julliard \n---\n builtin-checkout.c | 14 +++++++++-----\n t/t2011-checkout-invalid-head.sh | 18 ++++++++++++++++++\n 2 files changed, 27 insertions(+), 5 deletions(-)\n create mode 100755 t/t2011-checkout-invalid-head.sh\n\ndiff --git a/builtin-checkout.c b/builtin-checkout.c\nindex 57b94d2..7c1b8cd 100644\n--- a/builtin-checkout.c\n+++ b/builtin-checkout.c\n@@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n \n \tmemset(&proc, 0, sizeof(proc));\n \targv[0] = name;\n-\targv[1] = xstrdup(sha1_to_hex(old->object.sha1));\n+\targv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))\n+\t\t : \"0000000000000000000000000000000000000000\";\n \targv[2] = xstrdup(sha1_to_hex(new->object.sha1));\n \targv[3] = changed ? \"1\" : \"0\";\n \targv[4] = NULL;\n@@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n \t}\n \n \told_desc = old->name;\n-\tif (!old_desc)\n+\tif (!old_desc && old->commit)\n \t\told_desc = sha1_to_hex(old->commit->object.sha1);\n-\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n-\t\t old_desc, new->name);\n+\tif (old_desc)\n+\t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n+\t\t\t old_desc, new->name);\n+\telse\n+\t\tstrbuf_addf(&msg, \"checkout: moving to %s\", new->name);\n \n \tif (new->path) {\n \t\tcreate_symref(\"HEAD\", new->path, msg.buf);\n@@ -551,7 +555,7 @@ static int switch_branches(struct checkout_opts *opts, struct branch_info *new)\n \t * a new commit, we want to mention the old commit once more\n \t * to remind the user that it might be lost.\n \t */\n-\tif (!opts->quiet && !old.path && new->commit != old.commit)\n+\tif (!opts->quiet && !old.path && old.commit && new->commit != old.commit)\n \t\tdescribe_detached_head(\"Previous HEAD position was\", old.commit);\n \n \tif (!old.commit) {\ndiff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\nnew file mode 100755\nindex 0000000..764bb0a\n--- /dev/null\n+++ b/t/t2011-checkout-invalid-head.sh\n@@ -0,0 +1,18 @@\n+#!/bin/sh\n+\n+test_description='checkout switching away from an invalid branch'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup' '\n+\techo hello >world &&\n+\tgit add world &&\n+\tgit commit -m initial\n+'\n+\n+test_expect_success 'checkout master from invalid HEAD' '\n+\techo 0000000000000000000000000000000000000000 >.git/HEAD &&\n+\tgit checkout master --\n+'\n+\n+test_done\n-- \n1.6.0.3.669.g76740\n\n-- \nAlexandre Julliard\njulliard@winehq.org\n"},{"id":"95132","messageId":"alpine.DEB.1.00.0811071903300.30769@pacific.mpi-cbg.de","threadId":"16211","inReplyTo":"871vxnbhbh.fsf@wine.dyndns.org","subject":"Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-11-07T18:05:29Z","receivedAt":"2008-11-07T18:05:29Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Fri, 7 Nov 2008, Alexandre Julliard wrote:\n\n> I have a tree where for some reason HEAD was pointing to an invalid \n> commit. I'm not sure how this happened, but git checkout should be able \n> to recover from that situation without crashing.\n\nAgree.\n\n> diff --git a/builtin-checkout.c b/builtin-checkout.c\n> index 57b94d2..7c1b8cd 100644\n> --- a/builtin-checkout.c\n> +++ b/builtin-checkout.c\n> @@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n> \n> \tmemset(&proc, 0, sizeof(proc));\n> \targv[0] = name;\n> -\targv[1] = xstrdup(sha1_to_hex(old->object.sha1));\n> +\targv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))\n> +\t\t : \"0000000000000000000000000000000000000000\";\n\nI guess you want to use the variable null_sha1 here.\n\n> @@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n> \t}\n> \n> \told_desc = old->name;\n> -\tif (!old_desc)\n> +\tif (!old_desc && old->commit)\n> \t\told_desc = sha1_to_hex(old->commit->object.sha1);\n> -\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n> -\t\t old_desc, new->name);\n> +\tif (old_desc)\n> +\t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n> +\t\t\t old_desc, new->name);\n> +\telse\n> +\t\tstrbuf_addf(&msg, \"checkout: moving to %s\", new->name);\n\nWhy not\n\t\told_desc ? old_desc : \"(invalid)\"\n?\n\n> diff --git a/t/t2011-checkout-invalid-head.sh b/t/t2011-checkout-invalid-head.sh\n> new file mode 100755\n\nNice!\n\nThank you,\nDscho\n"},{"id":"95166","messageId":"87od0r9nnj.fsf@wine.dyndns.org","threadId":"16211","inReplyTo":"alpine.DEB.1.00.0811071903300.30769@pacific.mpi-cbg.de","subject":"Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.","fromName":"Alexandre Julliard","fromEmail":"julliard@winehq.org","sentAt":"2008-11-07T22:28:16Z","receivedAt":"2008-11-07T22:28:16Z","isPatch":true,"sender":{"key":"julliard@winehq.org","avatar":null},"body":"Johannes Schindelin writes:\n\n>> diff --git a/builtin-checkout.c b/builtin-checkout.c\n>> index 57b94d2..7c1b8cd 100644\n>> --- a/builtin-checkout.c\n>> +++ b/builtin-checkout.c\n>> @@ -47,7 +47,8 @@ static int post_checkout_hook(struct commit *old, struct commit *new,\n>> \n>> \tmemset(&proc, 0, sizeof(proc));\n>> \targv[0] = name;\n>> -\targv[1] = xstrdup(sha1_to_hex(old->object.sha1));\n>> +\targv[1] = old ? xstrdup(sha1_to_hex(old->object.sha1))\n>> +\t\t : \"0000000000000000000000000000000000000000\";\n>\n> I guess you want to use the variable null_sha1 here.\n\nI could, though it seemed to me a bit silly to format and strdup a\nstring that is a known constant. But I'm happy to change it if needed.\n\n>> @@ -492,10 +493,13 @@ static void update_refs_for_switch(struct checkout_opts *opts,\n>> \t}\n>> \n>> \told_desc = old->name;\n>> -\tif (!old_desc)\n>> +\tif (!old_desc && old->commit)\n>> \t\told_desc = sha1_to_hex(old->commit->object.sha1);\n>> -\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n>> -\t\t old_desc, new->name);\n>> +\tif (old_desc)\n>> +\t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n>> +\t\t\t old_desc, new->name);\n>> +\telse\n>> +\t\tstrbuf_addf(&msg, \"checkout: moving to %s\", new->name);\n>\n> Why not\n> \t\told_desc ? old_desc : \"(invalid)\"\n> ?\n\nIMO it looks more friendly to not display a branch that doesn't exist,\nrather than printing something like (invalid) or (null).\n\n-- \nAlexandre Julliard\njulliard@winehq.org\n"},{"id":"95172","messageId":"7vbpwrf85x.fsf@gitster.siamese.dyndns.org","threadId":"16211","inReplyTo":"87od0r9nnj.fsf@wine.dyndns.org","subject":"Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-11-07T23:06:18Z","receivedAt":"2008-11-07T23:06:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Alexandre Julliard writes:\n\n> Johannes Schindelin writes:\n> ...\n>> Why not\n>> \t\told_desc ? old_desc : \"(invalid)\"\n>> ?\n>\n> IMO it looks more friendly to not display a branch that doesn't exist,\n> rather than printing something like (invalid) or (null).\n\nActually I think it is a good idea to remind that you were in a funny\nstate.\n\nFor that matter, dying without removing the trace of that funny state\nmight be even preferrable if you need to do postmortem to figure out why\nyou got into such a funny state to begin with, but not everybody uses git\nto debug git. I think Dscho's suggestion is a reasonable middle ground.\n"},{"id":"95205","messageId":"87k5bea5uq.fsf@wine.dyndns.org","threadId":"16211","inReplyTo":"7vbpwrf85x.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH] checkout: Don't crash when switching away from an invalid branch.","fromName":"Alexandre Julliard","fromEmail":"julliard@winehq.org","sentAt":"2008-11-08T10:07:25Z","receivedAt":"2008-11-08T10:07:25Z","isPatch":true,"sender":{"key":"julliard@winehq.org","avatar":null},"body":"Junio C Hamano writes:\n\n> For that matter, dying without removing the trace of that funny state\n> might be even preferrable if you need to do postmortem to figure out why\n> you got into such a funny state to begin with, but not everybody uses git\n> to debug git.\n\nIt turns out to be user error, that was a tree I hadn't used in a long\ntime and I didn't realize it was using alternates, so HEAD was pointing\nto a commit that had been rebased and garbage-collected in the source\nrepository.\n\nMost other commands die with a \"bad object HEAD\" in that situation, and\ncheckout could certainly do that too, but I think it's nicer to provide\nan easy way of getting out of that broken state. I'll resend an updated\npatch.\n\n-- \nAlexandre Julliard\njulliard@winehq.org\n"}]}