Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded

From: Jeff King <peff@peff.net>
Date: Jun 24, 2017, 12:48 UTC
Message-ID: <20170624124813.wajhxkrqhw4xoee4@sigill.intra.peff.net>
In-Reply-To: <xmqq8tklqkbe.fsf@gitster.mtv.corp.google.com>

On Wed, Jun 21, 2017 at 11:15:01AM -0700, Junio C Hamano wrote:

Show 16 quoted lines

> > +	if (!oi->typep && !oi->sizep && !oi->disk_sizep &&
> > +	    !oi->delta_base_sha1 && !oi->typename && !oi->contentp &&
> > +	    !oi->populate_u) {
> > +		oi->whence = OI_PACKED;
> > +		return 0;
> > +	}
> > +
> 
> ... this "if" statement feels like a maintenance nightmare.  The
> intent of the guard, I think, is "when the call wants absolutely
> nothing but whence", but the implementation of the guard will not
> stay true to the intent whenever somebody adds a new field to oi.
> 
> I wonder if it makes more sense to have a new field "whence_only",
> which is set only by such a specialized caller, which this guard
> checks (and no other fields).

The other nice thing about whence_only is that it flips the logic. So any existing callers which depend on filling the union automatically will not be affected (though I would be surprised if there are any such callers; most of that information isn't actually that interesting).

-Peff