{"thread":{"id":"51767","subject":"[PATCH 2/2] list-objects-filter: handle unresolved sparse filter OID","startedAt":"2019-08-28T20:26:49Z","lastAt":"2019-08-29T14:28:45Z","messageCount":8,"participants":["Jon Simons","Eric Sunshine","Jeff King","Derrick Stolee"],"isPatch":true,"patchVersion":1,"patchTotal":2},"messages":[{"id":"381467","messageId":"20190828201824.1255-3-jon@jonsimons.org","threadId":"51767","inReplyTo":"20190828201824.1255-1-jon@jonsimons.org","subject":"[PATCH 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-28T20:18:24Z","receivedAt":"2019-08-28T20:26:49Z","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 <peff@peff.net>\nSigned-off-by: Jeff King <peff@peff.net>\nSigned-off-by: Jon Simons <jon@jonsimons.org>\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 6c3aa06973..0adb11f17b 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -264,6 +264,13 @@ test_expect_success 'partial clone with sparse filter succeeds' '\n \tgit clone --no-local --no-checkout --filter=sparse:oid=master:odd-files \"file://$(pwd)/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 \"file://$(pwd)/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 \"file://$(pwd)/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":"381468","messageId":"20190828201824.1255-2-jon@jonsimons.org","threadId":"51767","inReplyTo":"20190828201824.1255-1-jon@jonsimons.org","subject":"[PATCH 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-28T20:18:23Z","receivedAt":"2019-08-28T20:26:49Z","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 <jon@jonsimons.org>\n---\n list-objects-filter-options.c |  3 ++-\n t/t5616-partial-clone.sh      | 23 +++++++++++++++++++++++\n 2 files changed, 25 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..6c3aa06973 100755\n--- a/t/t5616-partial-clone.sh\n+++ b/t/t5616-partial-clone.sh\n@@ -241,6 +241,29 @@ 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\n+\tdone &&\n+\techo \"/file.1.txt\" >> sparse-src/odd-files &&\n+\techo \"/file.3.txt\" >> sparse-src/odd-files &&\n+\techo \"/file.2.txt\" >> sparse-src/even-files &&\n+\techo \"/file.4.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 \"file://$(pwd)/sparse-src\" pc-all &&\n+\tgit clone --no-local --no-checkout --filter=sparse:oid=master:even-files \"file://$(pwd)/sparse-src\" pc-even &&\n+\tgit clone --no-local --no-checkout --filter=sparse:oid=master:odd-files \"file://$(pwd)/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":"381469","messageId":"20190828201824.1255-1-jon@jonsimons.org","threadId":"51767","inReplyTo":null,"subject":"[PATCH 0/2] partial-clone: fix two issues with sparse filter handling","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-28T20:18:22Z","receivedAt":"2019-08-28T20:26:51Z","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\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      | 30 ++++++++++++++++++++++++++++++\n 3 files changed, 37 insertions(+), 2 deletions(-)\n\n-- \n2.23.0.37.g745f681289.dirty\n\n"},{"id":"381471","messageId":"CAPig+cQeco6-aM+UrJsbj9LXtWSe4hXn0vbrBWwVn00cbDpbzQ@mail.gmail.com","threadId":"51767","inReplyTo":"20190828201824.1255-2-jon@jonsimons.org","subject":"Re: [PATCH 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-28T21:10:33Z","receivedAt":"2019-08-28T21:10:47Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Wed, Aug 28, 2019 at 4:27 PM Jon Simons <jon@jonsimons.org> wrote:\n> Fix a bug in partial cloning with sparse filters by ensuring to check\n> for 'have_git_dir' before attempting to resolve the sparse filter OID.\n> [...]\n> Signed-off-by: Jon Simons <jon@jonsimons.org>\n> ---\n> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh\n> @@ -241,6 +241,29 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '\n> +test_expect_success 'setup src repo for sparse filter' '\n> +       git init sparse-src &&\n> +       git -C sparse-src config --local uploadpack.allowfilter 1 &&\n> +       git -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&\n> +       for n in 1 2 3 4\n> +       do\n> +               test_commit -C sparse-src \"this-is-file-$n\" file.$n.txt\n> +       done &&\n\nThe way this is coded, a failure of the test_commit() invocation won't\nfail the test overall. You need to do so manually:\n\n    for n in 1 2 3 4\n    do\n        test_commit -C sparse-src \"this-is-file-$n\" file.$n.txt || return 1\n    done &&\n\n> +       echo \"/file.1.txt\" >> sparse-src/odd-files &&\n> +       echo \"/file.3.txt\" >> sparse-src/odd-files &&\n> +       echo \"/file.2.txt\" >> sparse-src/even-files &&\n> +       echo \"/file.4.txt\" >> sparse-src/even-files &&\n\nSimpler:\n\n    test_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&\n    test_write_lines /file2.txt /file4.txt >sparse-src/even-files &&\n\n> +       echo \"/*\" >> sparse-src/all-files &&\n\nStyle nit: drop whitespace following redirection operator.\n\nAnd, using >> rather than just > here makes the test more confusing\nthan it need be; probably best to use >.\n"},{"id":"381482","messageId":"20190828233514.GA25355@sigill.intra.peff.net","threadId":"51767","inReplyTo":"20190828201824.1255-2-jon@jonsimons.org","subject":"Re: [PATCH 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-28T23:35:15Z","receivedAt":"2019-08-28T23:35:17Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Aug 28, 2019 at 04:18:23PM -0400, Jon Simons wrote:\n\n> Fix a bug in partial cloning with sparse filters by ensuring to check\n> for 'have_git_dir' before attempting to resolve the sparse filter OID.\n> \n> Otherwise the client will trigger:\n> \n>     BUG: refs.c:1851: attempting to get main_ref_store outside of repository\n> \n> when attempting to git clone with a sparse filter.\n> \n> Note that this fix is the minimal one which avoids the BUG and allows\n> for the clone to complete successfully:\n> \n> There is an open question as to whether there should be any attempt\n> to resolve the OID provided by the client in this context, as a filter\n> for the clone to be used on the remote side.  For cases where local\n> and remote OID resolutions differ, resolving on the client side could\n> be considered a bug.  For now, the minimal approach here is used to\n> unblock further testing for partial clones with sparse filters, while\n> a more invasive fix could make sense to pursue as a future direction.\n\nJust to provide a little more of our findings to the list: I think the\nmain thing going on here is that the filter options-parsing code is\nshared on the client and server side (and doesn't have any idea which it\nis). That's why we see the \"do not complain\" comment in the context\nbelow:\n\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\nand why it's OK to just quietly ignore this case. I don't think it's\nhurting anything in practice. Whether we resolve the name or not, we\nsend the _original_ name to the other side (it would be a bug for us to\nresolve it ourselves and send the oid).\n\n> +test_expect_success 'partial clone with sparse filter succeeds' '\n> +\tgit clone --no-local --no-checkout --filter=sparse:oid=master:all-files \"file://$(pwd)/sparse-src\" pc-all &&\n> +\tgit clone --no-local --no-checkout --filter=sparse:oid=master:even-files \"file://$(pwd)/sparse-src\" pc-even &&\n> +\tgit clone --no-local --no-checkout --filter=sparse:oid=master:odd-files \"file://$(pwd)/sparse-src\" pc-odd\n> +'\n\nSince you're using \"--no-local\", you should be able to just say\n\"sparse-src\" without the full path or file URL.\n\nI think Eric's style suggestions elsewhere in the thread were sensible,\ntoo. And of course the code change itself looks good.\n\n-Peff\n"},{"id":"381504","messageId":"164fa8bc-bcf1-4cce-1b49-94b64535d332@gmail.com","threadId":"51767","inReplyTo":"20190828201824.1255-3-jon@jonsimons.org","subject":"Re: [PATCH 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-29T13:12:38Z","receivedAt":"2019-08-29T13:12:41Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/28/2019 4:18 PM, Jon Simons wrote:\n> Handle a potential NULL 'sparse_oid_value' when attempting to load\n> sparse filter exclusions by blob, to avoid segfaulting later during\n> 'add_excludes_from_blob_to_list'.\n> \n> While here, uniquify the errors emitted to distinguish between the\n> case that a given OID is NULL due to an earlier failure to resolve it,\n> and when an OID resolves but parsing the sparse filter spec fails.\n\nAdding localization here also seems like a good idea. Thanks!\n\n-Stolee\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 \"file://$(pwd)/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 \"file://$(pwd)/sparse-src\" sc2 2>err &&\n> +\ttest_i18ngrep \"unable to parse sparse filter data in $(git -C sparse-src rev-parse master)\" err\n\nJust as a sanity check: when we use test_i18ngrep, how does it know how to\nseparate the part that is translated and which part is not?\n\n\ttranslated: \"unable to read sparse filter specification from\"\n\tnot translated: \"sparse:oid=master\"\n\nThanks,\n-Stolee\n"},{"id":"381506","messageId":"20190829134439.GA1797@sigill.intra.peff.net","threadId":"51767","inReplyTo":"164fa8bc-bcf1-4cce-1b49-94b64535d332@gmail.com","subject":"Re: [PATCH 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2019-08-29T13:44:39Z","receivedAt":"2019-08-29T13:44:41Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Aug 29, 2019 at 09:12:38AM -0400, Derrick Stolee wrote:\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 \"file://$(pwd)/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 \"file://$(pwd)/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> Just as a sanity check: when we use test_i18ngrep, how does it know how to\n> separate the part that is translated and which part is not?\n> \n> \ttranslated: \"unable to read sparse filter specification from\"\n> \tnot translated: \"sparse:oid=master\"\n\nIt doesn't know. By default we run the suite in LOCALE=C and it checks\nthe whole string. Under a GETTEXT_POISON build, it checks nothing at\nall.\n\nThe poison stuff is really about helping people not accidentally mark a\nplumbing string (that we expect to get parsed by a machine) as\ntranslatable. So the idea is you'd build with GETTEXT_POISON and then\nrun the test suite to see if anything breaks. But that means we also\nhave to annotate the test suite with \"yes, I know this will be gibberish\nin a poison build, but that's OK because it's meant for humans\". And\nthat's what test_i18ngrep is.\n\ntest_i18ngrep could be more clever about matching the gibberish, but\nthere's not much point. The LOCALE=C run already covered the correctness\nof checking the message.\n\n-Peff\n"},{"id":"381512","messageId":"2ed6a754-07ef-9131-2655-6d3fd3308623@gmail.com","threadId":"51767","inReplyTo":"20190829134439.GA1797@sigill.intra.peff.net","subject":"Re: [PATCH 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Derrick Stolee","fromEmail":"stolee@gmail.com","sentAt":"2019-08-29T14:28:42Z","receivedAt":"2019-08-29T14:28:45Z","isPatch":true,"sender":{"key":"stolee@gmail.com","avatar":"https://avatars.githubusercontent.com/u/570044?v=4"},"body":"On 8/29/2019 9:44 AM, Jeff King wrote:\n> On Thu, Aug 29, 2019 at 09:12:38AM -0400, Derrick Stolee wrote:\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 \"file://$(pwd)/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 \"file://$(pwd)/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>> Just as a sanity check: when we use test_i18ngrep, how does it know how to\n>> separate the part that is translated and which part is not?\n>>\n>> \ttranslated: \"unable to read sparse filter specification from\"\n>> \tnot translated: \"sparse:oid=master\"\n> \n> It doesn't know. By default we run the suite in LOCALE=C and it checks\n> the whole string. Under a GETTEXT_POISON build, it checks nothing at\n> all.\n> \n> The poison stuff is really about helping people not accidentally mark a\n> plumbing string (that we expect to get parsed by a machine) as\n> translatable. So the idea is you'd build with GETTEXT_POISON and then\n> run the test suite to see if anything breaks. But that means we also\n> have to annotate the test suite with \"yes, I know this will be gibberish\n> in a poison build, but that's OK because it's meant for humans\". And\n> that's what test_i18ngrep is.\n> \n> test_i18ngrep could be more clever about matching the gibberish, but\n> there's not much point. The LOCALE=C run already covered the correctness\n> of checking the message.\n\nThanks for clearing this up for me!\n\n-Stolee\n"}]}