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

From: Junio C Hamano <gitster@pobox.com>
Date: Jun 24, 2017, 18:41 UTC
Message-ID: <xmqq7f019qjg.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <20170624124813.wajhxkrqhw4xoee4@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 23 quoted lines

> 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).

Hmph, but the solution does not scale. When a caller wants whence and something else that cannot be asked for or ignored by being a "pointer to a result" field, such a request cannot be expressed. We either need to make all fields in oi request to "pointer to a result, if the result is needed, or NULL when the result is not of interest", or give a bit for each non-pointer field to allow the caller to express "I am not interested in the value of this field".

In the usecase Jonathan has, the caller's wish is a very narrow "I am interested in nothing; just checking if the object is there", and passing NULL for oi works fine. So I'm inclined to suggest that we take that approach now and worry about a more generic and scalable "how would one tell the machinery that the value for a field is uninteresting when the field is not a pointer to result?" mechanism until a real need arises.

Thanks.