Re: [BUG REPORT] git 2.9.0 clone --recursive fails on cloning a submodule
> How often do we see a depth != 1 in practice?
FYI: Here is an example of depth 10 from Swift Package Manager which is
git-based
https://github.com/apple/swift-package-manager/blob/61bdff3e9c2b8456be31861b4cda87a31440d517/Sources/Get/Git.swift
try system(Git.tool, "clone",
"--recursive", // get submodules too so that developers can use
these if they so choose
"--depth", "10",Stefan Beller <sbeller@google.com> wrote on 06/20/2016 04:09:28 AM:
Show 41 quoted lines
> From: Stefan Beller <sbeller@google.com>
> To: Jeff King <peff@peff.net>
> Cc: Junio C Hamano <gitster@pobox.com>, Vadim Eisenberg/Haifa/
> IBM@IBMIL, "git@vger.kernel.org" <git@vger.kernel.org>
> Date: 06/20/2016 04:09 AM
> Subject: Re: [BUG REPORT] git 2.9.0 clone --recursive fails on
> cloning a submodule
>
> On Sun, Jun 19, 2016 at 5:13 PM, Jeff King <peff@peff.net> wrote:
> > On Sun, Jun 19, 2016 at 01:51:56PM -0700, Junio C Hamano wrote:
> >
> >> Yup, something like this on top of d22eb04 to be merged before
> >> v2.9.1 for the maintenance track would be necessary.
> >>
> >> -- >8 --
> >> Subject: clone: do not let --depth imply --shallow-submodules
> >>
> >> In v2.9.0, we prematurely flipped the default to force cloning
> >> submodules shallowly, when the superproject is getting cloned
> >> shallowly. This is likely to fail when the upstream repositories
> >> submodules are cloned from a repository that is not prepared to
> >> serve histories that ends at a commit that is not at the tip of a
> >> branch, and we know the world is not yet ready.
> >>
> >> Use a safer default to clone the submodules fully, unless the user
> >> tells us that she knows that the upstream repository of the
> >> submodules are willing to cooperate with "--shallow-submodules"
> >> option.
> >
> > Yeah, this looks good. To minor comments:
>
> I agree, but I find the second concern a bit more than just minor.
>
> >
> >> @@ -730,8 +730,7 @@ static int checkout(void)
> >> struct argv_array args = ARGV_ARRAY_INIT;
> >> argv_array_pushl(&args, "submodule", "update", "--
> init", "--recursive", NULL);
> >>
> >> - if (option_shallow_submodules == 1
> >> - || (option_shallow_submodules == -1 &&
> >> + if (option_shallow_submodules == 1)
> >> argv_array_push(&args, "--depth=1");
> >
> > I hadn't paid much attention to this topic originally, but was
> > that "--depth 10" in the clone implies "--depth 1" in the submodule.
> > This is not really related to your patch (in fact, your patch makes
> > logic go away). But maybe something to consider if it's ever
> > (or possibly if somebody runs "--shallow-submodules --depth 5" we
Show 5 quoted lines
> > pass --depth=1; I dunno).
>
> How often do we see a depth != 1 in practice?
> I have the impression (and no data to back up my claim) that a binary
> switch for nonshallow or depth 1 would serve us just as good, which is
Show 7 quoted lines
> I did not want to ad complexity to the submodule depth.
> (What if you want submodule A with depth 2 and B with 5? In that
> case get them all shallow and deepen as appropriate, would be my answer)
>
> >
> >> -test_expect_success 'shallow clone implies shallow submodule' '
> >> +test_expect_success 'shallow clone does not imply shallow submodule'
Show 11 quoted lines
> >> test_when_finished "rm -rf super_clone" &&
> >> - git clone --recurse-submodules --depth 2 "file://$pwd/."
> super_clone &&
> >> + git clone --recurse-submodules --depth 2 --shallow-submodules "
> file://$pwd/." super_clone &&
> >> (
> >> cd super_clone &&
> >> git log --oneline >lines &&
> >
> > We are not really testing "does not imply" here, but "passing
> > --shallow-submodules works". The "does not imply" test would be
Show 6 quoted lines
> > without the option and checking that the resulting submodules are not
> > shallow.
>
> In case we want to be sure that it works for 2.9.1, i.e. we treat it
> as a regression,
> we need to test the "does not imply" a bit more I would think. I cansend
Show 8 quoted lines
> test on top tomorrow if you'd like to.
>
> Thanks,
> Stefan
>
> >
> > -Peff
>
