Re: [PATCH 1/2] archive-tar: write extended headers for file sizes >= 8GB

From: Jeff King <peff@peff.net>
Date: Jun 21, 2016, 15:59 UTC
Message-ID: <20160621155920.GA7549@sigill.intra.peff.net>
In-Reply-To: <57687413.3030609@web.de>

On Tue, Jun 21, 2016 at 12:54:11AM +0200, René Scharfe wrote:

Show 15 quoted lines

> > Unfortunately, it's quite an expensive test to run. For one
> > thing, unless your filesystem supports files with holes, it
> > takes 64GB of disk space (you might think piping straight to
> > `hash-object --stdin` would be better, but it's not; that
> > tries to buffer all 64GB in RAM!). Furthermore, hashing and
> > compressing the object takes several minutes of CPU time.
> > 
> > We could ship just the resulting compressed object data as a
> > loose object, but even that takes 64MB. So sadly, this code
> > path remains untested in the test suite.
> 
> If we could set the limit to a lower value than 8GB for testing then we
> could at least check if the extended header is written, e.g. if ustar_size()
> could be convinced to return 0 every time using a hidden command line
> parameter or an environment variable or something better.

Yes, we could do that, though I think it loses most of the value of the test. We can check that if we hit an arbitrary value we generate the pax header, but I think what we _really_ care about is: did we generate an output that somebody else's tar implementation can handle.

And for the smaller-than-64GB case, GNU tar happily handles our existing output (though I suspect other tars might fail at "only" 8GB).

Show 5 quoted lines

> > +static inline unsigned long ustar_size(uintmax_t size)
> > +{
> > +	if (size < 077777777777UL)
> 
> Shouldn't that be less-or-equal?

Yeah, you're right (and for the one in the next patch, too).

Show 6 quoted lines

> > +	if (ustar_size(size) != size)
> > +		strbuf_append_ext_header_uint(&ext_header, "size", size);
> 
> It needs "S_ISREG(mode) && " as well, no?  In practice it probably doesn't
> matter (until someone stores a 8GB long symlink target), but the size field
> should only be set for regular files.

Thanks for noticing that. I remembered wondering that when I was early in debugging/diagnosing, but forgot to follow up on it. I agree it's unlikely in practice, but we should have consistent checks (I think it would actually make sense to move the ISREG check inside ustar_size, and then we can apply it consistently here and when generating the header; my goal with ustar_size() was to avoid having the same logic in multiple places).

-Peff