Subject: Re: [PATCH v3 07/10] clone: add --submodule-spec=<pathspec> switch

From: Junio C Hamano <[email-masked]>
Date: Tue, 14 Mar 2017 12:38:01 -0700

Brandon Williams <[email-masked]> writes:

> The new switch passes the pathspec to `git submodule update
> --init-active` which is called after the actual clone is done.
>
> Additionally this configures the submodule.active option to
> be the given pathspec, such that any future invocation of
> `git submodule update --init-active` will keep up
> with the pathspec.
>
> Based on a patch by Stefan Beller <[email-masked]>
>
> Signed-off-by: Brandon Williams <[email-masked]>
> ---
>  Documentation/git-clone.txt | 23 ++++++++++-----
>  builtin/clone.c             | 36 +++++++++++++++++++++--
>  t/t7400-submodule-basic.sh  | 70 +++++++++++++++++++++++++++++++++++++++++++++
>  3 files changed, 120 insertions(+), 9 deletions(-)
>
> diff --git a/Documentation/git-clone.txt b/Documentation/git-clone.txt
> index 35cc34b2f..9692eab30 100644
> --- a/Documentation/git-clone.txt
> +++ b/Documentation/git-clone.txt
> @@ -15,7 +15,8 @@ SYNOPSIS
>  	  [--dissociate] [--separate-git-dir <git dir>]
>  	  [--depth <depth>] [--[no-]single-branch]
>  	  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
> -	  [--jobs <n>] [--] <repository> [<directory>]
> +	  [--submodule-spec <pathspec>] [--jobs <n>] [--]
> +	  <repository> [<directory>]

Hmph.  Can we then make "--recurse-submodules" an obsolete way to
spell "--submodule-spec ."?  I am not actively suggesting to
deprecate it; I am trying to see if there are semantic differences
between the two.

I am also wondering "--recurse-submodules=<pathspec>" would be a
better UI, instead of introducing yet another option.

> @@ -217,12 +218,20 @@ objects from the source repository into a pack in the cloned repository.
>  
>  --recursive::
>  --recurse-submodules::
> -	After the clone is created, initialize all submodules within,
> -	using their default settings. This is equivalent to running
> -	`git submodule update --init --recursive` immediately after
> -	the clone is finished. This option is ignored if the cloned
> -	repository does not have a worktree/checkout (i.e. if any of
> -	`--no-checkout`/`-n`, `--bare`, or `--mirror` is given)
> +	After the clone is created, initialize and clone all submodules
> +	within, using their default settings. This is equivalent to
> +	running `git submodule update --recursive --init` immediately
> +	after the clone is finished. This option is ignored if the
> +	cloned repository does not have a worktree/checkout (i.e.  if
> +	any of `--no-checkout`/`-n`, `--bare`, or `--mirror` is given)

With reflowing it is unnecessarily harder to spot what got changed.
"and clone" is inserted, "--init" and "--recursive" were swapped.
Any other changes?

> diff --git a/builtin/clone.c b/builtin/clone.c
> index 3f63edbbf..c6731379b 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -56,6 +56,16 @@ static struct string_list option_required_reference = STRING_LIST_INIT_NODUP;
>  static struct string_list option_optional_reference = STRING_LIST_INIT_NODUP;
>  static int option_dissociate;
>  static int max_jobs = -1;
> +static struct string_list submodule_spec;
> +
> +static int submodule_spec_cb(const struct option *opt, const char *arg, int unset)
> +{
> +	if (unset)
> +		return -1;
> +
> +	string_list_append((struct string_list *)opt->value, arg);
> +	return 0;
> +}

Hmph,  doesn't OPT_STRING_LIST work for this thing?

> +	if (submodule_spec.nr > 0) {
> +		struct string_list_item *item;
> +		struct strbuf sb = STRBUF_INIT;
> +		for_each_string_list_item(item, &submodule_spec) {
> +			strbuf_addf(&sb, "submodule.active=%s",
> +				    item->string);
> +			string_list_append(&option_config,
> +					   strbuf_detach(&sb, NULL));
> +		}
> +	}

OK.  Each pathspec becomes submodule.active in the newly created
repository.