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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: May 1, 2018, 20:25 UTC
Message-ID: <CAPig+cSbOsdibX588ObTG5_O=Mmp1+pbxiaUFMx-P0jFLLUuHQ@mail.gmail.com>
In-Reply-To: <20180501180908.17443-3-kcghost@gmail.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/

Show 12 quoted lines

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

Show 6 quoted lines

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

Show 8 quoted lines

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