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

From: Casey Fitzpatrick <kcghost@gmail.com>
Date: May 1, 2018, 21:21 UTC
Message-ID: <CAEp-SHWwOin9dbLpYaX+5UFRWpGP=k7zHU_a54AEMc6C9FDCAw@mail.gmail.com>
In-Reply-To: <CAPig+cSbOsdibX588ObTG5_O=Mmp1+pbxiaUFMx-P0jFLLUuHQ@mail.gmail.com>

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 <sunshine@sunshineco.com> wrote:

Show 57 quoted lines

> 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"} \