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

Thanks, I will clean up the braces and commit message.

I have to disagree with the 's/reference/dissociate/' comments. It
appears this section of option descriptions mostly copies from the
descriptions given by 'git clone -h', which outputs:
    --reference <repo>    reference repository
    --reference-if-able <repo>
                          reference repository
    --dissociate          use --reference only while cloning
It is perhaps not the best description, but it means to say when
--dissociate is used --reference is only in play for reducing network
transfer, not keeping alternates.

Those expansions *are* certainly off-putting, they make a long line
even more difficult to parse. I just searched that file for ":+" for
those types of expressions and I think a patch to fix them would be
fairly short. I'll look into making that cleanup patch.



On Tue, May 1, 2018 at 4:25 PM, Eric Sunshine [off-list ref] wrote:

On Tue, May 1, 2018 at 2:09 PM, Casey Fitzpatrick [off-list ref] wrote:

quoted

submodule: Add --dissociate option to add/update commands

s/Add/add/

quoted

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

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

quoted

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

quoted

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

quoted

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

quoted

@@ -493,6 +497,9 @@ cmd_update()
+               --dissociate)
+                       dissociate="--dissociate"
+                       ;;

@@ -550,6 +557,7 @@ cmd_update()
                ${reference:+"$reference"} \
+               ${dissociate:+"$dissociate"} \
                ${depth:+--depth "$depth"} \