recursive submodules: git-reset!
v2 cover recursive submodules: git-reset!
v2:
* improved commit message to be proper English (Thanks, Philip!)
* clarified why the patch 2 is so short (i.e. it doesn't matter if the submodule is initialized in the preparation repo, we care about the actual testing repo! Thanks, Brandon)
* reworded patch 1 (Thanks Jonathan)
Thanks,
Stefan
v1: https://public-inbox.org/git/[email protected]/
Now that the BIG one has landed, e394fa01d6 (Merge branch 'sb/checkout-recurse-submodules', 2017-03-28), you would expect that teaching to recurse into submodules is easy for all the remaining working tree manipulations?
It turns out it is. See the last patch how we teach git-reset to recurse into submodules.
However when thinking more about what git-reset is expected to do, I added tests and some fixes for them (patch 2+3).
patch 1 is a correctness thing, required for patch 3.
Thanks,
Stefan
Stefan Beller (4):
entry.c: submodule recursing: respect force flag correctly submodule.c: uninitialized submodules are ignored in recursive
commands
submodule.c: submodule_move_head works with broken submodules builtin/reset: add --recurse-submodules switch
builtin/reset.c | 30 ++++++++++++++++++++++++++++++ entry.c | 8 ++++---- submodule.c | 31 +++++++++++++++++++++++++++---- t/lib-submodule-update.sh | 24 +++++++++++++++++++++--- t/t7112-reset-submodule.sh | 8 ++++++++ unpack-trees.c | 7 ++++++- 6 files changed, 96 insertions(+), 12 deletions(-)
PATCH v2 1/4 entry.c: submodule recursing: respect force flag correctly
In case of a non-forced worktree update, the submodule movement is tested in a dry run first, such that it doesn't matter if the actual update is done via the force flag. However for correctness, we want to give the flag as specified by the user. All callers have been inspected and updated if needed.
--- entry.c | 8 ++++---- unpack-trees.c | 7 ++++++- 2 files changed, 10 insertions(+), 5 deletions(-)
entry.c +4 -4
@@ -208,7 +208,8 @@ static int write_entry(struct cache_entry *ce, sub =submodule_from_ce (ce);if (sub)return submodule_move_head (ce->name, -NULL ,oid_to_hex (&ce->oid),SUBMODULE_MOVE_HEAD_FORCE ); +NULL ,oid_to_hex (&ce->oid), + state->force ?SUBMODULE_MOVE_HEAD_FORCE :0 );break ;default :return error ("unknown file mode for %s in index" , path);
@@ -282,12 +283,11 @@ int checkout_entry(struct cache_entry *ce,unlink_or_warn (ce->name);return submodule_move_head (ce->name, -NULL ,oid_to_hex (&ce->oid), -SUBMODULE_MOVE_HEAD_FORCE ); +NULL ,oid_to_hex (&ce->oid),0 ); }else return submodule_move_head (ce->name,"HEAD" ,oid_to_hex (&ce->oid), -SUBMODULE_MOVE_HEAD_FORCE ); + state->force ?SUBMODULE_MOVE_HEAD_FORCE :0 ); }if (!changed)
unpack-trees.c +6 -1
@@ -252,14 +252,18 @@ static int check_submodule_move_head(const struct cache_entry *ce,const char *new_id, struct unpack_trees_options *o) { + unsigned flags = SUBMODULE_MOVE_HEAD_DRY_RUN;const struct submodule *sub =submodule_from_ce (ce);if (!sub)return 0 ; +if (o->reset) + flags |=SUBMODULE_MOVE_HEAD_FORCE ; +switch (sub->update_strategy.type) {case SM_UPDATE_UNSPECIFIED :case SM_UPDATE_CHECKOUT : -if (submodule_move_head (ce->name, old_id, new_id,SUBMODULE_MOVE_HEAD_DRY_RUN )) +if (submodule_move_head (ce->name, old_id, new_id, flags))return o->gently ?-1 :add_rejected_path (o,ERROR_WOULD_LOSE_SUBMODULE , ce->name);return 0 ;
@@ -308,6 +312,7 @@ static void unlink_entry(const struct cache_entry *ce)case SM_UPDATE_CHECKOUT :case SM_UPDATE_REBASE :case SM_UPDATE_MERGE : +/* state.force is set at the caller. */ submodule_move_head (ce->name,"HEAD" ,NULL ,SUBMODULE_MOVE_HEAD_FORCE );break ;
Signature · 1 line
2.12.2.642.g1b8cc69eee.dirty
PATCH v2 2/4 submodule.c: uninitialized submodules are ignored in recursive commands
In reply to Stefan BellerThis was an oversight when working on the working tree modifying commands recursing into submodules.
To test for uninitialized submodules, introduce another submodule "uninitialized_sub". Adding it via submodule add will activate the submodule in the preparation area (in create_lib_submodule_repo we setup all the things in submodule_update_repo), but the later tests will use a new testing repo that clones the preparation repo in which the new submodule is not initialized.
By adding it to the branch "add_sub1", which is the starting point of all other branches, we have wide coverage.
--- submodule.c | 3 +++ t/lib-submodule-update.sh | 1 + 2 files changed, 4 insertions(+)
submodule.c +3 -0
@@ -1333,6 +1333,9 @@ int submodule_move_head(const char *path,struct child_process cp =CHILD_PROCESS_INIT ;const struct submodule *sub; +if (!is_submodule_initialized (path)) +return 0 ; + sub =submodule_from_path (null_sha1, path);if (!sub)
t/lib-submodule-update.sh +1 -0
@@ -73,6 +73,7 @@ create_lib_submodule_repo () { git checkout -b "add_sub1" && git submodule add ../submodule_update_sub1 sub1 && + git submodule add ../submodule_update_sub1 uninitialized_sub && git config -f .gitmodules submodule.sub1.ignore all && git config submodule.sub1.ignore all && git add .gitmodules &&
Signature · 1 line
2.12.2.642.g1b8cc69eee.dirty
PATCH v2 3/4 submodule.c: submodule_move_head works with broken submodules
In reply to Stefan BellerEarly on in submodule_move_head just after the check if the submodule is initialized, we need to check if the submodule is populated correctly.
If the submodule is initialized but doesn't look like it is populated, this is a red flag and can indicate multiple sorts of failures:
(1) The submodule may be recorded at an object name, that is missing.
(2) The submodule '.git' file link may be broken and it is not pointing at a repository.
In both cases we want to complain to the user in the non-forced mode, and in the forced mode ignoring the old state and just moving the submodule into its new state with a fixed '.git' file link.
--- submodule.c | 28 ++++++++++++++++++++++++---- t/lib-submodule-update.sh | 23 ++++++++++++++++++++--- 2 files changed, 44 insertions(+), 7 deletions(-)
submodule.c +24 -4
@@ -1332,10 +1332,24 @@ int submodule_move_head(const char *path,int ret =0 ;struct child_process cp =CHILD_PROCESS_INIT ;const struct submodule *sub; +int *error_code_ptr, error_code;if (!is_submodule_initialized (path))return 0 ; +if (flags &SUBMODULE_MOVE_HEAD_FORCE ) +/* +* Pass non NULL pointer to is_submodule_populated_gently +* to prevent die()-ing. We'll use connect_work_tree_and_git_dir +* to fixup the submodule in the force case later. +*/ + error_code_ptr = &error_code; +else + error_code_ptr =NULL ; + +if (old && !is_submodule_populated_gently (path, error_code_ptr)) +return 0 ; + sub =submodule_from_path (null_sha1, path);if (!sub)
@@ -1353,15 +1367,21 @@ int submodule_move_head(const char *path,absorb_git_dir_into_superproject ("" , path,ABSORB_GITDIR_RECURSE_SUBMODULES ); }else { -struct strbuf sb =STRBUF_INIT ; -strbuf_addf (&sb,"%s/modules/%s" , +char *gitdir =xstrfmt ("%s/modules/%s" ,get_git_common_dir (), sub->name); -connect_work_tree_and_git_dir (path, sb.buf); -strbuf_release (&sb); +connect_work_tree_and_git_dir (path, gitdir); +free (gitdir);/* make sure the index is clean as well */ submodule_reset_index (path); } + +if (old && (flags &SUBMODULE_MOVE_HEAD_FORCE )) { +char *gitdir =xstrfmt ("%s/modules/%s" , +get_git_common_dir (), sub->name); +connect_work_tree_and_git_dir (path, gitdir); +free (gitdir); + } }prepare_submodule_repo_env_no_git_dir (&cp.env_array);
t/lib-submodule-update.sh +20 -3
@@ -1213,14 +1213,31 @@ test_submodule_forced_switch_recursing () { ) ' # Updating a submodule from an invalid sha1 updates - test_expect_success "$command: modified submodule does not update submodule work tree from invalid commit" ' + test_expect_success "$command: modified submodule does update submodule work tree from invalid commit" ' prolog && reset_work_tree_to_interested invalid_sub1 && ( cd submodule_update && git branch -t valid_sub1 origin/valid_sub1 && - test_must_fail $command valid_sub1 && - test_superproject_content origin/invalid_sub1 + $command valid_sub1 && + test_superproject_content origin/valid_sub1 && + test_submodule_content sub1 origin/valid_sub1 + ) + ' + + # Old versions of Git were buggy writing the .git link file + # (e.g. before f8eaa0ba98b and then moving the superproject repo + # whose submodules contained absolute paths) + test_expect_success "$command: updating submodules fixes .git links" ' + prolog && + reset_work_tree_to_interested add_sub1 && + ( + cd submodule_update && + git branch -t modify_sub1 origin/modify_sub1 && + echo "gitdir: bogus/path" >sub1/.git && + $command modify_sub1 && + test_superproject_content origin/modify_sub1 && + test_submodule_content sub1 origin/modify_sub1 ) ' }
Signature · 1 line
2.12.2.642.g1b8cc69eee.dirty
PATCH v2 4/4 builtin/reset: add --recurse-submodules switch
In reply to Stefan Bellergit-reset is yet another working tree manipulator, which should be taught about submodules.
One use case of "git-reset" is to reset to a known good state, and dropping commits that did not work as expected. In that case one of the expected outcomes from a hard reset would be to have broken submodules reset to a known good state as well. A test for this was added in a prior patch.
--- builtin/reset.c | 30 ++++++++++++++++++++++++++++++ t/t7112-reset-submodule.sh | 8 ++++++++ 2 files changed, 38 insertions(+)
builtin/reset.c +30 -0
@@ -21,6 +21,27 @@#include "parse-options.h" #include "unpack-trees.h" #include "cache-tree.h" +#include "submodule.h" +#include "submodule-config.h" + +static int recurse_submodules =RECURSE_SUBMODULES_DEFAULT ; + +static int option_parse_recurse_submodules (const struct option *opt, +const char *arg,int unset) +{ +if (unset) { + recurse_submodules =RECURSE_SUBMODULES_OFF ; +return 0 ; + } +if (arg) + recurse_submodules = +parse_update_recurse_submodules_arg (opt->long_name, + arg); +else + recurse_submodules =RECURSE_SUBMODULES_ON ; + +return 0 ; +}static const char *const git_reset_usage[] = {N_ ("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]" ),
@@ -283,6 +304,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix) N_("reset HEAD, index and working tree"), MERGE), OPT_SET_INT(0, "keep", &reset_type, N_("reset HEAD but keep local changes"), KEEP), + {OPTION_CALLBACK ,0 ,"recurse-submodules" , &recurse_submodules, +"reset" ,"control recursive updating of submodules" , + PARSE_OPT_OPTARG, option_parse_recurse_submodules },OPT_BOOL ('p' ,"patch" , &patch_mode,N_ ("select hunks interactively" )),OPT_BOOL ('N' ,"intent-to-add" , &intent_to_add, N_("record only the fact that removed paths will be added later")),
@@ -295,6 +319,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH);parse_args (&pathspec, argv, prefix, patch_mode, &rev); +if (recurse_submodules !=RECURSE_SUBMODULES_DEFAULT ) { +gitmodules_config (); +git_config (submodule_config,NULL ); +set_config_update_recurse_submodules (RECURSE_SUBMODULES_ON ); + } + unborn = !strcmp (rev,"HEAD" ) &&get_sha1 ("HEAD" , oid.hash);if (unborn) {/* reset on unborn branch: treat as reset to empty tree */
t/t7112-reset-submodule.sh +8 -0
@@ -5,6 +5,14 @@ test_description='reset can handle submodules' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-submodule-update.sh +KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1 +KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1 +KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1 + +test_submodule_switch_recursing "git reset --recurse-submodules --keep" + +test_submodule_forced_switch_recursing "git reset --hard --recurse-submodules" + test_submodule_switch "git reset --keep" test_submodule_switch "git reset --merge"
Signature · 1 line
2.12.2.642.g1b8cc69eee.dirty
Re: [PATCHv2 4/4] builtin/reset: add --recurse-submodules switch
Quoted 9 lines · Stefan Beller
git-reset is yet another working tree manipulator, which should be taught about submodules.
One use case of "git-reset" is to reset to a known good state, and dropping commits that did not work as expected.
In that case one of the expected outcomes from a hard reset would be to have broken submodules reset to a known good state as well. A test for this was added in a prior patch.
When "git reset --hard" at the top-level superproject updates a gitlink in the index to a commit that was different from what was checked out in the working tree of the submodule, what should happen? Do we reset the tip of the current branch in the submodule to point at the commit the index of the top-level records? Do we detach the HEAD in the submodule to point at the commit? Something else that is configurable? Or do we just run "git reset --hard" in each submodule (which may leave submodule's HEAD different from what is recorded in the index of the superproject)?
"... to a known good state as well" does not help answering the above.
Re: [PATCHv2 4/4] builtin/reset: add --recurse-submodules switch
Quoted 8 lines · Junio C Hamano
Quoted 6 lines · Stefan Beller
git-reset is yet another working tree manipulator, which should be taught about submodules.
One use case of "git-reset" is to reset to a known good state, and dropping commits that did not work as expected.
Quoted 8 lines
Quoted 3 lines
In that case one of the expected outcomes from a hard reset would be to have broken submodules reset to a known good state as well. A test for this was added in a prior patch.
When "git reset --hard" at the top-level superproject updates a gitlink in the index to a commit that was different from what was checked out in the working tree of the submodule, what should happen?
We reset the submodule to the commit as recorded in the superproject, detaching its HEAD.
Quoted 8 lines
Do we reset the tip of the current branch in the submodule
to point at the commit the index of the top-level records? Do we detach the HEAD in the submodule to point at the commit? Something else that is configurable? Or do we just run "git reset --hard" in each submodule (which may leave submodule's HEAD different from what is recorded in the index of the superproject)?"... to a known good state as well" does not help answering the above.
I agree.
Quoted 1 line
PATCH v3 4/4 builtin/reset: add --recurse-submodules switch
git-reset is yet another working tree manipulator, which should be taught about submodules.
When a user uses git-reset and requests to recurse into submodules, this will reset the submodules to the object name as recorded in the superproject, detaching the HEADs.
---
This replaces the topmost patch in sb/reset-recurse-submodules. The only difference is the rewording of the commit message.
Thanks,
Stefan
builtin/reset.c | 30 ++++++++++++++++++++++++++++++ t/t7112-reset-submodule.sh | 8 ++++++++ 2 files changed, 38 insertions(+)
builtin/reset.c +30 -0
@@ -21,6 +21,27 @@#include "parse-options.h" #include "unpack-trees.h" #include "cache-tree.h" +#include "submodule.h" +#include "submodule-config.h" + +static int recurse_submodules =RECURSE_SUBMODULES_DEFAULT ; + +static int option_parse_recurse_submodules (const struct option *opt, +const char *arg,int unset) +{ +if (unset) { + recurse_submodules =RECURSE_SUBMODULES_OFF ; +return 0 ; + } +if (arg) + recurse_submodules = +parse_update_recurse_submodules_arg (opt->long_name, + arg); +else + recurse_submodules =RECURSE_SUBMODULES_ON ; + +return 0 ; +}static const char *const git_reset_usage[] = {N_ ("git reset [--mixed | --soft | --hard | --merge | --keep] [-q] [<commit>]" ),
@@ -283,6 +304,9 @@ int cmd_reset(int argc, const char **argv, const char *prefix) N_("reset HEAD, index and working tree"), MERGE), OPT_SET_INT(0, "keep", &reset_type, N_("reset HEAD but keep local changes"), KEEP), + {OPTION_CALLBACK ,0 ,"recurse-submodules" , &recurse_submodules, +"reset" ,"control recursive updating of submodules" , + PARSE_OPT_OPTARG, option_parse_recurse_submodules },OPT_BOOL ('p' ,"patch" , &patch_mode,N_ ("select hunks interactively" )),OPT_BOOL ('N' ,"intent-to-add" , &intent_to_add, N_("record only the fact that removed paths will be added later")),
@@ -295,6 +319,12 @@ int cmd_reset(int argc, const char **argv, const char *prefix) PARSE_OPT_KEEP_DASHDASH);parse_args (&pathspec, argv, prefix, patch_mode, &rev); +if (recurse_submodules !=RECURSE_SUBMODULES_DEFAULT ) { +gitmodules_config (); +git_config (submodule_config,NULL ); +set_config_update_recurse_submodules (RECURSE_SUBMODULES_ON ); + } + unborn = !strcmp (rev,"HEAD" ) &&get_sha1 ("HEAD" , oid.hash);if (unborn) {/* reset on unborn branch: treat as reset to empty tree */
t/t7112-reset-submodule.sh +8 -0
@@ -5,6 +5,14 @@ test_description='reset can handle submodules' . ./test-lib.sh . "$TEST_DIRECTORY"/lib-submodule-update.sh +KNOWN_FAILURE_SUBMODULE_RECURSIVE_NESTED=1 +KNOWN_FAILURE_DIRECTORY_SUBMODULE_CONFLICTS=1 +KNOWN_FAILURE_SUBMODULE_OVERWRITE_IGNORED_UNTRACKED=1 + +test_submodule_switch_recursing "git reset --recurse-submodules --keep" + +test_submodule_forced_switch_recursing "git reset --hard --recurse-submodules" + test_submodule_switch "git reset --keep" test_submodule_switch "git reset --merge"
Signature · 1 line
2.13.0.rc0.2.g0d1ae48b0e
Quoted 5 lines · Stefan Beller
v2:
* improved commit message to be proper English (Thanks, Philip!)
* clarified why the patch 2 is so short (i.e. it doesn't matter if the submodule is initialized in the preparation repo, we care about the actual testing repo! Thanks, Brandon)
That was the only thing I was unsure about in v1. v2 lgtm.
Quoted 39 lines
* reworded patch 1 (Thanks Jonathan)
Thanks,
Stefanv1: https://public-inbox.org/git/[email protected]/
Now that the BIG one has landed,
e394fa01d6(Merge branch 'sb/checkout-recurse-submodules', 2017-03-28), you would expect that teaching to recurse into submodules is easy for all the remaining working tree manipulations?It turns out it is. See the last patch how we teach git-reset to recurse into submodules.
However when thinking more about what git-reset is expected to do, I added tests and some fixes for them (patch 2+3).
patch 1 is a correctness thing, required for patch 3.
Thanks,
StefanStefan Beller (4):
entry.c: submodule recursing: respect force flag correctlysubmodule.c: uninitialized submodules are ignored in recursive
commands
submodule.c:submodule_move_headworks with broken submodules builtin/reset: add --recurse-submodules switchbuiltin/reset.c | 30 ++++++++++++++++++++++++++++++ entry.c | 8 ++++---- submodule.c | 31 +++++++++++++++++++++++++++---- t/lib-submodule-update.sh | 24 +++++++++++++++++++++--- t/t7112-reset-submodule.sh | 8 ++++++++ unpack-trees.c | 7 ++++++- 6 files changed, 96 insertions(+), 12 deletions(-)
Signature · 1 line
Brandon Williams
Quoted 8 lines · Brandon Williams
Quoted 5 lines · Stefan Beller
v2:
* improved commit message to be proper English (Thanks, Philip!)
* clarified why the patch 2 is so short (i.e. it doesn't matter if the submodule is initialized in the preparation repo, we care about the actual testing repo! Thanks, Brandon)That was the only thing I was unsure about in v1. v2 lgtm.
Thanks for the review.
Well the last commit still needs a better commit message as Junio pointed out, so might just resend the last patch with better wording.