Re: [PATCHv2] write_index: optionally allow broken null sha1s

[Jeff King <peff@peff.net>]

On Sun, Aug 25, 2013 at 11:03:54PM -0700, Junio C Hamano wrote:

> Jonathan Nieder <jrnieder@gmail.com> 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).

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.

> > 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