Re: [PATCH v2] fetch-pack: optionally save packs to disk
- From
- Augie Fackler <augie@google.com>
- Date
- Jun 12, 2015, 17:02 UTC
- Message-ID
- <CAHcr6HYv+3z+QLFqwKxVFNOu0Nk4pfWeVhGe_bxu6Fn=wruD8Q@mail.gmail.com>
- In-Reply-To
- <xmqqmw05dkxz.fsf@gitster.dls.corp.google.com>
On Fri, Jun 12, 2015 at 11:07 AM, Junio C Hamano <gitster@pobox.com> wrote:
Show 18 quoted lines
>> What is the problem with the current fetch-pack implementation? Does >> it remove a bogus packfile after download? Does it abort during >> download when it detects a broken packfile? Does --keep not do what >> you need? > > Doesn't the incoming data still go through the fattening process, > though? You will not be able to inspect the byte-for-byte identical > stream that came out of the server end whose packfile generation > logic is suspect. > > For the purpose of debugging your own new server implementation, it > might be a better approach to capture the pack as it comes out at > the server end, instead of doing it at the fetch-pack end as it > comes in. But the approach to add this "dump" at the fetch-pack side > is that it gives us a tool to diagnose problems that come from > broken server (re)implementations by other people we cannot debug, > i.e. "you are spewing this corrupt pack against this request; here > is a dump we took to help you go fix your server".
*nod* that's an important part of it. Also, in the small-pull case, the pack data gets sent to unpack-objects anyway, so git is never saving the packfile anywhere in that case (I think it's for a pull of less than 100 objects, which characterizes most of my reduced test cases for weirdness.)
Show 12 quoted lines
> >> Instead of your approach (which forks off tee to dump a copy of the >> packfile), would it not be simpler to add an option --debug-pack >> (probably not the best name) that skips the cleanup step when a broken >> packfile is detected and prints the name of the downloaded packfile? > > As long as we need to debug a thin pack that comes from the other > end, that approach is not sufficient, I am afraid. > > I anticipated that you'd have problem with its use of "tee". It > probably can do this internally with async interface, perhaps, > instead?
I'd be happy to make such changes if that's the consensus and someone can give me pointers. My C is admittedly pretty rusty from non-use, and I'm not at all familiar with git's codebase, but I'll at least try.
Thanks!