[1/2] list-objects-filter: only parse sparse OID when 'have_git_dir'
diff mbox series

Message ID 20190828201824.1255-2-jon@jonsimons.org
State New
Headers show
Series
  • partial-clone: fix two issues with sparse filter handling
Related show

Commit Message

Jon Simons Aug. 28, 2019, 8:18 p.m. UTC
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      | 23 +++++++++++++++++++++++
 2 files changed, 25 insertions(+), 1 deletion(-)

Comments

Eric Sunshine Aug. 28, 2019, 9:10 p.m. UTC | #1
On Wed, Aug 28, 2019 at 4:27 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,29 @@ 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
> +       done &&

The way this is coded, a failure of the test_commit() invocation won't
fail the test overall. You need to do so manually:

    for n in 1 2 3 4
    do
        test_commit -C sparse-src "this-is-file-$n" file.$n.txt || return 1
    done &&

> +       echo "/file.1.txt" >> sparse-src/odd-files &&
> +       echo "/file.3.txt" >> sparse-src/odd-files &&
> +       echo "/file.2.txt" >> sparse-src/even-files &&
> +       echo "/file.4.txt" >> sparse-src/even-files &&

Simpler:

    test_write_lines /file1.txt /file3.txt >sparse-src/odd-files &&
    test_write_lines /file2.txt /file4.txt >sparse-src/even-files &&

> +       echo "/*" >> sparse-src/all-files &&

Style nit: drop whitespace following redirection operator.

And, using >> rather than just > here makes the test more confusing
than it need be; probably best to use >.
Jeff King Aug. 28, 2019, 11:35 p.m. UTC | #2
On Wed, Aug 28, 2019 at 04:18:23PM -0400, Jon Simons 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.
> 
> 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.

Just to provide a little more of our findings to the list: I think the
main thing going on here is that the filter options-parsing code is
shared on the client and server side (and doesn't have any idea which it
is). That's why we see the "do not complain" comment in the context
below:

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

and why it's OK to just quietly ignore this case. I don't think it's
hurting anything in practice. Whether we resolve the name or not, we
send the _original_ name to the other side (it would be a bug for us to
resolve it ourselves and send the oid).

> +test_expect_success 'partial clone with sparse filter succeeds' '
> +	git clone --no-local --no-checkout --filter=sparse:oid=master:all-files "file://$(pwd)/sparse-src" pc-all &&
> +	git clone --no-local --no-checkout --filter=sparse:oid=master:even-files "file://$(pwd)/sparse-src" pc-even &&
> +	git clone --no-local --no-checkout --filter=sparse:oid=master:odd-files "file://$(pwd)/sparse-src" pc-odd
> +'

Since you're using "--no-local", you should be able to just say
"sparse-src" without the full path or file URL.

I think Eric's style suggestions elsewhere in the thread were sensible,
too. And of course the code change itself looks good.

-Peff

Patch
diff mbox series

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..6c3aa06973 100755
--- a/t/t5616-partial-clone.sh
+++ b/t/t5616-partial-clone.sh
@@ -241,6 +241,29 @@  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
+	done &&
+	echo "/file.1.txt" >> sparse-src/odd-files &&
+	echo "/file.3.txt" >> sparse-src/odd-files &&
+	echo "/file.2.txt" >> sparse-src/even-files &&
+	echo "/file.4.txt" >> sparse-src/even-files &&
+	echo "/*" >> 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 "file://$(pwd)/sparse-src" pc-all &&
+	git clone --no-local --no-checkout --filter=sparse:oid=master:even-files "file://$(pwd)/sparse-src" pc-even &&
+	git clone --no-local --no-checkout --filter=sparse:oid=master:odd-files "file://$(pwd)/sparse-src" pc-odd
+'
+
 . "$TEST_DIRECTORY"/lib-httpd.sh
 start_httpd