Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'

From: Jeff Hostetler <git@jeffhostetler.com>
Date: Sep 9, 2019, 13:54 UTC
Message-ID: <f32d2e8c-abec-0ec1-daa7-4c10470c5553@jeffhostetler.com>
In-Reply-To: <xmqqv9u6po4j.fsf@gitster-ct.c.googlers.com>

On 9/5/2019 2:57 PM, Junio C Hamano wrote:

Show 28 quoted lines

> Jeff King <peff@peff.net> writes:
> 
>> So these patches are punting on the greater question of why we want to
>> parse so early, and are not making anything worse. AFAICT, "clone
>> --filter=sparse:oid" has never worked (even though our tests did cover
>> the underlying rev-list and pack-objects code paths).
>> ...
>> TBH, I'm not sure why the original is so eager to parse early. I guess
>> it allows:
>>
>>    - a dual use of the options parser; we can use it both to sanity-check
>>      the options before sending them to a server, and to actually use the
>>      filter ourselves.
>>
>>    - earlier detection maybe gives us a cleaner error path (e.g.,
>>      rev-list can do its own error handling). But I'd think doing it when
>>      we actually initialize the filter would be enough.
>>
>> I.e., if we want to go all the way, I think this two-patch series could
>> basically be replaced with something like the (totally untested)
>> approach below, which just pushes the parsing closer to the
>> point-of-use.
>>
>> Adding Jeff Hostetler to the cc, in case he recalls any reason not to
>> use that approach.
> 
> Thanks.
>

I think both of Peff's guesses are correct.

IIRC I wrote the original parse_list_objects_filter() and friends to syntax check the command line arguments of rev-list. In hindsight, this looks a bit aggressive at that layer, or rather now that it is being used by various places in other commands (such as parsing messages from the wire), it shouldn't call die() as Peff suggests.

I like the code Peff suggests. Making parse_list_objects_filter() a bit simpler and not call die(). Callers should then check the function return value as necessary.

It would be nice if we could continue to let parse_list_objects_filter() do the syntax checking -- that is, we can still check that we received a ulong in "blob:limit:<nr>" and that "sparse:oid:<oid>" looks like a hex value, for example. Just save the actual <oid> lookup to the higher layer, if and when that makes sense.

Jeff