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

[Brandon Williams <bmwill@google.com>]

On 03/14, Junio C Hamano wrote:
> Brandon Williams <bmwill@google.com> 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 <sbeller@google.com>
> >
> > Signed-off-by: Brandon Williams <bmwill@google.com>
> > ---
> >  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.

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=<pathspec>" 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