git-submodule is missing --dissociate option

This seems to be a hole in the git feature set. I believe it is fairly easily worked around, but it would be best to provide the option for ease of use (and maybe performance?).

git clone has both a --reference feature and a --dissociate option, with dissociate allowing for a reference to *only* speed up network transfers rather than have the resulting clone rely upon the reference always being there (creates an independent repo).
But git submodule only allows for --reference, so there isn't a an option to make a speedy independent submodule clone in one shot:
https://git-scm.com/docs/git-submodule
I checked the latest online documentation (currently at 2.16.3) and the documentation in the latest sources (almost 2.18):
https://github.com/git/git/blob/next/Documentation/git-submodule.txt

As far as I am aware this can be worked around with 'git repack -a' and manual removal of the objects/info/alternates file afterward. Though I don't know if this results in a less speedy clone than dissociate would.

It also seems to be missing "--progress", and I imagine others. Perhaps submodule add/update should be reworked to automatically accept all the options that clone would?

Quoted 19 lines

This seems to be a hole in the git feature set. I believe it is fairly easily worked around, but it would be best to provide the option for ease of use (and maybe performance?).

git clone has both a --reference feature and a --dissociate option, with dissociate allowing for a reference to *only* speed up network transfers rather than have the resulting clone rely upon the reference always being there (creates an independent repo).
But git submodule only allows for --reference, so there isn't a an option to make a speedy independent submodule clone in one shot:
https://git-scm.com/docs/git-submodule
I checked the latest online documentation (currently at 2.16.3) and the documentation in the latest sources (almost 2.18):
https://github.com/git/git/blob/next/Documentation/git-submodule.txt

As far as I am aware this can be worked around with 'git repack -a' and manual removal of the objects/info/alternates file afterward. Though I don't know if this results in a less speedy clone than dissociate would.

Quoted 3 lines · Casey Fitzpatrick

It also seems to be missing "--progress", and I imagine others. Perhaps submodule add/update should be reworked to automatically accept all the options that clone would?

--progress is not missing, but I see that it isn't documented. It was added in 72c5f88311 ("clone: pass --progress decision to recursive submodules", 2016-09-22). What you're suggesting makes sense, but as shown in that commit it's not easy for it to happen automatically, there's a lot of boilerplate involved.

But since you're interested you can see how to add new options with that patch, it should be easy for anyone not experienced with the codebase, it's all just boilerplate + adding a test.

This seems to be a hole in the git feature set. I believe it is fairly easily worked around, but it would be best to provide the option for ease of use (and maybe performance?).

git clone has both a --reference feature and a --dissociate option, with dissociate allowing for a reference to *only* speed up network transfers rather than have the resulting clone rely upon the reference always being there (creates an independent repo).

But git submodule only allows for --reference, so there isn't a an option to make a speedy independent submodule clone in one shot:
https://git-scm.com/docs/git-submodule
I checked the latest online documentation (currently at 2.16.3) and the documentation in the latest sources (almost 2.18):
https://github.com/git/git/blob/next/Documentation/git-submodule.txt

As far as I am aware this can be worked around with 'git repack -a' and manual removal of the objects/info/alternates file afterward. Though I don't know if this results in a less speedy clone than dissociate would.

Quoted 8 lines · Casey Fitzpatrick

This seems to be a hole in the git feature set. I believe it is fairly easily worked around, but it would be best to provide the option for ease of use (and maybe performance?).

git clone has both a --reference feature and a --dissociate option, with dissociate allowing for a reference to *only* speed up network transfers rather than have the resulting clone rely upon the reference always being there (creates an independent repo).

With the advent of git-worktree, I claim that --reference without further --dissociate is dangerous, such that the combination of these two are not the best UX, you could wish for.

Quoted 11 lines

But git submodule only allows for --reference, so there isn't a an option to make a speedy independent submodule clone in one shot:
https://git-scm.com/docs/git-submodule
I checked the latest online documentation (currently at 2.16.3) and the documentation in the latest sources (almost 2.18):
https://github.com/git/git/blob/next/Documentation/git-submodule.txt

As far as I am aware this can be worked around with 'git repack -a' and manual removal of the objects/info/alternates file afterward. Though I don't know if this results in a less speedy clone than dissociate would.

That is an interesting workaround!
I agree we should have the --dissociate option available for submodules. Care to implement it?

Thanks,
Stefan

--progress was missing from add command, was only in update. Add --progress where it makes sense (both clone helper commands). Add missing documentation entry.
Add test.

+test_expect_success 'clone -o' '

+ test_i18ngrep ! "Checking connectivity" err

+ )

Quoted 4 lines · Casey Fitzpatrick

--progress was missing from add command, was only in update. Add --progress where it makes sense (both clone helper commands). Add missing documentation entry.
Add test.

Maybe:
The '--progress' was introduced in 72c5f88311d (clone: pass --progress decision to recursive submodules, 2016-09-22) to fix the progress reporting of the clone command. Also add the progress option to the 'submodule add' command. The update command already support the progress flag, but it is not documented.

git-submodule.sh cmd_add() · line 122

Quoted 7 lines

git-submodule.sh +3 -0

@@ -117,6 +117,9 @@ cmd_add()
                -q|--quiet)
                        GIT_QUIET=1
                        ;;
+               --progress)
+                       progress="--progress"
+                       ;;

The code looks good, though unlike the other commands progress is a boolean decision.

If we want to support --no-progress as well, we can do so by adding

    --no-progress)
        progress="--no-progress"
        ;;

and then the submodule helper ought to cope with it.

t/t7400-submodule-basic.sh test_expect_success 'submodule add' ' · line 134

Quoted 6 lines

t/t7400-submodule-basic.sh +6 -0

+test_expect_success 'setup test repo' '
+       mkdir parent &&
+       (cd parent && git init &&
+        echo one >file && git add file &&
+        git commit -m one)
+'

Coding style:
(
parens open on its own line, and its contents are indented by a tab.

Instead of coding this yourself, you could write the test as:

test_create_repo parent &&
test_create_commit -C parent one

Quoted 1 line

+test_expect_success 'clone -o' '

What are we testing here? I do not quite see the connection to testing --progress here.

t/t7400-submodule-basic.sh test_expect_success 'submodule add' ' · line 146

Quoted 13 lines

t/t7400-submodule-basic.sh +8 -0

+       git clone -o foo parent clone-o &&
+       (cd clone-o && git rev-parse --verify refs/remotes/foo/master)
 
 
+test_expect_success 'redirected submodule add does not show progress' '
+       (
+               cd addtest &&
 
 
 
+               git submodule add "file://$submodurl/parent" submod-redirected \
+                       >out 2>err &&
+               ! grep % err &&

We're grepping for percent to see that there is no progress. At first I thought the percent sign might be a special character, had to research it to know it's meant literally. TiL!

Quoted 1 line

+ test_i18ngrep ! "Checking connectivity" err

Makes sense.

Quoted 1 line

+ )

We could omit the extra shell by using

    git -C addtest submodule add "file://$... \
            >../out 2>../err &&

Why do we need 'out' ?

t/t7400-submodule-basic.sh test_expect_success 'submodule add' ' · line 158

Quoted 8 lines

t/t7400-submodule-basic.sh +8 -0

+test_expect_success 'redirected submodule add --progress does show progress' '
+       (
+               cd addtest &&
+               git submodule add --progress "file://$submodurl/parent" \
+                       submod-redirected-progress >out 2>err && \
+               grep % err
+       )
+'

Thanks for writing these tests!
Stefan

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

git-submodule.sh +4 -0

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

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

Quoted 1 line · Casey Fitzpatrick

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

s/Add/add/

builtin/submodule--helper.c static int clone_submodule(const char *path, const char *gitdir, const char *url · line 1077

Quoted 12 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 <[email protected]>

---

builtin/submodule--helper.c +3 -0

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

builtin/submodule--helper.c static int module_clone(int argc, const char **argv, const char *prefix) · line 1213

Quoted 3 lines

builtin/submodule--helper.c +2 -0

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

builtin/submodule--helper.c static int update_clone(int argc, const char **argv, const char *prefix) · line 1585

Quoted 3 lines

builtin/submodule--helper.c +2 -0

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

git-submodule.sh or you are unsure what this means choose another name with the '--name' option." · line 262

Quoted 6 lines

git-submodule.sh +3 -1

+               --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 8 lines

git-submodule.sh +4 -0

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

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

These patches add --progress and --dissociate options to git submodule.

The --progress option existed beforehand, but only for the update command and it was left undocumented.

Both add and update submodule commands supported --reference, but not its pair option --dissociate which allows for independent clones rather than depending on the reference.

I apologize for any errors I may have made in creation of these patches or the formatting of these emails. This is the first time I have submitted patches to a mailing list.

Add --dissociate option to add and update commands, both clone helper commands that already have the --reference option --dissociate pairs with.

Signed-off-by Casey Fitzpatrick <[email protected]>

---
 Documentation/git-submodule.txt | 10 +++++++++-
 builtin/submodule--helper.c     | 16 +++++++++++++---
 git-submodule.sh                | 10 +++++++++-
 t/t7408-submodule-reference.sh  | 17 +++++++++++++++++
 4 files changed, 48 insertions(+), 5 deletions(-)

Documentation/git-submodule.txt +9 -1

index d1ebe32e8..a75b95043 100644
@@ -369,7 +369,15 @@ the submodule itself.
        this option will be passed to the linkgit:git-clone[1] command.
 +
 *NOTE*: Do *not* use this option unless you have read the note
-for linkgit:git-clone[1]'s `--reference` and `--shared` options carefully.
+for linkgit:git-clone[1]'s `--reference`, `--shared`, and `--dissociate`
+options carefully.
+
+--dissociate::
+       This option is only valid for add and update commands.  These
+       commands sometimes need to clone a remote repository. In this case,
+       this option will be passed to the linkgit:git-clone[1] command.
++
+*NOTE*: see the NOTE for the `--reference` option.
 
 --recursive::
        This option is only valid for foreach, update, status and sync commands.

builtin/submodule--helper.c +13 -3

index 6ba8587b6..a85b30419 100644
@@ -1056,7 +1056,7 @@ static int module_deinit(int argc, const char **argv, const char *prefix)
 }
 
 static int clone_submodule(const char *path, const char *gitdir, const char *url,
-                          const char *depth, struct string_list *reference,
+                          const char *depth, struct string_list *reference, int dissociate,
                           int quiet, int progress)
 {
        struct child_process cp = CHILD_PROCESS_INIT;

@@ -1075,6 +1075,8 @@ static int clone_submodule(const char *path, const char *gitdir, const char *url
                        argv_array_pushl(&cp.args, "--reference",
                                         item->string, NULL);
        }
+       if (dissociate)
+               argv_array_push(&cp.args, "--dissociate");
        if (gitdir && *gitdir)
                argv_array_pushl(&cp.args, "--separate-git-dir", gitdir, NULL);
 

@@ -1190,6 +1192,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
        char *p, *path = NULL, *sm_gitdir;
        struct strbuf sb = STRBUF_INIT;
        struct string_list reference = STRING_LIST_INIT_NODUP;
+       int dissociate = 0;
        char *sm_alternate = NULL, *error_strategy = NULL;
 
        struct option module_clone_options[] = {

@@ -1208,6 +1211,8 @@ static int module_clone(int argc, const char **argv, const char *prefix)
                OPT_STRING_LIST(0, "reference", &reference,
                           N_("repo"),
                           N_("reference repository")),
+               OPT_BOOL(0, "dissociate", &dissociate,
+                          N_("use --reference only while cloning")),
                OPT_STRING(0, "depth", &depth,
                           N_("string"),
                           N_("depth for shallow clones")),

@@ -1247,7 +1252,7 @@ static int module_clone(int argc, const char **argv, const char *prefix)
 
                prepare_possible_alternates(name, &reference);
 
-               if (clone_submodule(path, sm_gitdir, url, depth, &reference,
+               if (clone_submodule(path, sm_gitdir, url, depth, &reference, dissociate,
                                    quiet, progress))
                        die(_("clone of '%s' into submodule path '%s' failed"),
                            url, path);

@@ -1300,6 +1305,7 @@ struct submodule_update_clone {
        int quiet;
        int recommend_shallow;
        struct string_list references;
+       int dissociate;
        const char *depth;
        const char *recursive_prefix;
        const char *prefix;

@@ -1315,7 +1321,7 @@ struct submodule_update_clone {
        int failed_clones_nr, failed_clones_alloc;
 };
 #define SUBMODULE_UPDATE_CLONE_INIT {0, MODULE_LIST_INIT, 0, \
-       SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, \
+       SUBMODULE_UPDATE_STRATEGY_INIT, 0, 0, -1, STRING_LIST_INIT_DUP, 0, \
        NULL, NULL, NULL, \
        STRING_LIST_INIT_DUP, 0, NULL, 0, 0}
 

@@ -1442,6 +1448,8 @@ static int prepare_to_clone_next_submodule(const struct cache_entry *ce,
                for_each_string_list_item(item, &suc->references)
                        argv_array_pushl(&child->args, "--reference", item->string, NULL);
        }
+       if (suc->dissociate)
+               argv_array_push(&child->args, "--dissociate");
        if (suc->depth)
                argv_array_push(&child->args, suc->depth);
 

@@ -1575,6 +1583,8 @@ static int update_clone(int argc, const char **argv, const char *prefix)
                           N_("rebase, merge, checkout or none")),
                OPT_STRING_LIST(0, "reference", &suc.references, N_("repo"),
                           N_("reference repository")),
+               OPT_BOOL(0, "dissociate", &suc.dissociate,
+                          N_("use --reference only while cloning")),
                OPT_STRING(0, "depth", &suc.depth, "<depth>",
                           N_("Create a shallow clone truncated to the "
                              "specified number of revisions")),

git-submodule.sh +9 -1

index 39c241a1d..1fddc098f 100755
@@ -42,6 +42,7 @@ prefix=
 custom_name=
 depth=
 progress=
+dissociate=
 
 die_if_unmatched ()
 {

@@ -128,6 +129,9 @@ cmd_add()
                --reference=*)
                        reference_path="${1#--reference=}"
                        ;;
+               --dissociate)
+                       dissociate=1
+                       ;;
                --name)
                        case "$2" in '') usage ;; esac
                        custom_name=$2

@@ -258,7 +262,7 @@ or you are unsure what this means choose another name with the '--name' option."
                                eval_gettextln "Reactivating local git directory for submodule '\$sm_name'."
                        fi
                fi
-               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
                (
                        sanitize_submodule_env
                        cd "$sm_path" &&

@@ -493,6 +497,9 @@ cmd_update()
                --reference=*)
                        reference="$1"
                        ;;
+               --dissociate)
+                       dissociate=1
+                       ;;
                -m|--merge)
                        update="merge"
                        ;;

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

t/t7408-submodule-reference.sh +17 -1

index e159fc503..1b3e7c2f7 100755
@@ -59,6 +59,16 @@ test_expect_success 'submodule add --reference uses alternates' '
        test_alternate_is_used super/.git/modules/sub/objects/info/alternates super/sub
 '
 
+test_expect_success 'submodule add --reference with --dissociate doesnt use alternates' '
+       (
+               cd super &&
+               git submodule add --reference ../B --dissociate "file://$base_dir/A" sub-dissociate &&
+               git commit -m B-super-added &&
+               git repack -ad
+       ) &&
+       test_path_is_missing super/.git/modules/sub-dissociate/objects/info/alternates
+'
+
 test_expect_success 'that reference gets used with add' '
        (
                cd super/sub &&

@@ -82,6 +92,13 @@ test_expect_success 'updating superproject keeps alternates' '
        test_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
 '
 
+test_expect_success 'updating superproject with --dissociate doesnt keep alternates' '
+       test_when_finished "rm -rf super-clone" &&
+       git clone super super-clone &&
+       git -C super-clone submodule update --init --reference ../B --dissociate &&
+       test_must_fail test_alternate_is_used super-clone/.git/modules/sub/objects/info/alternates super-clone/sub
+'
+
 test_expect_success 'submodules use alternates when cloning a superproject' '
        test_when_finished "rm -rf super-clone" &&
        git clone --reference super --recursive super super-clone &&
--

2.17.0.1.ge0414f29c.dirty

'recommend_shallow' and 'jobs' variables do not need quotes (they never contain spaces) and do not require any additional prefix, therefore remove the unnecessary subsitition.

'progress' is a boolean value. Treat it like the other boolean values in the script by using a substitution.

git-submodule.sh +3 -3

 	git submodule--helper update-clone ${GIT_QUIET:+--quiet} \
-		${progress:+"$progress"} \
+		${progress:+"--progress"} \
 		${wt_prefix:+--prefix "$wt_prefix"} \
 		${prefix:+--recursive-prefix "$prefix"} \
 		${update:+--update "$update"} \
 		${reference:+"$reference"} \
 		${depth:+--depth "$depth"} \
-		${recommend_shallow:+"$recommend_shallow"} \
-		${jobs:+$jobs} \
+		$recommend_shallow \
+		$jobs \

Quoted 3 lines · Casey Fitzpatrick

'recommend_shallow' and 'jobs' variables do not need quotes (they never contain spaces) and do not require any additional prefix, therefore remove the unnecessary subsitition.

The resulting patch is good, but "they never contain spaces" is not a very good rationale. The real reason is that (1) we use them only to hold a single token value (or leave them empty) in the current code, and (2) if the feature they represent is enhanced in the future to make them multi-token options (e.g. we may allow $jobs to contain, in addition to "--jobs=2", "--jobs 2" for whatever reason later), it is likely that we would want these multi-tokens split at $IFS (e.g. "--jobs" and "2" get passed as separate option, not a single "--jobs 2" string).

Quoted 2 lines

'progress' is a boolean value. Treat it like the other boolean values in the script by using a substitution.

This is OK.

Quoted 12 lines

      git submodule--helper update-clone ${GIT_QUIET:+--quiet} \

git-submodule.sh +3 -3

-             ${progress:+"$progress"} \
+             ${progress:+"--progress"} \
              ${wt_prefix:+--prefix "$wt_prefix"} \
              ${prefix:+--recursive-prefix "$prefix"} \
              ${update:+--update "$update"} \
              ${reference:+"$reference"} \
              ${depth:+--depth "$depth"} \
-             ${recommend_shallow:+"$recommend_shallow"} \
-             ${jobs:+$jobs} \
+             $recommend_shallow \
+             $jobs \

These patches add --progress and --dissociate options to git submodule.

The --progress option existed beforehand, but only for the update command and it was left undocumented.

Both add and update submodule commands supported --reference, but not its pair option --dissociate which allows for independent clones rather than depending on the reference.

This is a resubmission with comments from Stefan Beller and Eric Sunshine addressed.

Quoted 11 lines · Casey Fitzpatrick

These patches add --progress and --dissociate options to git submodule.

The --progress option existed beforehand, but only for the update command and it was left undocumented.

Both add and update submodule commands supported --reference, but not its pair option --dissociate which allows for independent clones rather than depending on the reference.

This is a resubmission with comments from Stefan Beller and Eric Sunshine addressed.

Readers would really appreciate it if these are prepared with format-patch with -v$N option. Unless they read faster than you post patches, they will find a few messages identically titled, all unread in their mailbox, and it is not always easy to tell which ones are the latest.

Thanks.

Quoted 4 lines

As far as I am aware this can be worked around with 'git repack -a' and manual removal of the objects/info/alternates file afterward. Though I don't know if this results in a less speedy clone than dissociate would.

That is an interesting workaround!