Jonathan Nieder [off-list ref] writes:
In other words, why not use something like this?
write_index: optionally allow broken null sha1s
Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)
added a safety check preventing git from writing null sha1s into the
index. The intent was to catch errors in other parts of the code that
might let such an entry slip into the index (or worse, a tree).
Some existing repositories have some invalid trees that contain null
sha1s already, though. Until 4337b58, a common way to clean this up
would be to use git-filter-branch's index-filter to repair such broken
entries. That now fails when filter-branch tries to write out the
index.
Introduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check
and make it easier to recover from such a history.
I found this version more readable than Peff's (albeit slightly).
After this patch, do you think (in a separate change) it would make
sense for cache-tree.c::update_one() to check for null sha1 and error
out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of
the caveat from the last paragraph.
Hmm, interesting thought.
On Sun, Aug 25, 2013 at 11:03:54PM -0700, Junio C Hamano wrote:
Jonathan Nieder [off-list ref] writes:
quoted
In other words, why not use something like this?
write_index: optionally allow broken null sha1s
Commit 4337b58 (do not write null sha1s to on-disk index, 2012-07-28)
added a safety check preventing git from writing null sha1s into the
index. The intent was to catch errors in other parts of the code that
might let such an entry slip into the index (or worse, a tree).
Some existing repositories have some invalid trees that contain null
sha1s already, though. Until 4337b58, a common way to clean this up
would be to use git-filter-branch's index-filter to repair such broken
entries. That now fails when filter-branch tries to write out the
index.
Introduce a GIT_ALLOW_NULL_SHA1 environment variable to relax this check
and make it easier to recover from such a history.
I found this version more readable than Peff's (albeit slightly).
OK. Do you want to apply with Jonathan's wording, then?
There's one subtle thing I didn't mention in the "it is already on stack
overflow". If you have a version of git which complains about the null
sha1, then the SO advice is already broken. But if the SO works, then
you do not have a version of git which complains. So why do you care?
And the answer is: you may be pushing to a remote with a version of git
that complains, and which has receive.fsckObjects set (and in many
cases, that remote is GitHub, since we have had that check on for a
while).
I don't know if it is worth spelling that out or not.
quoted
After this patch, do you think (in a separate change) it would make
sense for cache-tree.c::update_one() to check for null sha1 and error
out unless GIT_ALLOW_NULL_SHA1 is true? That would let us get rid of
the caveat from the last paragraph.
Hmm, interesting thought.
I think it is worth doing. The main reason I put the original check on
writing to the index is that it more clearly pinpoints the source of the
error. If we just died during git-write-tree, then you know somebody
broke your index, but you don't know which command.
But checking in both places would add extra protection, and would make
possible the "relax on read, strict on write" policy that filter-branch
wants to do.
-Peff