Re: [PATCH v6] clone: set submodule.recurse=true if submodule.stickyRecursiveClone enabled

[Emily Shaffer <emilyshaffer@google.com>]

On Sat, Aug 14, 2021 at 11:05:41AM -0700, Junio C Hamano wrote:
> 
> "Mahi Kolla via GitGitGadget" <gitgitgadget@gmail.com> writes:
> 
> > diff --git a/builtin/clone.c b/builtin/clone.c
> > index 66fe66679c8..a08d9012243 100644
> > --- a/builtin/clone.c
> > +++ b/builtin/clone.c
> > @@ -986,6 +986,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  	struct remote *remote;
> >  	int err = 0, complete_refs_before_fetch = 1;
> >  	int submodule_progress;
> > +	int sticky_recursive_clone;
> 
> This variable does not have to be in such a wider scope, I think.
> 
> 
> >  	struct transport_ls_refs_options transport_ls_refs_options =
> >  		TRANSPORT_LS_REFS_OPTIONS_INIT;
> > @@ -1130,6 +1131,11 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
> >  					   strbuf_detach(&sb, NULL));
> >  		}
> 
> Just in this scope, where "struct string_list_item *item" and
> "struct strbuf sb" are declared, is where an extra int variable,
> which receives the configuration value, needs to exist.
> 
> Also, for a variable that is used only to receive value from
> git_config_get_bool(), immediately to be used and then never used
> again, we do not need such a long and descriptive name.
> 
> > +		if (!git_config_get_bool("submodule.stickyRecursiveClone", &sticky_recursive_clone)
> > +		    && sticky_recursive_clone) {
> > +		    string_list_append(&option_config, "submodule.recurse=true");
> > +		}
> 
> We do not need {} around a single statement block.
> 
> Taken together, perhaps like the attached.
> 
> I'll queue the patch posted as-is for now.
> 
> Thanks.
> 
> diff --git c/builtin/clone.c w/builtin/clone.c
> index 66fe66679c..c4e02d2f78 100644
> --- c/builtin/clone.c
> +++ w/builtin/clone.c
> @@ -1114,6 +1114,7 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  	if (option_recurse_submodules.nr > 0) {
>  		struct string_list_item *item;
>  		struct strbuf sb = STRBUF_INIT;
> +		int val;
>  
>  		/* remove duplicates */
>  		string_list_sort(&option_recurse_submodules);
> @@ -1130,6 +1131,10 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>  					   strbuf_detach(&sb, NULL));
>  		}
>  
> +		if (!git_config_get_bool("submodule.stickyRecursiveClone", &val) &&
> +		    val)
> +		    string_list_append(&option_config, "submodule.recurse=true");
> +
>  		if (option_required_reference.nr &&
>  		    option_optional_reference.nr)
>  			die(_("clone --recursive is not compatible with "

I like the changes you propose here. It also looks to me like you wanted
to wait for Mahi to send the updates herself, which I appreciate.

Mahi's internship ended last week and I believe she told me she's
planning to be very busy this week; so if we don't hear from her by this
time next week, I'll send a reroll with these changes (unless you want
to just take them yourself without the list churn).

 - Emily