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

From: Jeff King <peff@peff.net>
Date: Aug 26, 2013, 14:31 UTC
Message-ID: <20130826143135.GB14858@sigill.intra.peff.net>
In-Reply-To: <xmqqob8lj8dx.fsf@gitster.dls.corp.google.com>

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

Show 21 quoted lines

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

Show 6 quoted lines

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