| Message ID | 3ef8e021a586bd986e5a2a7e1bc956daf2d874d2.1579029963.git.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | Harden the sparse-checkout builtin | expand |
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
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)) 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 && /*