{"thread":{"id":"44645","subject":"[PATCHv5 0/5] submodule embedgitdirs","startedAt":"2016-12-07T21:02:07Z","lastAt":"2016-12-08T11:01:52Z","messageCount":16,"participants":["Stefan Beller","Junio C Hamano","Brandon Williams","Duy Nguyen"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307161","messageId":"20161207210157.18932-1-sbeller@google.com","threadId":"44645","inReplyTo":null,"subject":"[PATCHv5 0/5] submodule embedgitdirs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:52Z","receivedAt":"2016-12-07T21:02:07Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v5:\n* Add another layer of abstraction, i.e. the relocate_git_dir is only about \n  moving a git dir of one repository. The submodule specific stuff (e.g.\n  recursion into nested submodules) is in submodule.{c,h}\n  \n  This was motivated by reviews on the series of checkout aware of submodules\n  building on top of this series, as we want to directly call the embed-git-dirs\n  function without the overhead of spawning a child process.\n  \nThanks,\nStefan\n \nv4:\n* rebuilt on top of nd/worktree-list-fixup\n* fix and test behavior for un-init submodules (don't crash, rather do nothing)\n* incorporated a \"static\" as pointed out by Ramsay\n* use internal functions instead of duplicating code in worktree.c\n  (use get_common_dir_noenv for the submodule to actually get the common dir)\n* fixed a memory leak in relocate_gitdir\n\nv3:\n* have a slightly more generic function \"relocate_gitdir\".\n  The recursion is strictly related to submodules, though.\n* bail out if a submodule is using worktrees.\n  This also lays the groundwork for later doing the proper thing,\n  as worktree.h offers a function `get_submodule_worktrees(path)`\n* nit by duy: use git_path instead of git_common_dir\n\nv2:\n* fixed commit message for patch:\n \"submodule: use absolute path for computing relative path connecting\"\n* a new patch \"submodule helper: support super prefix\"\n* redid the final patch with more tests and fixing bugs along the way\n* \"test-lib-functions.sh: teach test_commit -C <dir>\" unchanged\n\nv1:\nThe discussion of the submodule checkout series revealed to me that a command\nis needed to move the git directory from the submodules working tree to be\nembedded into the superprojects git directory.\n\nSo I wrote the code to intern the submodules git dir into the superproject,\nbut whilst writing the code I realized this could be valueable for our use\nin testing too. So I exposed it via the submodule--helper. But as the\nsubmodule helper ought to be just an internal API, we could also\noffer it via the proper submodule command.\n\nThe command as it is has little value to the end user for now, but\nbreaking it out of the submodule checkout series hopefully makes review easier.\n\nThanks,\nStefan\n\nStefan Beller (5):\n  submodule: use absolute path for computing relative path connecting\n  submodule helper: support super prefix\n  test-lib-functions.sh: teach test_commit -C <dir>\n  worktree: get worktrees from submodules\n  submodule: add embed-git-dir function\n\n Documentation/git-submodule.txt   |  14 +++++\n builtin/submodule--helper.c       |  68 ++++++++++++++++----\n dir.c                             |  27 ++++++++\n dir.h                             |   3 +\n git-submodule.sh                  |   7 ++-\n git.c                             |   2 +-\n submodule.c                       | 127 ++++++++++++++++++++++++++++++++++++--\n submodule.h                       |   7 +++\n t/t7412-submodule-embedgitdirs.sh | 101 ++++++++++++++++++++++++++++++\n t/test-lib-functions.sh           |  20 ++++--\n worktree.c                        |  47 +++++++++++---\n worktree.h                        |   6 ++\n 12 files changed, 396 insertions(+), 33 deletions(-)\n create mode 100755 t/t7412-submodule-embedgitdirs.sh\n \ndiff to v4:\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 10df69c86a..321c9e250a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1106,31 +1106,8 @@ static int embed_git_dir(int argc, const char **argv, const char *prefix)\n \tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n \t\treturn 1;\n \n-\tfor (i = 0; i < list.nr; i++) {\n-\t\tconst char *path = list.entries[i]->name, *sub_git_dir, *v;\n-\t\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n-\t\tstruct strbuf gitdir = STRBUF_INIT;\n-\n-\t\tstrbuf_addf(&gitdir, \"%s/.git\", path);\n-\t\tsub_git_dir = resolve_gitdir(gitdir.buf);\n-\n-\t\t/* not populated? */\n-\t\tif (!sub_git_dir)\n-\t\t\tgoto free_and_continue;\n-\n-\t\t/* Is it already embedded? */\n-\t\treal_sub_git_dir = xstrdup(real_path(sub_git_dir));\n-\t\treal_common_git_dir = xstrdup(real_path(get_git_common_dir()));\n-\t\tif (skip_prefix(real_sub_git_dir, real_common_git_dir, &v), NULL)\n-\t\t\tgoto free_and_continue;\n-\n-\t\trelocate_gitdir(prefix, path, flags);\n-\n-free_and_continue:\n-\t\tstrbuf_release(&gitdir);\n-\t\tfree(real_sub_git_dir);\n-\t\tfree(real_common_git_dir);\n-\t}\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tsubmodule_embed_git_dir(prefix, list.entries[i]->name, flags);\n \n \treturn 0;\n }\ndiff --git a/dir.c b/dir.c\nindex d2f60b5abf..e023b04407 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -15,9 +15,6 @@\n #include \"utf8.h\"\n #include \"varint.h\"\n #include \"ewah/ewok.h\"\n-#include \"submodule-config.h\"\n-#include \"run-command.h\"\n-#include \"worktree.h\"\n \n struct path_simplify {\n \tint len;\n@@ -2753,79 +2750,28 @@ void untracked_cache_add_to_index(struct index_state *istate,\n }\n \n /*\n- * Migrate the given submodule (and all its submodules recursively) from\n- * having its git directory within the working tree to the git dir nested\n- * in its superprojects git dir under modules/.\n+ * Migrate the git directory of the given `path` from `old_git_dir` to\n+ * `new_git_dir`. If an error occurs, append it to `err` and return the\n+ * error code.\n  */\n-void relocate_gitdir(const char *prefix, const char *path, unsigned flags)\n+int relocate_gitdir(const char *path, const char *old_git_dir,\n+\t\t    const char *new_git_dir, const char *displaypath,\n+\t\t    struct strbuf *err)\n {\n-\tchar *old_git_dir;\n-\tconst char *new_git_dir;\n-\tconst struct submodule *sub;\n-\tstruct worktree **worktrees;\n-\tint i;\n-\n-\tworktrees = get_submodule_worktrees(path, 0);\n-\tif (worktrees) {\n-\t\tfor (i = 0; worktrees[i]; i++)\n-\t\t\t;\n-\t\tfree_worktrees(worktrees);\n-\t\tif (i > 1)\n-\t\t\tdie(_(\"relocate_gitdir for submodule with more than one worktree not supported\"));\n-\t}\n-\n-\told_git_dir = xstrfmt(\"%s/.git\", path);\n-\tif (read_gitfile(old_git_dir))\n-\t\t/* If it is an actual gitfile, it doesn't need migration. */\n-\t\tgoto out;\n-\n-\tsub = submodule_from_path(null_sha1, path);\n-\tif (!sub)\n-\t\tdie(_(\"Could not lookup name for submodule '%s'\"),\n-\t\t      path);\n+\tint ret = 0;\n \n-\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n-\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n-\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n+\tprintf(\"Migrating git directory of '%s' from\\n'%s' to\\n'%s'\\n\",\n+\t\tdisplaypath, old_git_dir, new_git_dir);\n \n-\tif (!prefix)\n-\t\tprefix = get_super_prefix();\n-\tprintf(\"Migrating git directory of %s%s from\\n'%s' to\\n'%s'\\n\",\n-\t\tprefix ? prefix : \"\", path,\n-\t\treal_path(old_git_dir), new_git_dir);\n-\n-\tif (rename(old_git_dir, new_git_dir) < 0)\n-\t\tdie_errno(_(\"Could not migrate git directory from '%s' to '%s'\"),\n+\tif (rename(old_git_dir, new_git_dir) < 0) {\n+\t\tret = errno;\n+\t\tstrbuf_addf(err,\n+\t\t\t_(\"could not migrate git directory from '%s' to '%s'\"),\n \t\t\told_git_dir, new_git_dir);\n+\t\treturn ret;\n+\t}\n \n \tconnect_work_tree_and_git_dir(path, new_git_dir);\n \n-out:\n-\tif (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {\n-\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n-\t\tstruct strbuf sb = STRBUF_INIT;\n-\n-\t\tif (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)\n-\t\t\tdie(\"BUG: we don't know how to pass the flags down?\");\n-\n-\t\tif (get_super_prefix())\n-\t\t\tstrbuf_addstr(&sb, get_super_prefix());\n-\t\tstrbuf_addstr(&sb, path);\n-\t\tstrbuf_addch(&sb, '/');\n-\n-\t\tcp.dir = path;\n-\t\tcp.git_cmd = 1;\n-\t\tcp.no_stdin = 1;\n-\t\targv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n-\t\t\t\t\t    \"submodule--helper\",\n-\t\t\t\t\t   \"embed-git-dirs\", NULL);\n-\t\tprepare_submodule_repo_env(&cp.env_array);\n-\t\tif (run_command(&cp))\n-\t\t\tdie(_(\"Could not migrate git directory in submodule '%s'\"),\n-\t\t\t    path);\n-\n-\t\tstrbuf_release(&sb);\n-\t}\n-\n-\tfree(old_git_dir);\n+\treturn ret;\n }\ndiff --git a/dir.h b/dir.h\nindex 0b5e99b21d..bf06729a86 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -335,8 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);\n void add_untracked_cache(struct index_state *istate);\n void remove_untracked_cache(struct index_state *istate);\n-\n-#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)\n-extern void relocate_gitdir(const char *prefix, const char *path, unsigned flags);\n-\n+extern int relocate_gitdir(const char *path, const char *old_git_dir,\n+\t\t\t   const char *new_git_dir, const char *displaypath,\n+\t\t\t   struct strbuf *err);\n #endif\ndiff --git a/submodule.c b/submodule.c\nindex 66c5ce5a24..67a91275b8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -14,6 +14,7 @@\n #include \"blob.h\"\n #include \"thread-utils.h\"\n #include \"quote.h\"\n+#include \"worktree.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int parallel_jobs = 1;\n@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t}\n \targv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+/* Embeds a single submodule, non recursively. */\n+static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)\n+{\n+\tstruct worktree **worktrees;\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct strbuf errbuf = STRBUF_INIT;\n+\tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n+\tconst char *new_git_dir;\n+\tconst struct submodule *sub;\n+\tint code;\n+\n+\tworktrees = get_submodule_worktrees(path, 0);\n+\tif (worktrees) {\n+\t\tint i;\n+\t\tfor (i = 0; worktrees[i]; i++)\n+\t\t\t;\n+\t\tfree_worktrees(worktrees);\n+\t\tif (i > 1)\n+\t\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n+\t\t\t    \"more than one worktree not supported\"), path);\n+\t}\n+\n+\told_git_dir = xstrfmt(\"%s/.git\", path);\n+\tif (read_gitfile(old_git_dir))\n+\t\t/* If it is an actual gitfile, it doesn't need migration. */\n+\t\treturn;\n+\n+\treal_old_git_dir = xstrdup(real_path(old_git_dir));\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\tif (!sub)\n+\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n+\n+\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n+\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n+\treal_new_git_dir = xstrdup(real_path(new_git_dir));\n+\n+\tif (!prefix)\n+\t\tprefix = get_super_prefix();\n+\tstrbuf_addf(&pathbuf, \"%s%s\", prefix ? prefix : \"\", path);\n+\n+\tcode = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,\n+\t\t\t       pathbuf.buf, &errbuf);\n+\tif (code) {\n+\t\terrno = code;\n+\t\tdie_errno(\"%s\\n\", errbuf.buf);\n+\t}\n+\n+\tfree(old_git_dir);\n+\tfree(real_old_git_dir);\n+\tfree(real_new_git_dir);\n+\tstrbuf_release(&pathbuf);\n+}\n+\n+/*\n+ * Migrate the git directory of the submodule given by path from\n+ * having its git directory within the working tree to the git dir nested\n+ * in its superprojects git dir under modules/.\n+ */\n+int submodule_embed_git_dir(const char *prefix,\n+\t\t\t    const char *path,\n+\t\t\t    unsigned flags)\n+{\n+\tconst char *sub_git_dir, *v;\n+\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\n+\n+\tstrbuf_addf(&gitdir, \"%s/.git\", path);\n+\tsub_git_dir = resolve_gitdir(gitdir.buf);\n+\n+\t/* Not populated? */\n+\tif (!sub_git_dir)\n+\t\tgoto out;\n+\n+\t/* Is it already embedded? */\n+\treal_sub_git_dir = xstrdup(real_path(sub_git_dir));\n+\treal_common_git_dir = xstrdup(real_path(get_git_common_dir()));\n+\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n+\t\tsubmodule_embed_git_dir_for_path(prefix, path);\n+\n+\tif (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tif (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)\n+\t\t\tdie(\"BUG: we don't know how to pass the flags down?\");\n+\n+\t\tif (get_super_prefix())\n+\t\t\tstrbuf_addstr(&sb, get_super_prefix());\n+\t\tstrbuf_addstr(&sb, path);\n+\t\tstrbuf_addch(&sb, '/');\n+\n+\t\tcp.dir = path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stdin = 1;\n+\t\targv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n+\t\t\t\t\t    \"submodule--helper\",\n+\t\t\t\t\t   \"embed-git-dirs\", NULL);\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n+\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+out:\n+\tstrbuf_release(&gitdir);\n+\tfree(real_sub_git_dir);\n+\tfree(real_common_git_dir);\n+\treturn 0;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a948..922cfd258f 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -75,4 +75,11 @@ int parallel_submodules(void);\n  */\n void prepare_submodule_repo_env(struct argv_array *out);\n \n+/*\n+ * Embed a git dir of the submodule given by path.\n+ */\n+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)\n+extern int submodule_embed_git_dir(const char *prefix,\n+\t\t\t\t   const char *path,\n+\t\t\t\t   unsigned flags);\n #endif\n\n \n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307162","messageId":"20161207210157.18932-3-sbeller@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"[PATCHv5 2/5] submodule helper: support super prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:54Z","receivedAt":"2016-12-07T21:02:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Just like main commands in Git, the submodule helper needs\naccess to the superproject prefix. Enable this in the git.c\nbut have its own fuse in the helper code by having a flag to\nturn on the super prefix.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------\n git.c                       |  2 +-\n 2 files changed, 21 insertions(+), 12 deletions(-)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 4beeda5f9f..806e29ce4e 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1076,21 +1076,24 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n \treturn 0;\n }\n \n+#define SUPPORT_SUPER_PREFIX (1<<0)\n+\n struct cmd_struct {\n \tconst char *cmd;\n \tint (*fn)(int, const char **, const char *);\n+\tint option;\n };\n \n static struct cmd_struct commands[] = {\n-\t{\"list\", module_list},\n-\t{\"name\", module_name},\n-\t{\"clone\", module_clone},\n-\t{\"update-clone\", update_clone},\n-\t{\"relative-path\", resolve_relative_path},\n-\t{\"resolve-relative-url\", resolve_relative_url},\n-\t{\"resolve-relative-url-test\", resolve_relative_url_test},\n-\t{\"init\", module_init},\n-\t{\"remote-branch\", resolve_remote_submodule_branch}\n+\t{\"list\", module_list, 0},\n+\t{\"name\", module_name, 0},\n+\t{\"clone\", module_clone, 0},\n+\t{\"update-clone\", update_clone, 0},\n+\t{\"relative-path\", resolve_relative_path, 0},\n+\t{\"resolve-relative-url\", resolve_relative_url, 0},\n+\t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n+\t{\"init\", module_init, 0},\n+\t{\"remote-branch\", resolve_remote_submodule_branch, 0}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n@@ -1100,9 +1103,15 @@ int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\n \t\tdie(_(\"submodule--helper subcommand must be \"\n \t\t      \"called with a subcommand\"));\n \n-\tfor (i = 0; i < ARRAY_SIZE(commands); i++)\n-\t\tif (!strcmp(argv[1], commands[i].cmd))\n+\tfor (i = 0; i < ARRAY_SIZE(commands); i++) {\n+\t\tif (!strcmp(argv[1], commands[i].cmd)) {\n+\t\t\tif (get_super_prefix() &&\n+\t\t\t    !(commands[i].option & SUPPORT_SUPER_PREFIX))\n+\t\t\t\tdie(\"%s doesn't support --super-prefix\",\n+\t\t\t\t    commands[i].cmd);\n \t\t\treturn commands[i].fn(argc - 1, argv + 1, prefix);\n+\t\t}\n+\t}\n \n \tdie(_(\"'%s' is not a valid submodule--helper \"\n \t      \"subcommand\"), argv[1]);\ndiff --git a/git.c b/git.c\nindex efa1059fe0..98dcf6c518 100644\n--- a/git.c\n+++ b/git.c\n@@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {\n \t{ \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n \t{ \"stripspace\", cmd_stripspace },\n-\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n+\t{ \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n \t{ \"symbolic-ref\", cmd_symbolic_ref, RUN_SETUP },\n \t{ \"tag\", cmd_tag, RUN_SETUP },\n \t{ \"unpack-file\", cmd_unpack_file, RUN_SETUP },\n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307163","messageId":"20161207210157.18932-4-sbeller@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"[PATCHv5 3/5] test-lib-functions.sh: teach test_commit -C <dir>","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:55Z","receivedAt":"2016-12-07T21:02:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Specifically when setting up submodule tests, it comes in handy if\nwe can create commits in repositories that are not at the root of\nthe tested trash dir. Add \"-C <dir>\" similar to gits -C parameter\nthat will perform the operation in the given directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n t/test-lib-functions.sh | 20 +++++++++++++++-----\n 1 file changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh\nindex fdaeb3a96b..579e812506 100644\n--- a/t/test-lib-functions.sh\n+++ b/t/test-lib-functions.sh\n@@ -157,16 +157,21 @@ debug () {\n \t GIT_TEST_GDB=1 \"$@\"\n }\n \n-# Call test_commit with the arguments \"<message> [<file> [<contents> [<tag>]]]\"\n+# Call test_commit with the arguments\n+# [-C <directory>] <message> [<file> [<contents> [<tag>]]]\"\n #\n # This will commit a file with the given contents and the given commit\n # message, and tag the resulting commit with the given tag name.\n #\n # <file>, <contents>, and <tag> all default to <message>.\n+#\n+# If the first argument is \"-C\", the second argument is used as a path for\n+# the git invocations.\n \n test_commit () {\n \tnotick= &&\n \tsignoff= &&\n+\tindir= &&\n \twhile test $# != 0\n \tdo\n \t\tcase \"$1\" in\n@@ -176,21 +181,26 @@ test_commit () {\n \t\t--signoff)\n \t\t\tsignoff=\"$1\"\n \t\t\t;;\n+\t\t-C)\n+\t\t\tindir=\"$2\"\n+\t\t\tshift\n+\t\t\t;;\n \t\t*)\n \t\t\tbreak\n \t\t\t;;\n \t\tesac\n \t\tshift\n \tdone &&\n+\tindir=${indir:+\"$indir\"/} &&\n \tfile=${2:-\"$1.t\"} &&\n-\techo \"${3-$1}\" > \"$file\" &&\n-\tgit add \"$file\" &&\n+\techo \"${3-$1}\" > \"$indir$file\" &&\n+\tgit ${indir:+ -C \"$indir\"} add \"$file\" &&\n \tif test -z \"$notick\"\n \tthen\n \t\ttest_tick\n \tfi &&\n-\tgit commit $signoff -m \"$1\" &&\n-\tgit tag \"${4:-$1}\"\n+\tgit ${indir:+ -C \"$indir\"} commit $signoff -m \"$1\" &&\n+\tgit ${indir:+ -C \"$indir\"} tag \"${4:-$1}\"\n }\n \n # Call test_merge with the arguments \"<message> <commit>\", where <commit>\n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307164","messageId":"20161207210157.18932-5-sbeller@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"[PATCHv5 4/5] worktree: get worktrees from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:56Z","receivedAt":"2016-12-07T21:02:17Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In a later patch we want to move around the the git directory of\na submodule. Both submodules as well as worktrees are involved in\nplacing git directories at unusual places, so their functionality\nmay collide. To react appropriately to situations where worktrees\nin submodules are in use, offer a new function to query the\nworktrees for submodules.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\n---\n worktree.c | 47 +++++++++++++++++++++++++++++++++++++----------\n worktree.h |  6 ++++++\n 2 files changed, 43 insertions(+), 10 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex eb6121263b..fa2b6dfa9a 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -72,7 +72,7 @@ static void add_head_info(struct strbuf *head_ref, struct worktree *worktree)\n /**\n  * get the main worktree\n  */\n-static struct worktree *get_main_worktree(void)\n+static struct worktree *get_main_worktree(const char *git_common_dir)\n {\n \tstruct worktree *worktree = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n@@ -81,12 +81,12 @@ static struct worktree *get_main_worktree(void)\n \tint is_bare = 0;\n \tint is_detached = 0;\n \n-\tstrbuf_add_absolute_path(&worktree_path, get_git_common_dir());\n+\tstrbuf_add_absolute_path(&worktree_path, git_common_dir);\n \tis_bare = !strbuf_strip_suffix(&worktree_path, \"/.git\");\n \tif (is_bare)\n \t\tstrbuf_strip_suffix(&worktree_path, \"/.\");\n \n-\tstrbuf_addf(&path, \"%s/HEAD\", get_git_common_dir());\n+\tstrbuf_addf(&path, \"%s/HEAD\", git_common_dir);\n \n \tworktree = xcalloc(1, sizeof(*worktree));\n \tworktree->path = strbuf_detach(&worktree_path, NULL);\n@@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n \treturn worktree;\n }\n \n-static struct worktree *get_linked_worktree(const char *id)\n+static struct worktree *get_linked_worktree(const char *git_common_dir,\n+\t\t\t\t\t    const char *id)\n {\n \tstruct worktree *worktree = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n@@ -112,7 +113,7 @@ static struct worktree *get_linked_worktree(const char *id)\n \tif (!id)\n \t\tdie(\"Missing linked worktree name\");\n \n-\tstrbuf_git_common_path(&path, \"worktrees/%s/gitdir\", id);\n+\tstrbuf_addf(&path, \"%s/worktrees/%s/gitdir\", git_common_dir, id);\n \tif (strbuf_read_file(&worktree_path, path.buf, 0) <= 0)\n \t\t/* invalid gitdir file */\n \t\tgoto done;\n@@ -125,7 +126,7 @@ static struct worktree *get_linked_worktree(const char *id)\n \t}\n \n \tstrbuf_reset(&path);\n-\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", get_git_common_dir(), id);\n+\tstrbuf_addf(&path, \"%s/worktrees/%s/HEAD\", git_common_dir, id);\n \n \tif (parse_ref(path.buf, &head_ref, &is_detached) < 0)\n \t\tgoto done;\n@@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)\n \treturn fspathcmp((*a)->path, (*b)->path);\n }\n \n-struct worktree **get_worktrees(unsigned flags)\n+static struct worktree **get_worktrees_internal(const char *git_common_dir,\n+\t\t\t\t\t\tunsigned flags)\n {\n \tstruct worktree **list = NULL;\n \tstruct strbuf path = STRBUF_INIT;\n@@ -177,9 +179,9 @@ struct worktree **get_worktrees(unsigned flags)\n \n \tlist = xmalloc(alloc * sizeof(struct worktree *));\n \n-\tlist[counter++] = get_main_worktree();\n+\tlist[counter++] = get_main_worktree(git_common_dir);\n \n-\tstrbuf_addf(&path, \"%s/worktrees\", get_git_common_dir());\n+\tstrbuf_addf(&path, \"%s/worktrees\", git_common_dir);\n \tdir = opendir(path.buf);\n \tstrbuf_release(&path);\n \tif (dir) {\n@@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)\n \t\t\tif (is_dot_or_dotdot(d->d_name))\n \t\t\t\tcontinue;\n \n-\t\t\tif ((linked = get_linked_worktree(d->d_name))) {\n+\t\t\tif ((linked = get_linked_worktree(git_common_dir, d->d_name))) {\n \t\t\t\tALLOC_GROW(list, counter + 1, alloc);\n \t\t\t\tlist[counter++] = linked;\n \t\t\t}\n@@ -209,6 +211,31 @@ struct worktree **get_worktrees(unsigned flags)\n \treturn list;\n }\n \n+struct worktree **get_worktrees(unsigned flags)\n+{\n+\treturn get_worktrees_internal(get_git_common_dir(), flags);\n+}\n+\n+struct worktree **get_submodule_worktrees(const char *path, unsigned flags)\n+{\n+\tchar *submodule_gitdir;\n+\tconst char *submodule_common_dir;\n+\tstruct strbuf sb = STRBUF_INIT;\n+\tstruct worktree **ret;\n+\n+\tsubmodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n+\tif (!submodule_gitdir)\n+\t\treturn NULL;\n+\n+\t/* the env would be set for the superproject */\n+\tget_common_dir_noenv(&sb, submodule_gitdir);\n+\tsubmodule_common_dir = strbuf_detach(&sb, NULL);\n+\tret = get_worktrees_internal(submodule_common_dir, flags);\n+\n+\tfree(submodule_gitdir);\n+\treturn ret;\n+}\n+\n const char *get_worktree_git_dir(const struct worktree *wt)\n {\n \tif (!wt)\ndiff --git a/worktree.h b/worktree.h\nindex d59ce1fee8..157fbc4a66 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -27,6 +27,12 @@ struct worktree {\n  */\n extern struct worktree **get_worktrees(unsigned flags);\n \n+/*\n+ * Get the worktrees of a submodule named by `path`.\n+ */\n+extern struct worktree **get_submodule_worktrees(const char *path,\n+\t\t\t\t\t\t unsigned flags);\n+\n /*\n  * Return git dir of the worktree. Note that the path may be relative.\n  * If wt is NULL, git dir of current worktree is returned.\n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307165","messageId":"20161207210157.18932-6-sbeller@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"[PATCHv5 5/5] submodule: add embed-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:57Z","receivedAt":"2016-12-07T21:02:20Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"When a submodule has its git dir inside the working dir, the submodule\nsupport for checkout that we plan to add in a later patch will fail.\n\nAdd functionality to migrate the git directory to be embedded\ninto the superprojects git directory.\n\nThe newly added code in this patch is structured such that\nother areas of Git can also make use of it. The code in the\nsubmodule--helper is a mere wrapper and option parser\nfor the function `submodule_embed_git_dir_for_path`, that\ntakes care of embedding the submodules git directory into\nthe superprojects git dir. That function makes use of the\nmore abstract function for this use case `relocate_gitdir`,\nwhich can be used by e.g. the worktree code eventually to\nmove around a git directory.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n Documentation/git-submodule.txt   |  14 +++++\n builtin/submodule--helper.c       |  39 ++++++++++++-\n dir.c                             |  27 +++++++++\n dir.h                             |   3 +\n git-submodule.sh                  |   7 ++-\n submodule.c                       | 115 ++++++++++++++++++++++++++++++++++++++\n submodule.h                       |   7 +++\n t/t7412-submodule-embedgitdirs.sh | 101 +++++++++++++++++++++++++++++++++\n 8 files changed, 311 insertions(+), 2 deletions(-)\n create mode 100755 t/t7412-submodule-embedgitdirs.sh\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex d841573475..34791cfc65 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -22,6 +22,7 @@ SYNOPSIS\n \t      [commit] [--] [<path>...]\n 'git submodule' [--quiet] foreach [--recursive] <command>\n 'git submodule' [--quiet] sync [--recursive] [--] [<path>...]\n+'git submodule' [--quiet] embedgitdirs [--] [<path>...]\n \n \n DESCRIPTION\n@@ -245,6 +246,19 @@ sync::\n If `--recursive` is specified, this command will recurse into the\n registered submodules, and sync any nested submodules within.\n \n+embedgitdirs::\n+\tMove the git directory of submodules into its superprojects\n+\t`$GIT_DIR/modules` path and then connect the git directory and\n+\tits working directory by setting the `core.worktree` and adding\n+\ta .git file pointing to the git directory interned into the\n+\tsuperproject.\n++\n+A repository that was cloned independently and later added as a submodule or\n+old setups have the submodules git directory inside the submodule instead of\n+embedded into the superprojects git directory.\n++\n+This command is recursive by default.\n+\n OPTIONS\n -------\n -q::\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 806e29ce4e..321c9e250a 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1076,6 +1076,42 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n \treturn 0;\n }\n \n+static int embed_git_dir(int argc, const char **argv, const char *prefix)\n+{\n+\tint i;\n+\tstruct pathspec pathspec;\n+\tstruct module_list list = MODULE_LIST_INIT;\n+\tunsigned flags = RELOCATE_GITDIR_RECURSE_SUBMODULES;\n+\n+\tstruct option embed_gitdir_options[] = {\n+\t\tOPT_STRING(0, \"prefix\", &prefix,\n+\t\t\t   N_(\"path\"),\n+\t\t\t   N_(\"path into the working tree\")),\n+\t\tOPT_BIT(0, \"--recursive\", &flags, N_(\"recurse into submodules\"),\n+\t\t\tRELOCATE_GITDIR_RECURSE_SUBMODULES),\n+\t\tOPT_END()\n+\t};\n+\n+\tconst char *const git_submodule_helper_usage[] = {\n+\t\tN_(\"git submodule--helper embed-git-dir [<path>...]\"),\n+\t\tNULL\n+\t};\n+\n+\targc = parse_options(argc, argv, prefix, embed_gitdir_options,\n+\t\t\t     git_submodule_helper_usage, 0);\n+\n+\tgitmodules_config();\n+\tgit_config(submodule_config, NULL);\n+\n+\tif (module_list_compute(argc, argv, prefix, &pathspec, &list) < 0)\n+\t\treturn 1;\n+\n+\tfor (i = 0; i < list.nr; i++)\n+\t\tsubmodule_embed_git_dir(prefix, list.entries[i]->name, flags);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {\n \t{\"resolve-relative-url\", resolve_relative_url, 0},\n \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n \t{\"init\", module_init, 0},\n-\t{\"remote-branch\", resolve_remote_submodule_branch, 0}\n+\t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n+\t{\"embed-git-dirs\", embed_git_dir, SUPPORT_SUPER_PREFIX}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/dir.c b/dir.c\nindex bfa8c8a9a5..e023b04407 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,\n {\n \tuntracked_cache_invalidate_path(istate, path);\n }\n+\n+/*\n+ * Migrate the git directory of the given `path` from `old_git_dir` to\n+ * `new_git_dir`. If an error occurs, append it to `err` and return the\n+ * error code.\n+ */\n+int relocate_gitdir(const char *path, const char *old_git_dir,\n+\t\t    const char *new_git_dir, const char *displaypath,\n+\t\t    struct strbuf *err)\n+{\n+\tint ret = 0;\n+\n+\tprintf(\"Migrating git directory of '%s' from\\n'%s' to\\n'%s'\\n\",\n+\t\tdisplaypath, old_git_dir, new_git_dir);\n+\n+\tif (rename(old_git_dir, new_git_dir) < 0) {\n+\t\tret = errno;\n+\t\tstrbuf_addf(err,\n+\t\t\t_(\"could not migrate git directory from '%s' to '%s'\"),\n+\t\t\told_git_dir, new_git_dir);\n+\t\treturn ret;\n+\t}\n+\n+\tconnect_work_tree_and_git_dir(path, new_git_dir);\n+\n+\treturn ret;\n+}\ndiff --git a/dir.h b/dir.h\nindex 97c83bb383..bf06729a86 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -335,4 +335,7 @@ struct untracked_cache *read_untracked_extension(const void *data, unsigned long\n void write_untracked_extension(struct strbuf *out, struct untracked_cache *untracked);\n void add_untracked_cache(struct index_state *istate);\n void remove_untracked_cache(struct index_state *istate);\n+extern int relocate_gitdir(const char *path, const char *old_git_dir,\n+\t\t\t   const char *new_git_dir, const char *displaypath,\n+\t\t\t   struct strbuf *err);\n #endif\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a024a135d6..b7e124f340 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1131,6 +1131,11 @@ cmd_sync()\n \tdone\n }\n \n+cmd_embedgitdirs()\n+{\n+\tgit submodule--helper embed-git-dirs --prefix \"$wt_prefix\" \"$@\"\n+}\n+\n # This loop parses the command line arguments to find the\n # subcommand name to dispatch.  Parsing of the subcommand specific\n # options are primarily done by the subcommand implementations.\n@@ -1140,7 +1145,7 @@ cmd_sync()\n while test $# != 0 && test -z \"$command\"\n do\n \tcase \"$1\" in\n-\tadd | foreach | init | deinit | update | status | summary | sync)\n+\tadd | foreach | init | deinit | update | status | summary | sync | embedgitdirs)\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\ndiff --git a/submodule.c b/submodule.c\nindex 66c5ce5a24..67a91275b8 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -14,6 +14,7 @@\n #include \"blob.h\"\n #include \"thread-utils.h\"\n #include \"quote.h\"\n+#include \"worktree.h\"\n \n static int config_fetch_recurse_submodules = RECURSE_SUBMODULES_ON_DEMAND;\n static int parallel_jobs = 1;\n@@ -1263,3 +1264,117 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t}\n \targv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+/* Embeds a single submodule, non recursively. */\n+static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)\n+{\n+\tstruct worktree **worktrees;\n+\tstruct strbuf pathbuf = STRBUF_INIT;\n+\tstruct strbuf errbuf = STRBUF_INIT;\n+\tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n+\tconst char *new_git_dir;\n+\tconst struct submodule *sub;\n+\tint code;\n+\n+\tworktrees = get_submodule_worktrees(path, 0);\n+\tif (worktrees) {\n+\t\tint i;\n+\t\tfor (i = 0; worktrees[i]; i++)\n+\t\t\t;\n+\t\tfree_worktrees(worktrees);\n+\t\tif (i > 1)\n+\t\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n+\t\t\t    \"more than one worktree not supported\"), path);\n+\t}\n+\n+\told_git_dir = xstrfmt(\"%s/.git\", path);\n+\tif (read_gitfile(old_git_dir))\n+\t\t/* If it is an actual gitfile, it doesn't need migration. */\n+\t\treturn;\n+\n+\treal_old_git_dir = xstrdup(real_path(old_git_dir));\n+\n+\tsub = submodule_from_path(null_sha1, path);\n+\tif (!sub)\n+\t\tdie(_(\"could not lookup name for submodule '%s'\"), path);\n+\n+\tnew_git_dir = git_path(\"modules/%s\", sub->name);\n+\tif (safe_create_leading_directories_const(new_git_dir) < 0)\n+\t\tdie(_(\"could not create directory '%s'\"), new_git_dir);\n+\treal_new_git_dir = xstrdup(real_path(new_git_dir));\n+\n+\tif (!prefix)\n+\t\tprefix = get_super_prefix();\n+\tstrbuf_addf(&pathbuf, \"%s%s\", prefix ? prefix : \"\", path);\n+\n+\tcode = relocate_gitdir(path, real_old_git_dir, real_new_git_dir,\n+\t\t\t       pathbuf.buf, &errbuf);\n+\tif (code) {\n+\t\terrno = code;\n+\t\tdie_errno(\"%s\\n\", errbuf.buf);\n+\t}\n+\n+\tfree(old_git_dir);\n+\tfree(real_old_git_dir);\n+\tfree(real_new_git_dir);\n+\tstrbuf_release(&pathbuf);\n+}\n+\n+/*\n+ * Migrate the git directory of the submodule given by path from\n+ * having its git directory within the working tree to the git dir nested\n+ * in its superprojects git dir under modules/.\n+ */\n+int submodule_embed_git_dir(const char *prefix,\n+\t\t\t    const char *path,\n+\t\t\t    unsigned flags)\n+{\n+\tconst char *sub_git_dir, *v;\n+\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n+\tstruct strbuf gitdir = STRBUF_INIT;\n+\n+\n+\tstrbuf_addf(&gitdir, \"%s/.git\", path);\n+\tsub_git_dir = resolve_gitdir(gitdir.buf);\n+\n+\t/* Not populated? */\n+\tif (!sub_git_dir)\n+\t\tgoto out;\n+\n+\t/* Is it already embedded? */\n+\treal_sub_git_dir = xstrdup(real_path(sub_git_dir));\n+\treal_common_git_dir = xstrdup(real_path(get_git_common_dir()));\n+\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n+\t\tsubmodule_embed_git_dir_for_path(prefix, path);\n+\n+\tif (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tif (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)\n+\t\t\tdie(\"BUG: we don't know how to pass the flags down?\");\n+\n+\t\tif (get_super_prefix())\n+\t\t\tstrbuf_addstr(&sb, get_super_prefix());\n+\t\tstrbuf_addstr(&sb, path);\n+\t\tstrbuf_addch(&sb, '/');\n+\n+\t\tcp.dir = path;\n+\t\tcp.git_cmd = 1;\n+\t\tcp.no_stdin = 1;\n+\t\targv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n+\t\t\t\t\t    \"submodule--helper\",\n+\t\t\t\t\t   \"embed-git-dirs\", NULL);\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\t\tif (run_command(&cp))\n+\t\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n+\n+\t\tstrbuf_release(&sb);\n+\t}\n+\n+out:\n+\tstrbuf_release(&gitdir);\n+\tfree(real_sub_git_dir);\n+\tfree(real_common_git_dir);\n+\treturn 0;\n+}\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a948..922cfd258f 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -75,4 +75,11 @@ int parallel_submodules(void);\n  */\n void prepare_submodule_repo_env(struct argv_array *out);\n \n+/*\n+ * Embed a git dir of the submodule given by path.\n+ */\n+#define RELOCATE_GITDIR_RECURSE_SUBMODULES (1<<0)\n+extern int submodule_embed_git_dir(const char *prefix,\n+\t\t\t\t   const char *path,\n+\t\t\t\t   unsigned flags);\n #endif\ndiff --git a/t/t7412-submodule-embedgitdirs.sh b/t/t7412-submodule-embedgitdirs.sh\nnew file mode 100755\nindex 0000000000..a02e7c447a\n--- /dev/null\n+++ b/t/t7412-submodule-embedgitdirs.sh\n@@ -0,0 +1,101 @@\n+#!/bin/sh\n+\n+test_description='Test submodule embedgitdirs\n+\n+This test verifies that `git submodue embedgitdirs` moves a submodules git\n+directory into the superproject.\n+'\n+\n+. ./test-lib.sh\n+\n+test_expect_success 'setup a real submodule' '\n+\tgit init sub1 &&\n+\ttest_commit -C sub1 first &&\n+\tgit submodule add ./sub1 &&\n+\ttest_tick &&\n+\tgit commit -m superproject\n+'\n+\n+test_expect_success 'embed the git dir' '\n+\t>expect.1 &&\n+\t>expect.2 &&\n+\t>actual.1 &&\n+\t>actual.2 &&\n+\tgit status >expect.1 &&\n+\tgit -C sub1 rev-parse HEAD >expect.2 &&\n+\tgit submodule embedgitdirs &&\n+\tgit fsck &&\n+\ttest -f sub1/.git &&\n+\ttest -d .git/modules/sub1 &&\n+\tgit status >actual.1 &&\n+\tgit -C sub1 rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n+test_expect_success 'embedding does not fail for deinitalized submodules' '\n+\ttest_when_finished \"git submodule update --init\" &&\n+\tgit submodule deinit --all &&\n+\tgit submodule embedgitdirs &&\n+\ttest -d .git/modules/sub1 &&\n+\t! test -f sub1/.git &&\n+\ttest -d sub1\n+'\n+\n+test_expect_success 'setup nested submodule' '\n+\tgit init sub1/nested &&\n+\ttest_commit -C sub1/nested first_nested &&\n+\tgit -C sub1 submodule add ./nested &&\n+\ttest_tick &&\n+\tgit -C sub1 commit -m \"add nested\" &&\n+\tgit add sub1 &&\n+\tgit commit -m \"sub1 to include nested submodule\"\n+'\n+\n+test_expect_success 'embed the git dir in a nested submodule' '\n+\tgit status >expect.1 &&\n+\tgit -C sub1/nested rev-parse HEAD >expect.2 &&\n+\tgit submodule embedgitdirs &&\n+\ttest -f sub1/nested/.git &&\n+\ttest -d .git/modules/sub1/modules/nested &&\n+\tgit status >actual.1 &&\n+\tgit -C sub1/nested rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n+test_expect_success 'setup a gitlink with missing .gitmodules entry' '\n+\tgit init sub2 &&\n+\ttest_commit -C sub2 first &&\n+\tgit add sub2 &&\n+\tgit commit -m superproject\n+'\n+\n+test_expect_success 'embedding the git dir fails for incomplete submodules' '\n+\tgit status >expect.1 &&\n+\tgit -C sub2 rev-parse HEAD >expect.2 &&\n+\ttest_must_fail git submodule embedgitdirs &&\n+\tgit -C sub2 fsck &&\n+\ttest -d sub2/.git &&\n+\tgit status >actual &&\n+\tgit -C sub2 rev-parse HEAD >actual.2 &&\n+\ttest_cmp expect.1 actual.1 &&\n+\ttest_cmp expect.2 actual.2\n+'\n+\n+test_expect_success 'setup a submodule with multiple worktrees' '\n+\t# first create another unembedded git dir in a new submodule\n+\tgit init sub3 &&\n+\ttest_commit -C sub3 first &&\n+\tgit submodule add ./sub3 &&\n+\ttest_tick &&\n+\tgit commit -m \"add another submodule\" &&\n+\tgit -C sub3 worktree add ../sub3_second_work_tree\n+'\n+\n+test_expect_success 'embed a submodule with multiple worktrees' '\n+\ttest_must_fail git submodule embedgitdirs sub3 2>error &&\n+\ttest_i18ngrep \"not supported\" error\n+'\n+\n+test_done\n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307166","messageId":"20161207210157.18932-2-sbeller@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"[PATCHv5 1/5] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T21:01:53Z","receivedAt":"2016-12-07T21:02:23Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The current caller of connect_work_tree_and_git_dir passes\nan absolute path for the `git_dir` parameter. In the future patch\nwe will also pass in relative path for `git_dir`. Extend the functionality\nof connect_work_tree_and_git_dir to take relative paths for parameters.\n\nWe could work around this in the future patch by computing the absolute\npath for the git_dir in the calling site, however accepting relative\npaths for either parameter makes the API for this function much harder\nto misuse.\n\nSigned-off-by: Stefan Beller <sbeller@google.com>\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n submodule.c | 12 +++++++-----\n 1 file changed, 7 insertions(+), 5 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 6f7d883de9..66c5ce5a24 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1227,23 +1227,25 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n {\n \tstruct strbuf file_name = STRBUF_INIT;\n \tstruct strbuf rel_path = STRBUF_INIT;\n-\tconst char *real_work_tree = xstrdup(real_path(work_tree));\n+\tchar *real_git_dir = xstrdup(real_path(git_dir));\n+\tchar *real_work_tree = xstrdup(real_path(work_tree));\n \n \t/* Update gitfile */\n \tstrbuf_addf(&file_name, \"%s/.git\", work_tree);\n \twrite_file(file_name.buf, \"gitdir: %s\",\n-\t\t   relative_path(git_dir, real_work_tree, &rel_path));\n+\t\t   relative_path(real_git_dir, real_work_tree, &rel_path));\n \n \t/* Update core.worktree setting */\n \tstrbuf_reset(&file_name);\n-\tstrbuf_addf(&file_name, \"%s/config\", git_dir);\n+\tstrbuf_addf(&file_name, \"%s/config\", real_git_dir);\n \tgit_config_set_in_file(file_name.buf, \"core.worktree\",\n-\t\t\t       relative_path(real_work_tree, git_dir,\n+\t\t\t       relative_path(real_work_tree, real_git_dir,\n \t\t\t\t\t     &rel_path));\n \n \tstrbuf_release(&file_name);\n \tstrbuf_release(&rel_path);\n-\tfree((void *)real_work_tree);\n+\tfree(real_work_tree);\n+\tfree(real_git_dir);\n }\n \n int parallel_submodules(void)\n-- \n2.11.0.rc2.28.g2af45f1.dirty\n\n"},{"id":"307181","messageId":"xmqqzik7v04b.fsf@gitster.mtv.corp.google.com","threadId":"44645","inReplyTo":"20161207210157.18932-1-sbeller@google.com","subject":"Re: [PATCHv5 0/5] submodule embedgitdirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T22:35:16Z","receivedAt":"2016-12-07T22:36:23Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> v5:\n> * Add another layer of abstraction, i.e. the relocate_git_dir is only about \n>   moving a git dir of one repository. The submodule specific stuff (e.g.\n>   recursion into nested submodules) is in submodule.{c,h}\n>   \n>   This was motivated by reviews on the series of checkout aware of submodules\n>   building on top of this series, as we want to directly call the embed-git-dirs\n>   function without the overhead of spawning a child process.\n\nOK.  Comparing the last steps between this round and the previous\none, I do think the separation of the responsibility among helpers\nis much more reasonable in this version, where:\n\n - submodule_embed_git_dir() is given a single path and is\n   responsible for that submodule itself, which is done by calling\n   submodule_embed_git_dir_for_path() on itself, and its\n   sub-submodules, which is done by spawning the helper recursively\n   with appropriate super-prefix;\n\n - submodule_embed_git_dir_for_path() computes where the given path\n   needs to be moved to using the knowledge specific to the\n   submodule subsystem, and asks relocate_gitdir() to perform the\n   actual relocation;\n\n - relocate_gitdir() used to do quite a lot more, but now it is only\n   about moving an existing .git directory elsewhere and pointing to\n   the new location with .git file placed in the old location.\n\nI would have called the second helper submodule_embed_one_git_dir(),\nbut that is a minor detail.\n\nVery nicely done.\n\n\n\n"},{"id":"307187","messageId":"xmqqvauvuzna.fsf@gitster.mtv.corp.google.com","threadId":"44645","inReplyTo":"20161207210157.18932-5-sbeller@google.com","subject":"Re: [PATCHv5 4/5] worktree: get worktrees from submodules","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T22:45:29Z","receivedAt":"2016-12-07T22:46:36Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> +\tsubmodule_common_dir = strbuf_detach(&sb, NULL);\n> +\tret = get_worktrees_internal(submodule_common_dir, flags);\n> +\n> +\tfree(submodule_gitdir);\n\nThis sequence felt somewhat unusual.  I would have written this\nwithout an extra variable, i.e.\n\n\tret = get_worktrees_internal(sb.buf, flags);\n\tstrbuf_release(&sb);\n"},{"id":"307191","messageId":"CAGZ79kZiS9dx6QUOcFYh8sSNoVsrv2eNLXJd6X54UekzUiC8VQ@mail.gmail.com","threadId":"44645","inReplyTo":"xmqqvauvuzna.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 4/5] worktree: get worktrees from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T22:51:58Z","receivedAt":"2016-12-07T22:52:10Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> +     submodule_common_dir = strbuf_detach(&sb, NULL);\n>> +     ret = get_worktrees_internal(submodule_common_dir, flags);\n>> +\n>> +     free(submodule_gitdir);\n>\n> This sequence felt somewhat unusual.  I would have written this\n> without an extra variable, i.e.\n>\n>         ret = get_worktrees_internal(sb.buf, flags);\n>         strbuf_release(&sb);\n\nYours is cleaner; I don't remember what I was thinking.\n\nFeel free to squash it in; in case a resend is needed I will do that.\n\nThanks,\nStefan\n"},{"id":"307192","messageId":"xmqqlgvruyt2.fsf@gitster.mtv.corp.google.com","threadId":"44645","inReplyTo":"20161207210157.18932-6-sbeller@google.com","subject":"Re: [PATCHv5 5/5] submodule: add embed-git-dir function","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T23:03:37Z","receivedAt":"2016-12-07T23:03:46Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller <sbeller@google.com> writes:\n\n> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {\n>  \t{\"resolve-relative-url\", resolve_relative_url, 0},\n>  \t{\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n>  \t{\"init\", module_init, 0},\n> -\t{\"remote-branch\", resolve_remote_submodule_branch, 0}\n> +\t{\"remote-branch\", resolve_remote_submodule_branch, 0},\n> +\t{\"embed-git-dirs\", embed_git_dir, SUPPORT_SUPER_PREFIX}\n>  };\n\nIf you want to avoid patch noise like this, your 2/5 can add a\ntrailing comma after the entry for remote-branch.  It is OK to end\nelements in an array literal with trailing comma, even though we\navoid doing similar in enum definition (which is only allowed in\nnewer C standards).\n\n> diff --git a/dir.c b/dir.c\n> index bfa8c8a9a5..e023b04407 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,\n>  {\n>  \tuntracked_cache_invalidate_path(istate, path);\n>  }\n> +\n> +/*\n> + * Migrate the git directory of the given `path` from `old_git_dir` to\n> + * `new_git_dir`. If an error occurs, append it to `err` and return the\n> + * error code.\n> + */\n> +int relocate_gitdir(const char *path, const char *old_git_dir,\n> +\t\t    const char *new_git_dir, const char *displaypath,\n> +\t\t    struct strbuf *err)\n> +{\n> +\tint ret = 0;\n> +\n> +\tprintf(\"Migrating git directory of '%s' from\\n'%s' to\\n'%s'\\n\",\n> +\t\tdisplaypath, old_git_dir, new_git_dir);\n\nBy using \"strbuf err\", it looks like that the calling convention of\nthis function wants to cater to callers who want to have tight\ncontrol over their output and an unconditional printing to the\nstandard output looks somewhat out of place.\n\nBesides, does it belong to the standard output?  It looks more like\na progress-bar eye candy that we send out to the standard error\nstream (and only when we are talking to a tty).\n\n> +/* Embeds a single submodule, non recursively. */\n> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)\n> +{\n> +\tstruct worktree **worktrees;\n> +\tstruct strbuf pathbuf = STRBUF_INIT;\n> +\tstruct strbuf errbuf = STRBUF_INIT;\n> +\tchar *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n> +\tconst char *new_git_dir;\n> +\tconst struct submodule *sub;\n> +\tint code;\n> +\n> +\tworktrees = get_submodule_worktrees(path, 0);\n> +\tif (worktrees) {\n> +\t\tint i;\n> +\t\tfor (i = 0; worktrees[i]; i++)\n> +\t\t\t;\n> +\t\tfree_worktrees(worktrees);\n> +\t\tif (i > 1)\n> +\t\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n> +\t\t\t    \"more than one worktree not supported\"), path);\n> +\t}\n\nWe may benefit from \"does this have secondary worktrees?\" boolean\nhelper function, perhaps?\n\n> +\told_git_dir = xstrfmt(\"%s/.git\", path);\n> +\tif (read_gitfile(old_git_dir))\n> +\t\t/* If it is an actual gitfile, it doesn't need migration. */\n> +\t\treturn;\n\nIsn't this \"no-op return\" a valid thing to do, even when the\nsubmodule has secondary worktrees that borrow from it?  I am\nwondering if the \"ah, we don't do a repository that has secondary\nworktrees\" check should come after this one.\n\n> +\treal_old_git_dir = xstrdup(real_path(old_git_dir));\n> +...\n> +}\n\n> +/*\n> + * Migrate the git directory of the submodule given by path from\n> + * having its git directory within the working tree to the git dir nested\n> + * in its superprojects git dir under modules/.\n> + */\n\nI think that this operation removes the git directories that are\nembedded in the working tree of the superproject and storing them\naway to safer place, i.e. unembedding.\n\n> +int submodule_embed_git_dir(const char *prefix,\n> +\t\t\t    const char *path,\n> +\t\t\t    unsigned flags)\n> +{\n> +\tconst char *sub_git_dir, *v;\n> +\tchar *real_sub_git_dir = NULL, *real_common_git_dir = NULL;\n> +\tstruct strbuf gitdir = STRBUF_INIT;\n> +\n> +\n\nLose the extra blank line here?\n\n> +\tstrbuf_addf(&gitdir, \"%s/.git\", path);\n> +\tsub_git_dir = resolve_gitdir(gitdir.buf);\n> +\n> +\t/* Not populated? */\n> +\tif (!sub_git_dir)\n> +\t\tgoto out;\n> +\n> +\t/* Is it already embedded? */\n> +\treal_sub_git_dir = xstrdup(real_path(sub_git_dir));\n> +\treal_common_git_dir = xstrdup(real_path(get_git_common_dir()));\n> +\tif (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n\nYeah, checking for NULL-ness with !skip_prefix() helps ;-)\n\n> +\t\tsubmodule_embed_git_dir_for_path(prefix, path);\n> +\n> +\tif (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf sb = STRBUF_INIT;\n> +\n> +\t\tif (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)\n> +\t\t\tdie(\"BUG: we don't know how to pass the flags down?\");\n> +\n> +\t\tif (get_super_prefix())\n> +\t\t\tstrbuf_addstr(&sb, get_super_prefix());\n> +\t\tstrbuf_addstr(&sb, path);\n> +\t\tstrbuf_addch(&sb, '/');\n> +\n> +\t\tcp.dir = path;\n> +\t\tcp.git_cmd = 1;\n> +\t\tcp.no_stdin = 1;\n> +\t\targv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n> +\t\t\t\t\t    \"submodule--helper\",\n> +\t\t\t\t\t   \"embed-git-dirs\", NULL);\n> +\t\tprepare_submodule_repo_env(&cp.env_array);\n> +\t\tif (run_command(&cp))\n> +\t\t\tdie(_(\"could not recurse into submodule '%s'\"), path);\n> +\t\tstrbuf_release(&sb);\n> +\t}\n\nHmph.  We cannot use run_processes_parallel() thing here?  Is its\nAPI too hard to use to be worth it?\n\n> +test_expect_success 'embedding does not fail for deinitalized submodules' '\n> +\ttest_when_finished \"git submodule update --init\" &&\n> +\tgit submodule deinit --all &&\n> +\tgit submodule embedgitdirs &&\n> +\ttest -d .git/modules/sub1 &&\n> +\t! test -f sub1/.git &&\n\nDoes this expect \"sub1/.git is not a regular file (we want directory\ninstead)\"?  Or \"there is no filesystem entity at sub1/.git\"?\n\nIf the former, write \"test -d sub1/.git\"; if the latter, you\nprobably want \"! test -e sub1/.git\" instead.\n"},{"id":"307195","messageId":"xmqqeg1juxd7.fsf@gitster.mtv.corp.google.com","threadId":"44645","inReplyTo":"xmqqzik7v04b.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 0/5] submodule embedgitdirs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-12-07T23:34:44Z","receivedAt":"2016-12-07T23:35:53Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> v5:\n>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about \n>>   moving a git dir of one repository. The submodule specific stuff (e.g.\n>>   recursion into nested submodules) is in submodule.{c,h}\n>>   \n>>   This was motivated by reviews on the series of checkout aware of submodules\n>>   building on top of this series, as we want to directly call the embed-git-dirs\n>>   function without the overhead of spawning a child process.\n>\n> OK.  Comparing the last steps between this round and the previous\n> one, I do think the separation of the responsibility among helpers\n> is much more reasonable in this version, where:\n>\n>  - submodule_embed_git_dir() is given a single path and is\n>    responsible for that submodule itself, which is done by calling\n>    submodule_embed_git_dir_for_path() on itself, and its\n>    sub-submodules, which is done by spawning the helper recursively\n>    with appropriate super-prefix;\n>\n>  - submodule_embed_git_dir_for_path() computes where the given path\n>    needs to be moved to using the knowledge specific to the\n>    submodule subsystem, and asks relocate_gitdir() to perform the\n>    actual relocation;\n>\n>  - relocate_gitdir() used to do quite a lot more, but now it is only\n>    about moving an existing .git directory elsewhere and pointing to\n>    the new location with .git file placed in the old location.\n>\n> I would have called the second helper submodule_embed_one_git_dir(),\n> but that is a minor detail.\n>\n> Very nicely done.\n\nOne thing that is not so nice from the code organization point of\nview is that \"dir.c\" now needs to include \"submodule.h\" because it\nwants to call connect_work_tree_and_git_dir().\n\nI wonder if that function belongs to dir.c or worktree.c not\nsubmodule.c; I do not see anything that is submodule specific about\nwhat the function does.\n"},{"id":"307196","messageId":"CAGZ79kb7WJKDBqgy3jgOv_FQGJvKfJk5nYQ_eVBuGUW+R6weeA@mail.gmail.com","threadId":"44645","inReplyTo":"xmqqlgvruyt2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 5/5] submodule: add embed-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T23:37:07Z","receivedAt":"2016-12-07T23:37:13Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 3:03 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Stefan Beller <sbeller@google.com> writes:\n>\n>> @@ -1093,7 +1129,8 @@ static struct cmd_struct commands[] = {\n>>       {\"resolve-relative-url\", resolve_relative_url, 0},\n>>       {\"resolve-relative-url-test\", resolve_relative_url_test, 0},\n>>       {\"init\", module_init, 0},\n>> -     {\"remote-branch\", resolve_remote_submodule_branch, 0}\n>> +     {\"remote-branch\", resolve_remote_submodule_branch, 0},\n>> +     {\"embed-git-dirs\", embed_git_dir, SUPPORT_SUPER_PREFIX}\n>>  };\n>\n> If you want to avoid patch noise like this, your 2/5 can add a\n> trailing comma after the entry for remote-branch.  It is OK to end\n> elements in an array literal with trailing comma, even though we\n> avoid doing similar in enum definition (which is only allowed in\n> newer C standards).\n\nOk, thanks for pointing out!\n\n>\n>> diff --git a/dir.c b/dir.c\n>> index bfa8c8a9a5..e023b04407 100644\n>> --- a/dir.c\n>> +++ b/dir.c\n>> @@ -2748,3 +2748,30 @@ void untracked_cache_add_to_index(struct index_state *istate,\n>>  {\n>>       untracked_cache_invalidate_path(istate, path);\n>>  }\n>> +\n>> +/*\n>> + * Migrate the git directory of the given `path` from `old_git_dir` to\n>> + * `new_git_dir`. If an error occurs, append it to `err` and return the\n>> + * error code.\n>> + */\n>> +int relocate_gitdir(const char *path, const char *old_git_dir,\n>> +                 const char *new_git_dir, const char *displaypath,\n>> +                 struct strbuf *err)\n>> +{\n>> +     int ret = 0;\n>> +\n>> +     printf(\"Migrating git directory of '%s' from\\n'%s' to\\n'%s'\\n\",\n>> +             displaypath, old_git_dir, new_git_dir);\n>\n> By using \"strbuf err\", it looks like that the calling convention of\n> this function wants to cater to callers who want to have tight\n> control over their output and an unconditional printing to the\n> standard output looks somewhat out of place.\n\nBefore sending it out to the list I had a version with another flag\nthat controlled whether we die on error or just print a warning.\nThat was not fully true however as the connect_work_tree_and_git_dir\nwould die nevertheless, we'd only warn for the failed rename().\n\nIt turns out we do not need a fully functional non-die()ing version\nfor the checkout series, so I ripped that out again and the version sent out\njust dies in case of failure in relocate_gitdir.\n\nThat said, I think the printing of the migration message ought to go\ninto the caller and to stderr as you note.\n\n>\n> Besides, does it belong to the standard output?  It looks more like\n> a progress-bar eye candy that we send out to the standard error\n> stream (and only when we are talking to a tty).\n>\n>> +/* Embeds a single submodule, non recursively. */\n>> +static void submodule_embed_git_dir_for_path(const char *prefix, const char *path)\n>> +{\n>> +     struct worktree **worktrees;\n>> +     struct strbuf pathbuf = STRBUF_INIT;\n>> +     struct strbuf errbuf = STRBUF_INIT;\n>> +     char *old_git_dir = NULL, *real_old_git_dir = NULL, *real_new_git_dir = NULL;\n>> +     const char *new_git_dir;\n>> +     const struct submodule *sub;\n>> +     int code;\n>> +\n>> +     worktrees = get_submodule_worktrees(path, 0);\n>> +     if (worktrees) {\n>> +             int i;\n>> +             for (i = 0; worktrees[i]; i++)\n>> +                     ;\n>> +             free_worktrees(worktrees);\n>> +             if (i > 1)\n>> +                     die(_(\"relocate_gitdir for submodule '%s' with \"\n>> +                         \"more than one worktree not supported\"), path);\n>> +     }\n>\n> We may benefit from \"does this have secondary worktrees?\" boolean\n> helper function, perhaps?\n\nok, I'll add that helper as its own patch to the worktree code earlier\nin the series.\n\n>\n>> +     old_git_dir = xstrfmt(\"%s/.git\", path);\n>> +     if (read_gitfile(old_git_dir))\n>> +             /* If it is an actual gitfile, it doesn't need migration. */\n>> +             return;\n>\n> Isn't this \"no-op return\" a valid thing to do, even when the\n> submodule has secondary worktrees that borrow from it?\n\nIf we have 2 worktrees, all bets are off (in my non-understanding).\nThe git file here *could* point to git directory living inside the\nother working tree, which then would need migration from that other\nworking tree to some embedded place.\n\nI don't think that is how worktrees work (or have ever worked?)\nbut I am unfamiliar with worktrees, so I erred on the safe side.\n\n>  I am\n> wondering if the \"ah, we don't do a repository that has secondary\n> worktrees\" check should come after this one.\n\nMaybe Duy can answer that? (Where are git directories\nlocated in a worktree setup, even historically? Were they\never inside one of the submodules? The other working tree may\nnot be a submodule, but a standalone thing as well?)\n\n>\n>> +     real_old_git_dir = xstrdup(real_path(old_git_dir));\n>> +...\n>> +}\n>\n>> +/*\n>> + * Migrate the git directory of the submodule given by path from\n>> + * having its git directory within the working tree to the git dir nested\n>> + * in its superprojects git dir under modules/.\n>> + */\n>\n> I think that this operation removes the git directories that are\n> embedded in the working tree of the superproject and storing them\n> away to safer place, i.e. unembedding.\n\nOh right we had that discussion what the embedding actually means.\nI asked around for English language help here:\nWhat about \"absorbGitDir\" ? (absorb as in eat/consume) for the external\nUI and internally I can call it relocate_submodule_git_dir_into_superproject\n(or embed_git_dir_into_superproject)\n\n>> +\n>> +\n>\n> Lose the extra blank line here?\n\nok\n\n>> +     if (!skip_prefix(real_sub_git_dir, real_common_git_dir, &v))\n>\n> Yeah, checking for NULL-ness with !skip_prefix() helps ;-)\n\nI think you are referring to the interdiff to the previous patches..\nYes we should just use it as it is here.\n\n>\n>> +             submodule_embed_git_dir_for_path(prefix, path);\n>> +\n>> +     if (flags & RELOCATE_GITDIR_RECURSE_SUBMODULES) {\n>> +             struct child_process cp = CHILD_PROCESS_INIT;\n>> +             struct strbuf sb = STRBUF_INIT;\n>> +\n>> +             if (flags & ~RELOCATE_GITDIR_RECURSE_SUBMODULES)\n>> +                     die(\"BUG: we don't know how to pass the flags down?\");\n>> +\n>> +             if (get_super_prefix())\n>> +                     strbuf_addstr(&sb, get_super_prefix());\n>> +             strbuf_addstr(&sb, path);\n>> +             strbuf_addch(&sb, '/');\n>> +\n>> +             cp.dir = path;\n>> +             cp.git_cmd = 1;\n>> +             cp.no_stdin = 1;\n>> +             argv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n>> +                                         \"submodule--helper\",\n>> +                                        \"embed-git-dirs\", NULL);\n>> +             prepare_submodule_repo_env(&cp.env_array);\n>> +             if (run_command(&cp))\n>> +                     die(_(\"could not recurse into submodule '%s'\"), path);\n>> +             strbuf_release(&sb);\n>> +     }\n>\n> Hmph.  We cannot use run_processes_parallel() thing here?  Is its\n> API too hard to use to be worth it?\n\nThe problem here is that we do not know about the names of\nthe nested submodules.  We could do a \"git -C path submodule--helper list\"\nand then use the run_processes_parallel for\n\n    git -C <submodule> embed-git-dir <nested-sub1>\n    git -C <submodule> embed-git-dir <nested-sub2>\n    git -C <submodule> embed-git-dir <nested-sub3>\n\netc. As a first approach I considered\n\n    git -C <submodule> embed-git-dir <no-pathspec>\n\nto be fast enough as the functionality is not implemented is only\na filesystem-local rename() (fast regardless of directory size).\n\nSo if we want to make that relocate git dir aware of\nnon-atomic-cross-filesystem moves, we want to revisit the decision\nto run it in parallel?\n\n>\n>> +test_expect_success 'embedding does not fail for deinitalized submodules' '\n>> +     test_when_finished \"git submodule update --init\" &&\n>> +     git submodule deinit --all &&\n>> +     git submodule embedgitdirs &&\n>> +     test -d .git/modules/sub1 &&\n>> +     ! test -f sub1/.git &&\n>\n> Does this expect \"sub1/.git is not a regular file (we want directory\n> instead)\"?  Or \"there is no filesystem entity at sub1/.git\"?\n\nThis mainly ought to test that the new call doesn't crash or segfault\nbut accepts this as a valid state.\n\n>\n> If the former, write \"test -d sub1/.git\"; if the latter, you\n> probably want \"! test -e sub1/.git\" instead.\n\nHowever the -e is the correct thing to do.\n"},{"id":"307198","messageId":"CAGZ79kYD61XaKMtnNOy11BYEzw5byWKHWchfVs+gwwHB_QdMSg@mail.gmail.com","threadId":"44645","inReplyTo":"xmqqeg1juxd7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCHv5 0/5] submodule embedgitdirs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-07T23:39:58Z","receivedAt":"2016-12-07T23:46:16Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Wed, Dec 7, 2016 at 3:34 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Junio C Hamano <gitster@pobox.com> writes:\n>\n>> Stefan Beller <sbeller@google.com> writes:\n>>\n>>> v5:\n>>> * Add another layer of abstraction, i.e. the relocate_git_dir is only about\n>>>   moving a git dir of one repository. The submodule specific stuff (e.g.\n>>>   recursion into nested submodules) is in submodule.{c,h}\n>>>\n>>>   This was motivated by reviews on the series of checkout aware of submodules\n>>>   building on top of this series, as we want to directly call the embed-git-dirs\n>>>   function without the overhead of spawning a child process.\n>>\n>> OK.  Comparing the last steps between this round and the previous\n>> one, I do think the separation of the responsibility among helpers\n>> is much more reasonable in this version, where:\n>>\n>>  - submodule_embed_git_dir() is given a single path and is\n>>    responsible for that submodule itself, which is done by calling\n>>    submodule_embed_git_dir_for_path() on itself, and its\n>>    sub-submodules, which is done by spawning the helper recursively\n>>    with appropriate super-prefix;\n>>\n>>  - submodule_embed_git_dir_for_path() computes where the given path\n>>    needs to be moved to using the knowledge specific to the\n>>    submodule subsystem, and asks relocate_gitdir() to perform the\n>>    actual relocation;\n>>\n>>  - relocate_gitdir() used to do quite a lot more, but now it is only\n>>    about moving an existing .git directory elsewhere and pointing to\n>>    the new location with .git file placed in the old location.\n>>\n>> I would have called the second helper submodule_embed_one_git_dir(),\n>> but that is a minor detail.\n>>\n>> Very nicely done.\n>\n> One thing that is not so nice from the code organization point of\n> view is that \"dir.c\" now needs to include \"submodule.h\" because it\n> wants to call connect_work_tree_and_git_dir().\n>\n> I wonder if that function belongs to dir.c or worktree.c not\n> submodule.c; I do not see anything that is submodule specific about\n> what the function does.\n\nRight, it just happened historically for that function to live\nin submodule area. I'll move the connect_work_tree_and_git_dir\nto dir.c as well.\n"},{"id":"307206","messageId":"20161208010958.GL116201@google.com","threadId":"44645","inReplyTo":"20161207210157.18932-6-sbeller@google.com","subject":"Re: [PATCHv5 5/5] submodule: add embed-git-dir function","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-08T01:09:58Z","receivedAt":"2016-12-08T01:10:08Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Stefan Beller wrote:\n> +\t\targv_array_pushl(&cp.args, \"--super-prefix\", sb.buf,\n> +\t\t\t\t\t    \"submodule--helper\",\n> +\t\t\t\t\t   \"embed-git-dirs\", NULL);\n\ncheck the spacing on these lines, looks like there is an extra space\nbefore \"submodule--helper\"\n\n-- \nBrandon Williams\n"},{"id":"307208","messageId":"20161208011440.GM116201@google.com","threadId":"44645","inReplyTo":"CAGZ79kZiS9dx6QUOcFYh8sSNoVsrv2eNLXJd6X54UekzUiC8VQ@mail.gmail.com","subject":"Re: [PATCHv5 4/5] worktree: get worktrees from submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-08T01:14:40Z","receivedAt":"2016-12-08T01:16:18Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/07, Stefan Beller wrote:\n> On Wed, Dec 7, 2016 at 2:45 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> > Stefan Beller <sbeller@google.com> writes:\n> >\n> >> +     submodule_common_dir = strbuf_detach(&sb, NULL);\n> >> +     ret = get_worktrees_internal(submodule_common_dir, flags);\n> >> +\n> >> +     free(submodule_gitdir);\n> >\n> > This sequence felt somewhat unusual.  I would have written this\n> > without an extra variable, i.e.\n> >\n> >         ret = get_worktrees_internal(sb.buf, flags);\n> >         strbuf_release(&sb);\n> \n> Yours is cleaner; I don't remember what I was thinking.\n> \n> Feel free to squash it in; in case a resend is needed I will do that.\n\nJust make sure to leave that free in as it refers to another variable\n(submodule_gitdir).  It actually turns out there is a memory leak in the\noriginal code because submodule_common_dir is never freed after being\ndetached from the strbuf.\n\n-- \nBrandon Williams\n"},{"id":"307233","messageId":"CACsJy8CY7dj8p+oTL2ZrncO49r8BcoMK3CwdoFYZH3eJ47bc_w@mail.gmail.com","threadId":"44645","inReplyTo":"20161207210157.18932-6-sbeller@google.com","subject":"Re: [PATCHv5 5/5] submodule: add embed-git-dir function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T11:01:15Z","receivedAt":"2016-12-08T11:01:52Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 4:01 AM, Stefan Beller <sbeller@google.com> wrote:\n> +/*\n> + * Migrate the git directory of the given `path` from `old_git_dir` to\n> + * `new_git_dir`. If an error occurs, append it to `err` and return the\n> + * error code.\n> + */\n> +int relocate_gitdir(const char *path, const char *old_git_dir,\n> +                   const char *new_git_dir, const char *displaypath,\n> +                   struct strbuf *err)\n> +{\n> +       int ret = 0;\n> +\n> +       printf(\"Migrating git directory of '%s' from\\n'%s' to\\n'%s'\\n\",\n> +               displaypath, old_git_dir, new_git_dir);\n\nI'm going to nag you about _() until your fingers automatically type\n\"_(\" after \"(\" :-D\n-- \nDuy\n"}]}