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 <sunsh...@sunshineco.com> wrote:
> On Tue, May 1, 2018 at 2:09 PM, Casey Fitzpatrick <kcgh...@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 <kcgh...@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"} \
