threads / patch / 51777

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

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

## tl;dr

7 messages between Aug 29, 2019 and Aug 30, 2019. Diffs are folded; open one to read it.

replies: 6people: 2as markdown or json

Jon Simons· Aug 29, 2019, 22:38 UTC · lore

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

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

Jon Simons· Aug 29, 2019, 22:38 UTC · re: Jon Simons · lore

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

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(-)

Show changes to 2 files +23 −1

list-objects-filter-options.c, t/t5616-partial-clone.sh

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

Eric Sunshine· Aug 29, 2019, 22:45 UTC · re: Jon Simons · lore

On Thu, Aug 29, 2019 at 6:38 PM Jon Simons <jon@jonsimons.org> wrote:

Show 18 quoted lines

> 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"
> +'

Jon Simons· Aug 29, 2019, 23:12 UTC · re: Eric Sunshine · lore

On 8/29/19 3:45 PM, Eric Sunshine wrote:

Show 27 quoted lines

> 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

Eric Sunshine· Aug 29, 2019, 23:48 UTC · re: Jon Simons · lore

On Thu, Aug 29, 2019 at 7:12 PM Jon Simons <jon@jonsimons.org> wrote:

Show 11 quoted lines

> 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?

Jon Simons· Aug 30, 2019, 00:24 UTC · re: Eric Sunshine · lore

On 8/29/19 4:48 PM, Eric Sunshine wrote:

Show 15 quoted lines

> 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

Jon Simons· Aug 29, 2019, 22:38 UTC · re: Jon Simons · lore

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

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(-)

Show changes to 2 files +12 −1

list-objects-filter.c, t/t5616-partial-clone.sh

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