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

From: Stefan Beller <sbeller@google.com>
Date: Mar 17, 2017, 17:37 UTC
Message-ID: <CAGZ79kbvxpAEzVnNYRQ7VqzY30_oH4FGw3cH0TCwzCwCfy3HZw@mail.gmail.com>
In-Reply-To: <20170316222952.53801-9-bmwill@google.com>

On Thu, Mar 16, 2017 at 3:29 PM, Brandon Williams <bmwill@google.com> wrote:

Show 10 quoted lines

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

Show 21 quoted lines

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

Show 6 quoted lines

> +               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.

Thanks, Stefan