Re: [PATCH 3/8] clone: fix --sparse option with URLs

From: Taylor Blau <me@ttaylorr.com>
To: Derrick Stolee via GitGitGadget <gitgitgadget@gmail.com>
Cc: git@vger.kernel.org, me@ttaylorr.com, peff@peff.net,
	newren@gmail.com, Derrick Stolee <dstolee@microsoft.com>
Subject: Re: [PATCH 3/8] clone: fix --sparse option with URLs
Date: Tue, 14 Jan 2020 11:30:32 -0800	[thread overview]
Message-ID: <20200114193032.GA8593@syl.local> (raw)
In-Reply-To: <3ef8e021a586bd986e5a2a7e1bc956daf2d874d2.1579029963.git.gitgitgadget@gmail.com>

Hi Stolee,

On Tue, Jan 14, 2020 at 07:25:57PM +0000, Derrick Stolee via GitGitGadget wrote:
> From: Derrick Stolee <dstolee@microsoft.com>
>
> The --sparse option was added to the clone builtin in d89f09c (clone:
> add --sparse mode, 2019-11-21) and was tested with a local path clone
> in t1091-sparse-checkout-builtin.sh. However, due to a difference in
> how local paths are handled versus URLs, this mechanism does not work
> with URLs.

As we discussed off-list, both of us (as well as Peff) were able to
reproduce this issue. I think that this paragraph is a good description
of what's going on heee.

> Modify the test to use a "file://" URL, which would output this error
> before the code change:
>
>   Cloning into 'clone'...
>   fatal: cannot change to 'file://.../repo': No such file or directory
>   error: failed to initialize sparse-checkout

Nice, this should give us confidence that there won't be a regression
here in the future. I don't think that the explanation is complicated
enough for a single commit which introduced an expected failure, so
grouping it all together in this patch seems good to me.

> These errors are due to using a "-C <path>" option to call 'git -C
> <path> sparse-checkout init' but the URL is being given instead of
> the target directory.
>
> Update that target directory to evaluate this correctly. I have also
> manually tested that https:// URLs are handled correctly as well.
>
> Signed-off-by: Derrick Stolee <dstolee@microsoft.com>
> ---
>  builtin/clone.c                    | 2 +-
>  t/t1091-sparse-checkout-builtin.sh | 2 +-
>  2 files changed, 2 insertions(+), 2 deletions(-)
>
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 4348d962c9..2caefc44fb 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -1130,7 +1130,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_required_reference.nr || option_optional_reference.nr)
>  		setup_reference();
>
> -	if (option_sparse_checkout && git_sparse_checkout_init(repo))
> +	if (option_sparse_checkout && git_sparse_checkout_init(dir))

I agree that 'dir' is the right thing to use here. It's the string we
read from to print "Cloning into ...", which always displays the
directory relative to the cwd. Looking at the implementation in
'git_sparse_checkout_init', this matches my understanding, too.

>  		return 1;
>
>  	remote = remote_get(option_origin);
> diff --git a/t/t1091-sparse-checkout-builtin.sh b/t/t1091-sparse-checkout-builtin.sh
> index 37365dc668..58d9c69163 100755
> --- a/t/t1091-sparse-checkout-builtin.sh
> +++ b/t/t1091-sparse-checkout-builtin.sh
> @@ -90,7 +90,7 @@ test_expect_success 'init with existing sparse-checkout' '
>  '
>
>  test_expect_success 'clone --sparse' '
> -	git clone --sparse repo clone &&
> +	git clone --sparse "file://$(pwd)/repo" clone &&
>  	git -C clone sparse-checkout list >actual &&
>  	cat >expect <<-\EOF &&
>  	/*
> --
> gitgitgadget

This all looks good to me.

  Acked-by: Taylor Blau <me@ttaylorr.com>

Thanks,
Taylor