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

From: Jeff King <peff@peff.net>
To: Junio C Hamano <gitster@pobox.com>
Cc: Jonathan Tan <jonathantanmy@google.com>, git@vger.kernel.org
Subject: Re: [PATCH v4 7/8] sha1_file: do not access pack if unneeded
Date: Sat, 24 Jun 2017 08:48:14 -0400	[thread overview]
Message-ID: <20170624124813.wajhxkrqhw4xoee4@sigill.intra.peff.net> (raw)
In-Reply-To: <xmqq8tklqkbe.fsf@gitster.mtv.corp.google.com>

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

> > +	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