{"thread":{"id":"45671","subject":"[PATCH 0/4] recursive submodules: git-reset!","startedAt":"2017-04-11T23:49:53Z","lastAt":"2017-04-14T20:13:34Z","messageCount":15,"participants":["Stefan Beller","Philip Oakley","Brandon Williams","Jonathan Tan"],"isPatch":true,"patchVersion":1,"patchTotal":4},"messages":[{"id":"316655","messageId":"20170411234923.1860-1-sbeller@google.com","threadId":"45671","inReplyTo":null,"subject":"[PATCH 0/4] recursive submodules: git-reset!","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T23:49:19Z","receivedAt":"2017-04-11T23:49:53Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Now that the BIG one has landed, e394fa01d6 (Merge branch\n'sb/checkout-recurse-submodules', 2017-03-28), you would expect that\nteaching to recurse into submodules is easy for all the remaining \nworking tree manipulations?\n\nIt turns out it is. See the last patch how we teach git-reset to recurse\ninto submodules.\n\nHowever when thinking more about what git-reset is expected to do,\nI added tests and some fixes for them (patch 2+3).\n\npatch 1 is a correctness thing, required for patch 3.\n\nThanks,\nStefan\n\nStefan Beller (4):\n entry.c: submodule recursing: respect force flag correctly\n submodule.c: uninitialized submodules are ignored in recursive\n commands\n submodule.c: harden submodule_move_head against broken submodules\n builtin/reset: add --recurse-submodules switch\n\n builtin/reset.c | 30 ++++++++++++++++++++++++++++++\n entry.c | 8 ++++----\n submodule.c | 20 ++++++++++++++++++++\n t/lib-submodule-update.sh | 24 +++++++++++++++++++++---\n t/t7112-reset-submodule.sh | 8 ++++++++\n unpack-trees.c | 7 ++++++-\n 6 files changed, 89 insertions(+), 8 deletions(-)\n\n-- \n2.12.2.603.g7b28dc31ba\n\n"},{"id":"316656","messageId":"20170411234923.1860-2-sbeller@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-1-sbeller@google.com","subject":"[PATCH 1/4] entry.c: submodule recursing: respect force flag correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T23:49:20Z","receivedAt":"2017-04-11T23:49:55Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"In case of a non-forced worktree update, the submodule movement is tested\nin a dry run first, such that it doesn't matter if the actual update is\ndone via the force flag. However for correctness, we want to give the\nflag is specified by the user. All callers have been inspected and updated\nif needed.\n\nSigned-off-by: Stefan Beller \n---\n entry.c | 8 ++++----\n unpack-trees.c | 7 ++++++-\n 2 files changed, 10 insertions(+), 5 deletions(-)\n\ndiff --git a/entry.c b/entry.c\nindex d2b512da90..d6b263f78e 100644\n--- a/entry.c\n+++ b/entry.c\n@@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce,\n \t\tsub = submodule_from_ce(ce);\n \t\tif (sub)\n \t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\tNULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);\n+\t\t\t\tNULL, oid_to_hex(&ce->oid),\n+\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\tbreak;\n \tdefault:\n \t\treturn error(\"unknown file mode for %s in index\", path);\n@@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,\n \t\t\t\t\tunlink_or_warn(ce->name);\n \n \t\t\t\treturn submodule_move_head(ce->name,\n-\t\t\t\t\tNULL, oid_to_hex(&ce->oid),\n-\t\t\t\t\tSUBMODULE_MOVE_HEAD_FORCE);\n+\t\t\t\t\tNULL, oid_to_hex(&ce->oid), 0);\n \t\t\t} else\n \t\t\t\treturn submodule_move_head(ce->name,\n \t\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid),\n-\t\t\t\t\tSUBMODULE_MOVE_HEAD_FORCE);\n+\t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n \t\t}\n \n \t\tif (!changed)\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 8333da2cc9..7316ca99c2 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n \t\t\t\t const char *new_id,\n \t\t\t\t struct unpack_trees_options *o)\n {\n+\tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n \tconst struct submodule *sub = submodule_from_ce(ce);\n \tif (!sub)\n \t\treturn 0;\n \n+\tif (o->reset)\n+\t\tflags |= SUBMODULE_MOVE_HEAD_FORCE;\n+\n \tswitch (sub->update_strategy.type) {\n \tcase SM_UPDATE_UNSPECIFIED:\n \tcase SM_UPDATE_CHECKOUT:\n-\t\tif (submodule_move_head(ce->name, old_id, new_id, SUBMODULE_MOVE_HEAD_DRY_RUN))\n+\t\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n \t\t\treturn o->gently ? -1 :\n \t\t\t\tadd_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n \t\treturn 0;\n@@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)\n \t\tcase SM_UPDATE_CHECKOUT:\n \t\tcase SM_UPDATE_REBASE:\n \t\tcase SM_UPDATE_MERGE:\n+\t\t\t/* state.force is set at the caller. */\n \t\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n \t\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n \t\t\tbreak;\n-- \n2.12.2.603.g7b28dc31ba\n\n"},{"id":"316657","messageId":"20170411234923.1860-3-sbeller@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-1-sbeller@google.com","subject":"[PATCH 2/4] submodule.c: uninitialized submodules are ignored in recursive commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T23:49:21Z","receivedAt":"2017-04-11T23:49:56Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"This was an oversight when working on the working tree modifying commands\nrecursing into submodules.\n\nTo test for uninitialized submodules, introduce another submodule, that is\nuninitialized in the actual tests. By adding it to the branch \"add_sub1\",\nwhich is the starting point of all other branches, we have wide coverage.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 3 +++\n t/lib-submodule-update.sh | 1 +\n 2 files changed, 4 insertions(+)\n\ndiff --git a/submodule.c b/submodule.c\nindex c52d6634c5..2fa42519a4 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1332,6 +1332,9 @@ int submodule_move_head(const char *path,\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tconst struct submodule *sub;\n \n+\tif (!is_submodule_initialized(path))\n+\t\treturn 0;\n+\n \tsub = submodule_from_path(null_sha1, path);\n \n \tif (!sub)\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex fb4f7b014e..22dd9e060c 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -73,6 +73,7 @@ create_lib_submodule_repo () {\n \n \t\tgit checkout -b \"add_sub1\" &&\n \t\tgit submodule add ../submodule_update_sub1 sub1 &&\n+\t\tgit submodule add ../submodule_update_sub1 uninitialized_sub &&\n \t\tgit config -f .gitmodules submodule.sub1.ignore all &&\n \t\tgit config submodule.sub1.ignore all &&\n \t\tgit add .gitmodules &&\n-- \n2.12.2.603.g7b28dc31ba\n\n"},{"id":"316658","messageId":"20170411234923.1860-5-sbeller@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-1-sbeller@google.com","subject":"[PATCH 4/4] builtin/reset: add --recurse-submodules switch","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T23:49:23Z","receivedAt":"2017-04-11T23:50:00Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"git-reset is yet another working tree manipulator, which should\nbe taught about submodules.\n\nOne use case of \"git-reset\" is to reset to a known good state,\nand dropping commits that did not work as expected.\nIn that case one of the expected outcomes from a hard reset\nwould be to have broken submodules reset to a known good\nstate as well. A test for this was added in a prior patch.\n\nSigned-off-by: Stefan Beller \n---\n builtin/reset.c | 30 ++++++++++++++++++++++++++++++\n t/t7112-reset-submodule.sh | 8 ++++++++\n 2 files changed, 38 insertions(+)\n\ndiff --git a/builtin/reset.c b/builtin/reset.c\nindex fc3b906c47..5ce27fcaed 100644\n--- a/builtin/reset.c\n+++ b/builtin/reset.c\n@@ -21,6 +21,27 @@\n #include \"parse-options.h\"\n #include \"unpack-trees.h\"\n #include \"cache-tree.h\"\n+#include \"submodule.h\"\n+#include \"submodule-config.h\"\n+\n+static int recurse_submodules = RECURSE_SUBMODULES_DEFAULT;\n+\n+static int option_parse_recurse_submodules(const struct option *opt,\n+\t\t\t\t\t const char *arg, int unset)\n+{\n+\tif (unset) {\n+\t\trecurse_submodules = RECURSE_SUBMODULES_OFF;\n+\t\treturn 0;\n+\t}\n+\tif (arg)\n+\t\trecurse_submodules =\n+\t\t\tparse_update_recurse_submodules_arg(opt->long_name,\n+\t\t\t\t\t\t\t arg);\n+\telse\n+\t\trecurse_submodules = RECURSE_SUBMODULES_ON;\n+\n+\treturn 0;\n+}\n \n static const char * const git_reset_usage[] = {\n \tN_(\"git reset [--mixed | --soft | --hard | --merge | --keep] [-q] []\"),\n@@ -283,6 +304,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\tN_(\"reset HEAD, index and working tree\"), MERGE),\n \t\tOPT_SET_INT(0, \"keep\", &reset_type,\n \t\t\t\tN_(\"reset HEAD but keep local changes\"), KEEP),\n+\t\t{ OPTION_CALLBACK, 0, \"recurse-submodules\", &recurse_submodules,\n+\t\t\t \"reset\", \"control recursive updating of submodules\",\n+\t\t\t PARSE_OPT_OPTARG, option_parse_recurse_submodules },\n \t\tOPT_BOOL('p', \"patch\", &patch_mode, N_(\"select hunks interactively\")),\n \t\tOPT_BOOL('N', \"intent-to-add\", &intent_to_add,\n \t\t\t\tN_(\"record only the fact that removed paths will be added later\")),\n@@ -295,6 +319,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix)\n \t\t\t\t\t\tPARSE_OPT_KEEP_DASHDASH);\n \tparse_args(&pathspec, argv, prefix, patch_mode, &rev);\n \n+\tif (recurse_submodules != RECURSE_SUBMODULES_DEFAULT) {\n+\t\tgitmodules_config();\n+\t\tgit_config(submodule_config, NULL);\n+\t\tset_config_update_recurse_submodules(RECURSE_SUBMODULES_ON);\n+\t}\n+\n \tunborn = !strcmp(rev, \"HEAD\") && get_sha1(\"HEAD\", oid.hash);\n \tif (unborn) {\n \t\t/* reset on unborn branch: treat as reset to empty tree */\ndiff --git a/t/t7112-reset-submodule.sh b/t/t7112-reset-submodule.sh\nindex 2eda6adeb1..f86ccdf215 100755\n--- a/t/t7112-reset-submodule.sh\n+++ b/t/t7112-reset-submodule.sh\n@@ -5,6 +5,14 @@ test_description='reset 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+\n+test_submodule_switch_recursing \"git reset --recurse-submodules --keep\"\n+\n+test_submodule_forced_switch_recursing \"git reset --hard --recurse-submodules\"\n+\n test_submodule_switch \"git reset --keep\"\n \n test_submodule_switch \"git reset --merge\"\n-- \n2.12.2.603.g7b28dc31ba\n\n"},{"id":"316659","messageId":"20170411234923.1860-4-sbeller@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-1-sbeller@google.com","subject":"[PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-11T23:49:22Z","receivedAt":"2017-04-11T23:50:02Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"Early on in submodule_move_head just after the check if the submodule is\ninitialized, we need to check if the submodule is populated correctly.\n\nIf the submodule is initialized but doesn't look like populated, this\nis a red flag and can indicate multiple sorts of failures:\n(1) The submodule may be recorded at an object name, that is missing.\n(2) The submodule '.git' file link may be broken and it is not pointing\n at a repository.\n\nIn both cases we want to complain to the user in the non-forced mode,\nand in the forced mode ignoring the old state and just moving the\nsubmodule into its new state with a fixed '.git' file link.\n\nSigned-off-by: Stefan Beller \n---\n submodule.c | 17 +++++++++++++++++\n t/lib-submodule-update.sh | 23 ++++++++++++++++++++---\n 2 files changed, 37 insertions(+), 3 deletions(-)\n\ndiff --git a/submodule.c b/submodule.c\nindex 2fa42519a4..dda1ead854 100644\n--- a/submodule.c\n+++ b/submodule.c\n@@ -1331,10 +1331,19 @@ int submodule_move_head(const char *path,\n \tint ret = 0;\n \tstruct child_process cp = CHILD_PROCESS_INIT;\n \tconst struct submodule *sub;\n+\tint *errorcode, error_code;\n \n \tif (!is_submodule_initialized(path))\n \t\treturn 0;\n \n+\tif (flags & SUBMODULE_MOVE_HEAD_FORCE)\n+\t\terrorcode = &error_code;\n+\telse\n+\t\terrorcode = NULL;\n+\n+\tif (old && !is_submodule_populated_gently(path, errorcode))\n+\t\treturn 0;\n+\n \tsub = submodule_from_path(null_sha1, path);\n \n \tif (!sub)\n@@ -1361,6 +1370,14 @@ int submodule_move_head(const char *path,\n \t\t\t/* make sure the index is clean as well */\n \t\t\tsubmodule_reset_index(path);\n \t\t}\n+\n+\t\tif (old && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n+\t\t\tstruct strbuf sb = STRBUF_INIT;\n+\t\t\tstrbuf_addf(&sb, \"%s/modules/%s\",\n+\t\t\t\t get_git_common_dir(), sub->name);\n+\t\t\tconnect_work_tree_and_git_dir(path, sb.buf);\n+\t\t\tstrbuf_release(&sb);\n+\t\t}\n \t}\n \n \tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\ndiff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\nindex 22dd9e060c..f0b1b18206 100755\n--- a/t/lib-submodule-update.sh\n+++ b/t/lib-submodule-update.sh\n@@ -1213,14 +1213,31 @@ test_submodule_forced_switch_recursing () {\n \t\t)\n \t'\n \t# Updating a submodule from an invalid sha1 updates\n-\ttest_expect_success \"$command: modified submodule does not update submodule work tree from invalid commit\" '\n+\ttest_expect_success \"$command: modified submodule does update submodule work tree from invalid commit\" '\n \t\tprolog &&\n \t\treset_work_tree_to_interested invalid_sub1 &&\n \t\t(\n \t\t\tcd submodule_update &&\n \t\t\tgit branch -t valid_sub1 origin/valid_sub1 &&\n-\t\t\ttest_must_fail $command valid_sub1 &&\n-\t\t\ttest_superproject_content origin/invalid_sub1\n+\t\t\t$command valid_sub1 &&\n+\t\t\ttest_superproject_content origin/valid_sub1 &&\n+\t\t\ttest_submodule_content sub1 origin/valid_sub1\n+\t\t)\n+\t'\n+\n+\t# Old versions of Git were buggy writing the .git link file\n+\t# (e.g. before f8eaa0ba98b and then moving the superproject repo\n+\t# whose submodules contained absolute paths)\n+\ttest_expect_success \"$command: updating submodules fixes .git links\" '\n+\t\tprolog &&\n+\t\treset_work_tree_to_interested add_sub1 &&\n+\t\t(\n+\t\t\tcd submodule_update &&\n+\t\t\tgit branch -t modify_sub1 origin/modify_sub1 &&\n+\t\t\techo \"gitdir: bogus/path\" >sub1/.git &&\n+\t\t\t$command modify_sub1 &&\n+\t\t\ttest_superproject_content origin/modify_sub1 &&\n+\t\t\ttest_submodule_content sub1 origin/modify_sub1\n \t\t)\n \t'\n }\n-- \n2.12.2.603.g7b28dc31ba\n\n"},{"id":"316701","messageId":"64236D980AD8478AA295BD7C263479FF@PhilipOakley","threadId":"45671","inReplyTo":"20170411234923.1860-2-sbeller@google.com","subject":"Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-04-12T11:28:51Z","receivedAt":"2017-04-12T21:25:21Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> In case of a non-forced worktree update, the submodule movement is tested\n> in a dry run first, such that it doesn't matter if the actual update is\n> done via the force flag. However for correctness, we want to give the\n> flag is specified by the user. All callers have been inspected and updated\ns/flag is/flag as/\n\n> if needed.\n>\n> Signed-off-by: Stefan Beller \n> ---\n--\nPhilip\n\n\n> entry.c | 8 ++++----\n> unpack-trees.c | 7 ++++++-\n> 2 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index d2b512da90..d6b263f78e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce,\n> sub = submodule_from_ce(ce);\n> if (sub)\n> return submodule_move_head(ce->name,\n> - NULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);\n> + NULL, oid_to_hex(&ce->oid),\n> + state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n> break;\n> default:\n> return error(\"unknown file mode for %s in index\", path);\n> @@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,\n> unlink_or_warn(ce->name);\n>\n> return submodule_move_head(ce->name,\n> - NULL, oid_to_hex(&ce->oid),\n> - SUBMODULE_MOVE_HEAD_FORCE);\n> + NULL, oid_to_hex(&ce->oid), 0);\n> } else\n> return submodule_move_head(ce->name,\n> \"HEAD\", oid_to_hex(&ce->oid),\n> - SUBMODULE_MOVE_HEAD_FORCE);\n> + state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n> }\n>\n> if (!changed)\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8333da2cc9..7316ca99c2 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct \n> cache_entry *ce,\n> const char *new_id,\n> struct unpack_trees_options *o)\n> {\n> + unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n> const struct submodule *sub = submodule_from_ce(ce);\n> if (!sub)\n> return 0;\n>\n> + if (o->reset)\n> + flags |= SUBMODULE_MOVE_HEAD_FORCE;\n> +\n> switch (sub->update_strategy.type) {\n> case SM_UPDATE_UNSPECIFIED:\n> case SM_UPDATE_CHECKOUT:\n> - if (submodule_move_head(ce->name, old_id, new_id, \n> SUBMODULE_MOVE_HEAD_DRY_RUN))\n> + if (submodule_move_head(ce->name, old_id, new_id, flags))\n> return o->gently ? -1 :\n> add_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n> return 0;\n> @@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)\n> case SM_UPDATE_CHECKOUT:\n> case SM_UPDATE_REBASE:\n> case SM_UPDATE_MERGE:\n> + /* state.force is set at the caller. */\n> submodule_move_head(ce->name, \"HEAD\", NULL,\n> SUBMODULE_MOVE_HEAD_FORCE);\n> break;\n> -- \n> 2.12.2.603.g7b28dc31ba\n>\n> \n\n"},{"id":"316702","messageId":"18BFDBF530F144FBA7EAE731625E3C91@PhilipOakley","threadId":"45671","inReplyTo":"20170411234923.1860-4-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules","fromName":"Philip Oakley","fromEmail":"philipoakley@iee.org","sentAt":"2017-04-12T11:32:02Z","receivedAt":"2017-04-12T21:25:22Z","isPatch":true,"sender":{"key":"philipoakley@iee.email","avatar":"https://avatars.githubusercontent.com/u/914343?v=4"},"body":"From: \"Stefan Beller\" \n> Early on in submodule_move_head just after the check if the submodule is\n> initialized, we need to check if the submodule is populated correctly.\n>\n> If the submodule is initialized but doesn't look like populated, this\ns/like// or s/like/like it is/\n\n> is a red flag and can indicate multiple sorts of failures:\n> (1) The submodule may be recorded at an object name, that is missing.\n> (2) The submodule '.git' file link may be broken and it is not pointing\n> at a repository.\n>\n> In both cases we want to complain to the user in the non-forced mode,\n> and in the forced mode ignoring the old state and just moving the\n> submodule into its new state with a fixed '.git' file link.\n>\n> Signed-off-by: Stefan Beller \n> ---\n--\nPhilip\n\n> submodule.c | 17 +++++++++++++++++\n> t/lib-submodule-update.sh | 23 ++++++++++++++++++++---\n> 2 files changed, 37 insertions(+), 3 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 2fa42519a4..dda1ead854 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1331,10 +1331,19 @@ int submodule_move_head(const char *path,\n> int ret = 0;\n> struct child_process cp = CHILD_PROCESS_INIT;\n> const struct submodule *sub;\n> + int *errorcode, error_code;\n>\n> if (!is_submodule_initialized(path))\n> return 0;\n>\n> + if (flags & SUBMODULE_MOVE_HEAD_FORCE)\n> + errorcode = &error_code;\n> + else\n> + errorcode = NULL;\n> +\n> + if (old && !is_submodule_populated_gently(path, errorcode))\n> + return 0;\n> +\n> sub = submodule_from_path(null_sha1, path);\n>\n> if (!sub)\n> @@ -1361,6 +1370,14 @@ int submodule_move_head(const char *path,\n> /* make sure the index is clean as well */\n> submodule_reset_index(path);\n> }\n> +\n> + if (old && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n> + struct strbuf sb = STRBUF_INIT;\n> + strbuf_addf(&sb, \"%s/modules/%s\",\n> + get_git_common_dir(), sub->name);\n> + connect_work_tree_and_git_dir(path, sb.buf);\n> + strbuf_release(&sb);\n> + }\n> }\n>\n> prepare_submodule_repo_env_no_git_dir(&cp.env_array);\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 22dd9e060c..f0b1b18206 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -1213,14 +1213,31 @@ test_submodule_forced_switch_recursing () {\n> )\n> '\n> # Updating a submodule from an invalid sha1 updates\n> - test_expect_success \"$command: modified submodule does not update \n> submodule work tree from invalid commit\" '\n> + test_expect_success \"$command: modified submodule does update submodule \n> work tree from invalid commit\" '\n> prolog &&\n> reset_work_tree_to_interested invalid_sub1 &&\n> (\n> cd submodule_update &&\n> git branch -t valid_sub1 origin/valid_sub1 &&\n> - test_must_fail $command valid_sub1 &&\n> - test_superproject_content origin/invalid_sub1\n> + $command valid_sub1 &&\n> + test_superproject_content origin/valid_sub1 &&\n> + test_submodule_content sub1 origin/valid_sub1\n> + )\n> + '\n> +\n> + # Old versions of Git were buggy writing the .git link file\n> + # (e.g. before f8eaa0ba98b and then moving the superproject repo\n> + # whose submodules contained absolute paths)\n> + test_expect_success \"$command: updating submodules fixes .git links\" '\n> + prolog &&\n> + reset_work_tree_to_interested add_sub1 &&\n> + (\n> + cd submodule_update &&\n> + git branch -t modify_sub1 origin/modify_sub1 &&\n> + echo \"gitdir: bogus/path\" >sub1/.git &&\n> + $command modify_sub1 &&\n> + test_superproject_content origin/modify_sub1 &&\n> + test_submodule_content sub1 origin/modify_sub1\n> )\n> '\n> }\n> -- \n> 2.12.2.603.g7b28dc31ba\n>\n> \n\n"},{"id":"316764","messageId":"20170413190541.GD96917@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-3-sbeller@google.com","subject":"Re: [PATCH 2/4] submodule.c: uninitialized submodules are ignored in recursive commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T19:05:41Z","receivedAt":"2017-04-13T19:05:48Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Stefan Beller wrote:\n> This was an oversight when working on the working tree modifying commands\n> recursing into submodules.\n> \n> To test for uninitialized submodules, introduce another submodule, that is\n> uninitialized in the actual tests. By adding it to the branch \"add_sub1\",\n> which is the starting point of all other branches, we have wide coverage.\n> \n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 3 +++\n> t/lib-submodule-update.sh | 1 +\n> 2 files changed, 4 insertions(+)\n> \n> diff --git a/submodule.c b/submodule.c\n> index c52d6634c5..2fa42519a4 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1332,6 +1332,9 @@ int submodule_move_head(const char *path,\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tconst struct submodule *sub;\n> \n> +\tif (!is_submodule_initialized(path))\n> +\t\treturn 0;\n> +\n> \tsub = submodule_from_path(null_sha1, path);\n> \n> \tif (!sub)\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index fb4f7b014e..22dd9e060c 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -73,6 +73,7 @@ create_lib_submodule_repo () {\n> \n> \t\tgit checkout -b \"add_sub1\" &&\n> \t\tgit submodule add ../submodule_update_sub1 sub1 &&\n> +\t\tgit submodule add ../submodule_update_sub1 uninitialized_sub &&\n\nThe 'submodule add' command will make the submodule active, so you'll\nneed to add in a line to subsequently make the submodule inactive for\nthis to work, unless you do in at a later point in time.\n\n> \t\tgit config -f .gitmodules submodule.sub1.ignore all &&\n> \t\tgit config submodule.sub1.ignore all &&\n> \t\tgit add .gitmodules &&\n> -- \n> 2.12.2.603.g7b28dc31ba\n> \n\n-- \nBrandon Williams\n"},{"id":"316765","messageId":"20170413190839.GE96917@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-4-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T19:08:39Z","receivedAt":"2017-04-13T19:08:48Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/11, Stefan Beller wrote:\n> Early on in submodule_move_head just after the check if the submodule is\n> initialized, we need to check if the submodule is populated correctly.\n> \n> If the submodule is initialized but doesn't look like populated, this\n> is a red flag and can indicate multiple sorts of failures:\n> (1) The submodule may be recorded at an object name, that is missing.\n> (2) The submodule '.git' file link may be broken and it is not pointing\n> at a repository.\n> \n> In both cases we want to complain to the user in the non-forced mode,\n> and in the forced mode ignoring the old state and just moving the\n> submodule into its new state with a fixed '.git' file link.\n\nWhat about the case where you have marked a submodule as active but\ndon't have its respective .gitdir yet? For now i think it would be\nacceptable to complain and do nothing/ignore it, in the future we may\nwant to actually clone and then check it out.\n\n-- \nBrandon Williams\n"},{"id":"316766","messageId":"CAGZ79kZybFNPPAuw4GmVUSmQVBcCG402uCur8M9hSoY3CbhbXw@mail.gmail.com","threadId":"45671","inReplyTo":"20170413190541.GD96917@google.com","subject":"Re: [PATCH 2/4] submodule.c: uninitialized submodules are ignored in recursive commands","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T19:12:08Z","receivedAt":"2017-04-13T19:12:14Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 12:05 PM, Brandon Williams wrote:\n> On 04/11, Stefan Beller wrote:\n>> This was an oversight when working on the working tree modifying commands\n>> recursing into submodules.\n>>\n>> To test for uninitialized submodules, introduce another submodule, that is\n>> uninitialized in the actual tests. By adding it to the branch \"add_sub1\",\n>> which is the starting point of all other branches, we have wide coverage.\n>>\n\n...\n\n>\n> The 'submodule add' command will make the submodule active, so you'll\n> need to add in a line to subsequently make the submodule inactive for\n> this to work, unless you do in at a later point in time.\n\nYes, it will make it active, but that doesn't matter here, because at this\npoint (in create_lib_submodule_repo) we prepare an upstream\nin submodule_update_repo\n\nAny later test follows the structure of\n\n prolog &&\n reset_work_tree_to no_submodule &&\n (\n cd submodule_update &&\n # do actual test here, in submodule_update\n )\n\nNote that 'prolog' performs a clone of submodule_update_repo\nto submodule_update, manually setting 'sub1' to active.\n\n'uninitialized_sub' is not active.\n\nI tried to explain it via\n To test for uninitialized submodules, introduce another submodule,\n that is uninitialized in the actual tests.\nin the commit message, but that is too concise apparently.\nSo the resend will explain that a bit more.\n\nThanks,\nStefan\n"},{"id":"316769","messageId":"20170413191443.GF96917@google.com","threadId":"45671","inReplyTo":"CAGZ79kZybFNPPAuw4GmVUSmQVBcCG402uCur8M9hSoY3CbhbXw@mail.gmail.com","subject":"Re: [PATCH 2/4] submodule.c: uninitialized submodules are ignored in recursive commands","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-04-13T19:14:43Z","receivedAt":"2017-04-13T19:14:50Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 04/13, Stefan Beller wrote:\n> On Thu, Apr 13, 2017 at 12:05 PM, Brandon Williams wrote:\n> > On 04/11, Stefan Beller wrote:\n> >> This was an oversight when working on the working tree modifying commands\n> >> recursing into submodules.\n> >>\n> >> To test for uninitialized submodules, introduce another submodule, that is\n> >> uninitialized in the actual tests. By adding it to the branch \"add_sub1\",\n> >> which is the starting point of all other branches, we have wide coverage.\n> >>\n> \n> ...\n> \n> >\n> > The 'submodule add' command will make the submodule active, so you'll\n> > need to add in a line to subsequently make the submodule inactive for\n> > this to work, unless you do in at a later point in time.\n> \n> Yes, it will make it active, but that doesn't matter here, because at this\n> point (in create_lib_submodule_repo) we prepare an upstream\n> in submodule_update_repo\n> \n> Any later test follows the structure of\n> \n> prolog &&\n> reset_work_tree_to no_submodule &&\n> (\n> cd submodule_update &&\n> # do actual test here, in submodule_update\n> )\n> \n> Note that 'prolog' performs a clone of submodule_update_repo\n> to submodule_update, manually setting 'sub1' to active.\n> \n> 'uninitialized_sub' is not active.\n> \n> I tried to explain it via\n> To test for uninitialized submodules, introduce another submodule,\n> that is uninitialized in the actual tests.\n> in the commit message, but that is too concise apparently.\n> So the resend will explain that a bit more.\n\nThanks! I just wanted to be sure as you're more familiar with these\ntests.\n\n-- \nBrandon Williams\n"},{"id":"316770","messageId":"CAGZ79kZktYHFnPfHQvW_7H2Ec=ftgWwqRgLHBZxQOMmy5R8y_g@mail.gmail.com","threadId":"45671","inReplyTo":"20170413190839.GE96917@google.com","subject":"Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-13T19:17:28Z","receivedAt":"2017-04-13T19:17:33Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Thu, Apr 13, 2017 at 12:08 PM, Brandon Williams wrote:\n> On 04/11, Stefan Beller wrote:\n>> Early on in submodule_move_head just after the check if the submodule is\n>> initialized, we need to check if the submodule is populated correctly.\n>>\n>> If the submodule is initialized but doesn't look like populated, this\n>> is a red flag and can indicate multiple sorts of failures:\n>> (1) The submodule may be recorded at an object name, that is missing.\n>> (2) The submodule '.git' file link may be broken and it is not pointing\n>> at a repository.\n>>\n>> In both cases we want to complain to the user in the non-forced mode,\n>> and in the forced mode ignoring the old state and just moving the\n>> submodule into its new state with a fixed '.git' file link.\n>\n> What about the case where you have marked a submodule as active but\n> don't have its respective .gitdir yet? For now i think it would be\n> acceptable to complain and do nothing/ignore it, in the future we may\n> want to actually clone and then check it out.\n\nI agree. With this patch we'd complain in non-forced mode, and in\nforced mode we'd also complain as we lack the object.\n\nIn both cases in the future we may want to fetch the contents instead.\n"},{"id":"316834","messageId":"15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-2-sbeller@google.com","subject":"Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-14T18:28:56Z","receivedAt":"2017-04-14T18:29:11Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"On 04/11/2017 04:49 PM, Stefan Beller wrote:\n> In case of a non-forced worktree update, the submodule movement is tested\n> in a dry run first, such that it doesn't matter if the actual update is\n> done via the force flag. However for correctness, we want to give the\n> flag is specified by the user. All callers have been inspected and updated\n> if needed.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> entry.c | 8 ++++----\n> unpack-trees.c | 7 ++++++-\n> 2 files changed, 10 insertions(+), 5 deletions(-)\n>\n> diff --git a/entry.c b/entry.c\n> index d2b512da90..d6b263f78e 100644\n> --- a/entry.c\n> +++ b/entry.c\n> @@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce,\n> \t\tsub = submodule_from_ce(ce);\n> \t\tif (sub)\n> \t\t\treturn submodule_move_head(ce->name,\n> -\t\t\t\tNULL, oid_to_hex(&ce->oid), SUBMODULE_MOVE_HEAD_FORCE);\n> +\t\t\t\tNULL, oid_to_hex(&ce->oid),\n> +\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n> \t\tbreak;\n> \tdefault:\n> \t\treturn error(\"unknown file mode for %s in index\", path);\n> @@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,\n> \t\t\t\t\tunlink_or_warn(ce->name);\n>\n> \t\t\t\treturn submodule_move_head(ce->name,\n> -\t\t\t\t\tNULL, oid_to_hex(&ce->oid),\n> -\t\t\t\t\tSUBMODULE_MOVE_HEAD_FORCE);\n> +\t\t\t\t\tNULL, oid_to_hex(&ce->oid), 0);\n\nShould we be consistent (with the \"else\" block below and with the \nexisting code) to use \"state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0\" \ninstead of 0? (I glanced briefly through the code and \nSUBMODULE_MOVE_HEAD_FORCE might have no effect anyway if \"old\" is NULL, \nbut it's probably still better to be consistent.)\n\n> \t\t\t} else\n> \t\t\t\treturn submodule_move_head(ce->name,\n> \t\t\t\t\t\"HEAD\", oid_to_hex(&ce->oid),\n> -\t\t\t\t\tSUBMODULE_MOVE_HEAD_FORCE);\n> +\t\t\t\t\tstate->force ? SUBMODULE_MOVE_HEAD_FORCE : 0);\n> \t\t}\n>\n> \t\tif (!changed)\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 8333da2cc9..7316ca99c2 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct cache_entry *ce,\n> \t\t\t\t const char *new_id,\n> \t\t\t\t struct unpack_trees_options *o)\n> {\n> +\tunsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;\n> \tconst struct submodule *sub = submodule_from_ce(ce);\n> \tif (!sub)\n> \t\treturn 0;\n>\n> +\tif (o->reset)\n> +\t\tflags |= SUBMODULE_MOVE_HEAD_FORCE;\n\nIt seems to me that this is independent of the entry.c change, and might \nbe better in its own patch. (Or if it is not, maybe the subject should \nbe \"entry, unpack-trees: propagate force when submodule recursing\" or \nsomething like that, containing the names of both modified components.)\n\nAlso, you mentioned in the parent message that this patch is required \nfor patch 3. Is only the entry.c part required, or unpack-trees.c, or both?\n\n> +\n> \tswitch (sub->update_strategy.type) {\n> \tcase SM_UPDATE_UNSPECIFIED:\n> \tcase SM_UPDATE_CHECKOUT:\n> -\t\tif (submodule_move_head(ce->name, old_id, new_id, SUBMODULE_MOVE_HEAD_DRY_RUN))\n> +\t\tif (submodule_move_head(ce->name, old_id, new_id, flags))\n> \t\t\treturn o->gently ? -1 :\n> \t\t\t\tadd_rejected_path(o, ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n> \t\treturn 0;\n> @@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)\n> \t\tcase SM_UPDATE_CHECKOUT:\n> \t\tcase SM_UPDATE_REBASE:\n> \t\tcase SM_UPDATE_MERGE:\n> +\t\t\t/* state.force is set at the caller. */\n\nI don't understand the relevance of this comment - it is indeed set \nthere, but \"state\" is not used there until after the invocation to \nunlink_entry so it doesn't seem related.\n\n> \t\t\tsubmodule_move_head(ce->name, \"HEAD\", NULL,\n> \t\t\t\t\t SUBMODULE_MOVE_HEAD_FORCE);\n> \t\t\tbreak;\n>\n"},{"id":"316851","messageId":"CAGZ79kYQpVKc1GGj4hxw6t-6eDo8zPjwf=CtEPFTc9b4St1EOQ@mail.gmail.com","threadId":"45671","inReplyTo":"15b87ca2-98c4-edfc-1e7e-7a25c28bd8da@google.com","subject":"Re: [PATCH 1/4] entry.c: submodule recursing: respect force flag correctly","fromName":"Stefan Beller","fromEmail":"sbeller@google.com","sentAt":"2017-04-14T20:12:22Z","receivedAt":"2017-04-14T20:12:28Z","isPatch":true,"sender":{"key":"stefanbeller@gmail.com","avatar":"https://avatars.githubusercontent.com/u/455868?v=4"},"body":"On Fri, Apr 14, 2017 at 11:28 AM, Jonathan Tan wrote:\n\n>> @@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,\n>> unlink_or_warn(ce->name);\n>>\n>> return submodule_move_head(ce->name,\n>> - NULL, oid_to_hex(&ce->oid),\n>> - SUBMODULE_MOVE_HEAD_FORCE);\n>> + NULL, oid_to_hex(&ce->oid), 0);\n>\n>\n> Should we be consistent (with the \"else\" block below and with the existing\n> code) to use \"state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0\" instead of 0? (I\n> glanced briefly through the code and SUBMODULE_MOVE_HEAD_FORCE might have no\n> effect anyway if \"old\" is NULL, but it's probably still better to be\n> consistent.)\n\nok, will do.\n\n>>\n>> + if (o->reset)\n>> + flags |= SUBMODULE_MOVE_HEAD_FORCE;\n>\n>\n> It seems to me that this is independent of the entry.c change, and might be\n> better in its own patch. (Or if it is not, maybe the subject should be\n> \"entry, unpack-trees: propagate force when submodule recursing\" or something\n> like that, containing the names of both modified components.)\n\neh. I realize the patch evolved after writing the commit message initially.\nMaybe:\n\n fix all submodule_move_head force flags\n\n Audit all callers of submodule_move_head and make sure the\n force flag is handled correctly.\n\n\n>\n> Also, you mentioned in the parent message that this patch is required for\n> patch 3. Is only the entry.c part required, or unpack-trees.c, or both?\n>\n>> +\n>> switch (sub->update_strategy.type) {\n>> case SM_UPDATE_UNSPECIFIED:\n>> case SM_UPDATE_CHECKOUT:\n>> - if (submodule_move_head(ce->name, old_id, new_id,\n>> SUBMODULE_MOVE_HEAD_DRY_RUN))\n>> + if (submodule_move_head(ce->name, old_id, new_id, flags))\n>> return o->gently ? -1 :\n>> add_rejected_path(o,\n>> ERROR_WOULD_LOSE_SUBMODULE, ce->name);\n>> return 0;\n>> @@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)\n>> case SM_UPDATE_CHECKOUT:\n>> case SM_UPDATE_REBASE:\n>> case SM_UPDATE_MERGE:\n>> + /* state.force is set at the caller. */\n>\n>\n> I don't understand the relevance of this comment - it is indeed set there,\n> but \"state\" is not used there until after the invocation to unlink_entry so\n> it doesn't seem related.\n\nWell we would have wanted to put\n state->force ? SUBMODULE_MOVE_HEAD_FORCE : 0\nhere, but state is not passed into this function, so just make a comment\nwhy we keep it at force all the time.\n\nThanks,\nStefan\n"},{"id":"316852","messageId":"5441277f-8d8a-23dc-2c1a-d637286f007c@google.com","threadId":"45671","inReplyTo":"20170411234923.1860-4-sbeller@google.com","subject":"Re: [PATCH 3/4] submodule.c: harden submodule_move_head against broken submodules","fromName":"Jonathan Tan","fromEmail":"jonathantanmy@google.com","sentAt":"2017-04-14T20:13:27Z","receivedAt":"2017-04-14T20:13:34Z","isPatch":true,"sender":{"key":"jonathantanmy@fastmail.com","avatar":null},"body":"I think \"harden\" is the wrong word to use in the subject - to me, \n\"harden\" implies that you're defending against an invalid use. But here, \nnot only is the use valid, but this patch also takes steps to support it.\n\nOn 04/11/2017 04:49 PM, Stefan Beller wrote:\n> Early on in submodule_move_head just after the check if the submodule is\n> initialized, we need to check if the submodule is populated correctly.\n>\n> If the submodule is initialized but doesn't look like populated, this\n> is a red flag and can indicate multiple sorts of failures:\n> (1) The submodule may be recorded at an object name, that is missing.\n> (2) The submodule '.git' file link may be broken and it is not pointing\n> at a repository.\n>\n> In both cases we want to complain to the user in the non-forced mode,\n> and in the forced mode ignoring the old state and just moving the\n> submodule into its new state with a fixed '.git' file link.\n>\n> Signed-off-by: Stefan Beller \n> ---\n> submodule.c | 17 +++++++++++++++++\n> t/lib-submodule-update.sh | 23 ++++++++++++++++++++---\n> 2 files changed, 37 insertions(+), 3 deletions(-)\n>\n> diff --git a/submodule.c b/submodule.c\n> index 2fa42519a4..dda1ead854 100644\n> --- a/submodule.c\n> +++ b/submodule.c\n> @@ -1331,10 +1331,19 @@ int submodule_move_head(const char *path,\n> \tint ret = 0;\n> \tstruct child_process cp = CHILD_PROCESS_INIT;\n> \tconst struct submodule *sub;\n> +\tint *errorcode, error_code;\n\nMaybe call it error_code_ptr?\n\n>\n> \tif (!is_submodule_initialized(path))\n> \t\treturn 0;\n>\n> +\tif (flags & SUBMODULE_MOVE_HEAD_FORCE)\n> +\t\terrorcode = &error_code;\n> +\telse\n> +\t\terrorcode = NULL;\n> +\n> +\tif (old && !is_submodule_populated_gently(path, errorcode))\n\nThis behavior of the null-ness of the pointer controlling whether the \nfunction dies upon error or not is quite confusing to me, so I would add \na comment (e.g. under the \"errorcode = &error_code\" statement, saying \n\"Pass a non-null pointer to make is_submodule_populated_gently report \nerrors to use instead of die()-ing\").\n\n> +\t\treturn 0;\n> +\n> \tsub = submodule_from_path(null_sha1, path);\n>\n> \tif (!sub)\n> @@ -1361,6 +1370,14 @@ int submodule_move_head(const char *path,\n> \t\t\t/* make sure the index is clean as well */\n> \t\t\tsubmodule_reset_index(path);\n> \t\t}\n> +\n> +\t\tif (old && (flags & SUBMODULE_MOVE_HEAD_FORCE)) {\n> +\t\t\tstruct strbuf sb = STRBUF_INIT;\n> +\t\t\tstrbuf_addf(&sb, \"%s/modules/%s\",\n> +\t\t\t\t get_git_common_dir(), sub->name);\n> +\t\t\tconnect_work_tree_and_git_dir(path, sb.buf);\n> +\t\t\tstrbuf_release(&sb);\n\nxstrfmt might be simpler, but I don't have a strong preference either way.\n\n> +\t\t}\n> \t}\n>\n> \tprepare_submodule_repo_env_no_git_dir(&cp.env_array);\n> diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh\n> index 22dd9e060c..f0b1b18206 100755\n> --- a/t/lib-submodule-update.sh\n> +++ b/t/lib-submodule-update.sh\n> @@ -1213,14 +1213,31 @@ test_submodule_forced_switch_recursing () {\n> \t\t)\n> \t'\n> \t# Updating a submodule from an invalid sha1 updates\n> -\ttest_expect_success \"$command: modified submodule does not update submodule work tree from invalid commit\" '\n> +\ttest_expect_success \"$command: modified submodule does update submodule work tree from invalid commit\" '\n> \t\tprolog &&\n> \t\treset_work_tree_to_interested invalid_sub1 &&\n> \t\t(\n> \t\t\tcd submodule_update &&\n> \t\t\tgit branch -t valid_sub1 origin/valid_sub1 &&\n> -\t\t\ttest_must_fail $command valid_sub1 &&\n> -\t\t\ttest_superproject_content origin/invalid_sub1\n> +\t\t\t$command valid_sub1 &&\n> +\t\t\ttest_superproject_content origin/valid_sub1 &&\n> +\t\t\ttest_submodule_content sub1 origin/valid_sub1\n> +\t\t)\n> +\t'\n> +\n> +\t# Old versions of Git were buggy writing the .git link file\n> +\t# (e.g. before f8eaa0ba98b and then moving the superproject repo\n> +\t# whose submodules contained absolute paths)\n> +\ttest_expect_success \"$command: updating submodules fixes .git links\" '\n> +\t\tprolog &&\n> +\t\treset_work_tree_to_interested add_sub1 &&\n> +\t\t(\n> +\t\t\tcd submodule_update &&\n> +\t\t\tgit branch -t modify_sub1 origin/modify_sub1 &&\n> +\t\t\techo \"gitdir: bogus/path\" >sub1/.git &&\n> +\t\t\t$command modify_sub1 &&\n> +\t\t\ttest_superproject_content origin/modify_sub1 &&\n> +\t\t\ttest_submodule_content sub1 origin/modify_sub1\n> \t\t)\n> \t'\n> }\n>\n"}]}