Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag

From: Mahi Kolla <mahikolla@google.com>
To: Philippe Blain <levraiphilippeblain@gmail.com>
Cc: Emily Shaffer <emilyshaffer@google.com>,
	Junio C Hamano <gitster@pobox.com>,
	Mahi Kolla via GitGitGadget <gitgitgadget@gmail.com>,
	git@vger.kernel.org
Subject: Re: [PATCH v5] clone: set submodule.recurse=true if user enables feature.experimental flag
Date: Thu, 12 Aug 2021 21:12:15 -0700	[thread overview]
Message-ID: <CAN3QUFa3TRh9rvV3xS8XWd-dTvMqS=2j5Vhvp0wP33oPte9_2w@mail.gmail.com> (raw)
In-Reply-To: <c74a9d75-cd89-d020-dcb3-76509bc95284@gmail.com>

Hi all,

Thank you all for the great feedback! I'm learning a lot as a
first-time contributor :) I will be wrapping my internship this week
and will continue contributing externally.

> >>
> >>>
> >>> Since V1: Made this an opt in feature under the experimental flag. Updated tests to reflect this design change. Also updated commit message.
> >>
> >> This does not belong to the commit log message proper.  Noting the
> >> difference between the version being submitted and the pervious one
> >> this way is a way to help reviewers and is very much appreciated,
> >> but please do so below the three-dash line below your sign-off.
>
> Mahi, since you're using Gitgitgadget, you would put this "since v1"
> content in the PR description, and Gitgitgadget will append it under
> the three-dash line when you /submit :) (Do keep the CC's automatically
> added by GGG so that your next version is CC'ed to those that participated
> in earlier rounds).
>

Got it, thank you!

> >
> > It seemed to me that trying out this change on feature.experimental flag
> > was the right approach, because users with that flag have already
> > volunteered to be testers for upcoming behavior changes; this seems like
> > one such that is likely to be welcome. By contrast, turning the behavior
> > on with a separate config variable reduces the pool of testers
> > essentially to "users who know about this change" - or, to be more
> > reductive, "a handful of users at Google who we Google Git contributors
> > already know want this change". I recommended to Mahi that we stick this
> > feature under 'feature.experimental' because I really wanted to hear
> > from more users than just Googlers.
>
> I agree that we would not want yet another config variable that users would
> have to set. If people know about submodule.recurse and want to always use this
> behaviour, they already have it in their ~/.gitconfig, so they do not need a new
> variable. If they do not know about submodule.recurse, then they probably won't learn
> about this new variable either ;) That's why I suggested to Mahi that in any case it would
> be a good thing that 'git clone --recurse-submodules' would at least inform users, using
> an advice, that they might want to set submodule.recurse.
>

When discussing with the team, we revisited the feature.experimental
design. As we have yet to gain strong consensus on making this a
default config value, we've decided to ship it under a different
config value: submodule.stickyRecursiveClone. Now, if the user sets
submodule.stickyRecursiveClone=true, when they run git clone
--recurse-submodules, we will set submodule.recurse=true. While this
may mean a smaller dataset (only people who know of this flag), we can
still collect valuable data.

As for the advice message, I agree that would be a really useful
feature. I'll submit that as a different patch.

> >>
> >> Perhaps a separate (and new) configuration variable (in ~/.gitconfig
> >> perhaps) can be used as that opt-in flag (I wonder if the existing
> >> submodule.recurse variable can be that opt-in flag, though).
> >>

Unfortunately, the submodule.recurse variable can't be used as the
opt-in flag because this would cause commands to run recursively even
if developers don't have submodules in their project (aka don't run
git clone --recurse-submodules). That's why the alternate config value
seems a better choice at the moment.

Let me know what you guys think!

Best,
Mahi Kolla