Re: [PATCH v3 3/4] archive-tar: write extended headers for far-future mtime

From: Jeff King <peff@peff.net>
Date: Jun 24, 2016, 19:16 UTC
Message-ID: <20160624191648.GC32118@sigill.intra.peff.net>
In-Reply-To: <xmqqshw2flt8.fsf@gitster.mtv.corp.google.com>

On Fri, Jun 24, 2016 at 12:06:59PM -0700, Junio C Hamano wrote:

Show 11 quoted lines

> > +	if (args->time > 077777777777UL) {
> > +		strbuf_append_ext_header_uint(&ext_header, "mtime",
> > +					      args->time);
> > +		args->time = 077777777777UL;
> > +	}
> > +
> > +	if (!ext_header.len)
> > +		return 0;
> 
> Another symbolic constant to explain this, e.g. TAR_TIME_LIMIT, may
> want to exist.

This one at least appears twice. I think one of the reasons I am slightly resistant to a symbolic constant is that it tempts people to think that it's OK to change it. It's not. These values are mandated by POSIX, and must match the size of the ustar header field.

So the least-repetitive thing would be to define it as:

  (1UL << (1 + (3 * (sizeof(ustar_header.mtime) - 1)))) - 1

That's pretty horrible to read, but if wrapped in a symbolic constant, at least people would think twice before touching it. ;)

-Peff