Re: [PATCHv5 8/8] clone: recursive and reference option triggers submodule alternates

[Jacob Keller <jacob.keller@gmail.com>]

On Mon, Aug 15, 2016 at 2:53 PM, Stefan Beller <sbeller@google.com> wrote:
> When `--recursive` and `--reference` is given, it is reasonable to
> expect that the submodules are created with references to the submodules
> of the given alternate for the superproject.
>
>   An initial attempt to do this was presented to the mailing list, which
>   used flags that are passed around ("--super-reference") that instructed
>   the submodule clone to look for a reference in the submodules of the
>   referenced superproject. This is not well thought out, as any further
>   `submodule update` should also respect the initial setup.
>
>   When a new  submodule is added to the superproject and the alternate
>   of the superproject does not know about that submodule yet, we rather
>   error out informing the user instead of being unclear if we did or did
>   not use a submodules alternate.
>
> To solve this problem introduce new options that store the configuration
> for what the user wanted originally.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
>  Documentation/config.txt       | 12 ++++++
>  builtin/clone.c                | 19 +++++++++
>  builtin/submodule--helper.c    | 87 ++++++++++++++++++++++++++++++++++++++++++
>  t/t7408-submodule-reference.sh | 43 +++++++++++++++++++++
>  4 files changed, 161 insertions(+)
>
> diff --git a/Documentation/config.txt b/Documentation/config.txt
> index bc1c433..602f43a 100644
> --- a/Documentation/config.txt
> +++ b/Documentation/config.txt
> @@ -2837,6 +2837,18 @@ submodule.fetchJobs::
>         in parallel. A value of 0 will give some reasonable default.
>         If unset, it defaults to 1.
>
> +submodule.alternateLocation::
> +       Specifies how the submodules obtain alternates when submodules are
> +       cloned. Possible values are `no`, `superproject`.
> +       By default `no` is assumed, which doesn't add references. When the
> +       value is set to `superproject` the submodule to be cloned computes
> +       its alternates location relative to the superprojects alternate.
> +
> +submodule.alternateErrorStrategy
> +       Specifies how to treat errors with the alternates for a submodule
> +       as computed via `submodule.alternateLocation`. Possible values are
> +       `ignore`, `info`, `die`.
> +
>  tag.forceSignAnnotated::
>         A boolean to specify whether annotated tags created should be GPG signed.
>         If `--annotate` is specified on the command line, it takes
> diff --git a/builtin/clone.c b/builtin/clone.c
> index 0182665..404c5e8 100644
> --- a/builtin/clone.c
> +++ b/builtin/clone.c
> @@ -947,6 +947,25 @@ int cmd_clone(int argc, const char **argv, const char *prefix)
>                 else
>                         fprintf(stderr, _("Cloning into '%s'...\n"), dir);
>         }
> +
> +       if (option_recursive) {
> +               if (option_required_reference.nr &&
> +                   option_optional_reference.nr)
> +                       die(_("clone --recursive is not compatible with "
> +                             "both --reference and --reference-if-able"));

So if you have multiple references that don't all match we basically
just refuse to allow recursive?

Would it be better to simply assume that we want to die on missing
references instead of failing the clone here? That is, treat it so
that multiple reference and reference-if-able will die, and only info
if we got only reference-if-able?

Probably what's here is fine, and mixing reference and
reference-if-able doesn't make much sense.

Thanks,
Jake