[PATCH v2 0/2] partial-clone: fix two issues with sparse filter handling

[PATCH v2 0/2] partial-clone: fix two issues with sparse filter handling

[Jon Simons]

Included here are two fixes for partial cloning with sparse filters.
These issues were uncovered in early testing internally at GitHub,
where Taylor and Peff have provided early offlist review feedback.

In this second revision I've adopted Eric and Peff's additional
feedback on some of the test details: fixes, whitespace adjustment,
and some further simplifications -- thanks!

Jon Simons (2):
  list-objects-filter: only parse sparse OID when 'have_git_dir'
  list-objects-filter: handle unresolved sparse filter OID

 list-objects-filter-options.c |  3 ++-
 list-objects-filter.c         |  6 +++++-
 t/t5616-partial-clone.sh      | 28 ++++++++++++++++++++++++++++
 3 files changed, 35 insertions(+), 2 deletions(-)

-- 
2.23.0.37.g745f681289.dirty

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

[Jon Simons]

Fix a bug in partial cloning with sparse filters by ensuring to check
for 'have_git_dir' before attempting to resolve the sparse filter OID.

Otherwise the client will trigger:

    BUG: refs.c:1851: attempting to get main_ref_store outside of repository

when attempting to git clone with a sparse filter.

Note that this fix is the minimal one which avoids the BUG and allows
for the clone to complete successfully:

There is an open question as to whether there should be any attempt
to resolve the OID provided by the client in this context, as a filter
for the clone to be used on the remote side.  For cases where local
and remote OID resolutions differ, resolving on the client side could
be considered a bug.  For now, the minimal approach here is used to
unblock further testing for partial clones with sparse filters, while
a more invasive fix could make sense to pursue as a future direction.

t5616 is updated to demonstrate the change.

Signed-off-by: Jon Simons <jon@jonsimons.org>
---
 list-objects-filter-options.c |  3 ++-
 t/t5616-partial-clone.sh      | 21 +++++++++++++++++++++
 2 files changed, 23 insertions(+), 1 deletion(-)

diff --git a/list-objects-filter-options.c b/list-objects-filter-options.c
index 1cb20c659c..aaba312edb 100644
--- a/list-objects-filter-options.c
+++ b/list-objects-filter-options.c
@@ -71,7 +71,8 @@ static int gently_parse_list_objects_filter(
 		 * command, but DO NOT complain if we don't have the blob or
 		 * ref locally.
 		 */
-		if (!get_oid_with_context(the_repository, v0, GET_OID_BLOB,
+		if (have_git_dir() &&
+		    !get_oid_with_context(the_repository, v0, GET_OID_BLOB,
 					  &sparse_oid, &oc))
 			filter_options->sparse_oid_value = oiddup(&sparse_oid);
 		filter_options->choice = LOFC_SPARSE_OID;
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 565254558f..0c6f365bf2 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -241,6 +241,27 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
 	! grep "?$(cat blob)" missing_after
 '
 
+test_expect_success 'setup src repo for sparse filter' '
+	git init sparse-src &&
+	git -C sparse-src config --local uploadpack.allowfilter 1 &&
+	git -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&
+	for n in 1 2 3 4
+	do
+		test_commit -C sparse-src "this-is-file-$n" file.$n.txt || return 1
+	done &&
+	test_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&
+	test_write_lines /file2.txt /file4.txt >sparse-src/even-files &&
+	test_write_lines /* >sparse-src/all-files &&
+	git -C sparse-src add odd-files even-files all-files &&
+	git -C sparse-src commit -m "some sparse checkout files"
+'
+
+test_expect_success 'partial clone with sparse filter succeeds' '
+	git clone --no-local --no-checkout --filter=sparse:oid=master:all-files sparse-src pc-all &&
+	git clone --no-local --no-checkout --filter=sparse:oid=master:even-files sparse-src pc-even &&
+	git clone --no-local --no-checkout --filter=sparse:oid=master:odd-files sparse-src pc-odd
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.23.0.37.g745f681289.dirty

[PATCH v2 2/2] list-objects-filter: handle unresolved sparse filter OID

[Jon Simons]

Handle a potential NULL 'sparse_oid_value' when attempting to load
sparse filter exclusions by blob, to avoid segfaulting later during
'add_excludes_from_blob_to_list'.

While here, uniquify the errors emitted to distinguish between the
case that a given OID is NULL due to an earlier failure to resolve it,
and when an OID resolves but parsing the sparse filter spec fails.

t5616 is updated to demonstrate the change.

Co-authored-by: Jeff King <peff@peff.net>
Signed-off-by: Jeff King <peff@peff.net>
Signed-off-by: Jon Simons <jon@jonsimons.org>
---
 list-objects-filter.c    | 6 +++++-
 t/t5616-partial-clone.sh | 7 +++++++
 2 files changed, 12 insertions(+), 1 deletion(-)

diff --git a/list-objects-filter.c b/list-objects-filter.c
index 36e1f774bc..252fae5d4e 100644
--- a/list-objects-filter.c
+++ b/list-objects-filter.c
@@ -464,9 +464,13 @@ static void *filter_sparse_oid__init(
 {
 	struct filter_sparse_data *d = xcalloc(1, sizeof(*d));
 	d->omits = omitted;
+	if (!filter_options->sparse_oid_value)
+		die(_("unable to read sparse filter specification from %s"),
+		      filter_options->filter_spec);
 	if (add_excludes_from_blob_to_list(filter_options->sparse_oid_value,
 					   NULL, 0, &d->el) < 0)
-		die("could not load filter specification");
+		die(_("unable to parse sparse filter data in %s"),
+		      oid_to_hex(filter_options->sparse_oid_value));
 
 	ALLOC_GROW(d->array_frame, d->nr + 1, d->alloc);
 	d->array_frame[d->nr].defval = 0; /* default to include */
diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
index 0c6f365bf2..495998b7fb 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -262,6 +262,13 @@ test_expect_success 'partial clone with sparse filter succeeds' '
 	git clone --no-local --no-checkout --filter=sparse:oid=master:odd-files sparse-src pc-odd
 '
 
+test_expect_success 'partial clone with unresolvable sparse filter fails cleanly' '
+	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master:sparse-filter sparse-src sc1 2>err &&
+	test_i18ngrep "unable to read sparse filter specification from sparse:oid=master:sparse-filter" err &&
+	test_must_fail git clone --no-local --no-checkout --filter=sparse:oid=master sparse-src sc2 2>err &&
+	test_i18ngrep "unable to parse sparse filter data in $(git -C sparse-src rev-parse master)" err
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd
 
-- 
2.23.0.37.g745f681289.dirty

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

[Eric Sunshine]

On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:
> Fix a bug in partial cloning with sparse filters by ensuring to check
> for 'have_git_dir' before attempting to resolve the sparse filter OID.
> [...]
> Signed-off-by: Jon Simons <jon@jonsimons.org>
> ---
> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
> @@ -241,6 +241,27 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
> +test_expect_success 'setup src repo for sparse filter' '
> +       git init sparse-src &&
> +       git -C sparse-src config --local uploadpack.allowfilter 1 &&
> +       git -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&
> +       for n in 1 2 3 4
> +       do
> +               test_commit -C sparse-src "this-is-file-$n" file.$n.txt || return 1
> +       done &&
> +       test_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&
> +       test_write_lines /file2.txt /file4.txt >sparse-src/even-files &&
> +       test_write_lines /* >sparse-src/all-files &&

Hmm, does this work correctly? I would expect the /* to expand to all
names at the root of your filesystem, which isn't what you want. You
want the literal string "/*", which means you should quote it (with
double quotes inside the test body). I'd also suggest using the simple
'echo' for this one as you did in v1 since it's more obvious that
you're writing just a single line to the file, whereas using
test_write_lines() has the potential to confuses readers.

> +       git -C sparse-src add odd-files even-files all-files &&
> +       git -C sparse-src commit -m "some sparse checkout files"
> +'

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

[Jon Simons]

On 8/29/19 3:45 PM, Eric Sunshine wrote:
> On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:
>> Fix a bug in partial cloning with sparse filters by ensuring to check
>> for 'have_git_dir' before attempting to resolve the sparse filter OID.
>> [...]
>> Signed-off-by: Jon Simons <jon@jonsimons.org>
>> ---
>> diff --git a/t/t5616-partial-clone.sh b/t/t5616-partial-clone.sh
>> @@ -241,6 +241,27 @@ test_expect_success 'fetch what is specified on CLI even if already promised' '
>> +test_expect_success 'setup src repo for sparse filter' '
>> +       git init sparse-src &&
>> +       git -C sparse-src config --local uploadpack.allowfilter 1 &&
>> +       git -C sparse-src config --local uploadpack.allowanysha1inwant 1 &&
>> +       for n in 1 2 3 4
>> +       do
>> +               test_commit -C sparse-src "this-is-file-$n" file.$n.txt || return 1
>> +       done &&
>> +       test_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&
>> +       test_write_lines /file2.txt /file4.txt >sparse-src/even-files &&
>> +       test_write_lines /* >sparse-src/all-files &&
> 
> Hmm, does this work correctly? I would expect the /* to expand to all
> names at the root of your filesystem, which isn't what you want. You
> want the literal string "/*", which means you should quote it (with
> double quotes inside the test body). I'd also suggest using the simple
> 'echo' for this one as you did in v1 since it's more obvious that
> you're writing just a single line to the file, whereas using
> test_write_lines() has the potential to confuses readers.

Oof, thanks -- yes, this is incorrect.  I will revert this back to
'echo'.  I misapplied your initial suggesttion to all three lines.


-Jon

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

[Eric Sunshine]

On Thu, Aug 29, 2019 at 7:12 PM Jon Simons <jon@jonsimons.org> wrote:
> On 8/29/19 3:45 PM, Eric Sunshine wrote:
> > On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:
> >> +       test_write_lines /* >sparse-src/all-files &&
> >
> > Hmm, does this work correctly? I would expect the /* to expand to all
> > names at the root of your filesystem, which isn't what you want. You
> > want the literal string "/*", which means you should quote it (with
> > double quotes inside the test body). [...]
>
> Oof, thanks -- yes, this is incorrect.  I will revert this back to
> 'echo'.  I misapplied your initial suggesttion to all three lines.

Curious. Did the test still pass even with the unquoted "/*"? If so,
does that indicate a flaw in the test or somewhere else?

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

[Jon Simons]

On 8/29/19 4:48 PM, Eric Sunshine wrote:
> On Thu, Aug 29, 2019 at 7:12 PM Jon Simons <jon@jonsimons.org> wrote:
>> On 8/29/19 3:45 PM, Eric Sunshine wrote:
>>> On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:
>>>> +       test_write_lines /* >sparse-src/all-files &&
>>>
>>> Hmm, does this work correctly? I would expect the /* to expand to all
>>> names at the root of your filesystem, which isn't what you want. You
>>> want the literal string "/*", which means you should quote it (with
>>> double quotes inside the test body). [...]
>>
>> Oof, thanks -- yes, this is incorrect.  I will revert this back to
>> 'echo'.  I misapplied your initial suggesttion to all three lines.
> 
> Curious. Did the test still pass even with the unquoted "/*"? If so,
> does that indicate a flaw in the test or somewhere else?

Yes, the test also passes with the unquoted "/*".  I think this showcases
that the test really is a bare minimal sanity check that clones providing
a sparse filter argument are able to run to completion.  As-is the test
does not further assert any behavior of the filter, just that the BUG is
avoided.


-Jon