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

[Stefan Beller]

On Wed, Mar 15, 2017 at 4:08 PM, Brandon Williams  wrote:
> On 03/14, Junio C Hamano wrote:
>> Brandon Williams  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 
>> >
>> > Signed-off-by: Brandon Williams 
>> > ---
>> >  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 ]
>> >   [--depth ] [--[no-]single-branch]
>> >   [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
>> > - [--jobs ] [--]  []
>> > + [--submodule-spec ] [--jobs ] [--]
>> > +  []
>>
>> 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.
>
> We can if you think that would be better.  That way if at clone time you
> say "I want the submodules too" that your default config tracks all
> submodules even new ones yet to be added.
>
>>
>> I am also wondering "--recurse-submodules=" would be a
>> better UI, instead of introducing yet another option.
>
> Yeah we could do that, have --recurse-submodules be a repeated option
> and if you don't specify a value it defaults to "."
>
> Any thoughts on this Stefan?

I think when I first tried to tackle this problem, I realized that the recursing
beyond the first level is orthogonal to the selection of submodules in the
first level.

Consider the following submodules

/sub1
/sub1/nestedA
/sub2
/sub2/nestedB

To select sub1
  git clone --submodule-spec=sub1

To select sub1 +nestedA
  git clone --submodule-spec=sub1 --recurse-submodules

To select sub1+sub2
  git clone --submodule-spec=. --no-recurse-submodules

How to select sub1+nestedA+sub2, but not nestedB
  not possible in one command

I wonder if we want to be able to differentiate these cases at all,
I guess if you want sub1, you also care about nestedB ?

(Or is there any way to recurse pathspecs,
which could be gitattributes)

That said, I optimized for the complex case, which we might not have
and a combination into one switch makes sense, such that
  git clone --recurse-submodules=
is sufficient and turns on recursion beyond the first level.

Thanks,
Stefan

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

[Brandon Williams]

On 03/14, Junio C Hamano wrote:
> Brandon Williams  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 
> >
> > Signed-off-by: Brandon Williams 
> > ---
> >  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 ]
> >   [--depth ] [--[no-]single-branch]
> >   [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
> > - [--jobs ] [--]  []
> > + [--submodule-spec ] [--jobs ] [--]
> > +  []
> 
> 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.

We can if you think that would be better.  That way if at clone time you
say "I want the submodules too" that your default config tracks all
submodules even new ones yet to be added.

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

Yeah we could do that, have --recurse-submodules be a repeated option
and if you don't specify a value it defaults to "."

Any thoughts on this Stefan?

> 
> > @@ -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?

No other changes, it just reads a little bit clearer now IMO.

> 
> > 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?

You're right, I'll change to that.

-- 
Brandon Williams

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

[Junio C Hamano]

Brandon Williams  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 
>
> Signed-off-by: Brandon Williams 
> ---
>  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 ]
> [--depth ] [--[no-]single-branch]
> [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
> -   [--jobs ] [--]  []
> +   [--submodule-spec ] [--jobs ] [--]
> +[]

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=" 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, _spec) {
> + strbuf_addf(, "submodule.active=%s",
> + item->string);
> + string_list_append(_config,
> +strbuf_detach(, NULL));
> + }
> + }

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

[PATCH v3 07/10] clone: add --submodule-spec= switch

[Brandon Williams]

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 

Signed-off-by: Brandon Williams 
---
 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 ]
  [--depth ] [--[no-]single-branch]
  [--recursive | --recurse-submodules] [--[no-]shallow-submodules]
- [--jobs ] [--]  []
+ [--submodule-spec ] [--jobs ] [--]
+  []
 
 DESCRIPTION
 ---
@@ -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)
+
+--submodule-spec::
+   After the clone is created, initialize and clone specified
+   submodules within, using their default settings. It is possible
+   to give multiple specifications by giving this argument multiple
+   times. This is equivalent to configuring `submodule.active`
+   and then running `git submodule update --init-active`
+   immediately after the clone is finished.
 
 --[no-]shallow-submodules::
All submodules which are cloned will be shallow with a depth of 1.
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;
+}
 
 static struct option builtin_clone_options[] = {
OPT__VERBOSITY(_verbosity),
@@ -112,6 +122,9 @@ static struct option builtin_clone_options[] = {
TRANSPORT_FAMILY_IPV4),
OPT_SET_INT('6', "ipv6", , N_("use IPv6 addresses only"),
TRANSPORT_FAMILY_IPV6),
+   OPT_CALLBACK(0, "submodule-spec", _spec, N_(""),
+   N_("clone specific submodules. Pass multiple times for 
complex pathspecs"),
+   submodule_spec_cb),
OPT_END()
 };
 
@@ -733,13 +746,21 @@ static int checkout(int submodule_progress)
err |= run_hook_le(NULL, "post-checkout", sha1_to_hex(null_sha1),
   sha1_to_hex(sha1), "1", NULL);
 
-   if (!err && option_recursive) {
+   if (!err && (option_recursive || submodule_spec.nr > 0)) {
struct argv_array args = ARGV_ARRAY_INIT;
-   argv_array_pushl(, "submodule", "update", "--init", 
"--recursive", NULL);
+   argv_array_pushl(, "submodule", "update", NULL);
+
+   if (submodule_spec.nr > 0)
+   argv_array_pushf(, "--init-active");
+   else
+   argv_array_pushf(, "--init");
 
if (option_shallow_submodules == 1)
argv_array_push(, "--depth=1");
 
+   if (option_recursive)
+   argv_array_pushf(, "--recursive");
+
if (max_jobs != -1)
argv_array_pushf(, "--jobs=%d", max_jobs);
 
@@ -887,6 +908,17 @@ int cmd_clone(int argc, const char **argv, const char 
*prefix)
option_no_checkout = 1;
}
 
+   if (submodule_spec.nr > 0) {
+