| Message ID | 20181207235425.128568-5-sbeller@google.com (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [1/4] submodule update: add regression test with old style setups | expand |
Stefan Beller <sbeller@google.com> writes: > This re-introduces 984cd77ddb (submodule deinit: unset core.worktree, > 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge > branch 'sb/submodule-core-worktree'", 2018-09-07) > > The whole series was reverted as the offending commit e98317508c > (submodule: ensure core.worktree is set after update, 2018-06-18) > was relied on by other commits such as 984cd77ddb. > > Keep the offending commit reverted, but its functionality came back via > 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such > that we can reintroduce 984cd77ddb now. None of the above three explains the most important thing directly, so readers fail to grasp what the main theme of the three-patch series is, without looking at the commits that were reverted already. Is the theme of the overall series to make sure core.worktree is set to point at the working tree when submodule's working tree is instantiated, and unset it when it is not? 2/4 was also explained (in the original) that it wants to unset and did so when "move_head" gets called. This one does the unset when a submodule is deinited. Are these the only two cases a submodule loses its working tree? If so, the log message for this step should declare victory by ending with something like ... as we covered the only other case in which a submodule loses its working tree in the earlier step (i.e. switching branches of top-level project to move to a commit that did not have the submodule), this makes the code always maintain core.worktree correctly unset when there is no working tree for a submodule. Thanks. I think I agree with what the series wants to do (if I read what it wants to do correctly, that is). > Signed-off-by: Stefan Beller <sbeller@google.com> > --- > builtin/submodule--helper.c | 2 ++ > t/lib-submodule-update.sh | 2 +- > t/t7400-submodule-basic.sh | 5 +++++ > 3 files changed, 8 insertions(+), 1 deletion(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index 31ac30cf2f..672b74db89 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix, > if (!(flags & OPT_QUIET)) > printf(format, displaypath); > > + submodule_unset_core_worktree(sub); > + > strbuf_release(&sb_rm); > } > > diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh > index 51d4555549..5b56b23166 100755 > --- a/t/lib-submodule-update.sh > +++ b/t/lib-submodule-update.sh > @@ -235,7 +235,7 @@ reset_work_tree_to_interested () { > then > mkdir -p submodule_update/.git/modules/sub1/modules && > cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2 > - GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree > + # core.worktree is unset for sub2 as it is not checked out > fi && > # indicate we are interested in the submodule: > git -C submodule_update config submodule.sub1.url "bogus" && > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index 76a7cb0af7..aba2d4d6ee 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section > rmdir init > ' > > +test_expect_success 'submodule deinit should unset core.worktree' ' > + test_path_is_file .git/modules/example/config && > + test_must_fail git config -f .git/modules/example/config core.worktree > +' > + > test_expect_success 'submodule deinit from subdirectory' ' > git submodule update --init && > git config submodule.example.foo bar &&
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index 31ac30cf2f..672b74db89 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1131,6 +1131,8 @@ static void deinit_submodule(const char *path, const char *prefix, if (!(flags & OPT_QUIET)) printf(format, displaypath); + submodule_unset_core_worktree(sub); + strbuf_release(&sb_rm); } diff --git a/t/lib-submodule-update.sh b/t/lib-submodule-update.sh index 51d4555549..5b56b23166 100755 --- a/t/lib-submodule-update.sh +++ b/t/lib-submodule-update.sh @@ -235,7 +235,7 @@ reset_work_tree_to_interested () { then mkdir -p submodule_update/.git/modules/sub1/modules && cp -r submodule_update_repo/.git/modules/sub1/modules/sub2 submodule_update/.git/modules/sub1/modules/sub2 - GIT_WORK_TREE=. git -C submodule_update/.git/modules/sub1/modules/sub2 config --unset core.worktree + # core.worktree is unset for sub2 as it is not checked out fi && # indicate we are interested in the submodule: git -C submodule_update config submodule.sub1.url "bogus" && diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index 76a7cb0af7..aba2d4d6ee 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -984,6 +984,11 @@ test_expect_success 'submodule deinit should remove the whole submodule section rmdir init ' +test_expect_success 'submodule deinit should unset core.worktree' ' + test_path_is_file .git/modules/example/config && + test_must_fail git config -f .git/modules/example/config core.worktree +' + test_expect_success 'submodule deinit from subdirectory' ' git submodule update --init && git config submodule.example.foo bar &&
This re-introduces 984cd77ddb (submodule deinit: unset core.worktree, 2018-06-18), which was reverted as part of f178c13fda (Revert "Merge branch 'sb/submodule-core-worktree'", 2018-09-07) The whole series was reverted as the offending commit e98317508c (submodule: ensure core.worktree is set after update, 2018-06-18) was relied on by other commits such as 984cd77ddb. Keep the offending commit reverted, but its functionality came back via 4d6d6ef1fc (Merge branch 'sb/submodule-update-in-c', 2018-09-17), such that we can reintroduce 984cd77ddb now. Signed-off-by: Stefan Beller <sbeller@google.com> --- builtin/submodule--helper.c | 2 ++ t/lib-submodule-update.sh | 2 +- t/t7400-submodule-basic.sh | 5 +++++ 3 files changed, 8 insertions(+), 1 deletion(-)