> 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@xxxxxxxxxx> wrote on 06/20/2016 04:09:28 AM: > From: Stefan Beller <sbeller@xxxxxxxxxx> > To: Jeff King <peff@xxxxxxxx> > Cc: Junio C Hamano <gitster@xxxxxxxxx>, Vadim Eisenberg/Haifa/ > IBM@IBMIL, "git@xxxxxxxxxxxxxxx" <git@xxxxxxxxxxxxxxx> > 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@xxxxxxxx> 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 && option_depth)) > >> + if (option_shallow_submodules == 1) > >> argv_array_push(&args, "--depth=1"); > > > > I hadn't paid much attention to this topic originally, but was surprised > > 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 the > > logic go away). But maybe something to consider if it's ever resurrected > > (or possibly if somebody runs "--shallow-submodules --depth 5" we should > > 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 why > 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' ' > >> 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 cloning > > 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 that > test on top tomorrow if you'd like to. > > Thanks, > Stefan > > > > > -Peff > -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html