{"thread":{"id":"51778","subject":"[PATCH v3 0/2] partial-clone: fix two issues with sparse filter handling","startedAt":"2019-08-29T23:19:50Z","lastAt":"2019-09-17T19:22:27Z","messageCount":19,"participants":["Jon Simons","Junio C Hamano","Jeff King","Jeff Hostetler"],"isPatch":true,"patchVersion":3,"patchTotal":2},"messages":[{"id":"381552","messageId":"20190829231925.15223-1-jon@jonsimons.org","threadId":"51778","inReplyTo":null,"subject":"[PATCH v3 0/2] partial-clone: fix two issues with sparse filter handling","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T23:19:23Z","receivedAt":"2019-08-29T23:19:50Z","isPatch":true,"sender":{"key":"jon@jonsimons.org","avatar":"https://avatars.githubusercontent.com/u/118440?v=4"},"body":"Included here are two fixes for partial cloning with sparse filters.\nThese issues were uncovered in early testing internally at GitHub,\nwhere Taylor and Peff have provided early offlist review feedback.\n\nThis third revision includes a fix for a test bug introduced in the\nsecond revision.\n\nJon Simons (2):\n list-objects-filter: only parse sparse OID when 'have_git_dir'\n list-objects-filter: handle unresolved sparse filter OID\n\n list-objects-filter-options.c | 3 ++-\n list-objects-filter.c | 6 +++++-\n t/t5616-partial-clone.sh | 28 ++++++++++++++++++++++++++++\n 3 files changed, 35 insertions(+), 2 deletions(-)\n\n-- \n2.23.0.37.g745f681289.dirty\n\n"},{"id":"381553","messageId":"20190829231925.15223-2-jon@jonsimons.org","threadId":"51778","inReplyTo":"20190829231925.15223-1-jon@jonsimons.org","subject":"[PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T23:19:24Z","receivedAt":"2019-08-29T23:19:53Z","isPatch":true,"sender":{"key":"jon@jonsimons.org","avatar":"https://avatars.githubusercontent.com/u/118440?v=4"},"body":"Fix a bug in partial cloning with sparse filters by ensuring to check\nfor 'have_git_dir' before attempting to resolve the sparse filter OID.\n\nOtherwise the client will trigger:\n\n BUG: refs.c:1851: attempting to get main_ref_store outside of repository\n\nwhen attempting to git clone with a sparse filter.\n\nNote that this fix is the minimal one which avoids the BUG and allows\nfor the clone to complete successfully:\n\nThere is an open question as to whether there should be any attempt\nto resolve the OID provided by the client in this context, as a filter\nfor the clone to be used on the remote side. For cases where local\nand remote OID resolutions differ, resolving on the client side could\nbe considered a bug. For now, the minimal approach here is used to\nunblock further testing for partial clones with sparse filters, while\na more invasive fix could make sense to pursue as a future direction.\n\nt5616 is updated to demonstrate the change.\n\nSigned-off-by: Jon Simons \n---\n list-objects-filter-options.c | 3 ++-\n t/t5616-partial-clone.sh | 21 +++++++++++++++++++++\n 2 files changed, 23 insertions(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 1cb20c659c..aaba312edb 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -71,7 +71,8 @@ static int gently_parse_list_objects_filter(\n \t\t * command, but DO NOT complain if we don't have the blob or\n \t\t * ref locally.\n \t\t */\n-\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n+\t\tif (have_git_dir() &&\n+\t\t !get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n \t\t\t\t\t &sparse_oid, &oc))\n \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 565254558f..5c68431d10 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -241,6 +241,27 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '\n \t! grep \"?$(cat blob)\" missing_after\n '\n \n+test_expect_success 'setup src repo for sparse filter' '\n+\tgit init sparse-src &&\n+\tgit -C sparse-src config --local uploadpack.allowfilter 1 &&\n+\tgit -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&\n+\tfor n in 1 2 3 4\n+\tdo\n+\t\ttest_commit -C sparse-src \"this-is-file-$n\" file.$n.txt || return 1\n+\tdone &&\n+\ttest_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&\n+\ttest_write_lines /file2.txt /file4.txt >sparse-src/even-files &&\n+\techo \"/*\" >sparse-src/all-files &&\n+\tgit -C sparse-src add odd-files even-files all-files &&\n+\tgit -C sparse-src commit -m \"some sparse checkout files\"\n+'\n+\n+test_expect_success 'partial clone with sparse filter succeeds' '\n+\tgit clone --no-local --no-checkout --filter=sparse:oid=master:all-files sparse-src pc-all &&\n+\tgit clone --no-local --no-checkout --filter=sparse:oid=master:even-files sparse-src pc-even &&\n+\tgit clone --no-local --no-checkout --filter=sparse:oid=master:odd-files sparse-src pc-odd\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.23.0.37.g745f681289.dirty\n\n"},{"id":"381554","messageId":"20190829231925.15223-3-jon@jonsimons.org","threadId":"51778","inReplyTo":"20190829231925.15223-1-jon@jonsimons.org","subject":"[PATCH v3 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T23:19:25Z","receivedAt":"2019-08-29T23:19:56Z","isPatch":true,"sender":{"key":"jon@jonsimons.org","avatar":"https://avatars.githubusercontent.com/u/118440?v=4"},"body":"Handle a potential NULL 'sparse_oid_value' when attempting to load\nsparse filter exclusions by blob, to avoid segfaulting later during\n'add_excludes_from_blob_to_list'.\n\nWhile here, uniquify the errors emitted to distinguish between the\ncase that a given OID is NULL due to an earlier failure to resolve it,\nand when an OID resolves but parsing the sparse filter spec fails.\n\nt5616 is updated to demonstrate the change.\n\nCo-authored-by: Jeff King \nSigned-off-by: Jeff King \nSigned-off-by: Jon Simons \n---\n list-objects-filter.c | 6 +++++-\n t/t5616-partial-clone.sh | 7 +++++++\n 2 files changed, 12 insertions(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 36e1f774bc..252fae5d4e 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -464,9 +464,13 @@ static void *filter_sparse_oid__init(\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n \td->omits = omitted;\n+\tif (!filter_options->sparse_oid_value)\n+\t\tdie(_(\"unable to read sparse filter specification from %s\"),\n+\t\t filter_options->filter_spec);\n \tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n \t\t\t\t\t NULL, 0, &d->el) < 0)\n-\t\tdie(\"could not load filter specification\");\n+\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n+\t\t oid_to_hex(filter_options->sparse_oid_value));\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 5c68431d10..d01886c464 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -262,6 +262,13 @@ test_expect_success 'partial clone with sparse filter succeeds' '\n \tgit clone --no-local --no-checkout --filter=sparse:oid=master:odd-files sparse-src pc-odd\n '\n \n+test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '\n+\ttest_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter sparse-src sc1 2>err &&\n+\ttest_i18ngrep \"unable to read sparse filter specification from sparse:oid=master:sparse-filter\" err &&\n+\ttest_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master sparse-src sc2 2>err &&\n+\ttest_i18ngrep \"unable to parse sparse filter data in $(git -C sparse-src rev-parse master)\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.23.0.37.g745f681289.dirty\n\n"},{"id":"381597","messageId":"xmqqr252y199.fsf@gitster-ct.c.googlers.com","threadId":"51778","inReplyTo":"20190829231925.15223-2-jon@jonsimons.org","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-08-30T18:08:34Z","receivedAt":"2019-08-30T18:08:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jon Simons writes:\n\n> diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> index 1cb20c659c..aaba312edb 100644\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -71,7 +71,8 @@ static int gently_parse_list_objects_filter(\n> \t\t * command, but DO NOT complain if we don't have the blob or\n> \t\t * ref locally.\n> \t\t */\n> -\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> +\t\tif (have_git_dir() &&\n> +\t\t !get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> \t\t\t\t\t &sparse_oid, &oc))\n> \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n> \t\tfilter_options->choice = LOFC_SPARSE_OID;\n\nSorry, I do not quite understand what this wants to do. We say \"we\nparsed this correctly, this filter is sparse:oid=\" without\nfilling sparse_oid_value field at all. What do the consumers of\nsuch a filter_options instance do to such a half-read option?\n\nIf they say \"ah, the parser wanted to do sparse:oid but we couldn't\nreally figure the part out, so let's ignore it\", that might\nbe the best they could do anyway, but it somewhat feels iffy. I\nwonder if we are better off inventing a new \"we tried to parse but\nwe lack sufficient info to make it useful\" value to use in .choice\nfield and return it instead.\n\nIn the longer term, what do we want to happen in such a case where\n\"read this blob to figure out what I want you to do\" cannot be\nsatisfied due to chicken-and-egg situation like this? Can we\npostpone fetching or cloning that *wants* to use the named \nwhen we discover that the is not available (of which, your\n\"!have_git_dir()\" is a subset), grab that single first from\nthe other side before doing the main transfer, and then resume the\ntransfer that wants to use the after we successfully do so,\nor something?\n"},{"id":"381790","messageId":"20190904045424.GA6488@sigill.intra.peff.net","threadId":"51778","inReplyTo":"xmqqr252y199.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-04T04:54:24Z","receivedAt":"2019-09-04T04:54:29Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Aug 30, 2019 at 11:08:34AM -0700, Junio C Hamano wrote:\n\n> Jon Simons writes:\n> \n> > diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n> > index 1cb20c659c..aaba312edb 100644\n> > --- a/list-objects-filter-options.c\n> > +++ b/list-objects-filter-options.c\n> > @@ -71,7 +71,8 @@ static int gently_parse_list_objects_filter(\n> > \t\t * command, but DO NOT complain if we don't have the blob or\n> > \t\t * ref locally.\n> > \t\t */\n> > -\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> > +\t\tif (have_git_dir() &&\n> > +\t\t !get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> > \t\t\t\t\t &sparse_oid, &oc))\n> > \t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n> > \t\tfilter_options->choice = LOFC_SPARSE_OID;\n> \n> Sorry, I do not quite understand what this wants to do. We say \"we\n> parsed this correctly, this filter is sparse:oid=\" without\n> filling sparse_oid_value field at all. What do the consumers of\n> such a filter_options instance do to such a half-read option?\n\nEmpirically, they either die() or segfault. ;)\n\nI agree the code even before Jon's patch is pretty funny. Forgetting the\n\"not a repo\" case for a moment, we know that get_oid_with_context()\nmight not find the name. In the case of rev-list, we manually check the\nvalidity of sparse_oid_value later and die(). Which seems like a\nlayering violation, but at least gives the desired outcome.\n\nIn the case of clone, it gets parsed twice:\n\n 1. On the client side, we don't have a repo yet, and so we BUG(). But\n once fixed, we don't care about the value at all! We never use it,\n and instead send the original filter spec over to the server, so at\n most this whole option-parsing is giving us an early syntax check.\n\n 2. On the server side, upload-pack receives the filter spec, parses\n it, but doesn't remember to check whether it's a valid oid. It\n segfaults if it isn't (that's patch 2 here).\n\nSo these patches are punting on the greater question of why we want to\nparse so early, and are not making anything worse. AFAICT, \"clone\n--filter=sparse:oid\" has never worked (even though our tests did cover\nthe underlying rev-list and pack-objects code paths).\n\n> If they say \"ah, the parser wanted to do sparse:oid but we couldn't\n> really figure the part out, so let's ignore it\", that might\n> be the best they could do anyway, but it somewhat feels iffy. I\n> wonder if we are better off inventing a new \"we tried to parse but\n> we lack sufficient info to make it useful\" value to use in .choice\n> field and return it instead.\n\nTBH, I'm not sure why the original is so eager to parse early. I guess\nit allows:\n\n - a dual use of the options parser; we can use it both to sanity-check\n the options before sending them to a server, and to actually use the\n filter ourselves.\n\n - earlier detection maybe gives us a cleaner error path (e.g.,\n rev-list can do its own error handling). But I'd think doing it when\n we actually initialize the filter would be enough.\n\nI.e., if we want to go all the way, I think this two-patch series could\nbasically be replaced with something like the (totally untested)\napproach below, which just pushes the parsing closer to the\npoint-of-use.\n\nAdding Jeff Hostetler to the cc, in case he recalls any reason not to\nuse that approach.\n\n> In the longer term, what do we want to happen in such a case where\n> \"read this blob to figure out what I want you to do\" cannot be\n> satisfied due to chicken-and-egg situation like this? Can we\n> postpone fetching or cloning that *wants* to use the named \n> when we discover that the is not available (of which, your\n> \"!have_git_dir()\" is a subset), grab that single first from\n> the other side before doing the main transfer, and then resume the\n> transfer that wants to use the after we successfully do so,\n> or something?\n\nI don't think there's any chicken-and-egg here. The client side of a\nclone does not ever care about what's in sparse_oid_value at all, before\nor after (and the name needs to be resolved from the server's view of\nthe refs, anyway). I think there could be a feature where we do a narrow\nclone based on the spec in an oid, and then do a matching sparse\ncheckout. And that feature would want to make sure it knows how the\nserver resolved the spec in the first step. But AFAIK we don't yet have\nsuch a feature (and the simplest thing would probably be a capability\nwhere the server tells us the blob oid, and makes sure it is\ntransmitted).\n\n-Peff\n\n---\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 301ccb970b..74dbfad73d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -471,10 +471,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tparse_list_objects_filter(&filter_options, arg);\n \t\t\tif (filter_options.choice && !revs.blob_objects)\n \t\t\t\tdie(_(\"object filtering requires --objects\"));\n-\t\t\tif (filter_options.choice == LOFC_SPARSE_OID &&\n-\t\t\t !filter_options.sparse_oid_value)\n-\t\t\t\tdie(_(\"invalid sparse value '%s'\"),\n-\t\t\t\t filter_options.filter_spec);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 1cb20c659c..76a9a49a75 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -63,17 +63,8 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:oid=\", &v0)) {\n-\t\tstruct object_context oc;\n-\t\tstruct object_id sparse_oid;\n-\n-\t\t/*\n-\t\t * Try to parse into an OID for the current\n-\t\t * command, but DO NOT complain if we don't have the blob or\n-\t\t * ref locally.\n-\t\t */\n-\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n-\t\t\t\t\t &sparse_oid, &oc))\n-\t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n+\t\t/* v0 is durable because it points into our saved filter_spec */\n+\t\tfilter_options->sparse_oid_name = v0;\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n@@ -138,7 +129,6 @@ void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n \tfree(filter_options->filter_spec);\n-\tfree(filter_options->sparse_oid_value);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex c54f0000fb..a819e42ffb 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -42,7 +42,7 @@ struct list_objects_filter_options {\n \t * choice-specific; not all values will be defined for any given\n \t * choice.\n \t */\n-\tstruct object_id *sparse_oid_value;\n+\tconst char *sparse_oid_name;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 36e1f774bc..130c17b95c 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -463,9 +463,16 @@ static void *filter_sparse_oid__init(\n \tfilter_free_fn *filter_free_fn)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n+\tstruct object_context oc;\n+\tstruct object_id sparse_oid;\n+\n+\tif (!get_oid_with_context(the_repository,\n+\t\t\t\t filter_options->sparse_oid_name,\n+\t\t\t\t GET_OID_BLOB, &sparse_oid, &oc))\n+\t\tdie(\"unable to access sparse blob in '%s'\",\n+\t\t filter_options->sparse_oid_name);\n \td->omits = omitted;\n-\tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n-\t\t\t\t\t NULL, 0, &d->el) < 0)\n+\tif (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n"},{"id":"381897","messageId":"xmqqv9u6po4j.fsf@gitster-ct.c.googlers.com","threadId":"51778","inReplyTo":"20190904045424.GA6488@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-05T18:57:32Z","receivedAt":"2019-09-05T18:57:40Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> So these patches are punting on the greater question of why we want to\n> parse so early, and are not making anything worse. AFAICT, \"clone\n> --filter=sparse:oid\" has never worked (even though our tests did cover\n> the underlying rev-list and pack-objects code paths).\n> ...\n> TBH, I'm not sure why the original is so eager to parse early. I guess\n> it allows:\n>\n> - a dual use of the options parser; we can use it both to sanity-check\n> the options before sending them to a server, and to actually use the\n> filter ourselves.\n>\n> - earlier detection maybe gives us a cleaner error path (e.g.,\n> rev-list can do its own error handling). But I'd think doing it when\n> we actually initialize the filter would be enough.\n>\n> I.e., if we want to go all the way, I think this two-patch series could\n> basically be replaced with something like the (totally untested)\n> approach below, which just pushes the parsing closer to the\n> point-of-use.\n>\n> Adding Jeff Hostetler to the cc, in case he recalls any reason not to\n> use that approach.\n\nThanks.\n"},{"id":"382063","messageId":"f32d2e8c-abec-0ec1-daa7-4c10470c5553@jeffhostetler.com","threadId":"51778","inReplyTo":"xmqqv9u6po4j.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-09-09T13:54:36Z","receivedAt":"2019-09-09T13:54:40Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/5/2019 2:57 PM, Junio C Hamano wrote:\n> Jeff King writes:\n> \n>> So these patches are punting on the greater question of why we want to\n>> parse so early, and are not making anything worse. AFAICT, \"clone\n>> --filter=sparse:oid\" has never worked (even though our tests did cover\n>> the underlying rev-list and pack-objects code paths).\n>> ...\n>> TBH, I'm not sure why the original is so eager to parse early. I guess\n>> it allows:\n>>\n>> - a dual use of the options parser; we can use it both to sanity-check\n>> the options before sending them to a server, and to actually use the\n>> filter ourselves.\n>>\n>> - earlier detection maybe gives us a cleaner error path (e.g.,\n>> rev-list can do its own error handling). But I'd think doing it when\n>> we actually initialize the filter would be enough.\n>>\n>> I.e., if we want to go all the way, I think this two-patch series could\n>> basically be replaced with something like the (totally untested)\n>> approach below, which just pushes the parsing closer to the\n>> point-of-use.\n>>\n>> Adding Jeff Hostetler to the cc, in case he recalls any reason not to\n>> use that approach.\n> \n> Thanks.\n> \n\nI think both of Peff's guesses are correct.\n\nIIRC I wrote the original parse_list_objects_filter() and friends to\nsyntax check the command line arguments of rev-list. In hindsight,\nthis looks a bit aggressive at that layer, or rather now that it is\nbeing used by various places in other commands (such as parsing\nmessages from the wire), it shouldn't call die() as Peff suggests.\n\nI like the code Peff suggests. Making parse_list_objects_filter()\na bit simpler and not call die(). Callers should then check the\nfunction return value as necessary.\n\nIt would be nice if we could continue to let parse_list_objects_filter()\ndo the syntax checking -- that is, we can still check that we received a\nulong in \"blob:limit:\" and that \"sparse:oid:\" looks like a hex\nvalue, for example. Just save the actual lookup to the higher\nlayer, if and when that makes sense.\n\nJeff\n\n"},{"id":"382073","messageId":"20190909170823.GA30470@sigill.intra.peff.net","threadId":"51778","inReplyTo":"f32d2e8c-abec-0ec1-daa7-4c10470c5553@jeffhostetler.com","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-09T17:08:24Z","receivedAt":"2019-09-09T17:08:27Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2019 at 09:54:36AM -0400, Jeff Hostetler wrote:\n\n> It would be nice if we could continue to let parse_list_objects_filter()\n> do the syntax checking -- that is, we can still check that we received a\n> ulong in \"blob:limit:\" and that \"sparse:oid:\" looks like a hex\n> value, for example. Just save the actual lookup to the higher\n> layer, if and when that makes sense.\n\nYeah, I agree that is the right place to do syntactic checking. But I\nthink we can't do much checking for the sparse-oid. We currently accept\nnot just a hex oid, but any name. And I think that is useful; I should\nbe able to say \"master:sparse-file\" and have it resolved by the remote\nside.\n\nSo it really is \"any name which is syntactically resolvable as a sha1\nexpression\". At which point I think you might as well punt and just wait\nuntil we _actually_ try to resolve the name to see if it's valid.\n\nI'll work up what I sent earlier into a real patch, and include some of\nthis discussion.\n\n-Peff\n"},{"id":"382074","messageId":"xmqqv9u1l7ha.fsf@gitster-ct.c.googlers.com","threadId":"51778","inReplyTo":"f32d2e8c-abec-0ec1-daa7-4c10470c5553@jeffhostetler.com","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-09T17:12:01Z","receivedAt":"2019-09-09T17:12:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff Hostetler writes:\n\n> It would be nice if we could continue to let parse_list_objects_filter()\n> do the syntax checking -- that is, we can still check that we received a\n> ulong in \"blob:limit:\" and that \"sparse:oid:\" looks like a hex\n> value, for example. Just save the actual lookup to the higher\n> layer, if and when that makes sense.\n\nHmmm, am I misremembering things or did I hear somebody mention in\nthis thread that people give \"sparse:oid:master\" (not blob object\nname in hex, but a refname) and expect the other side to resolve it?\n"},{"id":"382094","messageId":"76c1470c-929e-4116-c0ba-9514b836fb24@jeffhostetler.com","threadId":"51778","inReplyTo":"xmqqv9u1l7ha.fsf@gitster-ct.c.googlers.com","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-09-09T19:49:22Z","receivedAt":"2019-09-09T19:49:25Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/9/2019 1:12 PM, Junio C Hamano wrote:\n> Jeff Hostetler writes:\n> \n>> It would be nice if we could continue to let parse_list_objects_filter()\n>> do the syntax checking -- that is, we can still check that we received a\n>> ulong in \"blob:limit:\" and that \"sparse:oid:\" looks like a hex\n>> value, for example. Just save the actual lookup to the higher\n>> layer, if and when that makes sense.\n> \n> Hmmm, am I misremembering things or did I hear somebody mention in\n> this thread that people give \"sparse:oid:master\" (not blob object\n> name in hex, but a refname) and expect the other side to resolve it?\n> \n\nYou're right. I misspoke. I was thinking about the hex OID case\nand forgetting about the : form.\n\nJeff\n"},{"id":"382097","messageId":"7883417c-56a8-a38e-af2f-81b90b2dd7d3@jeffhostetler.com","threadId":"51778","inReplyTo":"20190909170823.GA30470@sigill.intra.peff.net","subject":"Re: [PATCH v3 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-09-09T20:03:09Z","receivedAt":"2019-09-09T20:03:13Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/9/2019 1:08 PM, Jeff King wrote:\n> On Mon, Sep 09, 2019 at 09:54:36AM -0400, Jeff Hostetler wrote:\n> \n>> It would be nice if we could continue to let parse_list_objects_filter()\n>> do the syntax checking -- that is, we can still check that we received a\n>> ulong in \"blob:limit:\" and that \"sparse:oid:\" looks like a hex\n>> value, for example. Just save the actual lookup to the higher\n>> layer, if and when that makes sense.\n> \n> Yeah, I agree that is the right place to do syntactic checking. But I\n> think we can't do much checking for the sparse-oid. We currently accept\n> not just a hex oid, but any name. And I think that is useful; I should\n> be able to say \"master:sparse-file\" and have it resolved by the remote\n> side.\n\nRight. I forgot about the \"master:sparse-file\" case and was only\nthinking about the hex oid case. The sparse-file case is very\nuseful.\n\n> \n> So it really is \"any name which is syntactically resolvable as a sha1\n> expression\". At which point I think you might as well punt and just wait\n> until we _actually_ try to resolve the name to see if it's valid.\n> \n> I'll work up what I sent earlier into a real patch, and include some of\n> this discussion.\n> \n> -Peff\n> \n\nthanks\nJeff\n\n"},{"id":"382381","messageId":"20190915010942.GA19787@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190909170823.GA30470@sigill.intra.peff.net","subject":"[PATCH 0/3] clone --filter=sparse:oid bugs","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T01:09:43Z","receivedAt":"2019-09-15T01:09:45Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 09, 2019 at 01:08:24PM -0400, Jeff King wrote:\n\n> I'll work up what I sent earlier into a real patch, and include some of\n> this discussion.\n\nHere it is. I pulled Jon's tests out into their own patch (mostly\nbecause it makes it easier to give credit). Then patch 2 is my fix, and\npatch 3 is the message fixups he had done.\n\nThis replaces what's queued in js/partial-clone-sparse-blob.\n\n [1/3]: t5616: test cloning/fetching with sparse:oid= filter\n [2/3]: list-objects-filter: delay parsing of sparse oid\n [3/3]: list-objects-filter: give a more specific error sparse parsing error\n\n builtin/rev-list.c | 4 ----\n list-objects-filter-options.c | 14 ++------------\n list-objects-filter-options.h | 2 +-\n list-objects-filter.c | 14 +++++++++++---\n t/t5616-partial-clone.sh | 36 +++++++++++++++++++++++++++++++++++\n 5 files changed, 50 insertions(+), 20 deletions(-)\n\n"},{"id":"382382","messageId":"20190915011115.GA11208@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190915010942.GA19787@sigill.intra.peff.net","subject":"[PATCH 1/3] t5616: test cloning/fetching with sparse:oid= filter","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T01:11:16Z","receivedAt":"2019-09-15T01:11:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jon Simons \n\nWe test in t5317 that \"sparse:oid\" filters work with rev-list, but\nthere's no coverage at all confirming that they work with a fetch or\nclone (and in fact, there are several bugs). Let's do a basic test that\na clone fetches the correct objects.\n\n[jk: extracted from Jon's earlier fix patches. I also simplified the\n setup down to a single sparse file, and I added checks that we got the\n right blobs]\n\nSigned-off-by: Jon Simons \nSigned-off-by: Jeff King \n---\n t/t5616-partial-clone.sh | 36 ++++++++++++++++++++++++++++++++++++\n 1 file changed, 36 insertions(+)\n\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 565254558f..0bdbc819f1 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -241,6 +241,42 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '\n \t! grep \"?$(cat blob)\" missing_after\n '\n \n+test_expect_success 'setup src repo for sparse filter' '\n+\tgit init sparse-src &&\n+\tgit -C sparse-src config --local uploadpack.allowfilter 1 &&\n+\tgit -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&\n+\ttest_commit -C sparse-src one &&\n+\ttest_commit -C sparse-src two &&\n+\techo /one.t >sparse-src/only-one &&\n+\tgit -C sparse-src add . &&\n+\tgit -C sparse-src commit -m \"add sparse checkout files\"\n+'\n+\n+test_expect_failure 'partial clone with sparse filter succeeds' '\n+\trm -rf dst.git &&\n+\tgit clone --no-local --bare \\\n+\t\t --filter=sparse:oid=master:only-one \\\n+\t\t sparse-src dst.git &&\n+\t(\n+\t\tcd dst.git &&\n+\t\tgit rev-list --objects --missing=print HEAD >out &&\n+\t\tgrep \"^$(git rev-parse HEAD:one.t)\" out &&\n+\t\tgrep \"^?$(git rev-parse HEAD:two.t)\" out\n+\t)\n+'\n+\n+test_expect_failure 'partial clone with unresolvable sparse filter fails cleanly' '\n+\trm -rf dst.git &&\n+\ttest_must_fail git clone --no-local --bare \\\n+\t\t\t\t --filter=sparse:oid=master:no-such-name \\\n+\t\t\t\t sparse-src dst.git 2>err &&\n+\ttest_i18ngrep \"unable to access sparse blob in .master:no-such-name\" err &&\n+\ttest_must_fail git clone --no-local --bare \\\n+\t\t\t\t --filter=sparse:oid=master \\\n+\t\t\t\t sparse-src dst.git 2>err &&\n+\ttest_i18ngrep \"could not load filter specification\" err\n+'\n+\n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n start_httpd\n \n-- \n2.23.0.667.gcccf1fbb03\n\n"},{"id":"382383","messageId":"20190915011319.GB11208@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190915010942.GA19787@sigill.intra.peff.net","subject":"[PATCH 2/3] list-objects-filter: delay parsing of sparse oid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T01:13:20Z","receivedAt":"2019-09-15T01:13:23Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"The list-objects-filter code has two steps to its initialization:\n\n 1. parse_list_objects_filter() makes sure the spec is a filter we know\n about and is syntactically correct. This step is done by \"rev-list\"\n or \"upload-pack\" that is going to apply a filter, but also by \"git\n clone\" or \"git fetch\" before they send the spec across the wire.\n\n 2. list_objects_filter__init() runs the type-specific initialization\n (using function pointers established in step 1). This happens at\n the start of traverse_commit_list_filtered(), when we're about to\n actually use the filter.\n\nIt's a good idea to parse as much as we can in step 1, in order to catch\nproblems early (e.g., a blob size limit that isn't a number). But one\nthing we _shouldn't_ do is resolve any oids at that step (e.g., for\nsparse-file contents specified by oid). In the case of a fetch, the oid\nhas to be resolved on the remote side.\n\nThe current code does resolve the oid during the parse phase, but\nignores any error (which we must do, because we might just be sending\nthe spec across the wire). This leads to two bugs:\n\n - if we're not in a repository (e.g., because it's git-clone parsing\n the spec), then we trigger a BUG() trying to resolve the name\n\n - if we did hit the error case, we still have to notice that later and\n bail. The code path in rev-list handles this, but the one in\n upload-pack does not, leading to a segfault.\n\nWe can fix both by moving the oid resolution into the sparse-oid init\nfunction. At that point we know we have a repository (because we're\nabout to traverse), and handling the error there fixes the segfault.\n\nAs a bonus, we can drop the NULL sparse_oid_value check in rev-list,\nsince this is now handled in the sparse-oid-filter init function.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-list.c | 4 ----\n list-objects-filter-options.c | 14 ++------------\n list-objects-filter-options.h | 2 +-\n list-objects-filter.c | 11 +++++++++--\n t/t5616-partial-clone.sh | 4 ++--\n 5 files changed, 14 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 301ccb970b..74dbfad73d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -471,10 +471,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tparse_list_objects_filter(&filter_options, arg);\n \t\t\tif (filter_options.choice && !revs.blob_objects)\n \t\t\t\tdie(_(\"object filtering requires --objects\"));\n-\t\t\tif (filter_options.choice == LOFC_SPARSE_OID &&\n-\t\t\t !filter_options.sparse_oid_value)\n-\t\t\t\tdie(_(\"invalid sparse value '%s'\"),\n-\t\t\t\t filter_options.filter_spec);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 1cb20c659c..43f41f446c 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -63,17 +63,8 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:oid=\", &v0)) {\n-\t\tstruct object_context oc;\n-\t\tstruct object_id sparse_oid;\n-\n-\t\t/*\n-\t\t * Try to parse into an OID for the current\n-\t\t * command, but DO NOT complain if we don't have the blob or\n-\t\t * ref locally.\n-\t\t */\n-\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n-\t\t\t\t\t &sparse_oid, &oc))\n-\t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n+\t\t/* v0 points into filter_options->filter_spec; no allocation needed */\n+\t\tfilter_options->sparse_oid_name = v0;\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n@@ -138,7 +129,6 @@ void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n \tfree(filter_options->filter_spec);\n-\tfree(filter_options->sparse_oid_value);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex c54f0000fb..a819e42ffb 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -42,7 +42,7 @@ struct list_objects_filter_options {\n \t * choice-specific; not all values will be defined for any given\n \t * choice.\n \t */\n-\tstruct object_id *sparse_oid_value;\n+\tconst char *sparse_oid_name;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 36e1f774bc..d2cdc03a73 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -463,9 +463,16 @@ static void *filter_sparse_oid__init(\n \tfilter_free_fn *filter_free_fn)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n+\tstruct object_context oc;\n+\tstruct object_id sparse_oid;\n+\n+\tif (get_oid_with_context(the_repository,\n+\t\t\t\t filter_options->sparse_oid_name,\n+\t\t\t\t GET_OID_BLOB, &sparse_oid, &oc))\n+\t\tdie(\"unable to access sparse blob in '%s'\",\n+\t\t filter_options->sparse_oid_name);\n \td->omits = omitted;\n-\tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n-\t\t\t\t\t NULL, 0, &d->el) < 0)\n+\tif (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 0bdbc819f1..84365b802a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -252,7 +252,7 @@ test_expect_success 'setup src repo for sparse filter' '\n \tgit -C sparse-src commit -m \"add sparse checkout files\"\n '\n \n-test_expect_failure 'partial clone with sparse filter succeeds' '\n+test_expect_success 'partial clone with sparse filter succeeds' '\n \trm -rf dst.git &&\n \tgit clone --no-local --bare \\\n \t\t --filter=sparse:oid=master:only-one \\\n@@ -265,7 +265,7 @@ test_expect_failure 'partial clone with sparse filter succeeds' '\n \t)\n '\n \n-test_expect_failure 'partial clone with unresolvable sparse filter fails cleanly' '\n+test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '\n \trm -rf dst.git &&\n \ttest_must_fail git clone --no-local --bare \\\n \t\t\t\t --filter=sparse:oid=master:no-such-name \\\n-- \n2.23.0.667.gcccf1fbb03\n\n"},{"id":"382384","messageId":"20190915011347.GC11208@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190915010942.GA19787@sigill.intra.peff.net","subject":"[PATCH 3/3] list-objects-filter: give a more specific error sparse parsing error","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T01:13:47Z","receivedAt":"2019-09-15T01:13:49Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"From: Jon Simons \n\nThe sparse:oid filter has two error modes: we might fail to resolve the\nname to an OID, or we might fail to parse the contents of that OID. In\nthe latter case, let's give a less generic error message, and mention\nthe OID we did find.\n\nWhile we're here, let's also mark both messages as translatable.\n\nSigned-off-by: Jeff King \n---\n list-objects-filter.c | 5 +++--\n t/t5616-partial-clone.sh | 2 +-\n 2 files changed, 4 insertions(+), 3 deletions(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex d2cdc03a73..50f0c6d07b 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -469,11 +469,12 @@ static void *filter_sparse_oid__init(\n \tif (get_oid_with_context(the_repository,\n \t\t\t\t filter_options->sparse_oid_name,\n \t\t\t\t GET_OID_BLOB, &sparse_oid, &oc))\n-\t\tdie(\"unable to access sparse blob in '%s'\",\n+\t\tdie(_(\"unable to access sparse blob in '%s'\"),\n \t\t filter_options->sparse_oid_name);\n \td->omits = omitted;\n \tif (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)\n-\t\tdie(\"could not load filter specification\");\n+\t\tdie(_(\"unable to parse sparse filter data in %s\"),\n+\t\t oid_to_hex(&sparse_oid));\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\n \td->array_frame[d->nr].defval = 0; /* default to include */\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 84365b802a..b85d3f5e4c 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -274,7 +274,7 @@ test_expect_success 'partial clone with unresolvable sparse filter fails cleanly\n \ttest_must_fail git clone --no-local --bare \\\n \t\t\t\t --filter=sparse:oid=master \\\n \t\t\t\t sparse-src dst.git 2>err &&\n-\ttest_i18ngrep \"could not load filter specification\" err\n+\ttest_i18ngrep \"unable to parse sparse filter data in\" err\n '\n \n . \"$TEST_DIRECTORY\"/lib-httpd.sh\n-- \n2.23.0.667.gcccf1fbb03\n"},{"id":"382393","messageId":"20190915161244.GA2826@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190915011319.GB11208@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] list-objects-filter: delay parsing of sparse oid","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T16:12:44Z","receivedAt":"2019-09-15T16:12:47Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 14, 2019 at 09:13:19PM -0400, Jeff King wrote:\n\n> --- a/list-objects-filter-options.c\n> +++ b/list-objects-filter-options.c\n> @@ -63,17 +63,8 @@ static int gently_parse_list_objects_filter(\n> \t\treturn 0;\n> \n> \t} else if (skip_prefix(arg, \"sparse:oid=\", &v0)) {\n> -\t\tstruct object_context oc;\n> -\t\tstruct object_id sparse_oid;\n> -\n> -\t\t/*\n> -\t\t * Try to parse into an OID for the current\n> -\t\t * command, but DO NOT complain if we don't have the blob or\n> -\t\t * ref locally.\n> -\t\t */\n> -\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n> -\t\t\t\t\t &sparse_oid, &oc))\n> -\t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n> +\t\t/* v0 points into filter_options->filter_spec; no allocation needed */\n> +\t\tfilter_options->sparse_oid_name = v0;\n\nLooks like this comment is no longer true after merging with\nmd/list-objects-filter-combo, which is in 'next'.\n\nWe can obviously deal with it during the merge, but it probably makes\nsense to just be more defensive from the start. Here's a revised version\nof patch 2, with these changes:\n\n diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\n index 43f41f446c..adbea552a0 100644\n --- a/list-objects-filter-options.c\n +++ b/list-objects-filter-options.c\n @@ -63,8 +63,7 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:oid=\", &v0)) {\n -\t\t/* v0 points into filter_options->filter_spec; no allocation needed */\n -\t\tfilter_options->sparse_oid_name = v0;\n +\t\tfilter_options->sparse_oid_name = xstrdup(v0);\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n @@ -129,6 +128,7 @@ void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n \tfree(filter_options->filter_spec);\n +\tfree(filter_options->sparse_oid_name);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \n diff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\n index a819e42ffb..20b9d1e587 100644\n --- a/list-objects-filter-options.h\n +++ b/list-objects-filter-options.h\n @@ -42,7 +42,7 @@ struct list_objects_filter_options {\n \t * choice-specific; not all values will be defined for any given\n \t * choice.\n \t */\n -\tconst char *sparse_oid_name;\n +\tchar *sparse_oid_name;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\n\n-- >8 --\nSubject: [PATCH] list-objects-filter: delay parsing of sparse oid\n\nThe list-objects-filter code has two steps to its initialization:\n\n 1. parse_list_objects_filter() makes sure the spec is a filter we know\n about and is syntactically correct. This step is done by \"rev-list\"\n or \"upload-pack\" that is going to apply a filter, but also by \"git\n clone\" or \"git fetch\" before they send the spec across the wire.\n\n 2. list_objects_filter__init() runs the type-specific initialization\n (using function pointers established in step 1). This happens at\n the start of traverse_commit_list_filtered(), when we're about to\n actually use the filter.\n\nIt's a good idea to parse as much as we can in step 1, in order to catch\nproblems early (e.g., a blob size limit that isn't a number). But one\nthing we _shouldn't_ do is resolve any oids at that step (e.g., for\nsparse-file contents specified by oid). In the case of a fetch, the oid\nhas to be resolved on the remote side.\n\nThe current code does resolve the oid during the parse phase, but\nignores any error (which we must do, because we might just be sending\nthe spec across the wire). This leads to two bugs:\n\n - if we're not in a repository (e.g., because it's git-clone parsing\n the spec), then we trigger a BUG() trying to resolve the name\n\n - if we did hit the error case, we still have to notice that later and\n bail. The code path in rev-list handles this, but the one in\n upload-pack does not, leading to a segfault.\n\nWe can fix both by moving the oid resolution into the sparse-oid init\nfunction. At that point we know we have a repository (because we're\nabout to traverse), and handling the error there fixes the segfault.\n\nAs a bonus, we can drop the NULL sparse_oid_value check in rev-list,\nsince this is now handled in the sparse-oid-filter init function.\n\nSigned-off-by: Jeff King \n---\n builtin/rev-list.c | 4 ----\n list-objects-filter-options.c | 14 ++------------\n list-objects-filter-options.h | 2 +-\n list-objects-filter.c | 11 +++++++++--\n t/t5616-partial-clone.sh | 4 ++--\n 5 files changed, 14 insertions(+), 21 deletions(-)\n\ndiff --git a/builtin/rev-list.c b/builtin/rev-list.c\nindex 301ccb970b..74dbfad73d 100644\n--- a/builtin/rev-list.c\n+++ b/builtin/rev-list.c\n@@ -471,10 +471,6 @@ int cmd_rev_list(int argc, const char **argv, const char *prefix)\n \t\t\tparse_list_objects_filter(&filter_options, arg);\n \t\t\tif (filter_options.choice && !revs.blob_objects)\n \t\t\t\tdie(_(\"object filtering requires --objects\"));\n-\t\t\tif (filter_options.choice == LOFC_SPARSE_OID &&\n-\t\t\t !filter_options.sparse_oid_value)\n-\t\t\t\tdie(_(\"invalid sparse value '%s'\"),\n-\t\t\t\t filter_options.filter_spec);\n \t\t\tcontinue;\n \t\t}\n \t\tif (!strcmp(arg, (\"--no-\" CL_ARG__FILTER))) {\ndiff --git a/list-objects-filter-options.c b/list-objects-filter-options.c\nindex 1cb20c659c..adbea552a0 100644\n--- a/list-objects-filter-options.c\n+++ b/list-objects-filter-options.c\n@@ -63,17 +63,7 @@ static int gently_parse_list_objects_filter(\n \t\treturn 0;\n \n \t} else if (skip_prefix(arg, \"sparse:oid=\", &v0)) {\n-\t\tstruct object_context oc;\n-\t\tstruct object_id sparse_oid;\n-\n-\t\t/*\n-\t\t * Try to parse into an OID for the current\n-\t\t * command, but DO NOT complain if we don't have the blob or\n-\t\t * ref locally.\n-\t\t */\n-\t\tif (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,\n-\t\t\t\t\t &sparse_oid, &oc))\n-\t\t\tfilter_options->sparse_oid_value = oiddup(&sparse_oid);\n+\t\tfilter_options->sparse_oid_name = xstrdup(v0);\n \t\tfilter_options->choice = LOFC_SPARSE_OID;\n \t\treturn 0;\n \n@@ -138,7 +128,7 @@ void list_objects_filter_release(\n \tstruct list_objects_filter_options *filter_options)\n {\n \tfree(filter_options->filter_spec);\n-\tfree(filter_options->sparse_oid_value);\n+\tfree(filter_options->sparse_oid_name);\n \tmemset(filter_options, 0, sizeof(*filter_options));\n }\n \ndiff --git a/list-objects-filter-options.h b/list-objects-filter-options.h\nindex c54f0000fb..20b9d1e587 100644\n--- a/list-objects-filter-options.h\n+++ b/list-objects-filter-options.h\n@@ -42,7 +42,7 @@ struct list_objects_filter_options {\n \t * choice-specific; not all values will be defined for any given\n \t * choice.\n \t */\n-\tstruct object_id *sparse_oid_value;\n+\tchar *sparse_oid_name;\n \tunsigned long blob_limit_value;\n \tunsigned long tree_exclude_depth;\n };\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 36e1f774bc..d2cdc03a73 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -463,9 +463,16 @@ static void *filter_sparse_oid__init(\n \tfilter_free_fn *filter_free_fn)\n {\n \tstruct filter_sparse_data *d = xcalloc(1, sizeof(*d));\n+\tstruct object_context oc;\n+\tstruct object_id sparse_oid;\n+\n+\tif (get_oid_with_context(the_repository,\n+\t\t\t\t filter_options->sparse_oid_name,\n+\t\t\t\t GET_OID_BLOB, &sparse_oid, &oc))\n+\t\tdie(\"unable to access sparse blob in '%s'\",\n+\t\t filter_options->sparse_oid_name);\n \td->omits = omitted;\n-\tif (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,\n-\t\t\t\t\t NULL, 0, &d->el) < 0)\n+\tif (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)\n \t\tdie(\"could not load filter specification\");\n \n \tALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);\ndiff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\nindex 0bdbc819f1..84365b802a 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -252,7 +252,7 @@ test_expect_success 'setup src repo for sparse filter' '\n \tgit -C sparse-src commit -m \"add sparse checkout files\"\n '\n \n-test_expect_failure 'partial clone with sparse filter succeeds' '\n+test_expect_success 'partial clone with sparse filter succeeds' '\n \trm -rf dst.git &&\n \tgit clone --no-local --bare \\\n \t\t --filter=sparse:oid=master:only-one \\\n@@ -265,7 +265,7 @@ test_expect_failure 'partial clone with sparse filter succeeds' '\n \t)\n '\n \n-test_expect_failure 'partial clone with unresolvable sparse filter fails cleanly' '\n+test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '\n \trm -rf dst.git &&\n \ttest_must_fail git clone --no-local --bare \\\n \t\t\t\t --filter=sparse:oid=master:no-such-name \\\n-- \n2.23.0.667.gcccf1fbb03\n\n"},{"id":"382394","messageId":"20190915165156.GA28436@sigill.intra.peff.net","threadId":"51778","inReplyTo":"20190915010942.GA19787@sigill.intra.peff.net","subject":"[PATCH 4/3] list-objects-filter: use empty string instead of NULL for sparse \"base\"","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-09-15T16:51:56Z","receivedAt":"2019-09-15T16:52:04Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 14, 2019 at 09:09:42PM -0400, Jeff King wrote:\n\n> On Mon, Sep 09, 2019 at 01:08:24PM -0400, Jeff King wrote:\n> \n> > I'll work up what I sent earlier into a real patch, and include some of\n> > this discussion.\n> \n> Here it is. I pulled Jon's tests out into their own patch (mostly\n> because it makes it easier to give credit). Then patch 2 is my fix, and\n> patch 3 is the message fixups he had done.\n> \n> This replaces what's queued in js/partial-clone-sparse-blob.\n> \n> [1/3]: t5616: test cloning/fetching with sparse:oid= filter\n> [2/3]: list-objects-filter: delay parsing of sparse oid\n> [3/3]: list-objects-filter: give a more specific error sparse parsing error\n\nAnd here's a bonus patch that I found while running under ASan/UBSan\n(since I wanted to double-check the memory handling of patch 2 when\nmerged with 'next').\n\n-- >8 --\nSubject: list-objects-filter: use empty string instead of NULL for sparse \"base\"\n\nWe use add_excludes_from_blob_to_list() to parse a sparse blob. Since\nwe don't have a base path, we pass NULL and 0 for the base and baselen,\nrespectively. But the rest of the exclude code passes a literal empty\nstring instead of NULL for this case. And indeed, we eventually end up\nwith match_pathname() calling fspathncmp(), which then calls the system\nstrncmp(path, base, baselen).\n\nThis works on many platforms, which notice that baselen is 0 and do not\nlook at the bytes of \"base\" at all. But it does violate the C standard,\nand building with SANITIZE=undefined will complain. You can also see it\nby instrumenting fspathncmp like this:\n\n\tdiff --git a/dir.c b/dir.c\n\tindex d021c908e5..4bb3d3ec96 100644\n\t--- a/dir.c\n\t+++ b/dir.c\n\t@@ -71,6 +71,8 @@ int fspathcmp(const char *a, const char *b)\n\n\t int fspathncmp(const char *a, const char *b, size_t count)\n\t {\n\t+\tif (!a || !b)\n\t+\t\tBUG(\"null fspathncmp arguments\");\n\t \treturn ignore_case ? strncasecmp(a, b, count) : strncmp(a, b, count);\n\t }\n\nWe could perhaps be more defensive in match_pathname(), but even if we\ndid so, it makes sense for this code to match the rest of the exclude\ncallers.\n\nSigned-off-by: Jeff King \n---\n list-objects-filter.c | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/list-objects-filter.c b/list-objects-filter.c\nindex 50f0c6d07b..83c788e8b5 100644\n--- a/list-objects-filter.c\n+++ b/list-objects-filter.c\n@@ -472,7 +472,7 @@ static void *filter_sparse_oid__init(\n \t\tdie(_(\"unable to access sparse blob in '%s'\"),\n \t\t filter_options->sparse_oid_name);\n \td->omits = omitted;\n-\tif (add_excludes_from_blob_to_list(&sparse_oid, NULL, 0, &d->el) < 0)\n+\tif (add_excludes_from_blob_to_list(&sparse_oid, \"\", 0, &d->el) < 0)\n \t\tdie(_(\"unable to parse sparse filter data in %s\"),\n \t\t oid_to_hex(&sparse_oid));\n \n-- \n2.23.0.667.gcccf1fbb03\n\n"},{"id":"382424","messageId":"a5e9bfa0-5372-c444-cc42-5ca2f614b1b9@jeffhostetler.com","threadId":"51778","inReplyTo":"20190915010942.GA19787@sigill.intra.peff.net","subject":"Re: [PATCH 0/3] clone --filter=sparse:oid bugs","fromName":"Jeff Hostetler","fromEmail":"git@jeffhostetler.com","sentAt":"2019-09-16T14:31:59Z","receivedAt":"2019-09-16T14:32:02Z","isPatch":true,"sender":{"key":"git@jeffhostetler.com","avatar":null},"body":"\n\nOn 9/14/2019 9:09 PM, Jeff King wrote:\n> On Mon, Sep 09, 2019 at 01:08:24PM -0400, Jeff King wrote:\n> \n>> I'll work up what I sent earlier into a real patch, and include some of\n>> this discussion.\n> \n> Here it is. I pulled Jon's tests out into their own patch (mostly\n> because it makes it easier to give credit). Then patch 2 is my fix, and\n> patch 3 is the message fixups he had done.\n> \n> This replaces what's queued in js/partial-clone-sparse-blob.\n> \n> [1/3]: t5616: test cloning/fetching with sparse:oid= filter\n> [2/3]: list-objects-filter: delay parsing of sparse oid\n> [3/3]: list-objects-filter: give a more specific error sparse parsing error\n> \n> builtin/rev-list.c | 4 ----\n> list-objects-filter-options.c | 14 ++------------\n> list-objects-filter-options.h | 2 +-\n> list-objects-filter.c | 14 +++++++++++---\n> t/t5616-partial-clone.sh | 36 +++++++++++++++++++++++++++++++++++\n> 5 files changed, 50 insertions(+), 20 deletions(-)\n> \n\nThis series looks good to me.\n\nThanks!\nJeff\n"},{"id":"382515","messageId":"xmqqef0e9190.fsf@gitster-ct.c.googlers.com","threadId":"51778","inReplyTo":"20190915161244.GA2826@sigill.intra.peff.net","subject":"Re: [PATCH 2/3] list-objects-filter: delay parsing of sparse oid","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2019-09-17T19:22:19Z","receivedAt":"2019-09-17T19:22:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> It's a good idea to parse as much as we can in step 1, in order to catch\n> problems early (e.g., a blob size limit that isn't a number). But one\n> thing we _shouldn't_ do is resolve any oids at that step (e.g., for\n> sparse-file contents specified by oid). In the case of a fetch, the oid\n> has to be resolved on the remote side.\n> ...\n> We can fix both by moving the oid resolution into the sparse-oid init\n> function. At that point we know we have a repository (because we're\n> about to traverse), and handling the error there fixes the segfault.\n\nMakes sense. Thanks for a clean solution to a messy problem.\n\n"}]}