Re: [PATCH 2/2] submodule: Add --dissociate option to add/update commands

[Eric Sunshine <sunshine@sunshineco.com>]

On Tue, May 1, 2018 at 2:09 PM, Casey Fitzpatrick <kcghost@gmail.com> wrote:
> submodule: Add --dissociate option to add/update commands

s/Add/add/

> Add --dissociate option to add and update commands, both clone helper commands
> that already have the --reference option --dissociate pairs with.
> Add documentation.
> Add tests.
>
> Signed-off-by: Casey Fitzpatrick <kcghost@gmail.com>
> ---
> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c
> @@ -1075,6 +1075,9 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
> +       if (dissociate) {
> +               argv_array_push(&cp.args, "--dissociate");
> +       }

Style: drop unnecessary braces

> @@ -1208,6 +1212,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
> +               OPT_BOOL(0, "dissociate", &dissociate,
> +                          N_("use --reference only while cloning")),

s/reference/dissociate/

> @@ -1575,6 +1584,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
> +               OPT_BOOL(0, "dissociate", &suc.dissociate,
> +                          N_("use --reference only while cloning")),

s/reference/dissociate/

> diff --git a/git-submodule.sh b/git-submodule.sh
> +               --dissociate)
> +                       dissociate="--dissociate"
> @@ -258,7 +262,7 @@ or you are unsure what this means choose another name with the '--name' option."
> -               git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"$progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${depth:+"$depth"} || exit
> +               git submodule--helper clone ${GIT_QUIET:+--quiet} ${progress:+"$progress"} --prefix "$wt_prefix" --path "$sm_path" --name "$sm_name" --url "$realrepo" ${reference:+"$reference"} ${dissociate:+"$dissociate"} ${depth:+"$depth"} || exit

I realize that you're just following existing practice in this script,
but it's a bit off-putting to see expansions for the new --progress
and --dissociate options being done via unnecessarily complex
${foobar:+"$foobar"} when the simpler $foobar would work just as well.

Just a comment; not necessarily a request for change. (A separate
preparatory cleanup patch which simplifies the existing complex
expansion expressions would be welcome but could also be considered
outside the scope of this patch series.)

> @@ -493,6 +497,9 @@ cmd_update()
> +               --dissociate)
> +                       dissociate="--dissociate"
> +                       ;;
> @@ -550,6 +557,7 @@ cmd_update()
>                 ${reference:+"$reference"} \
> +               ${dissociate:+"$dissociate"} \
>                 ${depth:+--depth "$depth"} \