mailing list mirror (one of many)
 help / color / mirror / code / Atom feed
From: Jeff Hostetler <>
To: Junio C Hamano <>, Jeff King <>
Cc: Jeff Hostetler <>,
	Jon Simons <>,,,,
Subject: Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
Date: Mon, 9 Sep 2019 09:54:36 -0400	[thread overview]
Message-ID: <> (raw)
In-Reply-To: <>

On 9/5/2019 2:57 PM, Junio C Hamano wrote:
> Jeff King <> 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.


  reply	other threads:[~2019-09-09 13:54 UTC|newest]

Thread overview: 19+ messages / expand[flat|nested]  mbox.gz  Atom feed  top
2019-08-29 23:19 [PATCH v3 0/2] partial-clone: fix two issues with sparse filter handling Jon Simons
2019-08-29 23:19 ` [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir' Jon Simons
2019-08-30 18:08   ` Junio C Hamano
2019-09-04  4:54     ` Jeff King
2019-09-05 18:57       ` Junio C Hamano
2019-09-09 13:54         ` Jeff Hostetler [this message]
2019-09-09 17:08           ` Jeff King
2019-09-09 20:03             ` Jeff Hostetler
2019-09-15  1:09             ` [PATCH 0/3] clone --filter=sparse:oid bugs Jeff King
2019-09-15  1:11               ` [PATCH 1/3] t5616: test cloning/fetching with sparse:oid=<oid> filter Jeff King
2019-09-15  1:13               ` [PATCH 2/3] list-objects-filter: delay parsing of sparse oid Jeff King
2019-09-15 16:12                 ` Jeff King
2019-09-17 19:22                   ` Junio C Hamano
2019-09-15  1:13               ` [PATCH 3/3] list-objects-filter: give a more specific error sparse parsing error Jeff King
2019-09-15 16:51               ` [PATCH 4/3] list-objects-filter: use empty string instead of NULL for sparse "base" Jeff King
2019-09-16 14:31               ` [PATCH 0/3] clone --filter=sparse:oid bugs Jeff Hostetler
2019-09-09 17:12           ` [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir' Junio C Hamano
2019-09-09 19:49             ` Jeff Hostetler
2019-08-29 23:19 ` [PATCH v3 2/2] list-objects-filter: handle unresolved sparse filter OID Jon Simons

Reply instructions:

You may reply publicly to this message via plain-text email
using any one of the following methods:

* Save the following mbox file, import it into your mail client,
  and reply-to-all from there: mbox

  Avoid top-posting and favor interleaved quoting:

  List information:

* Reply using the --to, --cc, and --in-reply-to
  switches of git-send-email(1):

  git send-email \ \ \ \ \ \ \ \ \ \ \

* If your mail client supports setting the In-Reply-To header
  via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line before the message body.
Code repositories for project(s) associated with this public inbox

This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).