Re: [PATCH v4 08/10] clone: teach --recurse-submodules to optionally take a pathspec

[Brandon Williams <bmwill@google.com>]

On 03/17, Stefan Beller wrote:
> On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams <bmwill@google.com> wrote:
> > Teach clone --recurse-submodules to optionally take a pathspec argument
> > which describes which submodules should be recursively initialized and
> > cloned.  If no pathspec is provided, --recurse-submodules will
> > recursively initialize and clone all submodules by using a default
> > pathspec of ".".  In order to construct more complex pathspecs,
> > --recurse-submodules can be given multiple times.
> >
> > Additionally this configures the 'submodule.active' configuration option
> > to be the given pathspec, such that any future invocation of `git
> > submodule update` will keep up with the pathspec.
> 
>   Additionally the switch '--recurse' is removed from the Documentation
>   as well as marked hidden in the options array, to streamline the options
>   for submodules.  A simple '--recurse' doesn't convey what is being
>   recursed, e.g. it could mean directories or trees (c.f. ls-tree)
>   In a lot of other commands we already have '--recurse-submodules'
>   to mean recursing into submodules, so advertise this spelling here
>   as the genuine option.
> 
> would also be worth mentioning?
> 

Yeah I can just add this into the commit msg.

> 
> >  static int option_no_checkout, option_bare, option_mirror, option_single_branch = -1;
> > -static int option_local = -1, option_no_hardlinks, option_shared, option_recursive;
> > +static int option_local = -1, option_no_hardlinks, option_shared;
> >  static int option_shallow_submodules;
> >  static int deepen;
> >  static char *option_template, *option_depth, *option_since;
> > @@ -56,6 +56,22 @@ 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 option_recurse_submodules = STRING_LIST_INIT_NODUP;
> > +
> > +static int recurse_submodules_cb(const struct option *opt,
> > +                                const char *arg, int unset)
> > +{
> > +       if (unset)
> > +               return -1;
> > +
> > +       if (arg)
> > +               string_list_append((struct string_list *)opt->value, arg);
> > +       else
> 
> in this case I'd rather set the removed (int) option_recursive, because, then
> we would not need to sort and remove duplicates later on.
> Instead we can pass the string list literally to the config setter.
> (and in case option_recursive is set, we add an additional single
> "." then)

That's just one more thing to worry about though.  This felt a little
bit cleaner than doing more special casing.

> 
> > +               string_list_append((struct string_list *)opt->value,
> > +                                  (const char *)opt->defval);
> > +
> > +       return 0;
> > +}
> >
> 
> >
> > -       if (!err && option_recursive) {
> > +       if (!err && (option_recurse_submodules.nr > 0)) {
> 
> Well, checks like these would become more tangled.
> So maybe we could set option_recursive unconditionally
> in the callback (unless unset was given, then we reset it to 0)
> and later have a check if we need to add "." (when the string list
> is empty).
> 
> Speaking of unset, this seems like a regression here, as the callback
> would error out to "git clone --no-recurse", which is a valid use case
> currently? (Searching for "git clone --no-recurse" yields no results
> via Google, so maybe this use case is not so valid)
> 
> To get the behavior as is for unset we could just clear the string
> list instead of returning -1.

Yeah I can do that.

> 
> Thanks,
> Stefan

-- 
Brandon Williams