{"thread":{"id":"45842","subject":"[PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","startedAt":"2017-05-01T18:01:12Z","lastAt":"2017-05-31T22:09:17Z","messageCount":22,"participants":["Stefan Beller","Brandon Williams","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"318383","messageId":"20170501180058.8063-1-sbeller@google.com","threadId":"45842","inReplyTo":null,"subject":"[PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:53Z","receivedAt":"2017-05-01T18:01:12Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"The first three patches fix bugs in existing submodule code,\nsort of independent from the last 2 patches, which are RFCs.\n\n\n\nIn submodules as of today you always end up with a detached HEAD,\nwhich may be scary to people used to working on branches. (I myself\nenjoy working with a detached HEAD).\n\nThe detached HEAD in a submodule is the correct in the submodule-as-a-lib\ncase, but in case you actively want to work inside submodules, you\nmay want to have proper branch support, e.g. when typing:\n\n git checkout --recuse-submodules master\n\nyou may want to have the submodules on the master branch as well.\n\nThere are a couple of challenges though:\n* We'd probably want to pay attention to the branch configuration\n (.gitmodules submodule.NAME.branch to be \".\" or \"foo\")\n* What if the submodule does not have a master branch?\n Jonathan proposed off list that we may just want to create the\n branch in the submodule. This is not implemented in this series.\n* In case the branch exists in the submodule and doesn't point at the\n the object as recorded in the superproject, we may either want to \n update the branch to point at the superproject record or we want to\n reject the \"reattaching HEAD\". Patch 4 provides the later.\n\nAny other ideas on why this could be a bad idea or corner cases?\n\nThanks,\nStefan\n\nStefan Beller (5):\n submodule_move_head: fix leak of struct child_process\n submodule_move_head: prepare env for child correctly\n submodule: avoid auto-discovery in new working tree manipulator code\n submodule--helper: reattach-HEAD\n submodule_move_head: reattach submodule HEAD to branch if possible.\n\n builtin/submodule--helper.c | 12 ++++++\n submodule.c | 93 ++++++++++++++++++++++++++++++++++++++++-----\n submodule.h | 10 +++++\n 3 files changed, 106 insertions(+), 9 deletions(-)\n\n-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318384","messageId":"20170501180058.8063-2-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[PATCH 1/5] submodule_move_head: fix leak of struct child_process","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:54Z","receivedAt":"2017-05-01T18:01:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"While fixing the leak of `cp`, reuse it instead of having to declare\nanother struct child_process.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 15 +++++++++------\n 1 file changed, 9 insertions(+), 6 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d3299e29c0..cd098cf12b 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1466,17 +1466,19 @@ int submodule_move_head(const char *path,\n \t\tgoto out;\n \t}\n \n+\tchild_process_clear(&cp);\n+\n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n \t\tif (new) {\n-\t\t\tstruct child_process cp1 = CHILD_PROCESS_INIT;\n+\t\t\tchild_process_init(&cp);\n \t\t\t/* also set the HEAD accordingly */\n-\t\t\tcp1.git_cmd = 1;\n-\t\t\tcp1.no_stdin = 1;\n-\t\t\tcp1.dir = path;\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.dir = path;\n \n-\t\t\targv_array_pushl(&cp1.args, \"update-ref\", \"HEAD\", new, NULL);\n+\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n \n-\t\t\tif (run_command(&cp1)) {\n+\t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\n \t\t\t\tgoto out;\n \t\t\t}\n@@ -1492,6 +1494,7 @@ int submodule_move_head(const char *path,\n \t\t}\n \t}\n out:\n+\tchild_process_clear(&cp);\n \treturn ret;\n }\n \n-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318385","messageId":"20170501180058.8063-4-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[PATCH 3/5] submodule: avoid auto-discovery in new working tree manipulator code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:56Z","receivedAt":"2017-05-01T18:01:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All commands that are run in a submodule, are run in a correct setup,\nthere is no need to prepare the environment without setting the GIT_DIR\nvariable. By setting the GIT_DIR variable we fix issues as discussed in\n10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01)\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex c7a7a33916..df03691199 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1363,7 +1363,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \targv_array_pushl(&cp.args, \"diff-index\", \"--quiet\",\n@@ -1380,7 +1380,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)\n static void submodule_reset_index(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n@@ -1438,7 +1438,7 @@ int submodule_move_head(const char *path,\n \t\t}\n \t}\n \n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318386","messageId":"20170501180058.8063-3-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[PATCH 2/5] submodule_move_head: prepare env for child correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:55Z","receivedAt":"2017-05-01T18:01:19Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We forgot to prepare the submodule env, which is only a problem for\nnested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules\nwith nested submodules, 2017-04-13) for further explanation.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 1 +\n 1 file changed, 1 insertion(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex cd098cf12b..c7a7a33916 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1476,6 +1476,7 @@ int submodule_move_head(const char *path,\n \t\t\tcp.no_stdin = 1;\n \t\t\tcp.dir = path;\n \n+\t\t\tprepare_submodule_repo_env(&cp.env_array);\n \t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n \n \t\t\tif (run_command(&cp)) {\n-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318387","messageId":"20170501180058.8063-5-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[RFC PATCH 4/5] submodule--helper: reattach-HEAD","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:57Z","receivedAt":"2017-05-01T18:01:21Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Add a new command, that reattaches a detached HEAD to its configured\nbranch in a submodule.\n\nIn a later patch we will teach git-submodule as well as other submodule\nworktree manipulators (git reset/checkout --recurse-submodules) to not\nend up in a detached HEAD state in the submodules.\n\nSigned-off-by: Stefan Beller \n---\n builtin/submodule--helper.c | 12 ++++++++\n submodule.c | 69 +++++++++++++++++++++++++++++++++++++++++++++\n submodule.h | 10 +++++++\n 3 files changed, 91 insertions(+)\n\ndiff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c\nindex 36e4231821..645ceac999 100644\n--- a/builtin/submodule--helper.c\n+++ b/builtin/submodule--helper.c\n@@ -1196,6 +1196,17 @@ static int is_active(int argc, const char **argv, const char *prefix)\n \treturn !is_submodule_initialized(argv[1]);\n }\n \n+static int cmd_reattach_HEAD(int argc, const char **argv, const char *prefix)\n+{\n+\tif (argc != 2)\n+\t\tdie(\"submodule--helper reattach-HEAD takes exactly 1 argument\");\n+\n+\tgitmodules_config();\n+\tgit_config(submodule_config, NULL);\n+\n+\treturn reattach_HEAD(argv[1], REATTACH_HEAD_DIE_ON_ERROR);\n+}\n+\n #define SUPPORT_SUPER_PREFIX (1<<0)\n \n struct cmd_struct {\n@@ -1217,6 +1228,7 @@ static struct cmd_struct commands[] = {\n \t{\"push-check\", push_check, 0},\n \t{\"absorb-git-dirs\", absorb_git_dirs, SUPPORT_SUPER_PREFIX},\n \t{\"is-active\", is_active, 0},\n+\t{\"reattach-HEAD\", cmd_reattach_HEAD, 0}\n };\n \n int cmd_submodule__helper(int argc, const char **argv, const char *prefix)\ndiff --git a/submodule.c b/submodule.c\nindex df03691199..4e74e38829 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1499,6 +1499,75 @@ int submodule_move_head(const char *path,\n \treturn ret;\n }\n \n+int reattach_HEAD(const char *submodule_path, int flags)\n+{\n+\tconst char *branch;\n+\tstruct object_id sub_head_object;\n+\tstruct object_id sub_branch_object;\n+\n+\tconst struct submodule *sub = submodule_from_path(null_sha1, submodule_path);\n+\n+\tif (!sub) {\n+\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n+\t\t\treturn -1;\n+\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n+\t\t\tsubmodule_path);\n+\t}\n+\n+\tif (!sub->branch) {\n+\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n+\t\t\treturn -1;\n+\t\tdie(_(\"no branch configured to follow for submodule '%s'\"),\n+\t\t\tsub->path);\n+\t}\n+\n+\t/* lookup branch value in .gitmodules */\n+\tif (strcmp(\".\", sub->branch)) {\n+\t\tbranch = sub->branch;\n+\t} else {\n+\t\t/* special care for '.': Is the superproject on a branch? */\n+\t\tstruct object_id oid;\n+\t\tbranch = resolve_refdup(\"HEAD\", 0, oid.hash, NULL);\n+\t\tif (!branch) {\n+\t\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n+\t\t\t\treturn -1;\n+\t\t\tdie(_(\"Not on any branch, but submodule configured to follow superprojects branch\"));\n+\t\t}\n+\t}\n+\n+\tif (!strcmp(\"HEAD\", branch))\n+\t\treturn 0;\n+\n+\tresolve_gitlink_ref(sub->path, \"HEAD\", sub_head_object.hash);\n+\tresolve_gitlink_ref(sub->path, branch, sub_branch_object.hash);\n+\n+\tif (!oidcmp(&sub_head_object, &sub_branch_object)) {\n+\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n+\t\tstruct strbuf reason = STRBUF_INIT;\n+\n+\t\tcp.dir = sub->path;\n+\t\tprepare_submodule_repo_env(&cp.env_array);\n+\n+\t\tstrbuf_addf(&reason, \"reattach HEAD to %s\", branch);\n+\t\targv_array_pushl(&cp.args, \"git\", \"symbolic-ref\",\n+\t\t\t\t \"-m\", reason.buf, \"HEAD\", branch, NULL);\n+\t\tif (run_command(&cp)) {\n+\t\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n+\t\t\t\treturn -1;\n+\t\t\tdie(_(\"could not run symbolic-ref in submodule '%s'\"),\n+\t\t\t sub->path);\n+\t\t}\n+\t\tstrbuf_release(&reason);\n+\t\treturn 0;\n+\t} else {\n+\t\tfprintf(stderr, \"not reattaching HEAD, object ids differ:\\n\"\n+\t\t\t\t\"HEAD is %s\\n branch is %s\",\n+\t\t\t\toid_to_hex(&sub_head_object),\n+\t\t\t\toid_to_hex(&sub_branch_object));\n+\t\treturn 1;\n+\t}\n+}\n+\n static int find_first_merges(struct object_array *result, const char *path,\n \t\tstruct commit *a, struct commit *b)\n {\ndiff --git a/submodule.h b/submodule.h\nindex 1277480add..f7bb565a6d 100644\n--- a/submodule.h\n+++ b/submodule.h\n@@ -119,6 +119,16 @@ extern int submodule_move_head(const char *path,\n */\n extern void prepare_submodule_repo_env(struct argv_array *out);\n \n+/*\n+ * Attach the submodules HEAD to its configured branch if the underlying\n+ * object id are the same. Returns\n+ * - 0 when the branch could be reattached.\n+ * - +1 when the branch could not be reattached due to object name mismatch\n+ * - <0 when an error occurred (e.g. missing .gitmodules file)\n+ */\n+#define REATTACH_HEAD_DIE_ON_ERROR (1<<0)\n+extern int reattach_HEAD(const char *submodule_path, int flags);\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-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318388","messageId":"20170501180058.8063-6-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[RFC PATCH 5/5] submodule_move_head: reattach submodule HEAD to branch if possible.","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T18:00:58Z","receivedAt":"2017-05-01T18:01:22Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Side note: We also want to call this from git submodule update\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 2 ++\n 1 file changed, 2 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex 4e74e38829..66651ffa34 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1483,6 +1483,8 @@ int submodule_move_head(const char *path,\n \t\t\t\tret = -1;\n \t\t\t\tgoto out;\n \t\t\t}\n+\n+\t\t\treattach_HEAD(path, 0);\n \t\t} else {\n \t\t\tstruct strbuf sb = STRBUF_INIT;\n \n-- \n2.13.0.rc1.1.gbc33f0f778\n\n"},{"id":"318389","messageId":"20170501180610.GF39135@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-2-sbeller@google.com","subject":"Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T18:06:10Z","receivedAt":"2017-05-01T18:06:20Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> While fixing the leak of `cp`, reuse it instead of having to declare\n> another struct child_process.\n> \n> Signed-off-by: Stefan Beller \n\nThis shouldn't be needed as 'finish_command' does the cleanup for you by\ncalling 'child_prcoess_clear()'.\n\n\n> ---\n> submodule.c | 15 +++++++++------\n> 1 file changed, 9 insertions(+), 6 deletions(-)\n> \n> diff --git a/submodule.c b/submodule.c\n> index d3299e29c0..cd098cf12b 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1466,17 +1466,19 @@ int submodule_move_head(const char *path,\n> \t\tgoto out;\n> \t}\n> \n> +\tchild_process_clear(&cp);\n> +\n> \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n> \t\tif (new) {\n> -\t\t\tstruct child_process cp1 = CHILD_PROCESS_INIT;\n> +\t\t\tchild_process_init(&cp);\n> \t\t\t/* also set the HEAD accordingly */\n> -\t\t\tcp1.git_cmd = 1;\n> -\t\t\tcp1.no_stdin = 1;\n> -\t\t\tcp1.dir = path;\n> +\t\t\tcp.git_cmd = 1;\n> +\t\t\tcp.no_stdin = 1;\n> +\t\t\tcp.dir = path;\n> \n> -\t\t\targv_array_pushl(&cp1.args, \"update-ref\", \"HEAD\", new, NULL);\n> +\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n> \n> -\t\t\tif (run_command(&cp1)) {\n> +\t\t\tif (run_command(&cp)) {\n> \t\t\t\tret = -1;\n> \t\t\t\tgoto out;\n> \t\t\t}\n> @@ -1492,6 +1494,7 @@ int submodule_move_head(const char *path,\n> \t\t}\n> \t}\n> out:\n> +\tchild_process_clear(&cp);\n> \treturn ret;\n> }\n> \n> -- \n> 2.13.0.rc1.1.gbc33f0f778\n> \n\n-- \nBrandon Williams\n"},{"id":"318390","messageId":"20170501182405.GG39135@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"Re: [PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T18:24:05Z","receivedAt":"2017-05-01T18:24:13Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> The first three patches fix bugs in existing submodule code,\n> sort of independent from the last 2 patches, which are RFCs.\n> \n> \n> \n> In submodules as of today you always end up with a detached HEAD,\n> which may be scary to people used to working on branches. (I myself\n> enjoy working with a detached HEAD).\n> \n> The detached HEAD in a submodule is the correct in the submodule-as-a-lib\n> case, but in case you actively want to work inside submodules, you\n> may want to have proper branch support, e.g. when typing:\n> \n> git checkout --recuse-submodules master\n> \n> you may want to have the submodules on the master branch as well.\n\nI don't know why submodules were originally designed to be in a\ndetached HEAD state but I much prefer working on branches (as I'm sure\nmany other developers do) so the prospect of this becoming the norm is\nexciting! :D\n> \n> There are a couple of challenges though:\n> * We'd probably want to pay attention to the branch configuration\n> (.gitmodules submodule.NAME.branch to be \".\" or \"foo\")\n\nYes, I agree. If we have a particular name of a branch configured to be\ntracked, then we should respect that and try to attach HEAD onto that\nbranch name.\n\n> * What if the submodule does not have a master branch?\n> Jonathan proposed off list that we may just want to create the\n> branch in the submodule. This is not implemented in this series.\n\nI think it would be fine to create the branch, just as long as it\ndoesn't already exist as I can't think of a negative impact of this\n(aside from maybe having more branches in the submodules after a\nprolonged time period?)\n\n> * In case the branch exists in the submodule and doesn't point at the\n> the object as recorded in the superproject, we may either want to \n> update the branch to point at the superproject record or we want to\n> reject the \"reattaching HEAD\". Patch 4 provides the later.\n\nThe later seems like the most correct thing to do. It would be\nunfortunate if I had done some work on top of the master branch in a\nsubmodule (which wasn't reflected in the superproject), then done a\n'checkout master' in the super project only to go back to the submodule\nand find that all my work is gone! (Well not gone, but you'd have to go\ninto the scary reflog!) It just makes sense to leave HEAD in a detached\nstate here as it prevents loss of work.\n\n> Any other ideas on why this could be a bad idea or corner cases?\n> \n> Thanks,\n> Stefan\n> \n> Stefan Beller (5):\n> submodule_move_head: fix leak of struct child_process\n> submodule_move_head: prepare env for child correctly\n> submodule: avoid auto-discovery in new working tree manipulator code\n> submodule--helper: reattach-HEAD\n> submodule_move_head: reattach submodule HEAD to branch if possible.\n> \n> builtin/submodule--helper.c | 12 ++++++\n> submodule.c | 93 ++++++++++++++++++++++++++++++++++++++++-----\n> submodule.h | 10 +++++\n> 3 files changed, 106 insertions(+), 9 deletions(-)\n> \n> -- \n> 2.13.0.rc1.1.gbc33f0f778\n> \n\n-- \nBrandon Williams\n"},{"id":"318391","messageId":"20170501183656.GH39135@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-5-sbeller@google.com","subject":"Re: [RFC PATCH 4/5] submodule--helper: reattach-HEAD","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-01T18:36:56Z","receivedAt":"2017-05-01T18:37:08Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/01, Stefan Beller wrote:\n> Add a new command, that reattaches a detached HEAD to its configured\n> branch in a submodule.\n> \n> In a later patch we will teach git-submodule as well as other submodule\n> worktree manipulators (git reset/checkout --recurse-submodules) to not\n> end up in a detached HEAD state in the submodules.\n> \n> Signed-off-by: Stefan Beller \n> \n> +int reattach_HEAD(const char *submodule_path, int flags)\n> +{\n> +\tconst char *branch;\n> +\tstruct object_id sub_head_object;\n> +\tstruct object_id sub_branch_object;\n> +\n> +\tconst struct submodule *sub = submodule_from_path(null_sha1, submodule_path);\n> +\n> +\tif (!sub) {\n> +\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n> +\t\t\treturn -1;\n> +\t\tdie(_(\"no submodule mapping found in .gitmodules for path '%s'\"),\n> +\t\t\tsubmodule_path);\nIt may be a bit clearer to do the following:\n\n if (flags & REATTACH_HEAD_DIE_ON_ERROR)\n die(...);\n\n return -1;\n\nIt just feels weird to me to have the inverted logic, that's my opinion\nthough.\n\n> +\t}\n> +\n> +\tif (!sub->branch) {\n> +\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n> +\t\t\treturn -1;\n> +\t\tdie(_(\"no branch configured to follow for submodule '%s'\"),\n> +\t\t\tsub->path);\n> +\t}\n> +\n> +\t/* lookup branch value in .gitmodules */\n> +\tif (strcmp(\".\", sub->branch)) {\n> +\t\tbranch = sub->branch;\n> +\t} else {\n> +\t\t/* special care for '.': Is the superproject on a branch? */\n> +\t\tstruct object_id oid;\n> +\t\tbranch = resolve_refdup(\"HEAD\", 0, oid.hash, NULL);\n> +\t\tif (!branch) {\n> +\t\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n> +\t\t\t\treturn -1;\n> +\t\t\tdie(_(\"Not on any branch, but submodule configured to follow superprojects branch\"));\n> +\t\t}\n> +\t}\n> +\n> +\tif (!strcmp(\"HEAD\", branch))\n> +\t\treturn 0;\n\nSo this is the case where the superproject is in a detached-HEAD state?\nIn that case then we don't need to continue because if the superproject\nisn't on a branch, then there isn't a reason the submodule should be on\na branch.\n\n> +\n> +\tresolve_gitlink_ref(sub->path, \"HEAD\", sub_head_object.hash);\n> +\tresolve_gitlink_ref(sub->path, branch, sub_branch_object.hash);\n> +\n> +\tif (!oidcmp(&sub_head_object, &sub_branch_object)) {\n> +\t\tstruct child_process cp = CHILD_PROCESS_INIT;\n> +\t\tstruct strbuf reason = STRBUF_INIT;\n> +\n> +\t\tcp.dir = sub->path;\n> +\t\tprepare_submodule_repo_env(&cp.env_array);\n> +\n> +\t\tstrbuf_addf(&reason, \"reattach HEAD to %s\", branch);\n> +\t\targv_array_pushl(&cp.args, \"git\", \"symbolic-ref\",\n> +\t\t\t\t \"-m\", reason.buf, \"HEAD\", branch, NULL);\n> +\t\tif (run_command(&cp)) {\n> +\t\t\tif (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n> +\t\t\t\treturn -1;\n> +\t\t\tdie(_(\"could not run symbolic-ref in submodule '%s'\"),\n> +\t\t\t sub->path);\n> +\t\t}\n> +\t\tstrbuf_release(&reason);\n> +\t\treturn 0;\n> +\t} else {\n> +\t\tfprintf(stderr, \"not reattaching HEAD, object ids differ:\\n\"\n> +\t\t\t\t\"HEAD is %s\\n branch is %s\",\n> +\t\t\t\toid_to_hex(&sub_head_object),\n> +\t\t\t\toid_to_hex(&sub_branch_object));\n> +\t\treturn 1;\n> +\t}\n> +}\n> +\n> static int find_first_merges(struct object_array *result, const char *path,\n> \t\tstruct commit *a, struct commit *b)\n> {\n> diff --git a/submodule.h b/submodule.h\n> index 1277480add..f7bb565a6d 100644\n> --- a/submodule.h\n> +++ b/submodule.h\n> @@ -119,6 +119,16 @@ extern int submodule_move_head(const char *path,\n> */\n> extern void prepare_submodule_repo_env(struct argv_array *out);\n> \n> +/*\n> + * Attach the submodules HEAD to its configured branch if the underlying\n> + * object id are the same. Returns\n> + * - 0 when the branch could be reattached.\n> + * - +1 when the branch could not be reattached due to object name mismatch\n> + * - <0 when an error occurred (e.g. missing .gitmodules file)\n> + */\n> +#define REATTACH_HEAD_DIE_ON_ERROR (1<<0)\n> +extern int reattach_HEAD(const char *submodule_path, int flags);\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> -- \n> 2.13.0.rc1.1.gbc33f0f778\n> \n\n-- \nBrandon Williams\n"},{"id":"318392","messageId":"CAGZ79kb-J6W4XSWoD7LBFB_o4Dp86o1oK5ReBmeqV7WB0h5VgA@mail.gmail.com","threadId":"45842","inReplyTo":"20170501183656.GH39135@google.com","subject":"Re: [RFC PATCH 4/5] submodule--helper: reattach-HEAD","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-01T19:00:49Z","receivedAt":"2017-05-01T19:00:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 11:36 AM, Brandon Williams wrote:\n> if (flags & REATTACH_HEAD_DIE_ON_ERROR)\n> die(...);\n>\n> return -1;\n>\n> It just feels weird to me to have the inverted logic, that's my opinion\n> though.\n\nYeah, me too. But my feelings were not as important as staying\nunder 80 characters a line. And as the error message is longer than\nthe \"return -1\", I wanted to have it not indented yet another level.\n\n>\n>> + }\n>> +\n>> + if (!sub->branch) {\n>> + if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n>> + return -1;\n>> + die(_(\"no branch configured to follow for submodule '%s'\"),\n>> + sub->path);\n>> + }\n>> +\n>> + /* lookup branch value in .gitmodules */\n>> + if (strcmp(\".\", sub->branch)) {\n>> + branch = sub->branch;\n>> + } else {\n>> + /* special care for '.': Is the superproject on a branch? */\n>> + struct object_id oid;\n>> + branch = resolve_refdup(\"HEAD\", 0, oid.hash, NULL);\n>> + if (!branch) {\n>> + if (!(flags & REATTACH_HEAD_DIE_ON_ERROR))\n>> + return -1;\n>> + die(_(\"Not on any branch, but submodule configured to follow superprojects branch\"));\n>> + }\n>> + }\n>> +\n>> + if (!strcmp(\"HEAD\", branch))\n>> + return 0;\n>\n> So this is the case where the superproject is in a detached-HEAD state?\n\nand the submodule config is branch='.'\n\n> In that case then we don't need to continue because if the superproject\n> isn't on a branch, then there isn't a reason the submodule should be on\n> a branch.\n\nagreed.\n"},{"id":"318447","messageId":"xmqqbmrcf3cn.fsf@gitster.mtv.corp.google.com","threadId":"45842","inReplyTo":"20170501182405.GG39135@google.com","subject":"Re: [PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-02T01:35:04Z","receivedAt":"2017-05-02T01:35:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> I don't know why submodules were originally designed to be in a\n> detached HEAD state but I much prefer working on branches (as I'm sure\n> many other developers do) so the prospect of this becoming the norm is\n> exciting! :D\n\nThe reason is because the superproject already records where the\nHEAD in the submodule should be, when any of its commits is checked\nout. The tip of a branch (which one???) in a submodule may match\nthat commit, in which case there is nothing gained by having you on\nthat branch, or it may not match that commit, in which case it is\nunclear what should happen. Leaving your submodule on the branch\nwould mean the state of your tree as a whole does not match what the\ncheckout operation in the superproject wanted to create. Resetting\nthe branch would mean you may lose the history of the branch.\n\nThinking of the detached HEAD state as an implicit unnamed branch\nthat matches the state the superproject checkout expects was\nconceptually one of the cleanest choices.\n\nBut all of the above concentrates on what should happen immediately\nafter you do a checkout in the superproject, and it would be OK for\na sight-seer. Once you want to work in the submodules to build on\ntheir histories, not being on a branch does become awkward. For one\nthing, after you are done with the work in your submodule, you would\nwant to update the superproject and make a commit that records the\ncommit in the submodule, and then you would want to publish both the\nsuperproject and the submodule because both of them are now\nupdated. The commit in the superproject may be on the branch that\nis currently checked out, but we don't know what branch the commit\nin the submoudule should go.\n\nThe submodule..branch configuration may be a good source to\nlearn that piece of information, but it does not fully solve the\nissue. It is OK for the tip of that branch to be at or behind the\ncommit the superproject records, but it is unclear what should\nhappen if the local tip of that branch is ahead of the commit in the\nsuperproject when checkout happens.\n\nBy the way, how does this topic work with the various checkout modes\nthat can be specified with submodule..update?\n\n\n"},{"id":"318450","messageId":"xmqqy3ugdnfu.fsf@gitster.mtv.corp.google.com","threadId":"45842","inReplyTo":"20170501180058.8063-3-sbeller@google.com","subject":"Re: [PATCH 2/5] submodule_move_head: prepare env for child correctly","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-02T02:04:05Z","receivedAt":"2017-05-02T02:04:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Stefan Beller writes:\n\n> We forgot to prepare the submodule env, which is only a problem for\n> nested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules\n> with nested submodules, 2017-04-13) for further explanation.\n>\n> Signed-off-by: Stefan Beller \n> ---\n\nSounds good (if only because this makes it similar to other\ncodepaths).\n\nIs this something whose breakage before the patch is easily\ndemonstrated with a test?\n\n> submodule.c | 1 +\n> 1 file changed, 1 insertion(+)\n>\n> diff --git a/submodule.c b/submodule.c\n> index cd098cf12b..c7a7a33916 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1476,6 +1476,7 @@ int submodule_move_head(const char *path,\n> \t\t\tcp.no_stdin = 1;\n> \t\t\tcp.dir = path;\n> \n> +\t\t\tprepare_submodule_repo_env(&cp.env_array);\n> \t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n> \n> \t\t\tif (run_command(&cp)) {\n"},{"id":"318453","messageId":"xmqqpofsdkim.fsf@gitster.mtv.corp.google.com","threadId":"45842","inReplyTo":"20170501180610.GF39135@google.com","subject":"Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-05-02T03:07:13Z","receivedAt":"2017-05-02T03:07:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Brandon Williams writes:\n\n> On 05/01, Stefan Beller wrote:\n>> While fixing the leak of `cp`, reuse it instead of having to declare\n>> another struct child_process.\n>> \n>> Signed-off-by: Stefan Beller \n>\n> This shouldn't be needed as 'finish_command' does the cleanup for you by\n> calling 'child_prcoess_clear()'.\n\nYes, indeed. \n\nIt might not hurt to update the code so that the cp is reused in the\nsecond run instead of using a separate instance cp1, but that is a\ntopic separate from fixing a non-existing leak.\n\n>> ---\n>> submodule.c | 15 +++++++++------\n>> 1 file changed, 9 insertions(+), 6 deletions(-)\n>> \n>> diff --git a/submodule.c b/submodule.c\n>> index d3299e29c0..cd098cf12b 100644\n>> --- a/submodule.c\n>> +++ b/submodule.c\n>> @@ -1466,17 +1466,19 @@ int submodule_move_head(const char *path,\n>> \t\tgoto out;\n>> \t}\n>> \n>> +\tchild_process_clear(&cp);\n>> +\n>> \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n>> \t\tif (new) {\n>> -\t\t\tstruct child_process cp1 = CHILD_PROCESS_INIT;\n>> +\t\t\tchild_process_init(&cp);\n>> \t\t\t/* also set the HEAD accordingly */\n>> -\t\t\tcp1.git_cmd = 1;\n>> -\t\t\tcp1.no_stdin = 1;\n>> -\t\t\tcp1.dir = path;\n>> +\t\t\tcp.git_cmd = 1;\n>> +\t\t\tcp.no_stdin = 1;\n>> +\t\t\tcp.dir = path;\n>> \n>> -\t\t\targv_array_pushl(&cp1.args, \"update-ref\", \"HEAD\", new, NULL);\n>> +\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n>> \n>> -\t\t\tif (run_command(&cp1)) {\n>> +\t\t\tif (run_command(&cp)) {\n>> \t\t\t\tret = -1;\n>> \t\t\t\tgoto out;\n>> \t\t\t}\n>> @@ -1492,6 +1494,7 @@ int submodule_move_head(const char *path,\n>> \t\t}\n>> \t}\n>> out:\n>> +\tchild_process_clear(&cp);\n>> \treturn ret;\n>> }\n>> \n>> -- \n>> 2.13.0.rc1.1.gbc33f0f778\n>> \n"},{"id":"318468","messageId":"CAGZ79kb52QDUG0RtTXNEEpMJR1CSMYMrRHTRRvGn0-cF=HnzWw@mail.gmail.com","threadId":"45842","inReplyTo":"xmqqbmrcf3cn.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T04:04:32Z","receivedAt":"2017-05-02T04:04:38Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 6:35 PM, Junio C Hamano wrote:\n> Brandon Williams writes:\n>\n>> I don't know why submodules were originally designed to be in a\n>> detached HEAD state but I much prefer working on branches (as I'm sure\n>> many other developers do) so the prospect of this becoming the norm is\n>> exciting! :D\n>\n> The reason is because the superproject already records where the\n> HEAD in the submodule should be, when any of its commits is checked\n> out. The tip of a branch (which one???)\n\nThe one as configured (submodule.NAME.branch\n\n> in a submodule may match\n> that commit, in which case there is nothing gained by having you on\n> that branch,\n\nBeing on a branch has some advantages, e.g. easier pushing, not\nworrying about losing commits due to gc, an easier \"name\" in a literal sense.\n\n\n> or it may not match that commit, in which case it is\n> unclear what should happen.\n\nYes, I anticipate this to be the main point of discussion.\n\n> Leaving your submodule on the branch\n> would mean the state of your tree as a whole does not match what the\n> checkout operation in the superproject wanted to create. Resetting\n> the branch would mean you may lose the history of the branch.\n\nor telling the user via die(), that there is a mismatch.\n(you may want to run git submodule update --remote to fix the situation)\n\n> Thinking of the detached HEAD state as an implicit unnamed branch\n> that matches the state the superproject checkout expects was\n> conceptually one of the cleanest choices.\n\nAssuming this is the cleanest design, we may want to change the\nmessage of git-status, then.\nUnlike the scary detached HEAD message (incl long hint), we may just\nwant to say\n\n $ git status\n In submodule 'foo'\n You're detached exactly as the superprojects wants you to be.\n Nothing to worry.\n\n\n\n> But all of the above concentrates on what should happen immediately\n> after you do a checkout in the superproject, and it would be OK for\n> a sight-seer. Once you want to work in the submodules to build on\n> their histories, not being on a branch does become awkward. For one\n> thing, after you are done with the work in your submodule, you would\n> want to update the superproject and make a commit that records the\n> commit in the submodule, and then you would want to publish both the\n> superproject and the submodule because both of them are now\n> updated. The commit in the superproject may be on the branch that\n> is currently checked out, but we don't know what branch the commit\n> in the submoudule should go.\n>\n> The submodule..branch configuration may be a good source to\n> learn that piece of information,\n\nGlad we agree up to this point.\n\n> but it does not fully solve the\n> issue. It is OK for the tip of that branch to be at or behind the\n> commit the superproject records, but it is unclear what should\n> happen if the local tip of that branch is ahead of the commit in the\n> superproject when checkout happens.\n\nright. It is still unclear to me as well. I'll have to think about the\nvarious modes of operation.\n\n> By the way, how does this topic work with the various checkout modes\n> that can be specified with submodule..update?\n\nThis series currently does not touch git-submodule, but in principle\nwe could just run \"submodule--helper reattach-HEAD\" after any operation\nand then see if we can attach a HEAD (likely for \"update=checkout\",\nbut in \"merge\" we may want to fast-forward the branch, and in \"rebase\"\nwe might want to reset (noff) to the tip.\n\nI'll think about this more.\nThanks,\nStefan\n"},{"id":"318471","messageId":"CAGZ79ka61UjNkQ7Avhp5hvq39JvVX2_b2m=Rf7dTOe+XpkGUxA@mail.gmail.com","threadId":"45842","inReplyTo":"xmqqy3ugdnfu.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/5] submodule_move_head: prepare env for child correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T04:18:55Z","receivedAt":"2017-05-02T04:19:01Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 7:04 PM, Junio C Hamano wrote:\n> Stefan Beller writes:\n>\n>> We forgot to prepare the submodule env, which is only a problem for\n>> nested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules\n>> with nested submodules, 2017-04-13) for further explanation.\n>>\n>> Signed-off-by: Stefan Beller \n>> ---\n>\n> Sounds good (if only because this makes it similar to other\n> codepaths).\n>\n> Is this something whose breakage before the patch is easily\n> demonstrated with a test?\n\nI'll try to come up with a test in a reroll.\n\nThanks,\nStefan\n"},{"id":"318473","messageId":"CAGZ79kYC9sycR_R-EM0jFA2YrMjA6jZvXqQud7ShPziW6WaMBA@mail.gmail.com","threadId":"45842","inReplyTo":"xmqqpofsdkim.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 1/5] submodule_move_head: fix leak of struct child_process","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T04:20:58Z","receivedAt":"2017-05-02T04:21:03Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 8:07 PM, Junio C Hamano wrote:\n> Brandon Williams writes:\n>\n>> On 05/01, Stefan Beller wrote:\n>>> While fixing the leak of `cp`, reuse it instead of having to declare\n>>> another struct child_process.\n>>>\n>>> Signed-off-by: Stefan Beller \n>>\n>> This shouldn't be needed as 'finish_command' does the cleanup for you by\n>> calling 'child_prcoess_clear()'.\n>\n> Yes, indeed.\n>\n> It might not hurt to update the code so that the cp is reused in the\n> second run instead of using a separate instance cp1, but that is a\n> topic separate from fixing a non-existing leak.\n\nok, I'll drop the patch and may send another patch later for re-using cp\n\nThanks,\nStefan\n"},{"id":"318559","messageId":"20170502193214.7121-1-sbeller@google.com","threadId":"45842","inReplyTo":"20170501180058.8063-1-sbeller@google.com","subject":"[PATCHv2 0/3] Some submodule bugfixes","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T19:32:11Z","receivedAt":"2017-05-02T19:32:29Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"v2:\n * I dropped the RFC patches for reattaching HEAD as that is not the main\n concern of this series.\n * patch1 is just about dropping cp1 to reusing cp, instead of additionally\n \"fixing\" mem leaks as finish_command does that for us\n * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery\n in new working tree manipulator code)\n * Regarding former patch2, I wrote:\n> Junio wrote:\n>> Sounds good (if only because this makes it similar to other\n>> codepaths).\n>>\n>> Is this something whose breakage before the patch is easily\n>> demonstrated with a test?\n\n> I'll try to come up with a test in a reroll.\n \ndone\n\nThanks,\nStefan\n\n\nStefan Beller (3):\n submodule_move_head: reuse child_process structure for futher commands\n submodule: avoid auto-discovery in new working tree manipulator code\n submodule: properly recurse for read-tree and checkout\n\n submodule.c | 21 +++++++++++----------\n t/lib-submodule-update.sh | 7 +------\n t/t1013-read-tree-submodule.sh | 1 -\n t/t2013-checkout-submodule.sh | 1 -\n 4 files changed, 12 insertions(+), 18 deletions(-)\n\n-- \n2.13.0.rc1.19.g083305f9b1\n\n"},{"id":"318560","messageId":"20170502193214.7121-2-sbeller@google.com","threadId":"45842","inReplyTo":"20170502193214.7121-1-sbeller@google.com","subject":"[PATCHv2 1/3] submodule_move_head: reuse child_process structure for futher commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T19:32:12Z","receivedAt":"2017-05-02T19:32:31Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We do not need to declare another struct child_process, but we can just\nreuse the existing `cp` struct.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 12 ++++++------\n 1 file changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex d3299e29c0..a2377ce019 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1468,15 +1468,15 @@ int submodule_move_head(const char *path,\n \n \tif (!(flags & SUBMODULE_MOVE_HEAD_DRY_RUN)) {\n \t\tif (new) {\n-\t\t\tstruct child_process cp1 = CHILD_PROCESS_INIT;\n+\t\t\tchild_process_init(&cp);\n \t\t\t/* also set the HEAD accordingly */\n-\t\t\tcp1.git_cmd = 1;\n-\t\t\tcp1.no_stdin = 1;\n-\t\t\tcp1.dir = path;\n+\t\t\tcp.git_cmd = 1;\n+\t\t\tcp.no_stdin = 1;\n+\t\t\tcp.dir = path;\n \n-\t\t\targv_array_pushl(&cp1.args, \"update-ref\", \"HEAD\", new, NULL);\n+\t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n \n-\t\t\tif (run_command(&cp1)) {\n+\t\t\tif (run_command(&cp)) {\n \t\t\t\tret = -1;\n \t\t\t\tgoto out;\n \t\t\t}\n-- \n2.13.0.rc1.19.g083305f9b1\n\n"},{"id":"318561","messageId":"20170502193214.7121-3-sbeller@google.com","threadId":"45842","inReplyTo":"20170502193214.7121-1-sbeller@google.com","subject":"[PATCHv2 2/3] submodule: avoid auto-discovery in new working tree manipulator code","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T19:32:13Z","receivedAt":"2017-05-02T19:32:34Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"All commands that are run in a submodule, are run in a correct setup,\nthere is no need to prepare the environment without setting the GIT_DIR\nvariable. By setting the GIT_DIR variable we fix issues as discussed in\n10f5c52656 (submodule: avoid auto-discovery in\nprepare_submodule_repo_env(), 2016-09-01)\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 6 +++---\n 1 file changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex a2377ce019..b0141a66dd 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1363,7 +1363,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \targv_array_pushl(&cp.args, \"diff-index\", \"--quiet\",\n@@ -1380,7 +1380,7 @@ static int submodule_has_dirty_index(const struct submodule *sub)\n static void submodule_reset_index(const char *path)\n {\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n@@ -1438,7 +1438,7 @@ int submodule_move_head(const char *path,\n \t\t}\n \t}\n \n-\tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n+\tprepare_submodule_repo_env(&cp.env_array);\n \n \tcp.git_cmd = 1;\n \tcp.no_stdin = 1;\n-- \n2.13.0.rc1.19.g083305f9b1\n\n"},{"id":"318562","messageId":"20170502193214.7121-4-sbeller@google.com","threadId":"45842","inReplyTo":"20170502193214.7121-1-sbeller@google.com","subject":"[PATCHv2 3/3] submodule: properly recurse for read-tree and checkout","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-02T19:32:14Z","receivedAt":"2017-05-02T19:32:38Z","isPatch":false,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"We forgot to prepare the submodule env, which is only a problem for\nnested submodules. See 2e5d6503bd (ls-files: fix recurse-submodules\nwith nested submodules, 2017-04-13) for further explanation.\n\nTo come up with a proper test for this, we'd need to look at nested\nsubmodules just as in that given commit. It turns out we're lucky\nand these tests already exist, but are marked as failing. We need\nto pass `--recurse-submodules` to read-tree additionally to make\nthese tests pass. Passing that flag alone would not make the tests\npass, such that this covers testing for the bug fix of the submodule\nenv as well.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 ++-\n t/lib-submodule-update.sh | 7 +------\n t/t1013-read-tree-submodule.sh | 1 -\n t/t2013-checkout-submodule.sh | 1 -\n 4 files changed, 3 insertions(+), 9 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex b0141a66dd..b3ae642f29 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1446,7 +1446,7 @@ int submodule_move_head(const char *path,\n \n \targv_array_pushf(&cp.args, \"--super-prefix=%s%s/\",\n \t\t\tget_super_prefix_or_empty(), path);\n-\targv_array_pushl(&cp.args, \"read-tree\", NULL);\n+\targv_array_pushl(&cp.args, \"read-tree\", \"--recurse-submodules\", NULL);\n \n \tif (flags & SUBMODULE_MOVE_HEAD_DRY_RUN)\n \t\targv_array_push(&cp.args, \"-n\");\n@@ -1474,6 +1474,7 @@ int submodule_move_head(const char *path,\n \t\t\tcp.no_stdin = 1;\n \t\t\tcp.dir = path;\n \n+\t\t\tprepare_submodule_repo_env(&cp.env_array);\n \t\t\targv_array_pushl(&cp.args, \"update-ref\", \"HEAD\", new, NULL);\n \n \t\t\tif (run_command(&cp)) {\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex fb4f7b014e..2c17826e95 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -787,11 +787,6 @@ test_submodule_switch_recursing () {\n \tthen\n \t\tRESULTDS=failure\n \tfi\n-\tRESULTR=success\n-\tif test \"$KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED\" = 1\n-\tthen\n-\t\tRESULTR=failure\n-\tfi\n \tRESULTOI=success\n \tif test \"$KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED\" = 1\n \tthen\n@@ -1003,7 +998,7 @@ test_submodule_switch_recursing () {\n \t'\n \n \t# recursing deeper than one level doesn't work yet.\n-\ttest_expect_$RESULTR \"$command: modified submodule updates submodule recursively\" '\n+\ttest_expect_success \"$command: modified submodule updates submodule recursively\" '\n \t\tprolog &&\n \t\treset_work_tree_to_interested add_nested_sub &&\n \t\t(\ndiff --git a/t/t1013-read-tree-submodule.sh b/t/t1013-read-tree-submodule.sh\nindex de1ba02dc5..7019d0a04f 100755\n--- a/t/t1013-read-tree-submodule.sh\n+++ b/t/t1013-read-tree-submodule.sh\n@@ -5,7 +5,6 @@ test_description='read-tree can handle submodules'\n . ./test-lib.sh\n . \"$TEST_DIRECTORY\"/lib-submodule-update.sh\n \n-KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1\n KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1\n \ndiff --git a/t/t2013-checkout-submodule.sh b/t/t2013-checkout-submodule.sh\nindex e8f70b806f..aa35223369 100755\n--- a/t/t2013-checkout-submodule.sh\n+++ b/t/t2013-checkout-submodule.sh\n@@ -64,7 +64,6 @@ test_expect_success '\"checkout \" honors submodule.*.ignore from .git/\n '\n \n KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1\n-KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1\n test_submodule_switch_recursing \"git checkout --recurse-submodules\"\n \n test_submodule_forced_switch_recursing \"git checkout -f --recurse-submodules\"\n-- \n2.13.0.rc1.19.g083305f9b1\n\n"},{"id":"318563","messageId":"20170502194235.GH181268@google.com","threadId":"45842","inReplyTo":"20170502193214.7121-1-sbeller@google.com","subject":"Re: [PATCHv2 0/3] Some submodule bugfixes","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-05-02T19:42:35Z","receivedAt":"2017-05-02T19:42:43Z","isPatch":false,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 05/02, Stefan Beller wrote:\n> v2:\n> * I dropped the RFC patches for reattaching HEAD as that is not the main\n> concern of this series.\n> * patch1 is just about dropping cp1 to reusing cp, instead of additionally\n> \"fixing\" mem leaks as finish_command does that for us\n> * reordered the patches, former patch 3 now as patch 2 (avoid auto-discovery\n> in new working tree manipulator code)\n> * Regarding former patch2, I wrote:\n> > Junio wrote:\n> >> Sounds good (if only because this makes it similar to other\n> >> codepaths).\n> >>\n> >> Is this something whose breakage before the patch is easily\n> >> demonstrated with a test?\n> \n> > I'll try to come up with a test in a reroll.\n> \n> done\n> \n> Thanks,\n> Stefan\n\nChanges look good.\n\n> \n> \n> Stefan Beller (3):\n> submodule_move_head: reuse child_process structure for futher commands\n> submodule: avoid auto-discovery in new working tree manipulator code\n> submodule: properly recurse for read-tree and checkout\n> \n> submodule.c | 21 +++++++++++----------\n> t/lib-submodule-update.sh | 7 +------\n> t/t1013-read-tree-submodule.sh | 1 -\n> t/t2013-checkout-submodule.sh | 1 -\n> 4 files changed, 12 insertions(+), 18 deletions(-)\n> \n> -- \n> 2.13.0.rc1.19.g083305f9b1\n> \n\n-- \nBrandon Williams\n"},{"id":"321207","messageId":"CAGZ79kYEGQit+7Dqs7ggzRMq9y=pHFhpn-Spk3zVw2ffsc2nXw@mail.gmail.com","threadId":"45842","inReplyTo":"CAGZ79kb52QDUG0RtTXNEEpMJR1CSMYMrRHTRRvGn0-cF=HnzWw@mail.gmail.com","subject":"Re: [PATCH 0/5] Some submodule bugfixes and \"reattaching detached HEADs\"","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-05-31T22:09:12Z","receivedAt":"2017-05-31T22:09:17Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Mon, May 1, 2017 at 9:04 PM, Stefan Beller wrote:\n>>\n>>> I don't know why submodules were originally designed to be in a\n>>> detached HEAD state but I much prefer working on branches (as I'm sure\n>>> many other developers do) so the prospect of this becoming the norm is\n>>> exciting! :D\n>>\n>\n> I'll think about this more.\n\nWhat the current model is missing is the possibility to have\na symbolic link not just to a ref within a repository, but to the outside\nof a repository (such as the superproject in this case).\n\nSo we could have a HEAD with a content like:\n\n \"super: [LF ]\"\n\nThen we would use the HEAD to determine if the superproject\nwould touch a submodule at all. Example workflow:\n\n git -C checkout --reattach-to-superproject\n\n # hack away in the submodule\n\n # This will make a commit in and add the\n # resulting object to the index of the superproject\n # because HEAD is tracking the superproject.\n # so in order to have HEAD containing the new\n # commit we have to change the superproject:\n git -C commit -a -m \"message\"\n\n # This has also interesting consequences for\n # submodule related commands:\n git checkout --recurse-submodules \n # Any submodule whose HEAD is attached to the\n # superproject would be touched, the others would\n # not.\n\nBy being directly attached to the superproject, it would be\neasy to find all submodules that are changed, via a\n\n git -C status # no need to recurse, even!\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\n\nThe whole \"checkout --recurse-submodules\" series is based on\nassumptions of the current mental model of how branches and\ndetached HEADs work.\n\n\nA submodule would have a symref\n"}]}