Re: [PATCH 2/2] index-pack: handle duplicate base objects gracefully

[Jeff King <peff@peff.net>]

On Sat, Aug 30, 2014 at 06:10:33PM -0700, Shawn Pearce wrote:

> > We do detect and complain if --strict is given. Should we make it the
> > default instead? I think it is still worthwhile to have a mode that can
> > handle these packs. It may be the only reasonable way to recover the
> > data from such a broken pack (and that broken pack may be the only copy
> > of the data you have, if you are stuck getting it out of a broken
> > implementation on a remote server).
> 
> Eh, OK, that does make a lot of sense.
> 
> Unfortunately accepting it by default encourages broken
> implementations to stay broken and ship packs with duplicate objects,
> which they shouldn't do since there are readers out there that cannot
> handle it.

I was pretty much convinced by this line of reasoning after you reading
your message. But after discovering some horrible side effects that I
just posted elsewhere, I think it's a no-brainer.

> In my opinion we should make --strict default, but its an excellent
> point made that users should have an escape hatch to disable this
> check, attempt accepting a broken pack and try fixing it locally with
> repack.

I am not sure if you meant it this way, but --strict controls a lot more
than just duplicate objects. It also runs fsck checks against the
incoming objects. Turning all of that on is a much bigger change than I
think we've been talking about.

But it's one I think we should consider. We've had --strict on at GitHub
for a few years, and it has caught many problems. Frequently we get
push-back from users saying "but nowhere else I pushed this to
complains". My opinion is that it is not that we are wrong for being
picky, but that other servers are not being picky enough.

It's also a bit of a hassle, though. E.g., a lot of projects have broken
ident lines deep in their history (from old versions of git, or other
broken implementations) and it is often way too late to fix them. We
usually try to get people to rewrite the history if it's not a problem,
but otherwise will lift the restrictions temporarily to let them push up
old history.

Broken ident lines are annoying, but not _too_ fundamentally bad.
Duplicate tree entries are a lot worse. Fsck even distinguishes between
"error" and "warning", but "index-pack --strict" treats both as a reason
to reject the object. We could perhaps loosen that, and make sure our
error/warning categories are reasonable.

-Peff