{"thread":{"id":"34756","subject":"[PATCH] write_index: optionally allow broken null sha1s","startedAt":"2013-08-24T01:33:11Z","lastAt":"2013-08-27T20:41:12Z","messageCount":13,"participants":["Jeff King","Jonathan Nieder","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"225765","messageId":"20130824013310.GA9343@sigill.intra.peff.net","threadId":"34756","inReplyTo":null,"subject":"[PATCH] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-24T01:33:11Z","receivedAt":"2013-08-24T01:33:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Commit 4337b58 (do not write null sha1s to on-disk index,\n2012-07-28) unconditionally prevents git from writing null\nsha1s into the index. The intent was to catch errors in\nother parts of the code that might let such an entry slip\ninto the index (or worse, a tree).\n\nHowever, some repositories have history in which the trees\ncontain entries with null sha1s. Because the restriction is\nunconditional, it can be difficult to work with these older\nhistories (for example, you cannot even check out the\nhistory, because you are forbidden to write the broken\nindex). Worst of all, you cannot use git-filter-branch's\nindex-filter to repair such a broken history, because\nfilter-branch will try to write an index for each tree.\n\nWe could potentially work around this by using a\ncommit-filter, and munging the tree manually. However,\nfilter-branch unconditionally writes the index, even if we\nonly asked for a commit-filter, so this doesn't work. That\nwould be possible to fix, but figuring out that a\ncommit-filter is needed (and what it should contain) is\nsomewhat non-intuitive.\n\nInstead, let's introduce an environment variable for\nrelaxing this check, and use it when checking out the index\nin filter-branch. We turn it on by default, so users do not\nhave to learn any special tricks to perform such a\nfilter-branch. But we only do so for the index check-out, so\nany further manipulations the user does will fail unless\nthey remove the broken entry.\n\nWe cannot catch every case, though, since some filter-branch\ninvocations might not rewrite the index at all. But we still\nprint a loud warning, so it is unlikely to go unnoticed by\nthe user.\n\nSigned-off-by: Jeff King \n---\nI was tempted to not involve filter-branch in this commit at all, and\ninstead require the user to manually invoke\n\n GIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n\nto perform such a filter. That would be slightly safer, but requires\nsome specialized knowledge from the user (and advice on using\nfilter-branch to remove such entries already exists on places like\nstackoverflow, and this patch makes it Just Work on recent versions of\ngit).\n\n git-filter-branch.sh | 5 ++--\n read-cache.c | 13 +++++++--\n t/t7009-filter-branch-null-sha1.sh | 54 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 68 insertions(+), 4 deletions(-)\n create mode 100755 t/t7009-filter-branch-null-sha1.sh\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex ac2a005..98e8fe4 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -283,11 +283,12 @@ while read commit parents; do\n \n \tcase \"$filter_subdir\" in\n \t\"\")\n-\t\tgit read-tree -i -m $commit\n+\t\tGIT_ALLOW_NULL_SHA1=1 git read-tree -i -m $commit\n \t\t;;\n \t*)\n \t\t# The commit may not have the subdirectory at all\n-\t\terr=$(git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n+\t\terr=$(GIT_ALLOW_NULL_SHA1=1 \\\n+\t\t git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n \t\t\tif ! git rev-parse -q --verify $commit:\"$filter_subdir\"\n \t\t\tthen\n \t\t\t\trm -f \"$GIT_INDEX_FILE\"\ndiff --git a/read-cache.c b/read-cache.c\nindex c3d5e35..83a7414 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,8 +1817,17 @@ int write_index(struct index_state *istate, int newfd)\n \t\t\tcontinue;\n \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n \t\t\tce_smudge_racily_clean_entry(ce);\n-\t\tif (is_null_sha1(ce->sha1))\n-\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n+\t\tif (is_null_sha1(ce->sha1)) {\n+\t\t\tstatic const char msg[] = \"cache entry has null sha1: %s\";\n+\t\t\tstatic int allow = -1;\n+\n+\t\t\tif (allow < 0)\n+\t\t\t\tallow = git_env_bool(\"GIT_ALLOW_NULL_SHA1\", 0);\n+\t\t\tif (allow)\n+\t\t\t\twarning(msg, ce->name);\n+\t\t\telse\n+\t\t\t\treturn error(msg, ce->name);\n+\t\t}\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n \t}\ndiff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh\nnew file mode 100755\nindex 0000000..2944ac9\n--- /dev/null\n+++ b/t/t7009-filter-branch-null-sha1.sh\n@@ -0,0 +1,54 @@\n+#!/bin/sh\n+\n+test_description='filter-branch removal of trees with null sha1'\n+. ./test-lib.sh\n+\n+test_expect_success 'create base commits' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'create a commit with a bogus null sha1 in the tree' '\n+\ttest_tick &&\n+\ttree=$(\n+\t\t{\n+\t\t\tgit ls-tree HEAD &&\n+\t\t\tprintf \"160000 commit $_z40\\\\tbroken\"\n+\t\t} | git mktree\n+\t) &&\n+\tcommit=$(\n+\t\techo \"add broken entry\" |\n+\t\tgit commit-tree $tree -p HEAD\n+\t) &&\n+\tgit update-ref HEAD $commit\n+'\n+\n+# we have to make one more commit on top removing the broken\n+# entry, since otherwise our index does not match HEAD (and filter-branch will\n+# complain). We could make the index match HEAD, but doing so would involve\n+# writing a null sha1 into the index.\n+test_expect_success 'create a commit dropping the broken entry' '\n+\ttest_tick &&\n+\tgit commit -a -m \"back to normal\"\n+'\n+\n+test_expect_success 'filter commands are still checked' '\n+\ttest_must_fail git filter-branch \\\n+\t\t--force --prune-empty \\\n+\t\t--index-filter \"git rm --cached --ignore-unmatch three.t\"\n+'\n+\n+test_expect_success 'removing the broken entry works' '\n+\tgit filter-branch \\\n+\t\t--force --prune-empty \\\n+\t\t--index-filter \"git rm --cached --ignore-unmatch broken\"\n+'\n+\n+test_expect_success 'resulting history is clean' '\n+\techo three >expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.4.rc2.28.g6bb5f3f\n"},{"id":"225836","messageId":"20130825061500.GR2882@elie.Belkin","threadId":"34756","inReplyTo":"20130824013310.GA9343@sigill.intra.peff.net","subject":"Re: [PATCH] write_index: optionally allow broken null sha1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-25T06:15:00Z","receivedAt":"2013-08-25T06:15:00Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nJeff King wrote:\n\n> ---\n> I was tempted to not involve filter-branch in this commit at all, and\n> instead require the user to manually invoke\n>\n> GIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n>\n> to perform such a filter. That would be slightly safer, but requires\n> some specialized knowledge from the user (and advice on using\n> filter-branch to remove such entries already exists on places like\n> stackoverflow, and this patch makes it Just Work on recent versions of\n> git).\n\nThe above few paragraphs explained the most mysterious part of the\npatch to me. I think they would be fine to include in the commit\nmessage.\n\n[...]\n> --- a/read-cache.c\n> +++ b/read-cache.c\n> @@ -1817,8 +1817,17 @@ int write_index(struct index_state *istate, int newfd)\n> \t\t\tcontinue;\n> \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n> \t\t\tce_smudge_racily_clean_entry(ce);\n> -\t\tif (is_null_sha1(ce->sha1))\n> -\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n> +\t\tif (is_null_sha1(ce->sha1)) {\n> +\t\t\tstatic const char msg[] = \"cache entry has null sha1: %s\";\n> +\t\t\tstatic int allow = -1;\n> +\n> +\t\t\tif (allow < 0)\n> +\t\t\t\tallow = git_env_bool(\"GIT_ALLOW_NULL_SHA1\", 0);\n> +\t\t\tif (allow)\n> +\t\t\t\twarning(msg, ce->name);\n> +\t\t\telse\n> +\t\t\t\treturn error(msg, ce->name);\n> +\t\t}\n\nMakes sense.\n\n[...]\n> --- /dev/null\n> +++ b/t/t7009-filter-branch-null-sha1.sh\n> @@ -0,0 +1,54 @@\n> +#!/bin/sh\n> +\n> +test_description='filter-branch removal of trees with null sha1'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'create base commits' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three\n> +'\n> +\n> +test_expect_success 'create a commit with a bogus null sha1 in the tree' '\n> +\ttest_tick &&\n> +\ttree=$(\n> +\t\t{\n> +\t\t\tgit ls-tree HEAD &&\n> +\t\t\tprintf \"160000 commit $_z40\\\\tbroken\"\n> +\t\t} | git mktree\n> +\t) &&\n\nTo avoid pipes involving git commands, since they can losing the exit\nstatus (and hence information about whether git crashed):\n\n\t{\n\t\tgit ls-tree HEAD &&\n\t\techo \"160000 commit $_z40\tbroken\"\n\t} >listing &&\n\techo \"add broken entry\" >msg &&\n\n\ttree=$(git mktree > I was tempted to not involve filter-branch in this commit at all, and\n> > instead require the user to manually invoke\n> >\n> > GIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n> >\n> > to perform such a filter. That would be slightly safer, but requires\n> > some specialized knowledge from the user (and advice on using\n> > filter-branch to remove such entries already exists on places like\n> > stackoverflow, and this patch makes it Just Work on recent versions of\n> > git).\n> \n> The above few paragraphs explained the most mysterious part of the\n> patch to me. I think they would be fine to include in the commit\n> message.\n\nI rolled them into the commit message.\n\n> To avoid pipes involving git commands, since they can losing the exit\n> status (and hence information about whether git crashed):\n> [...]\n\nI didn't bother, as we do not expect a simple ls-tree to fail (and we\nwould notice anyway as we check that we have later introduced a broken\ntree state). Still, the resulting code is not any harder to read (which\nwas my main hesitation), so let's do it as you suggest.\n\n-- >8 --\nSubject: write_index: optionally allow broken null sha1s\n\nCommit 4337b58 (do not write null sha1s to on-disk index,\n2012-07-28) unconditionally prevents git from writing null\nsha1s into the index. The intent was to catch errors in\nother parts of the code that might let such an entry slip\ninto the index (or worse, a tree).\n\nHowever, some repositories have history in which the trees\ncontain entries with null sha1s. Because the restriction is\nunconditional, it can be difficult to work with these older\nhistories (for example, you cannot even check out the\nhistory, because you are forbidden to write the broken\nindex). Worst of all, you cannot use git-filter-branch's\nindex-filter to repair such a broken history, because\nfilter-branch will try to write an index for each tree.\n\nWe could potentially work around this by using a\ncommit-filter, and munging the tree manually. However,\nfilter-branch unconditionally writes the index, even if we\nonly asked for a commit-filter, so this doesn't work. That\nwould be possible to fix, but figuring out that a\ncommit-filter is needed (and what it should contain) is\nsomewhat non-intuitive.\n\nInstead, let's introduce an environment variable for\nrelaxing this check. We could stop there, and require that\nusers rewriting such broken history run:\n\n GIT_ALLOW_NULL_SHA=1 git filter-branch ...\n\nthemselves. This is the safest option, as it means\nfilter-branch will never successfully run on history that\ncontains null sha1s. However, it is also inconvenient for\nthe user, as they need to figure out the magic \"ALLOW_SHA1\"\nvariable.\n\nInstead, let's have filter-branch turn on the variable\nautomatically, but only when checkout out the to-be-filtered\nindex. This means that further filter commands that touch\nthe index will still notice and fail, unless they actually\nremove the broken entry.\n\nWe would still succeed on a filter-branch whose filters do\nnot touch the index at all (since we complain of the null\nsha1 only on writing, not when making a tree out of the\nindex). This is acceptable, though, as we still print a loud\nwarning (in addition to warning of the problem via fsck), so\nthe problem is unlikely to go unnoticed.\n\nSigned-off-by: Jeff King \n---\n git-filter-branch.sh | 5 ++--\n read-cache.c | 13 ++++++++--\n t/t7009-filter-branch-null-sha1.sh | 52 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 66 insertions(+), 4 deletions(-)\n create mode 100755 t/t7009-filter-branch-null-sha1.sh\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex ac2a005..98e8fe4 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -283,11 +283,12 @@ while read commit parents; do\n \n \tcase \"$filter_subdir\" in\n \t\"\")\n-\t\tgit read-tree -i -m $commit\n+\t\tGIT_ALLOW_NULL_SHA1=1 git read-tree -i -m $commit\n \t\t;;\n \t*)\n \t\t# The commit may not have the subdirectory at all\n-\t\terr=$(git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n+\t\terr=$(GIT_ALLOW_NULL_SHA1=1 \\\n+\t\t git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n \t\t\tif ! git rev-parse -q --verify $commit:\"$filter_subdir\"\n \t\t\tthen\n \t\t\t\trm -f \"$GIT_INDEX_FILE\"\ndiff --git a/read-cache.c b/read-cache.c\nindex c3d5e35..83a7414 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,8 +1817,17 @@ int write_index(struct index_state *istate, int newfd)\n \t\t\tcontinue;\n \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n \t\t\tce_smudge_racily_clean_entry(ce);\n-\t\tif (is_null_sha1(ce->sha1))\n-\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n+\t\tif (is_null_sha1(ce->sha1)) {\n+\t\t\tstatic const char msg[] = \"cache entry has null sha1: %s\";\n+\t\t\tstatic int allow = -1;\n+\n+\t\t\tif (allow < 0)\n+\t\t\t\tallow = git_env_bool(\"GIT_ALLOW_NULL_SHA1\", 0);\n+\t\t\tif (allow)\n+\t\t\t\twarning(msg, ce->name);\n+\t\t\telse\n+\t\t\t\treturn error(msg, ce->name);\n+\t\t}\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n \t}\ndiff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh\nnew file mode 100755\nindex 0000000..50d5689\n--- /dev/null\n+++ b/t/t7009-filter-branch-null-sha1.sh\n@@ -0,0 +1,52 @@\n+#!/bin/sh\n+\n+test_description='filter-branch removal of trees with null sha1'\n+. ./test-lib.sh\n+\n+test_expect_success 'create base commits' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'create a commit with a bogus null sha1 in the tree' '\n+\t{\n+\t\tgit ls-tree HEAD &&\n+\t\tprintf \"160000 commit $_z40\\\\tbroken\\\\n\"\n+\t} >broken-tree\n+\techo \"add broken entry\" >msg &&\n+\n+\ttree=$(git mktree expect &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.4.2.g87d4a77\n"},{"id":"225871","messageId":"20130825195412.GA2752@elie.Belkin","threadId":"34756","inReplyTo":"20130825095818.GA12556@sigill.intra.peff.net","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-25T19:54:12Z","receivedAt":"2013-08-25T19:54:12Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"On Sun, Aug 25, 2013 at 05:58:18AM -0400, Jeff King wrote:\n> On Sat, Aug 24, 2013 at 11:15:00PM -0700, Jonathan Nieder wrote:\n\n>>> I was tempted to not involve filter-branch in this commit at all, and\n>>> instead require the user to manually invoke\n>>>\n>>> GIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n>>>\n>>> to perform such a filter. That would be slightly safer, but requires\n>>> some specialized knowledge from the user (and advice on using\n>>> filter-branch to remove such entries already exists on places like\n>>> stackoverflow, and this patch makes it Just Work on recent versions of\n>>> git).\n>>\n>> The above few paragraphs explained the most mysterious part of the\n>> patch to me. I think they would be fine to include in the commit\n>> message.\n>\n> I rolled them into the commit message.\n\nHm --- the most useful part was \"advice on using filter-branch to\nremove such entries already exists on places like stackoverflow\",\nwhich was dropped. From my point of view, that is exactly the\nmotivation of the patch.\n\nI also found the \"I was tempted to ... That would be slightly safer,\nbut requires ...\" structure easier to read.\n\nIn other words, why not use something like this?\n\n\twrite_index: optionally allow broken null sha1s\n\n\tCommit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)\n\tadded a safety check preventing git from writing null sha1s into the\n\tindex. The intent was to catch errors in other parts of the code that\n\tmight let such an entry slip into the index (or worse, a tree).\n\n\tSome existing repositories have some invalid trees that contain null\n\tsha1s already, though. Until 4337b58, a common way to clean this up\n\twould be to use git-filter-branch's index-filter to repair such broken\n\tentries. That now fails when filter-branch tries to write out the\n\tindex.\n\n\tIntroduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check\n\tand make it easier to recover from such a history.\n\n\tIt is tempting to not involve filter-branch in this commit at all, and\n\tinstead require the user to manually invoke\n\n\t\tGIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n\n\tto perform an index-filter on a history with trees with null sha1s.\n\tThat would be slightly safer, but requires some specialized knowledge\n\tfrom the user. So let's set the GIT_ALLOW_NULL_SHA1 variable\n\tautomatically when checking out the to-be-filtered trees. Advice on\n\tusing filter-branch to remove such entries already exists on places like\n\tstackoverflow, and this patch makes it Just Work again on recent\n\tversions of git.\n\n\tFurther commands that touch the index will still notice and fail,\tunless\n\tthey actually remove the broken entries. A filter-branch whose filters\n\tdo not touch the index at all will not error out (since we complain of\n\tthe null sha1 only on writing, not when making a tree out of the index),\n\tbut this is acceptable, as we still print a loud warning, so the problem\n\tis unlikely to go unnoticed.\n\nWith or without such a change,\nReviewed-by: Jonathan Nieder \n\nAfter this patch, do you think (in a separate change) it would make\nsense for cache-tree.c::update_one() to check for null sha1 and error\nout unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of\nthe caveat from the last paragraph.\n\n[...]\n> --- /dev/null\n> +++ b/t/t7009-filter-branch-null-sha1.sh\n> @@ -0,0 +1,52 @@\n> +#!/bin/sh\n> +\n> +test_description='filter-branch removal of trees with null sha1'\n> +. ./test-lib.sh\n> +\n> +test_expect_success 'create base commits' '\n> +\ttest_commit one &&\n> +\ttest_commit two &&\n> +\ttest_commit three\n> +'\n> +\n> +test_expect_success 'create a commit with a bogus null sha1 in the tree' '\n> +\t{\n> +\t\tgit ls-tree HEAD &&\n> +\t\tprintf \"160000 commit $_z40\\\\tbroken\\\\n\"\n> +\t} >broken-tree\n> +\techo \"add broken entry\" >msg &&\n> +\n> +\ttree=$(git mktree +\ttest_tick &&\n> +\tcommit=$(git commit-tree $tree -p HEAD +\tgit update-ref HEAD \"$commit\"\n> +'\n> +\n> +# we have to make one more commit on top removing the broken\n> +# entry, since otherwise our index does not match HEAD (and filter-branch will\n> +# complain). We could make the index match HEAD, but doing so would involve\n> +# writing a null sha1 into the index.\n> +test_expect_success 'create a commit dropping the broken entry' '\n> +\ttest_tick &&\n> +\tgit commit -a -m \"back to normal\"\n> +'\n\nThis is kind of an old-fashioned test, since each step of the setup is\ntreated as a separate test assertion. I don't really mind until we\nget better automation to make it easy to skip or rearrange tests.\nJust for reference, I think the usual way to do this now is\n\n\ttest_expect_success 'setup' '\n\t\t# create base commits\n\t\t...\n\n\t\t# create a commit with bogus null sha1 in the tree\n\t\t...\n\n\t\t# We have to make one more commit on top removing the broken\n\t\t# entry, since otherwise our index does not match HEAD and\n\t\t# filter-branch will complain. We could make the index match\n\t\t# HEAD, but doing so would involve writing a null sha1 into\n\t\t# the index.\n\t\t...\n\t'\n\n\ttest_expect_success 'nontrivial filter-branch bails out on null sha1' '\n\t\told_head=$(git rev-parse HEAD) &&\n\t\ttest_must_fail git filter-branch ... &&\n\t\ttest_cmp_rev \"$old_head\" HEAD\n\t'\n\n\ttest_expect_success 'filter-branch can filter out null sha1' '\n\t\tgit filter-branch ... &&\n\n\t\t# resulting history is clean\n\t\techo three >expect &&\n\t\tgit log -1 --format=%s >actual &&\n\t\ttest_cmp expect actual\n\t'\n\nThanks,\nJonathan\n"},{"id":"225881","messageId":"xmqqob8lj8dx.fsf@gitster.dls.corp.google.com","threadId":"34756","inReplyTo":"20130825195412.GA2752@elie.Belkin","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-26T06:03:54Z","receivedAt":"2013-08-26T06:03:54Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jonathan Nieder writes:\n\n> In other words, why not use something like this?\n>\n> \twrite_index: optionally allow broken null sha1s\n>\n> \tCommit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)\n> \tadded a safety check preventing git from writing null sha1s into the\n> \tindex. The intent was to catch errors in other parts of the code that\n> \tmight let such an entry slip into the index (or worse, a tree).\n>\n> \tSome existing repositories have some invalid trees that contain null\n> \tsha1s already, though. Until 4337b58, a common way to clean this up\n> \twould be to use git-filter-branch's index-filter to repair such broken\n> \tentries. That now fails when filter-branch tries to write out the\n> \tindex.\n>\n> \tIntroduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check\n> \tand make it easier to recover from such a history.\n\nI found this version more readable than Peff's (albeit slightly).\n\n> After this patch, do you think (in a separate change) it would make\n> sense for cache-tree.c::update_one() to check for null sha1 and error\n> out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of\n> the caveat from the last paragraph.\n\nHmm, interesting thought.\n"},{"id":"225900","messageId":"20130826142704.GA14858@sigill.intra.peff.net","threadId":"34756","inReplyTo":"20130825195412.GA2752@elie.Belkin","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-26T14:27:04Z","receivedAt":"2013-08-26T14:27:04Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2013 at 12:54:12PM -0700, Jonathan Nieder wrote:\n\n> [setup split across three tests]\n>\n> This is kind of an old-fashioned test, since each step of the setup is\n> treated as a separate test assertion. I don't really mind until we\n> get better automation to make it easy to skip or rearrange tests.\n> Just for reference, I think the usual way to do this now is\n\nI don't see that splitting it up more hurts this. If we wanted more\nautomatic rearranging or skipping of tests, we would need tests to\ndeclare dependencies on their setup. And we would need to be able to\ndeclare dependencies on multiple tests, since having a single setup test\ndoes not work in all cases (e.g., sometimes you are testing each step,\nand the final step relies on earlier steps).\n\nAnd I do think splitting it up has more granularity. For example, in the\nsequence here:\n\n> \ttest_expect_success 'setup' '\n> \t\t# create base commits\n> \t\t...\n> \n> \t\t# create a commit with bogus null sha1 in the tree\n> \t\t...\n> \n> \t\t# We have to make one more commit on top removing the broken\n> \t\t# entry, since otherwise our index does not match HEAD and\n> \t\t# filter-branch will complain. We could make the index match\n> \t\t# HEAD, but doing so would involve writing a null sha1 into\n> \t\t# the index.\n> \t\t...\n> \t'\n\nRight now it is not hard to do step 2. But I could easily imagine a\nworld in which git-mktree learns to complain about such bogus trees. And\nthe sequence would become:\n\n 1. create base commits\n\n 2. check that mktree barfs\n\n 3. check that we can override mktree to create a broken tree\n\n 4. clean up tip state\n\nin which case you really want to have individual tests.\n\nI do not care _that_ much, since mktree does not behave that way now,\nand the setup is otherwise not that likely to fail. But I do not think\nmore granularity actually hurts us, and it can sometimes help (as\ndescribed above, but also when something fails, the test output more\nclearly pinpoints what happened).\n\n-Peff\n"},{"id":"225901","messageId":"20130826143135.GB14858@sigill.intra.peff.net","threadId":"34756","inReplyTo":"xmqqob8lj8dx.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-26T14:31:35Z","receivedAt":"2013-08-26T14:31:35Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Aug 25, 2013 at 11:03:54PM -0700, Junio C Hamano wrote:\n\n> Jonathan Nieder writes:\n> \n> > In other words, why not use something like this?\n> >\n> > \twrite_index: optionally allow broken null sha1s\n> >\n> > \tCommit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)\n> > \tadded a safety check preventing git from writing null sha1s into the\n> > \tindex. The intent was to catch errors in other parts of the code that\n> > \tmight let such an entry slip into the index (or worse, a tree).\n> >\n> > \tSome existing repositories have some invalid trees that contain null\n> > \tsha1s already, though. Until 4337b58, a common way to clean this up\n> > \twould be to use git-filter-branch's index-filter to repair such broken\n> > \tentries. That now fails when filter-branch tries to write out the\n> > \tindex.\n> >\n> > \tIntroduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check\n> > \tand make it easier to recover from such a history.\n> \n> I found this version more readable than Peff's (albeit slightly).\n\nOK. Do you want to apply with Jonathan's wording, then?\n\nThere's one subtle thing I didn't mention in the \"it is already on stack\noverflow\". If you have a version of git which complains about the null\nsha1, then the SO advice is already broken. But if the SO works, then\nyou do not have a version of git which complains. So why do you care?\n\nAnd the answer is: you may be pushing to a remote with a version of git\nthat complains, and which has receive.fsckObjects set (and in many\ncases, that remote is GitHub, since we have had that check on for a\nwhile).\n\nI don't know if it is worth spelling that out or not.\n\n> > After this patch, do you think (in a separate change) it would make\n> > sense for cache-tree.c::update_one() to check for null sha1 and error\n> > out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of\n> > the caveat from the last paragraph.\n> \n> Hmm, interesting thought.\n\nI think it is worth doing. The main reason I put the original check on\nwriting to the index is that it more clearly pinpoints the source of the\nerror. If we just died during git-write-tree, then you know somebody\nbroke your index, but you don't know which command.\n\nBut checking in both places would add extra protection, and would make\npossible the \"relax on read, strict on write\" policy that filter-branch\nwants to do.\n\n-Peff\n"},{"id":"225903","messageId":"xmqq38pwjv89.fsf@gitster.dls.corp.google.com","threadId":"34756","inReplyTo":"20130826143135.GB14858@sigill.intra.peff.net","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-26T16:02:46Z","receivedAt":"2013-08-26T16:02:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> I found this version more readable than Peff's (albeit slightly).\n>\n> OK. Do you want to apply with Jonathan's wording, then?\n\nI can do that, as it seems all of us are in agreement.\n\n> There's one subtle thing I didn't mention in the \"it is already on stack\n> overflow\". If you have a version of git which complains about the null\n> sha1, then the SO advice is already broken. But if the SO works, then\n> you do not have a version of git which complains. So why do you care?\n>\n> And the answer is: you may be pushing to a remote with a version of git\n> that complains, and which has receive.fsckObjects set (and in many\n> cases, that remote is GitHub, since we have had that check on for a\n> while).\n>\n> I don't know if it is worth spelling that out or not.\n\nProbably not.\n\nYou could aim to correct each and every wrong suggestions on a site\nwhere misguided leads other misguided, but it is a hopeless task.\n\n>> > After this patch, do you think (in a separate change) it would make\n>> > sense for cache-tree.c::update_one() to check for null sha1 and error\n>> > out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of\n>> > the caveat from the last paragraph.\n>> \n>> Hmm, interesting thought.\n>\n> I think it is worth doing. The main reason I put the original check on\n> writing to the index is that it more clearly pinpoints the source of the\n> error. If we just died during git-write-tree, then you know somebody\n> broke your index, but you don't know which command.\n>\n> But checking in both places would add extra protection, and would make\n> possible the \"relax on read, strict on write\" policy that filter-branch\n> wants to do.\n\nYeah, I agree with all of the above.\n"},{"id":"225917","messageId":"20130826173501.GS4110@google.com","threadId":"34756","inReplyTo":"20130826142704.GA14858@sigill.intra.peff.net","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2013-08-26T17:35:01Z","receivedAt":"2013-08-26T17:35:01Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Sun, Aug 25, 2013 at 12:54:12PM -0700, Jonathan Nieder wrote:\n\n>> [setup split across three tests]\n>>\n>> This is kind of an old-fashioned test, since each step of the setup is\n>> treated as a separate test assertion. I don't really mind until we\n>> get better automation to make it easy to skip or rearrange tests.\n>> Just for reference, I think the usual way to do this now is\n>\n> I don't see that splitting it up more hurts this. If we wanted more\n> automatic rearranging or skipping of tests, we would need tests to\n> declare dependencies on their setup. And we would need to be able to\n> declare dependencies on multiple tests, since having a single setup test\n> does not work in all cases (e.g., sometimes you are testing each step,\n> and the final step relies on earlier steps).\n\nActually dependencies can already be inferred for most test scripts\nusing the following rule:\n\n\tEach test depends on all tests with the word \"setup\" or the\n\tphrase \"set up\" in their title that precede it.\n\nAnd while there's no existing automation for testing that that's all\nthe tests rely on (by automatically skipping or reordering tests, or\nrunning non-setup tests in separate sandboxes in parallel), in\npractice it is still already useful since it makes it safe to use\nGIT_SKIP_TESTS subject to the following constraint:\n\n\tIn each test script, for every setup test skipped, all later\n\ttests are skipped as well.\n\nI don't care as much about GIT_SKIP_TESTS as about being able to\nintroduce new tests in the middle of a file.\n\nOf course splitting up the setup into 3 steps neither helps nor hurts\nthat. What I was complaining about is splitting the filter-branch\nfrom the verification that filter-branch had the right result.\n\nSorry for the confusion,\nJonathan\n"},{"id":"225939","messageId":"20130826212053.GA25452@sigill.intra.peff.net","threadId":"34756","inReplyTo":"20130826173501.GS4110@google.com","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-26T21:20:53Z","receivedAt":"2013-08-26T21:20:53Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 26, 2013 at 10:35:01AM -0700, Jonathan Nieder wrote:\n\n> > I don't see that splitting it up more hurts this. If we wanted more\n> > automatic rearranging or skipping of tests, we would need tests to\n> > declare dependencies on their setup. And we would need to be able to\n> > declare dependencies on multiple tests, since having a single setup test\n> > does not work in all cases (e.g., sometimes you are testing each step,\n> > and the final step relies on earlier steps).\n> \n> Actually dependencies can already be inferred for most test scripts\n> using the following rule:\n> \n> \tEach test depends on all tests with the word \"setup\" or the\n> \tphrase \"set up\" in their title that precede it.\n\nI'd be very surprised if this works in practice on most of our current\ntest scripts. There are often subtle dependencies on the state left over\nfrom previous tests. Running the script below up through t3800 (at which\npoint I lost patience) reveals 37 test scripts that are broken. Which is\nonly about 17%, but we're clearly not quite there yet.\n\nBut sure, I agree that is something to strive for. But I think my point\nstands; splitting up the setup doesn't hurt as long as you note all of\nthe tests as dependencies. And by your rule above, the advice would be\n\"each of these tests should say \"setup\" in the description\". That makes\nsense to me, and I don't mind doing it here.\n\n> Of course splitting up the setup into 3 steps neither helps nor hurts\n> that. What I was complaining about is splitting the filter-branch\n> from the verification that filter-branch had the right result.\n\nI totally missed your original point, then. I don't mind combining the\nlast two into a single test. That makes sense to me (I only split them\nat all because I added the second one much later during the\ndevelopment).\n\n-Peff\n\n-- >8 --\n#!/usr/bin/perl\n#\n# Run as \"perl foo.pl t0000-basic.sh\" (or whatever script you want to\n# check).\n\nmy $script = shift;\nmy ($script_num) = $script =~ /^(t\\d+)/;\n\n# run once to get the list of tests\nmy @tests = run_tests($script);\n\n# mark some tests as \"setup\" tests that will always be run\nforeach my $test (@tests) {\n\tif ($test->{desc} =~ /set.?up/i) {\n\t\tprint STDERR \"marking $test->{num} - $test->{desc} as setup\\n\";\n\t\t$test->{setup} = 1;\n\t}\n}\n\n# now try each test in isolation, but including setup tests\nforeach my $test (@tests) {\n\t$ENV{GIT_SKIP_TESTS} = skip_all_except($script_num, $test, @tests);\n\trun_tests($script) or die \"failed $test->{num} ($test->{desc})\\n\";\n}\n\nsub run_tests {\n\tmy @r;\n\topen(my $fh, '-|', qw(sh), $script);\n\twhile (<$fh>) {\n\t\t/^ok (\\d+) - (.*)/ and push @r, { num => $1, desc => $2 };\n\t}\n\t$? and return ();\n\treturn @r;\n}\n\nsub skip_all_except {\n\tmy $prefix = shift;\n\tmy $want = shift;\n\n\treturn join(' ',\n\t\tmap { \"$prefix.$_->{num}\" }\n\t\tgrep { !$_->{setup} && $_->{num} != $want->{num} }\n\t\t@_);\n}\n"},{"id":"225940","messageId":"20130826213628.GB25452@sigill.intra.peff.net","threadId":"34756","inReplyTo":"xmqq38pwjv89.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-26T21:36:29Z","receivedAt":"2013-08-26T21:36:29Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 26, 2013 at 09:02:46AM -0700, Junio C Hamano wrote:\n\n> > There's one subtle thing I didn't mention in the \"it is already on stack\n> > overflow\". If you have a version of git which complains about the null\n> > sha1, then the SO advice is already broken. But if the SO works, then\n> > you do not have a version of git which complains. So why do you care?\n> >\n> > And the answer is: you may be pushing to a remote with a version of git\n> > that complains, and which has receive.fsckObjects set (and in many\n> > cases, that remote is GitHub, since we have had that check on for a\n> > while).\n> >\n> > I don't know if it is worth spelling that out or not.\n> \n> Probably not.\n> \n> You could aim to correct each and every wrong suggestions on a site\n> where misguided leads other misguided, but it is a hopeless task.\n\nI do no think the advice on SO was misguided. My point was that a reader\nof the revised commit message mentioning SO may wonder \"why would people\non SO suggest that? It does not work since we introduced the check in\nthe first place\".\n\nBut if neither you nor Jonathan wondered about it, then it is probably\nnot worth caring.\n\n> >> > After this patch, do you think (in a separate change) it would make\n> >> > sense for cache-tree.c::update_one() to check for null sha1 and error\n> >> > out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of\n> >> > the caveat from the last paragraph.\n> [...]\n\nHmm. We are already complaining about the null sha1 in most cases,\nbecause we do not have the object. It is only for submodules that the\nbogus entry can make it into a tree. So I tried this:\n\ndiff --git a/cache-tree.c b/cache-tree.c\nindex 0bbec43..0ec3923 100644\n--- a/cache-tree.c\n+++ b/cache-tree.c\n@@ -343,7 +343,8 @@ static int update_one(struct cache_tree *it,\n \t\t\tentlen = pathlen - baselen;\n \t\t\ti++;\n \t\t}\n-\t\tif (mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1)) {\n+\t\tif ((mode != S_IFGITLINK && !missing_ok && !has_sha1_file(sha1))\n+\t\t || (mode == S_IFGITLINK && is_null_sha1(sha1))) {\n \t\t\tstrbuf_release(&buffer);\n \t\t\treturn error(\"invalid object %06o %s for '%.*s'\",\n \t\t\t\tmode, sha1_to_hex(sha1), entlen+baselen, path);\ndiff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh\nindex 50d5689..59c5edc 100755\n--- a/t/t7009-filter-branch-null-sha1.sh\n+++ b/t/t7009-filter-branch-null-sha1.sh\n@@ -31,10 +31,10 @@ test_expect_success 'filter commands are still checked' '\n \tgit commit -a -m \"back to normal\"\n '\n \n-test_expect_success 'filter commands are still checked' '\n+test_expect_success 'complain about trees that are broken after filtering' '\n \ttest_must_fail git filter-branch \\\n \t\t--force --prune-empty \\\n-\t\t--index-filter \"git rm --cached --ignore-unmatch three.t\"\n+\t\t--index-filter \"\"\n '\n \n test_expect_success 'removing the broken entry works' '\n\nBut it doesn't work. The reason is that we do not need to re-make the\ncache-tree, because we are just handing back the crap that we got from\nthe original tree. And I do not think we would want to invalidate the\nwhole cache tree just on the off chance that it has a problem;\nfilter-branch is slow enough as it is.\n\nSo I think we are better off to let filter-branch run at full speed,\neven if optimizations like cache-tree mean we are missing out on\nvalidating all of the data. If you want to know whether your repo is in\ngood shape, you should run `git fsck`.\n\n-Peff\n\nPS I got more information from the user who originally experienced this\n problem (and yes, it was a submodule entry). It turns out that he had\n done some unusual stuff with having a git repository inside his Eclipse\n project, and adding it as a sub-project of his build. So it was\n almost certainly EGit/JGit which wrote the bad entry in the first\n place (and he is making a bug report to EGit folks).\n"},{"id":"225950","messageId":"xmqqy57nhk35.fsf@gitster.dls.corp.google.com","threadId":"34756","inReplyTo":"20130826212053.GA25452@sigill.intra.peff.net","subject":"Re: [PATCHv2] write_index: optionally allow broken null sha1s","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2013-08-27T03:46:22Z","receivedAt":"2013-08-27T03:46:22Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I'd be very surprised if this works in practice on most of our current\n> test scripts. There are often subtle dependencies on the state left over\n> from previous tests. Running the script below up through t3800 (at which\n> point I lost patience) reveals 37 test scripts that are broken. Which is\n> only about 17%, but we're clearly not quite there yet.\n> ...\n\nYes, I agree that in an ideal world with infinite amount of time, we\nshould strive to make sure that each test is independent from\nprevious steps, and if we do not have infinite amount of time, we\nshould make sure we find some (say 5%) time to various code clean-up\neffort, in both writing and reviewing such, including tests.\n\nThe attached is cute. Thanks for a food for thought.\n\n> -- >8 --\n> #!/usr/bin/perl\n> #\n> # Run as \"perl foo.pl t0000-basic.sh\" (or whatever script you want to\n> # check).\n>\n> my $script = shift;\n> my ($script_num) = $script =~ /^(t\\d+)/;\n>\n> # run once to get the list of tests\n> my @tests = run_tests($script);\n>\n> # mark some tests as \"setup\" tests that will always be run\n> foreach my $test (@tests) {\n> \tif ($test->{desc} =~ /set.?up/i) {\n> \t\tprint STDERR \"marking $test->{num} - $test->{desc} as setup\\n\";\n> \t\t$test->{setup} = 1;\n> \t}\n> }\n>\n> # now try each test in isolation, but including setup tests\n> foreach my $test (@tests) {\n> \t$ENV{GIT_SKIP_TESTS} = skip_all_except($script_num, $test, @tests);\n> \trun_tests($script) or die \"failed $test->{num} ($test->{desc})\\n\";\n> }\n>\n> sub run_tests {\n> \tmy @r;\n> \topen(my $fh, '-|', qw(sh), $script);\n> \twhile (<$fh>) {\n> \t\t/^ok (\\d+) - (.*)/ and push @r, { num => $1, desc => $2 };\n> \t}\n> \t$? and return ();\n> \treturn @r;\n> }\n>\n> sub skip_all_except {\n> \tmy $prefix = shift;\n> \tmy $want = shift;\n>\n> \treturn join(' ',\n> \t\tmap { \"$prefix.$_->{num}\" }\n> \t\tgrep { !$_->{setup} && $_->{num} != $want->{num} }\n> \t\t@_);\n> }\n"},{"id":"226061","messageId":"20130827204112.GA19451@sigill.intra.peff.net","threadId":"34756","inReplyTo":"20130826212053.GA25452@sigill.intra.peff.net","subject":"[PATCHv3] write_index: optionally allow broken null sha1s","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-08-27T20:41:12Z","receivedAt":"2013-08-27T20:41:12Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Aug 26, 2013 at 05:20:53PM -0400, Jeff King wrote:\n\n> And by your rule above, the advice would be \"each of these tests\n> should say \"setup\" in the description\". That makes sense to me, and I\n> don't mind doing it here.\n>\n> [...]\n>\n> I totally missed your original point, then. I don't mind combining the\n> last two into a single test. That makes sense to me (I only split them\n> at all because I added the second one much later during the\n> development).\n\nSo here is a re-roll with:\n\n 1. The commit message tweaks you suggested\n\n 2. Annotating the setup tests by using the word \"setup\"\n\n 3. Combining the final two tests to filter and check the result.\n\nI also added your Reviewed-by, since you gave it for the previous\nversion, and all of the changes are taking your suggestions. :)\n\n-- >8 --\nSubject: write_index: optionally allow broken null sha1s\n\nCommit 4337b58 (do not write null sha1s to on-disk index,\n2012-07-28) added a safety check preventing git from writing\nnull sha1s into the index. The intent was to catch errors in\nother parts of the code that might let such an entry slip\ninto the index (or worse, a tree).\n\nSome existing repositories may have invalid trees that\ncontain null sha1s already, though. Until 4337b58, a common\nway to clean this up would be to use git-filter-branch's\nindex-filter to repair such broken entries. That now fails\nwhen filter-branch tries to write out the index.\n\nIntroduce a GIT_ALLOW_NULL_SHA1 environment variable to\nrelax this check and make it easier to recover from such a\nhistory.\n\nIt is tempting to not involve filter-branch in this commit\nat all, and instead require the user to manually invoke\n\n\tGIT_ALLOW_NULL_SHA1=1 git filter-branch ...\n\nto perform an index-filter on a history with trees with null\nsha1s. That would be slightly safer, but requires some\nspecialized knowledge from the user. So let's set the\nGIT_ALLOW_NULL_SHA1 variable automatically when checking out\nthe to-be-filtered trees. Advice on using filter-branch to\nremove such entries already exists on places like\nstackoverflow, and this patch makes it Just Work again on\nrecent versions of git.\n\nFurther commands that touch the index will still notice and\nfail, unless they actually remove the broken entries. A\nfilter-branch whose filters do not touch the index at all\nwill not error out (since we complain of the null sha1 only\non writing, not when making a tree out of the index), but\nthis is acceptable, as we still print a loud warning, so the\nproblem is unlikely to go unnoticed.\n\nSigned-off-by: Jeff King \nReviewed-by: Jonathan Nieder \n---\n git-filter-branch.sh | 5 ++--\n read-cache.c | 13 ++++++++--\n t/t7009-filter-branch-null-sha1.sh | 49 ++++++++++++++++++++++++++++++++++++++\n 3 files changed, 63 insertions(+), 4 deletions(-)\n create mode 100755 t/t7009-filter-branch-null-sha1.sh\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex ac2a005..98e8fe4 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -283,11 +283,12 @@ while read commit parents; do\n \n \tcase \"$filter_subdir\" in\n \t\"\")\n-\t\tgit read-tree -i -m $commit\n+\t\tGIT_ALLOW_NULL_SHA1=1 git read-tree -i -m $commit\n \t\t;;\n \t*)\n \t\t# The commit may not have the subdirectory at all\n-\t\terr=$(git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n+\t\terr=$(GIT_ALLOW_NULL_SHA1=1 \\\n+\t\t git read-tree -i -m $commit:\"$filter_subdir\" 2>&1) || {\n \t\t\tif ! git rev-parse -q --verify $commit:\"$filter_subdir\"\n \t\t\tthen\n \t\t\t\trm -f \"$GIT_INDEX_FILE\"\ndiff --git a/read-cache.c b/read-cache.c\nindex c3d5e35..83a7414 100644\n--- a/read-cache.c\n+++ b/read-cache.c\n@@ -1817,8 +1817,17 @@ int write_index(struct index_state *istate, int newfd)\n \t\t\tcontinue;\n \t\tif (!ce_uptodate(ce) && is_racy_timestamp(istate, ce))\n \t\t\tce_smudge_racily_clean_entry(ce);\n-\t\tif (is_null_sha1(ce->sha1))\n-\t\t\treturn error(\"cache entry has null sha1: %s\", ce->name);\n+\t\tif (is_null_sha1(ce->sha1)) {\n+\t\t\tstatic const char msg[] = \"cache entry has null sha1: %s\";\n+\t\t\tstatic int allow = -1;\n+\n+\t\t\tif (allow < 0)\n+\t\t\t\tallow = git_env_bool(\"GIT_ALLOW_NULL_SHA1\", 0);\n+\t\t\tif (allow)\n+\t\t\t\twarning(msg, ce->name);\n+\t\t\telse\n+\t\t\t\treturn error(msg, ce->name);\n+\t\t}\n \t\tif (ce_write_entry(&c, newfd, ce, previous_name) < 0)\n \t\t\treturn -1;\n \t}\ndiff --git a/t/t7009-filter-branch-null-sha1.sh b/t/t7009-filter-branch-null-sha1.sh\nnew file mode 100755\nindex 0000000..a997f7a\n--- /dev/null\n+++ b/t/t7009-filter-branch-null-sha1.sh\n@@ -0,0 +1,49 @@\n+#!/bin/sh\n+\n+test_description='filter-branch removal of trees with null sha1'\n+. ./test-lib.sh\n+\n+test_expect_success 'setup: base commits' '\n+\ttest_commit one &&\n+\ttest_commit two &&\n+\ttest_commit three\n+'\n+\n+test_expect_success 'setup: a commit with a bogus null sha1 in the tree' '\n+\t{\n+\t\tgit ls-tree HEAD &&\n+\t\tprintf \"160000 commit $_z40\\\\tbroken\\\\n\"\n+\t} >broken-tree\n+\techo \"add broken entry\" >msg &&\n+\n+\ttree=$(git mktree expect &&\n+\tgit filter-branch \\\n+\t\t--force --prune-empty \\\n+\t\t--index-filter \"git rm --cached --ignore-unmatch broken\" &&\n+\tgit log -1 --format=%s >actual &&\n+\ttest_cmp expect actual\n+'\n+\n+test_done\n-- \n1.8.4.2.g87d4a77\n"}]}