{"thread":{"id":"44650","subject":"[PATCHv6 0/7] submodule embedgitdirs","startedAt":"2016-12-08T01:46:47Z","lastAt":"2016-12-09T23:10:27Z","messageCount":20,"participants":["Stefan Beller","Duy Nguyen","Brandon Williams"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"307211","messageId":"20161208014623.7588-1-sbeller@google.com","threadId":"44650","inReplyTo":null,"subject":"[PATCHv6 0/7] submodule embedgitdirs","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:16Z","receivedAt":"2016-12-08T01:46:47Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v6:\n * renamed embedgitdirs to absorbgitdirs embedding may be interpreted as\n embedding the git dir into the working directory, whereas absorbing sounds\n more like the submodule is absorbed by the superproject, making the\n submodule less independent\n * Worktrees API offer uses_worktrees(void) and submodule_uses_worktree(path).\n * moved the printing to stderr and one layer up (out of the pure\n relocate_git_dir function).\n * connect_... is in dir.h now.\n\nv5:\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\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 \" 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 (7):\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 \n worktree: get worktrees from submodules\n worktree: add function to check if worktrees are in use\n move connect_work_tree_and_git_dir to dir.h\n submodule: add absorb-git-dir function\n\n Documentation/git-submodule.txt | 15 +++++\n builtin/submodule--helper.c | 69 ++++++++++++++++----\n dir.c | 38 +++++++++++\n dir.h | 4 ++\n git-submodule.sh | 7 +-\n git.c | 2 +-\n submodule.c | 127 ++++++++++++++++++++++++++++++-------\n submodule.h | 5 +-\n t/t7412-submodule-absorbgitdirs.sh | 101 +++++++++++++++++++++++++++++\n t/test-lib-functions.sh | 20 ++++--\n worktree.c | 70 +++++++++++++++++---\n worktree.h | 13 ++++\n 12 files changed, 418 insertions(+), 53 deletions(-)\n create mode 100755 t/t7412-submodule-absorbgitdirs.sh\n\n-- \n2.11.0.rc2.30.gc512cbd.dirty\n\n"},{"id":"307212","messageId":"20161208014623.7588-2-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 1/7] submodule: use absolute path for computing relative path connecting","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:17Z","receivedAt":"2016-12-08T01:46:50Z","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 \nSigned-off-by: Junio C Hamano \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.30.gc512cbd.dirty\n\n"},{"id":"307213","messageId":"20161208014623.7588-3-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 2/7] submodule helper: support super prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:18Z","receivedAt":"2016-12-08T01:46:52Z","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 \nSigned-off-by: Junio C Hamano \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..33676a57cf 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.30.gc512cbd.dirty\n\n"},{"id":"307214","messageId":"20161208014623.7588-5-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:20Z","receivedAt":"2016-12-08T01:46:55Z","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 \n---\n worktree.c | 46 ++++++++++++++++++++++++++++++++++++----------\n worktree.h | 6 ++++++\n 2 files changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/worktree.c b/worktree.c\nindex eb6121263b..75db689672 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,30 @@ 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+\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+\tret = get_worktrees_internal(sb.buf, flags);\n+\n+\tstrbuf_release(&sb);\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.30.gc512cbd.dirty\n\n"},{"id":"307215","messageId":"20161208014623.7588-8-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 7/7] submodule: add absorb-git-dir function","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:23Z","receivedAt":"2016-12-08T01:46:58Z","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 absorbed\ninto the superprojects git directory.\n\nThe newly added code in this patch is structured such that other areas of\nGit can also make use of it. The code in the submodule--helper is a mere\nwrapper and option parser for the function\n`absorb_git_dir_into_superproject`, that takes care of embedding the\nsubmodules git directory into the superprojects git dir. That function\nmakes use of the more abstract function for this use case\n`relocate_gitdir`, which can be used by e.g. the worktree code eventually\nto move around a git directory.\n\nSigned-off-by: Stefan Beller \n---\n Documentation/git-submodule.txt | 15 ++++++\n builtin/submodule--helper.c | 38 ++++++++++++++\n dir.c | 12 +++++\n dir.h | 3 ++\n git-submodule.sh | 7 ++-\n submodule.c | 103 +++++++++++++++++++++++++++++++++++++\n submodule.h | 4 ++\n t/t7412-submodule-absorbgitdirs.sh | 101 ++++++++++++++++++++++++++++++++++++\n 8 files changed, 282 insertions(+), 1 deletion(-)\n create mode 100755 t/t7412-submodule-absorbgitdirs.sh\n\ndiff --git a/Documentation/git-submodule.txt b/Documentation/git-submodule.txt\nindex d841573475..918bd1d1bd 100644\n--- a/Documentation/git-submodule.txt\n+++ b/Documentation/git-submodule.txt\n@@ -22,6 +22,7 @@ SYNOPSIS\n \t [commit] [--] [...]\n 'git submodule' [--quiet] foreach [--recursive] \n 'git submodule' [--quiet] sync [--recursive] [--] [...]\n+'git submodule' [--quiet] absorbgitdirs [--] [...]\n \n \n DESCRIPTION\n@@ -245,6 +246,20 @@ sync::\n If `--recursive` is specified, this command will recurse into the\n registered submodules, and sync any nested submodules within.\n \n+absorbgitdirs::\n+\tIf a git directory of a submodule is inside the submodule,\n+\tmove the git directory of the submodule 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 embedded in the\n+\tsuperprojects git directory.\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 33676a57cf..0108afac93 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1076,6 +1076,43 @@ static int resolve_remote_submodule_branch(int argc, const char **argv,\n \treturn 0;\n }\n \n+static int absorb_git_dirs(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 = ABSORB_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\tABSORB_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 [...]\"),\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\tabsorb_git_dir_into_superproject(prefix,\n+\t\t\t\tlist.entries[i]->name, flags);\n+\n+\treturn 0;\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -1094,6 +1131,7 @@ static struct cmd_struct commands[] = {\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{\"absorb-git-dirs\", absorb_git_dirs, 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 8b74997c66..cc5729f733 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n \tfree(real_work_tree);\n \tfree(real_git_dir);\n }\n+\n+/*\n+ * Migrate the git directory of the given path from old_git_dir to new_git_dir.\n+ */\n+void relocate_gitdir(const char *path, const char *old_git_dir, const char *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+\t\t\told_git_dir, new_git_dir);\n+\n+\tconnect_work_tree_and_git_dir(path, new_git_dir);\n+}\ndiff --git a/dir.h b/dir.h\nindex 051674a431..bf23a470af 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -336,4 +336,7 @@ void write_untracked_extension(struct strbuf *out, struct untracked_cache *untra\n void add_untracked_cache(struct index_state *istate);\n void remove_untracked_cache(struct index_state *istate);\n extern void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n+extern void relocate_gitdir(const char *path,\n+\t\t\t const char *old_git_dir,\n+\t\t\t const char *new_git_dir);\n #endif\ndiff --git a/git-submodule.sh b/git-submodule.sh\nindex a024a135d6..9285b5c43d 100755\n--- a/git-submodule.sh\n+++ b/git-submodule.sh\n@@ -1131,6 +1131,11 @@ cmd_sync()\n \tdone\n }\n \n+cmd_absorbgitdirs()\n+{\n+\tgit submodule--helper absorb-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 | absorbgitdirs)\n \t\tcommand=$1\n \t\t;;\n \t-q|--quiet)\ndiff --git a/submodule.c b/submodule.c\nindex 0bb50b4b62..6477746ce4 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@@ -1237,3 +1238,105 @@ void prepare_submodule_repo_env(struct argv_array *out)\n \t}\n \targv_array_push(out, \"GIT_DIR=.git\");\n }\n+\n+/*\n+ * Embeds a single submodules git directory into the superprojects git dir,\n+ * non recursively.\n+ */\n+static void relocate_single_git_dir_into_superproject(const char *prefix,\n+\t\t\t\t\t\t const char *path)\n+{\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+\n+\tif (submodule_uses_worktrees(path))\n+\t\tdie(_(\"relocate_gitdir for submodule '%s' with \"\n+\t\t \"more than one worktree not supported\"), path);\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+\n+\tfprintf(stderr, \"Migrating git directory of '%s%s' from\\n'%s' to\\n'%s'\\n\",\n+\t\tprefix ? prefix : \"\", path,\n+\t\treal_old_git_dir, real_new_git_dir);\n+\n+\trelocate_gitdir(path, real_old_git_dir, real_new_git_dir);\n+\n+\tfree(old_git_dir);\n+\tfree(real_old_git_dir);\n+\tfree(real_new_git_dir);\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+void absorb_git_dir_into_superproject(const char *prefix,\n+\t\t\t\t const char *path,\n+\t\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+\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 absorbed into the superprojects git dir? */\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\trelocate_single_git_dir_into_superproject(prefix, path);\n+\n+\tif (flags & ABSORB_GITDIR_RECURSE_SUBMODULES) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf sb = STRBUF_INIT;\n+\n+\t\tif (flags & ~ABSORB_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 \"absorb-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+}\ndiff --git a/submodule.h b/submodule.h\nindex 4e3bf469b4..6229054b99 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -74,4 +74,8 @@ int parallel_submodules(void);\n */\n void prepare_submodule_repo_env(struct argv_array *out);\n \n+#define ABSORB_GITDIR_RECURSE_SUBMODULES (1<<0)\n+extern void absorb_git_dir_into_superproject(const char *prefix,\n+\t\t\t\t\t const char *path,\n+\t\t\t\t\t unsigned flags);\n #endif\ndiff --git a/t/t7412-submodule-absorbgitdirs.sh b/t/t7412-submodule-absorbgitdirs.sh\nnew file mode 100755\nindex 0000000000..7c4e8b8d79\n--- /dev/null\n+++ b/t/t7412-submodule-absorbgitdirs.sh\n@@ -0,0 +1,101 @@\n+#!/bin/sh\n+\n+test_description='Test submodule absorbgitdirs\n+\n+This test verifies that `git submodue absorbgitdirs` 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 'absorb 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 absorbgitdirs &&\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 'absorbing does not fail for deinitalized submodules' '\n+\ttest_when_finished \"git submodule update --init\" &&\n+\tgit submodule deinit --all &&\n+\tgit submodule absorbgitdirs &&\n+\ttest -d .git/modules/sub1 &&\n+\ttest -d sub1 &&\n+\t! test -e sub1/.git\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 'absorb 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 absorbgitdirs &&\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 'absorbing 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 absorbgitdirs &&\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 'absorb a submodule with multiple worktrees' '\n+\ttest_must_fail git submodule absorbgitdirs sub3 2>error &&\n+\ttest_i18ngrep \"not supported\" error\n+'\n+\n+test_done\n-- \n2.11.0.rc2.30.gc512cbd.dirty\n\n"},{"id":"307216","messageId":"20161208014623.7588-6-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 5/7] worktree: add function to check if worktrees are in use","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:21Z","receivedAt":"2016-12-08T01:47:02Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Signed-off-by: Stefan Beller \n---\n worktree.c | 24 ++++++++++++++++++++++++\n worktree.h | 7 +++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/worktree.c b/worktree.c\nindex 75db689672..2559f33846 100644\n--- a/worktree.c\n+++ b/worktree.c\n@@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,\n \n \treturn existing;\n }\n+\n+static int uses_worktree_internal(struct worktree **worktrees)\n+{\n+\tint i;\n+\tfor (i = 0; worktrees[i]; i++)\n+\t\t; /* nothing */\n+\n+\tfree_worktrees(worktrees);\n+\treturn i > 1;\n+}\n+\n+int uses_worktrees(void)\n+{\n+\treturn uses_worktree_internal(get_worktrees(0));\n+}\n+\n+int submodule_uses_worktrees(const char *path)\n+{\n+\tstruct worktree **worktrees = get_submodule_worktrees(path, 0);\n+\tif (!worktrees)\n+\t\treturn 0;\n+\n+\treturn uses_worktree_internal(worktrees);\n+}\ndiff --git a/worktree.h b/worktree.h\nindex 157fbc4a66..76027b1fd2 100644\n--- a/worktree.h\n+++ b/worktree.h\n@@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);\n extern struct worktree **get_submodule_worktrees(const char *path,\n \t\t\t\t\t\t unsigned flags);\n \n+/*\n+ * Returns 1 if more than one worktree exists.\n+ * Returns 0 if only the main worktree exists.\n+ */\n+extern int uses_worktrees(void);\n+extern int submodule_uses_worktrees(const char *path);\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.30.gc512cbd.dirty\n\n"},{"id":"307217","messageId":"20161208014623.7588-4-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 3/7] test-lib-functions.sh: teach test_commit -C ","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:19Z","receivedAt":"2016-12-08T01:47:05Z","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 \" similar to gits -C parameter\nthat will perform the operation in the given directory.\n\nSigned-off-by: Stefan Beller \nSigned-off-by: Junio C Hamano \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 \" [ [ []]]\"\n+# Call test_commit with the arguments\n+# [-C ] [ [ []]]\"\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 # , , and all default to .\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 \" \", where \n-- \n2.11.0.rc2.30.gc512cbd.dirty\n\n"},{"id":"307218","messageId":"20161208014623.7588-7-sbeller@google.com","threadId":"44650","inReplyTo":"20161208014623.7588-1-sbeller@google.com","subject":"[PATCHv6 6/7] move connect_work_tree_and_git_dir to dir.h","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T01:46:22Z","receivedAt":"2016-12-08T01:47:07Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"That function was primarily used by submodule code, but the function\nitself is not inherently about submodules. In the next patch we'll\nintroduce relocate_git_dir, which can be used by worktrees as well,\nso find a neutral middle ground in dir.h.\n\nSigned-off-by: Stefan Beller \n---\n dir.c | 26 ++++++++++++++++++++++++++\n dir.h | 1 +\n submodule.c | 26 --------------------------\n submodule.h | 1 -\n 4 files changed, 27 insertions(+), 27 deletions(-)\n\ndiff --git a/dir.c b/dir.c\nindex bfa8c8a9a5..8b74997c66 100644\n--- a/dir.c\n+++ b/dir.c\n@@ -2748,3 +2748,29 @@ void untracked_cache_add_to_index(struct index_state *istate,\n {\n \tuntracked_cache_invalidate_path(istate, path);\n }\n+\n+/* Update gitfile and core.worktree setting to connect work tree and git dir */\n+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+\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(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\", real_git_dir);\n+\tgit_config_set_in_file(file_name.buf, \"core.worktree\",\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(real_work_tree);\n+\tfree(real_git_dir);\n+}\ndiff --git a/dir.h b/dir.h\nindex 97c83bb383..051674a431 100644\n--- a/dir.h\n+++ b/dir.h\n@@ -335,4 +335,5 @@ 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 void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n #endif\ndiff --git a/submodule.c b/submodule.c\nindex 66c5ce5a24..0bb50b4b62 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1222,32 +1222,6 @@ int merge_submodule(unsigned char result[20], const char *path,\n \treturn 0;\n }\n \n-/* Update gitfile and core.worktree setting to connect work tree and git dir */\n-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-\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(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\", real_git_dir);\n-\tgit_config_set_in_file(file_name.buf, \"core.worktree\",\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(real_work_tree);\n-\tfree(real_git_dir);\n-}\n-\n int parallel_submodules(void)\n {\n \treturn parallel_jobs;\ndiff --git a/submodule.h b/submodule.h\nindex d9e197a948..4e3bf469b4 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -65,7 +65,6 @@ int merge_submodule(unsigned char result[20], const char *path, const unsigned c\n int find_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name,\n \t\tstruct string_list *needs_pushing);\n int push_unpushed_submodules(unsigned char new_sha1[20], const char *remotes_name);\n-void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir);\n int parallel_submodules(void);\n \n /*\n-- \n2.11.0.rc2.30.gc512cbd.dirty\n\n"},{"id":"307225","messageId":"CACsJy8ChzxZCygrh6eNjqCkwv+Z43FmJh2T_6u6_qNE9Byq+sQ@mail.gmail.com","threadId":"44650","inReplyTo":"20161208014623.7588-2-sbeller@google.com","subject":"Re: [PATCHv6 1/7] submodule: use absolute path for computing relative path connecting","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T09:45:25Z","receivedAt":"2016-12-08T09:46:03Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n> The current caller of connect_work_tree_and_git_dir passes\n> an absolute path for the `git_dir` parameter. In the future patch\n> we will also pass in relative path for `git_dir`. Extend the functionality\n> of connect_work_tree_and_git_dir to take relative paths for parameters.\n>\n> We could work around this in the future patch by computing the absolute\n> path for the git_dir in the calling site, however accepting relative\n> paths for either parameter makes the API for this function much harder\n> to misuse.\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> submodule.c | 12 +++++++-----\n> 1 file changed, 7 insertions(+), 5 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 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> struct strbuf file_name = STRBUF_INIT;\n> struct strbuf rel_path = STRBUF_INIT;\n> - const char *real_work_tree = xstrdup(real_path(work_tree));\n> + char *real_git_dir = xstrdup(real_path(git_dir));\n\nIs it a bad idea to rename the argument git_dir to git_dir_input, or\nsomething, then have a new \"git_dir\" variable here? It certainly helps\nreduce diff size, but not sure if the end result looks better or\nworse.\n\n> + char *real_work_tree = xstrdup(real_path(work_tree));\n>\n> /* Update gitfile */\n> strbuf_addf(&file_name, \"%s/.git\", work_tree);\n> write_file(file_name.buf, \"gitdir: %s\",\n> - relative_path(git_dir, real_work_tree, &rel_path));\n> + relative_path(real_git_dir, real_work_tree, &rel_path));\n>\n> /* Update core.worktree setting */\n> strbuf_reset(&file_name);\n> - strbuf_addf(&file_name, \"%s/config\", git_dir);\n> + strbuf_addf(&file_name, \"%s/config\", real_git_dir);\n> git_config_set_in_file(file_name.buf, \"core.worktree\",\n> - relative_path(real_work_tree, git_dir,\n> + relative_path(real_work_tree, real_git_dir,\n> &rel_path));\n>\n> strbuf_release(&file_name);\n> strbuf_release(&rel_path);\n> - free((void *)real_work_tree);\n> + free(real_work_tree);\n> + free(real_git_dir);\n> }\n>\n> int parallel_submodules(void)\n> --\n> 2.11.0.rc2.30.gc512cbd.dirty\n>\n\n\n\n-- \nDuy\n"},{"id":"307226","messageId":"CACsJy8AohKMTm0inUyxu=kTwxNFcCbbBRO=O126SCzi5e=0tSQ@mail.gmail.com","threadId":"44650","inReplyTo":"20161208014623.7588-3-sbeller@google.com","subject":"Re: [PATCHv6 2/7] submodule helper: support super prefix","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T09:52:30Z","receivedAt":"2016-12-08T09:53:08Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n> Just like main commands in Git, the submodule helper needs\n> access to the superproject prefix. Enable this in the git.c\n> but have its own fuse in the helper code by having a flag to\n> turn on the super prefix.\n>\n> Signed-off-by: Stefan Beller \n> Signed-off-by: Junio C Hamano \n> ---\n> builtin/submodule--helper.c | 31 ++++++++++++++++++++-----------\n> git.c | 2 +-\n> 2 files changed, 21 insertions(+), 12 deletions(-)\n>\n> diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\n> index 4beeda5f9f..33676a57cf 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> return 0;\n> }\n>\n> +#define SUPPORT_SUPER_PREFIX (1<<0)\n> +\n> struct cmd_struct {\n> const char *cmd;\n> int (*fn)(int, const char **, const char *);\n> + int option;\n\nunsigned int is probably safer for variables that are used as bit-flags.\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> die(_(\"submodule--helper subcommand must be \"\n> \"called with a subcommand\"));\n>\n> - for (i = 0; i < ARRAY_SIZE(commands); i++)\n> - if (!strcmp(argv[1], commands[i].cmd))\n> + for (i = 0; i < ARRAY_SIZE(commands); i++) {\n> + if (!strcmp(argv[1], commands[i].cmd)) {\n> + if (get_super_prefix() &&\n> + !(commands[i].option & SUPPORT_SUPER_PREFIX))\n> + die(\"%s doesn't support --super-prefix\",\n> + commands[i].cmd);\n\nIf it's meant for users to see, please _() the string.\n\n> return commands[i].fn(argc - 1, argv + 1, prefix);\n> + }\n> + }\n>\n> die(_(\"'%s' is not a valid submodule--helper \"\n> \"subcommand\"), argv[1]);\n> diff --git a/git.c b/git.c\n> index efa1059fe0..98dcf6c518 100644\n> --- a/git.c\n> +++ b/git.c\n> @@ -493,7 +493,7 @@ static struct cmd_struct commands[] = {\n> { \"stage\", cmd_add, RUN_SETUP | NEED_WORK_TREE },\n> { \"status\", cmd_status, RUN_SETUP | NEED_WORK_TREE },\n> { \"stripspace\", cmd_stripspace },\n> - { \"submodule--helper\", cmd_submodule__helper, RUN_SETUP },\n> + { \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n\nThe same macro defined twice in two separate .c files? Hmm.. it\nconfused me a bit because i thought there was a connection.. I guess\nit's ok.\n-- \nDuy\n"},{"id":"307227","messageId":"CACsJy8Bs78ywGq5p6yAFGi1KACAXFEeroyQSJye5-RL5gqOS+Q@mail.gmail.com","threadId":"44650","inReplyTo":"20161208014623.7588-5-sbeller@google.com","subject":"Re: [PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T10:09:58Z","receivedAt":"2016-12-08T10:10:51Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n>\n> worktree = xcalloc(1, sizeof(*worktree));\n> worktree->path = strbuf_detach(&worktree_path, NULL);\n> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n\nAll the good stuff is outside context lines again.. Somewhere between\nhere we call add_head_info() which calls resolve_ref_unsafe(), which\nalways uses data from current repo, not the submodule we want it to\nlook at.\n\n> @@ -167,7 +168,8 @@ static int compare_worktree(const void *a_, const void *b_)\n> return fspathcmp((*a)->path, (*b)->path);\n\nfspathcmp() reads core.ignorecase from current repo. I guess it's\ninsane to have this key different between repos on the same machine,\nso it should be ok. But I want to point this out just in case I'm\nmissing something.\n\n> @@ -188,7 +190,7 @@ struct worktree **get_worktrees(unsigned flags)\n> if (is_dot_or_dotdot(d->d_name))\n> continue;\n>\n> - if ((linked = get_linked_worktree(d->d_name))) {\n> + if ((linked = get_linked_worktree(git_common_dir, d->d_name))) {\n> ALLOC_GROW(list, counter + 1, alloc);\n> list[counter++] = linked;\n> }\n> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)\n> return list;\n\nRight before this line is mark_current_worktree(), which in turn calls\nget_git_dir() and not suitable for peeking into another repository the\nway submodule code does. get_worktree_git_dir() called within that\nfunction shares the same problem.\n\n> }\n>\n> +struct worktree **get_worktrees(unsigned flags)\n> +{\n> + return get_worktrees_internal(get_git_common_dir(), flags);\n> +}\n> +\n> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)\n> +{\n> + char *submodule_gitdir;\n> + struct strbuf sb = STRBUF_INIT;\n> + struct worktree **ret;\n> +\n> + submodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n> + if (!submodule_gitdir)\n> + return NULL;\n> +\n> + /* the env would be set for the superproject */\n> + get_common_dir_noenv(&sb, submodule_gitdir);\n\nTechnically we need to read submodule_gitdir/.config and see if we can\nunderstand core.repositoryformatversion, or find any unrecognized\nextensions. But the problem is not specific to this code. And fixing\nit is no small task. But perhaps we could call a dummy\nvalidate_submodule_gitdir() here? Then when we implement that function\nfor real, we don't have to search the entire code base to see where to\nput it.\n\nKinda off-topic though. Feel free to ignore the above comment.\n\n> + ret = get_worktrees_internal(sb.buf, flags);\n> +\n> + strbuf_release(&sb);\n> + free(submodule_gitdir);\n> + return ret;\n> +}\n-- \nDuy\n"},{"id":"307230","messageId":"CACsJy8ANNz6FsJ4_5MOhj2Qqd+wHHu5UpVOAobqEiHU2KM26eg@mail.gmail.com","threadId":"44650","inReplyTo":"20161208014623.7588-6-sbeller@google.com","subject":"Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T10:40:40Z","receivedAt":"2016-12-08T10:41:17Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n> Signed-off-by: Stefan Beller \n> ---\n> worktree.c | 24 ++++++++++++++++++++++++\n> worktree.h | 7 +++++++\n> 2 files changed, 31 insertions(+)\n>\n> diff --git a/worktree.c b/worktree.c\n> index 75db689672..2559f33846 100644\n> --- a/worktree.c\n> +++ b/worktree.c\n> @@ -406,3 +406,27 @@ const struct worktree *find_shared_symref(const char *symref,\n>\n> return existing;\n> }\n> +\n> +static int uses_worktree_internal(struct worktree **worktrees)\n> +{\n> + int i;\n> + for (i = 0; worktrees[i]; i++)\n> + ; /* nothing */\n> +\n> + free_worktrees(worktrees);\n\n\nAyy.. caller allocates, callee frees. This might become a new\nmaintenance nightmare. Elsewhere I believe we (Junio and me) discussed\nthe possibility of returning the number of worktrees from\nget_worktrees() too. get_worktrees() would take an \"int *\", if not\nNULL, we return the number of worktrees in that pointer.\n\nIt's probably a better approach, although I'm afraid it'll add a bit\nmore work on you.\n\nAlternatively, we could add a new flag to get_worktrees() to tell it\nto return all worktrees if there is a least one linked worktree, or\nreturn NULL if there's only main worktree. I'm not sure if this is\nclever or very stupid.\n\n> + return i > 1;\n> +}\n> +\n> +int uses_worktrees(void)\n\n\"has\" may be a better verb than \"uses\". maybe \"has_linked_worktrees\"\nsince we always have and use the main worktree.\n\n> +{\n> + return uses_worktree_internal(get_worktrees(0));\n> +}\n> +\n> +int submodule_uses_worktrees(const char *path)\n> +{\n> + struct worktree **worktrees = get_submodule_worktrees(path, 0);\n> + if (!worktrees)\n> + return 0;\n> +\n> + return uses_worktree_internal(worktrees);\n> +}\n> diff --git a/worktree.h b/worktree.h\n> index 157fbc4a66..76027b1fd2 100644\n> --- a/worktree.h\n> +++ b/worktree.h\n> @@ -33,6 +33,13 @@ extern struct worktree **get_worktrees(unsigned flags);\n> extern struct worktree **get_submodule_worktrees(const char *path,\n> unsigned flags);\n>\n> +/*\n> + * Returns 1 if more than one worktree exists.\n> + * Returns 0 if only the main worktree exists.\n> + */\n> +extern int uses_worktrees(void);\n> +extern int submodule_uses_worktrees(const char *path);\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> --\n> 2.11.0.rc2.30.gc512cbd.dirty\n>\n\n\n\n-- \nDuy\n"},{"id":"307231","messageId":"CACsJy8DMhC7Umnqf+-ViOBKn46Emtan4647=MDdLoYXVNB0=6A@mail.gmail.com","threadId":"44650","inReplyTo":"CACsJy8ANNz6FsJ4_5MOhj2Qqd+wHHu5UpVOAobqEiHU2KM26eg@mail.gmail.com","subject":"Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T10:51:35Z","receivedAt":"2016-12-08T10:52:11Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen wrote:\n> Alternatively, we could add a new flag to get_worktrees() to tell it\n> to return all worktrees if there is a least one linked worktree, or\n> return NULL if there's only main worktree. I'm not sure if this is\n> clever or very stupid.\n\nNo, this may be better. Add a flag to say \"returns linked worktrees\n_only_\". Which means when you're in a \"normal\" repo, get_worktrees()\nwith this flag returns NULL. When you're in a multiple-worktree repo,\nit returns all linked worktrees (no main worktree). I think I might\nhave a use for this flag in addition to this uses_worktrees() here.\nuses_worktrees() look quite simple with that flag\n\nint uses_worktrees(void)\n{\n struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);\n int retval = worktrees != NULL;\n free_worktrees(worktrees);\n return retval;\n}\n-- \nDuy\n"},{"id":"307232","messageId":"CACsJy8DTeVVKjqT+eKGTDyoMQJ_ddh=Y-v46JCj6dqcQoSw7fg@mail.gmail.com","threadId":"44650","inReplyTo":"20161208014623.7588-8-sbeller@google.com","subject":"Re: [PATCHv6 7/7] submodule: add absorb-git-dir function","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-08T10:56:15Z","receivedAt":"2016-12-08T10:57:50Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n> diff --git a/dir.c b/dir.c\n> index 8b74997c66..cc5729f733 100644\n> --- a/dir.c\n> +++ b/dir.c\n> @@ -2774,3 +2774,15 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir)\n> free(real_work_tree);\n> free(real_git_dir);\n> }\n> +\n> +/*\n> + * Migrate the git directory of the given path from old_git_dir to new_git_dir.\n> + */\n> +void relocate_gitdir(const char *path, const char *old_git_dir, const char *new_git_dir)\n> +{\n> + if (rename(old_git_dir, new_git_dir) < 0)\n> + die_errno(_(\"could not migrate git directory from '%s' to '%s'\"),\n> + old_git_dir, new_git_dir);\n> +\n> + connect_work_tree_and_git_dir(path, new_git_dir);\n> +}\n\nThank you!\n-- \nDuy\n"},{"id":"307279","messageId":"CAGZ79kYtEUvuTX09sJm3C0rG0-BrBz4bN0FCs6E5d2jHhtKN6w@mail.gmail.com","threadId":"44650","inReplyTo":"CACsJy8Bs78ywGq5p6yAFGi1KACAXFEeroyQSJye5-RL5gqOS+Q@mail.gmail.com","subject":"Re: [PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T18:55:35Z","receivedAt":"2016-12-08T18:55:41Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen wrote:\n> On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n>>\n>> worktree = xcalloc(1, sizeof(*worktree));\n>> worktree->path = strbuf_detach(&worktree_path, NULL);\n>> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n>\n> All the good stuff is outside context lines again.. Somewhere between\n> here we call add_head_info() which calls resolve_ref_unsafe(), which\n> always uses data from current repo, not the submodule we want it to\n> look at.\n\nUnrelated side question: What would you think of \"variable context line\nconfiguration\" ? e.g. you could configure it to include anything from\nup that line\nthat is currently shown after the @@ which is the function signature line.\n\nAs to the add_head_info/resolve_ref_unsafe what impact does that have?\nIt produces a wrong head info but AFAICT it will never die(), such that for the\npurposes of this series (which only wants to know if a submodule uses the\nworktree feature) it should be fine.\n\nIt is highly misleading though for others to build upon this.\nSo maybe I'll only add the functionality internally in worktree.c\nand document why the values are wrong, and only expose the\n\"int submodule_uses_worktrees(const char *path)\" ?\n\n>> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)\n>> return list;\n>\n> Right before this line is mark_current_worktree(), which in turn calls\n> get_git_dir() and not suitable for peeking into another repository the\n> way submodule code does. get_worktree_git_dir() called within that\n> function shares the same problem.\n\nIt actually works correctly: \"No submodule is the current worktree\".\n\n\n>\n>> }\n>>\n>> +struct worktree **get_worktrees(unsigned flags)\n>> +{\n>> + return get_worktrees_internal(get_git_common_dir(), flags);\n>> +}\n>> +\n>> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)\n>> +{\n>> + char *submodule_gitdir;\n>> + struct strbuf sb = STRBUF_INIT;\n>> + struct worktree **ret;\n>> +\n>> + submodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n>> + if (!submodule_gitdir)\n>> + return NULL;\n>> +\n>> + /* the env would be set for the superproject */\n>> + get_common_dir_noenv(&sb, submodule_gitdir);\n>\n> Technically we need to read submodule_gitdir/.config and see if we can\n> understand core.repositoryformatversion, or find any unrecognized\n> extensions. But the problem is not specific to this code. And fixing\n> it is no small task. But perhaps we could call a dummy\n> validate_submodule_gitdir() here? Then when we implement that function\n> for real, we don't have to search the entire code base to see where to\n> put it.\n>\n> Kinda off-topic though. Feel free to ignore the above comment.\n\nok I'll add a TODO/emptyfunction for that.\n\nThanks for the review!\nStefan\n"},{"id":"307301","messageId":"CAGZ79kbNAZ81JqO+UEmKJC06oDRKWjxaF3Cb6jpZ3Xxo=des8g@mail.gmail.com","threadId":"44650","inReplyTo":"CACsJy8DMhC7Umnqf+-ViOBKn46Emtan4647=MDdLoYXVNB0=6A@mail.gmail.com","subject":"Re: [PATCHv6 5/7] worktree: add function to check if worktrees are in use","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T19:32:25Z","receivedAt":"2016-12-08T19:32:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Dec 8, 2016 at 2:51 AM, Duy Nguyen wrote:\n> On Thu, Dec 8, 2016 at 5:40 PM, Duy Nguyen wrote:\n>> Alternatively, we could add a new flag to get_worktrees() to tell it\n>> to return all worktrees if there is a least one linked worktree, or\n>> return NULL if there's only main worktree. I'm not sure if this is\n>> clever or very stupid.\n>\n> No, this may be better. Add a flag to say \"returns linked worktrees\n> _only_\". Which means when you're in a \"normal\" repo, get_worktrees()\n> with this flag returns NULL. When you're in a multiple-worktree repo,\n> it returns all linked worktrees (no main worktree). I think I might\n> have a use for this flag in addition to this uses_worktrees() here.\n> uses_worktrees() look quite simple with that flag\n>\n> int uses_worktrees(void)\n> {\n> struct worktree **worktrees = get_worktrees(WT_LINKED_ONLY);\n> int retval = worktrees != NULL;\n\nI am interested in the submodule case however, where we already return NULL\ne.g. when the submodule git dir cannot be found. Actually that would\nwork out fine\nas well:\n\n /* NOTE on accuracy of result, hence not exposed. */\n static worktree **submodule_get_worktrees(const char *path, unsigned flags)\n ..\n\n int submodule_uses_worktrees(const char *path)\n {\n struct worktree **worktrees = submodule_get_worktrees(path,\nWT_LINKED_ONLY);\n int retval = worktrees != NULL;\n free_worktrees(worktrees);\n return retval;\n }\n\nThanks for that inspiration!\n"},{"id":"307304","messageId":"CAGZ79kagKKE7fiwdPGgLsaQuQV-Y2ePgPdTLkwZJ0T9UN9uH_Q@mail.gmail.com","threadId":"44650","inReplyTo":"CACsJy8AohKMTm0inUyxu=kTwxNFcCbbBRO=O126SCzi5e=0tSQ@mail.gmail.com","subject":"Re: [PATCHv6 2/7] submodule helper: support super prefix","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-08T20:19:00Z","receivedAt":"2016-12-08T20:19:09Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":">\n> unsigned int is probably safer for variables that are used as bit-flags.\n\ndone\n\n> If it's meant for users to see, please _() the string.\n\ndone\n\n>> + { \"submodule--helper\", cmd_submodule__helper, RUN_SETUP | SUPPORT_SUPER_PREFIX},\n>\n> The same macro defined twice in two separate .c files? Hmm.. it\n> confused me a bit because i thought there was a connection.. I guess\n> it's ok.\n\nIts effect is exactly the same. just plain code duplication. I assume that\nis okay here as it really is only one constant for now. Maybe we can\nrefactor that to\nbe defined in a header file (but which? There is no such thing as a git.h, where\nI'd expect to put it.)\n\nThanks,\nStefan\n"},{"id":"307349","messageId":"CACsJy8BoPXDYZqqDOSZR-msNjuZgGzFL_mF2xQVsg82jUfdXmw@mail.gmail.com","threadId":"44650","inReplyTo":"CAGZ79kYtEUvuTX09sJm3C0rG0-BrBz4bN0FCs6E5d2jHhtKN6w@mail.gmail.com","subject":"Re: [PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Duy Nguyen","fromEmail":"pclouds@gmail.com","sentAt":"2016-12-09T12:46:37Z","receivedAt":"2016-12-09T12:47:25Z","isPatch":false,"sender":{"key":"pclouds@gmail.com","avatar":"https://avatars.githubusercontent.com/u/720?v=4"},"body":"On Fri, Dec 9, 2016 at 1:55 AM, Stefan Beller wrote:\n> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen wrote:\n>> On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n>>>\n>>> worktree = xcalloc(1, sizeof(*worktree));\n>>> worktree->path = strbuf_detach(&worktree_path, NULL);\n>>> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n>>\n>> All the good stuff is outside context lines again.. Somewhere between\n>> here we call add_head_info() which calls resolve_ref_unsafe(), which\n>> always uses data from current repo, not the submodule we want it to\n>> look at.\n>\n> Unrelated side question: What would you think of \"variable context line\n> configuration\" ? e.g. you could configure it to include anything from\n> up that line\n> that is currently shown after the @@ which is the function signature line.\n\nHmm.. no idea. I once dreamt of writing \"Diff-Options: -U10\" in the\ncommit message and let git-log and everybody use that option\nautomatically, though. I guess it's unrelated to your question :D\n\n> As to the add_head_info/resolve_ref_unsafe what impact does that have?\n> It produces a wrong head info but AFAICT it will never die(), such that for the\n> purposes of this series (which only wants to know if a submodule uses the\n> worktree feature) it should be fine.\n>\n> It is highly misleading though for others to build upon this.\n> So maybe I'll only add the functionality internally in worktree.c\n> and document why the values are wrong, and only expose the\n> \"int submodule_uses_worktrees(const char *path)\" ?\n\nYeah for submodule use then it should be ok. But people may start\nusing it for something else, not realizing that it does not work as\nexpected.\n-- \nDuy\n"},{"id":"307401","messageId":"20161209230029.GF88637@google.com","threadId":"44650","inReplyTo":"CAGZ79kYtEUvuTX09sJm3C0rG0-BrBz4bN0FCs6E5d2jHhtKN6w@mail.gmail.com","subject":"Re: [PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2016-12-09T23:00:29Z","receivedAt":"2016-12-09T23:00:38Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 12/08, Stefan Beller wrote:\n> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen wrote:\n> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n> >>\n> >> worktree = xcalloc(1, sizeof(*worktree));\n> >> worktree->path = strbuf_detach(&worktree_path, NULL);\n> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n> >\n> > All the good stuff is outside context lines again.. Somewhere between\n> > here we call add_head_info() which calls resolve_ref_unsafe(), which\n> > always uses data from current repo, not the submodule we want it to\n> > look at.\n> \n> Unrelated side question: What would you think of \"variable context line\n> configuration\" ? e.g. you could configure it to include anything from\n> up that line\n> that is currently shown after the @@ which is the function signature line.\n> \n> As to the add_head_info/resolve_ref_unsafe what impact does that have?\n> It produces a wrong head info but AFAICT it will never die(), such that for the\n> purposes of this series (which only wants to know if a submodule uses the\n> worktree feature) it should be fine.\n> \n> It is highly misleading though for others to build upon this.\n> So maybe I'll only add the functionality internally in worktree.c\n> and document why the values are wrong, and only expose the\n> \"int submodule_uses_worktrees(const char *path)\" ?\n> \n> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)\n> >> return list;\n> >\n> > Right before this line is mark_current_worktree(), which in turn calls\n> > get_git_dir() and not suitable for peeking into another repository the\n> > way submodule code does. get_worktree_git_dir() called within that\n> > function shares the same problem.\n> \n> It actually works correctly: \"No submodule is the current worktree\".\n> \n> \n> >\n> >> }\n> >>\n> >> +struct worktree **get_worktrees(unsigned flags)\n> >> +{\n> >> + return get_worktrees_internal(get_git_common_dir(), flags);\n> >> +}\n> >> +\n> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)\n> >> +{\n> >> + char *submodule_gitdir;\n> >> + struct strbuf sb = STRBUF_INIT;\n> >> + struct worktree **ret;\n> >> +\n> >> + submodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n> >> + if (!submodule_gitdir)\n> >> + return NULL;\n> >> +\n> >> + /* the env would be set for the superproject */\n> >> + get_common_dir_noenv(&sb, submodule_gitdir);\n> >\n> > Technically we need to read submodule_gitdir/.config and see if we can\n> > understand core.repositoryformatversion, or find any unrecognized\n> > extensions. But the problem is not specific to this code. And fixing\n> > it is no small task. But perhaps we could call a dummy\n> > validate_submodule_gitdir() here? Then when we implement that function\n> > for real, we don't have to search the entire code base to see where to\n> > put it.\n> >\n> > Kinda off-topic though. Feel free to ignore the above comment.\n> \n> ok I'll add a TODO/emptyfunction for that.\n\nSo is using resolve_gitdir not ok when trying to see if a submodule has\na gitdir at a particular path?\n\n-- \nBrandon Williams\n"},{"id":"307403","messageId":"CAGZ79kao0Ei_M8SERz0M8C+dbe=NeqRWFakcPd4ogAx3fk1Dtg@mail.gmail.com","threadId":"44650","inReplyTo":"20161209230029.GF88637@google.com","subject":"Re: [PATCHv6 4/7] worktree: get worktrees from submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2016-12-09T23:10:19Z","receivedAt":"2016-12-09T23:10:27Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Dec 9, 2016 at 3:00 PM, Brandon Williams wrote:\n> On 12/08, Stefan Beller wrote:\n>> On Thu, Dec 8, 2016 at 2:09 AM, Duy Nguyen wrote:\n>> > On Thu, Dec 8, 2016 at 8:46 AM, Stefan Beller wrote:\n>> >>\n>> >> worktree = xcalloc(1, sizeof(*worktree));\n>> >> worktree->path = strbuf_detach(&worktree_path, NULL);\n>> >> @@ -101,7 +101,8 @@ static struct worktree *get_main_worktree(void)\n>> >\n>> > All the good stuff is outside context lines again.. Somewhere between\n>> > here we call add_head_info() which calls resolve_ref_unsafe(), which\n>> > always uses data from current repo, not the submodule we want it to\n>> > look at.\n>>\n>> Unrelated side question: What would you think of \"variable context line\n>> configuration\" ? e.g. you could configure it to include anything from\n>> up that line\n>> that is currently shown after the @@ which is the function signature line.\n>>\n>> As to the add_head_info/resolve_ref_unsafe what impact does that have?\n>> It produces a wrong head info but AFAICT it will never die(), such that for the\n>> purposes of this series (which only wants to know if a submodule uses the\n>> worktree feature) it should be fine.\n>>\n>> It is highly misleading though for others to build upon this.\n>> So maybe I'll only add the functionality internally in worktree.c\n>> and document why the values are wrong, and only expose the\n>> \"int submodule_uses_worktrees(const char *path)\" ?\n>>\n>> >> @@ -209,6 +211,30 @@ struct worktree **get_worktrees(unsigned flags)\n>> >> return list;\n>> >\n>> > Right before this line is mark_current_worktree(), which in turn calls\n>> > get_git_dir() and not suitable for peeking into another repository the\n>> > way submodule code does. get_worktree_git_dir() called within that\n>> > function shares the same problem.\n>>\n>> It actually works correctly: \"No submodule is the current worktree\".\n>>\n>>\n>> >\n>> >> }\n>> >>\n>> >> +struct worktree **get_worktrees(unsigned flags)\n>> >> +{\n>> >> + return get_worktrees_internal(get_git_common_dir(), flags);\n>> >> +}\n>> >> +\n>> >> +struct worktree **get_submodule_worktrees(const char *path, unsigned flags)\n>> >> +{\n>> >> + char *submodule_gitdir;\n>> >> + struct strbuf sb = STRBUF_INIT;\n>> >> + struct worktree **ret;\n>> >> +\n>> >> + submodule_gitdir = git_pathdup_submodule(path, \"%s\", \"\");\n>> >> + if (!submodule_gitdir)\n>> >> + return NULL;\n>> >> +\n>> >> + /* the env would be set for the superproject */\n>> >> + get_common_dir_noenv(&sb, submodule_gitdir);\n>> >\n>> > Technically we need to read submodule_gitdir/.config and see if we can\n>> > understand core.repositoryformatversion, or find any unrecognized\n>> > extensions. But the problem is not specific to this code. And fixing\n>> > it is no small task. But perhaps we could call a dummy\n>> > validate_submodule_gitdir() here? Then when we implement that function\n>> > for real, we don't have to search the entire code base to see where to\n>> > put it.\n>> >\n>> > Kinda off-topic though. Feel free to ignore the above comment.\n>>\n>> ok I'll add a TODO/emptyfunction for that.\n>\n> So is using resolve_gitdir not ok when trying to see if a submodule has\n> a gitdir at a particular path?\n>\n\nWell it depends on the question that you are trying to answer:\n\nAssume we introduce a new repository format in Git version 3.0.\nBy some means the submodule repository is converted to the new format,\nbut the superproject is not. (e.g. it is auto migrating repositories that have\na lot of large blobs/files)\n\nNow for some reason you use the older git 2.X in the superproject.\n(e.g. the code is on a shared network mount, or git3 has a bug, or ...)\n\nThen the code above may tell that the submodule doesn't use worktrees\n(as we cannot make any assumptions on the new crazy repository format),\nbut in fact it is, so in this case we technically we would need to:\n\n1) read the config\n2) check the repository format version (if larger than we know of,\n assume the worst or just die?)\n\nAs the function in this patch is used for safe guarding we may want to be\nextra cautious, another function that is using resolve_gitdir may have other\nassumptions on what is ok even for newer repository formats.\n"}]}