On 03/17, Stefan Beller wrote:
> On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams <[email protected]> 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
