{"thread":{"id":"51777","subject":"Re: [PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","startedAt":"2019-08-29T22:45:37Z","lastAt":"2019-08-30T00:24:40Z","messageCount":7,"participants":["Eric Sunshine","Jon Simons"],"isPatch":true,"patchVersion":2,"patchTotal":2},"messages":[{"id":"381546","messageId":"CAPig+cQDPsXvOYa3HYo_cYeVq4PhJE_2RGw9n+92=t3BA0-yaw@mail.gmail.com","threadId":"51777","inReplyTo":"20190829223811.12072-2-jon@jonsimons.org","subject":"Re: [PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-29T22:45:21Z","receivedAt":"2019-08-29T22:45:37Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2019 at 6:38 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,27 @@ 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 || return 1\n> +       done &&\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> +       test_write_lines /* >sparse-src/all-files &&\n\nHmm, does this work correctly? I would expect the /* to expand to all\nnames at the root of your filesystem, which isn't what you want. You\nwant the literal string \"/*\", which means you should quote it (with\ndouble quotes inside the test body). I'd also suggest using the simple\n'echo' for this one as you did in v1 since it's more obvious that\nyou're writing just a single line to the file, whereas using\ntest_write_lines() has the potential to confuses readers.\n\n> +       git -C sparse-src add odd-files even-files all-files &&\n> +       git -C sparse-src commit -m \"some sparse checkout files\"\n> +'\n"},{"id":"381547","messageId":"20190829223811.12072-2-jon@jonsimons.org","threadId":"51777","inReplyTo":"20190829223811.12072-1-jon@jonsimons.org","subject":"[PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T22:38:10Z","receivedAt":"2019-08-29T22:46:20Z","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      | 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..0c6f365bf2 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+\ttest_write_lines /* >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":"381548","messageId":"20190829223811.12072-3-jon@jonsimons.org","threadId":"51777","inReplyTo":"20190829223811.12072-1-jon@jonsimons.org","subject":"[PATCH v2 2/2] list-objects-filter: handle unresolved sparse filter OID","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T22:38:11Z","receivedAt":"2019-08-29T22:46:21Z","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 0c6f365bf2..495998b7fb 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":"381549","messageId":"20190829223811.12072-1-jon@jonsimons.org","threadId":"51777","inReplyTo":null,"subject":"[PATCH v2 0/2] partial-clone: fix two issues with sparse filter handling","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T22:38:09Z","receivedAt":"2019-08-29T22:46:21Z","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\nIn this second revision I've adopted Eric and Peff's additional\nfeedback on some of the test details: fixes, whitespace adjustment,\nand some further simplifications -- thanks!\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":"381551","messageId":"d8f1b891-4ff7-9815-2405-76e53506027c@jonsimons.org","threadId":"51777","inReplyTo":"CAPig+cQDPsXvOYa3HYo_cYeVq4PhJE_2RGw9n+92=t3BA0-yaw@mail.gmail.com","subject":"Re: [PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-29T23:12:16Z","receivedAt":"2019-08-29T23:12:20Z","isPatch":true,"sender":{"key":"jon@jonsimons.org","avatar":"https://avatars.githubusercontent.com/u/118440?v=4"},"body":"On 8/29/19 3:45 PM, Eric Sunshine wrote:\n> On Thu, Aug 29, 2019 at 6:38 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,27 @@ 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 || return 1\n>> +       done &&\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>> +       test_write_lines /* >sparse-src/all-files &&\n> \n> Hmm, does this work correctly? I would expect the /* to expand to all\n> names at the root of your filesystem, which isn't what you want. You\n> want the literal string \"/*\", which means you should quote it (with\n> double quotes inside the test body). I'd also suggest using the simple\n> 'echo' for this one as you did in v1 since it's more obvious that\n> you're writing just a single line to the file, whereas using\n> test_write_lines() has the potential to confuses readers.\n\nOof, thanks -- yes, this is incorrect.  I will revert this back to\n'echo'.  I misapplied your initial suggesttion to all three lines.\n\n\n-Jon\n"},{"id":"381555","messageId":"CAPig+cTUEY9Z7KBqXXcvrjBzRVQJ6pJUi72BrevquN1xfvwZag@mail.gmail.com","threadId":"51777","inReplyTo":"d8f1b891-4ff7-9815-2405-76e53506027c@jonsimons.org","subject":"Re: [PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2019-08-29T23:48:15Z","receivedAt":"2019-08-29T23:48:30Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Thu, Aug 29, 2019 at 7:12 PM Jon Simons <jon@jonsimons.org> wrote:\n> On 8/29/19 3:45 PM, Eric Sunshine wrote:\n> > On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:\n> >> +       test_write_lines /* >sparse-src/all-files &&\n> >\n> > Hmm, does this work correctly? I would expect the /* to expand to all\n> > names at the root of your filesystem, which isn't what you want. You\n> > want the literal string \"/*\", which means you should quote it (with\n> > double quotes inside the test body). [...]\n>\n> Oof, thanks -- yes, this is incorrect.  I will revert this back to\n> 'echo'.  I misapplied your initial suggesttion to all three lines.\n\nCurious. Did the test still pass even with the unquoted \"/*\"? If so,\ndoes that indicate a flaw in the test or somewhere else?\n"},{"id":"381558","messageId":"c5ec26a3-c9a3-89ce-e83b-a7a831203770@jonsimons.org","threadId":"51777","inReplyTo":"CAPig+cTUEY9Z7KBqXXcvrjBzRVQJ6pJUi72BrevquN1xfvwZag@mail.gmail.com","subject":"Re: [PATCH v2 1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'","fromName":"Jon Simons","fromEmail":"jon@jonsimons.org","sentAt":"2019-08-30T00:24:37Z","receivedAt":"2019-08-30T00:24:40Z","isPatch":true,"sender":{"key":"jon@jonsimons.org","avatar":"https://avatars.githubusercontent.com/u/118440?v=4"},"body":"On 8/29/19 4:48 PM, Eric Sunshine wrote:\n> On Thu, Aug 29, 2019 at 7:12 PM Jon Simons <jon@jonsimons.org> wrote:\n>> On 8/29/19 3:45 PM, Eric Sunshine wrote:\n>>> On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:\n>>>> +       test_write_lines /* >sparse-src/all-files &&\n>>>\n>>> Hmm, does this work correctly? I would expect the /* to expand to all\n>>> names at the root of your filesystem, which isn't what you want. You\n>>> want the literal string \"/*\", which means you should quote it (with\n>>> double quotes inside the test body). [...]\n>>\n>> Oof, thanks -- yes, this is incorrect.  I will revert this back to\n>> 'echo'.  I misapplied your initial suggesttion to all three lines.\n> \n> Curious. Did the test still pass even with the unquoted \"/*\"? If so,\n> does that indicate a flaw in the test or somewhere else?\n\nYes, the test also passes with the unquoted \"/*\".  I think this showcases\nthat the test really is a bare minimal sanity check that clones providing\na sparse filter argument are able to run to completion.  As-is the test\ndoes not further assert any behavior of the filter, just that the BUG is\navoided.\n\n\n-Jon\n"}]}