Re: Zero padded file modes...

From: Jeff King <peff@peff.net>
Date: Sep 5, 2013, 19:35 UTC
Message-ID: <20130905193507.GB15910@sigill.intra.peff.net>
In-Reply-To: <CAEBDL5UiEurFeZg1AuNUKEvBMDs3K3D5ZiF5rB-dYWjp5nvrEA@mail.gmail.com>

On Thu, Sep 05, 2013 at 01:13:40PM -0400, John Szakmeister wrote:

Show 6 quoted lines

> > Yep. These were mostly caused by a bug in Grit that is long-fixed.  But
> > the objects remain in many histories. It would have painful to rewrite
> > them back then, and it would be even more painful now.
> 
> I guess there's still the other side of the question though.  Are
> these repositories busted in the sense that something no longer works?

No, as far as I know, everything still works fine. However, some diffs may be suboptimal, because we may have two different sha1s for the same subtree (so we may descend into the tree unnecessarily only to find that they are equivalent). And by the same token, any scripts doing non-recursive diffs may erroneously mark the trees as differing, even though they do not contain any differing files.

But neither is a big problem in practice. If you had two clients in active use which were flip-flopping a sub-tree back and forth between representations, it would be a problem. But we are talking about a few isolated incidents far back in history.

Show 5 quoted lines

> I doesn't appear to be the case, but I've not used it extensively say
> I can't say for certain one way or another.  In the sense that the
> content is not strictly compliant, transfer.fsckObjects did its job,
> but I wonder if fsck needs to be a little more tolerant now (at least
> with respect to transfer objects)?

Fsck actually treats this as a warning, not an error. It is transfer.fsckObjects (via "index-pack --strict") that actually treats warnings as errors.

It's possible that this should be loosened to allow through problems marked as FSCK_WARN (with a message, kind of like...a warning). Though it may also make sense to revisit some of the classifications in fsck (e.g., many of the warnings are indicative of seriously broken objects).

GitHub uses transfer.fsckObjects, rejecting all warnings[1]. In practice it is not usually a big deal, as people are happy to fix up their objects _before_ they get widely published. The biggest push-back we get is when somebody tries to re-push history they got from another GitHub repo, and then says "But why are you complaining? You served this crappy broken history?" And it's a fair point. If you are forking (but not joining the existing fork network) of an existing project with irregularities in the history, it's not really an option to simply rewrite the history you are basing on.

-Peff

[1] Actually, we do let through 0-padded modes with a warning,
    explicitly because of the problem mentioned above.