| Message ID | pull.1078.v2.git.git.1630044219145.gitgitgadget@gmail.com (mailing list archive) |
|---|---|
| State | Superseded |
| Headers | show |
| Series | [v2] submodule: absorb git dir instead of dying on deinit | expand |
"Mugdha Pattnaik via GitGitGadget" <gitgitgadget@gmail.com> writes: > From: mugdha <mugdhapattnaik@gmail.com> > > Currently, running 'git submodule deinit' on repos where the > submodule's '.git' is a folder aborts with a message that is not > exactly user friendly. Let's change this to instead warn the user > to rerun the command with '--force'. This makes sense, a lot of commands seem to follow this pattern of requiring a user to '--force' when the behaviour might not be different from what is normally expected. > This internally calls 'absorb_git_dir_into_superproject()', which > moves the '.git' folder into the superproject and replaces it with > a '.git' file. The rest of the deinit function can operate as it > already does with new-style submodules. Nice. Just like what the NEEDSWORK comment suggested. > We also edit a test case such that it matches the new behaviour of > deinit. > > Suggested-by: Atharva Raykar <raykar.ath@gmail.com> > Signed-off-by: Mugdha Pattnaik <mugdhapattnaik@gmail.com> > --- > submodule: absorb git dir instead of dying on deinit > > We also edit a test case such that it matches the new behaviour of > deinit. > > I have changed the 'cp -R ../.git/modules/example .git' to 'mv > ../.git/modules/example .git' since, at the time of testing, the test > would fail - deinit now would be moving the '.git' folder into the > superproject's '.git/modules/' folder, and since this same folder > already existed before, it was causing errors. So, before running > deinit, instead of copying the '.git' folder into the submodule, if we > move it there instead, this functionality can be appropriately tested. > > Changes since v1: > > * Removed extra indent within the if statements > * Moved absorb_git_dir_into_superproject() call outside the if > condition checking for --quiet flag > > Thank you, Mugdha > > Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v2 > Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v2 > Pull-Request: https://github.com/git/git/pull/1078 > > Range-diff vs v1: > > 1: 58814531d17 ! 1: 37c9b598fde submodule: absorb git dir instead of dying on deinit > @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons > - displaypath); > + if (is_directory(sub_git_dir)) { > + if (!(flags & OPT_FORCE)) > -+ die(_("Submodule work tree '%s' contains a " > -+ ".git directory (use --force if you want " > -+ "to move its contents to superproject's " > -+ "module folder and convert .git to a file " > -+ "and then proceed with deinit)"), > ++ die(_("Submodule work tree '%s' contains a " > ++ ".git directory.\nUse --force if you want " > ++ "to move its contents to superproject's " > ++ "module folder and convert .git to a file " > ++ "and then proceed with deinit."), > ++ displaypath); > ++ > ++ if (!(flags & OPT_QUIET)) > ++ warning(_("Submodule work tree '%s' contains a .git " > ++ "directory. This will be replaced with a " > ++ ".git file by using absorbgitdirs."), > + displaypath); > + > -+ if (!(flags & OPT_QUIET)) { > -+ warning(_("Submodule work tree '%s' contains a .git " > -+ "directory (this will be replaced with a " > -+ ".git file by using absorbgitdirs"), > -+ displaypath); > -+ absorb_git_dir_into_superproject(displaypath, flags); > -+ } > ++ absorb_git_dir_into_superproject(displaypath, flags); > ++ > + } > > if (!(flags & OPT_FORCE)) { > > > builtin/submodule--helper.c | 28 ++++++++++++++++++---------- > t/t7400-submodule-basic.sh | 10 +++++----- > 2 files changed, 23 insertions(+), 15 deletions(-) > > diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c > index ef2776a9e45..4730dc141d4 100644 > --- a/builtin/submodule--helper.c > +++ b/builtin/submodule--helper.c > @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix, > struct strbuf sb_rm = STRBUF_INIT; > const char *format; > > - /* > - * protect submodules containing a .git directory > - * NEEDSWORK: instead of dying, automatically call > - * absorbgitdirs and (possibly) warn. > - */ > - if (is_directory(sub_git_dir)) > - die(_("Submodule work tree '%s' contains a .git " > - "directory (use 'rm -rf' if you really want " > - "to remove it including all of its history)"), > - displaypath); > + if (is_directory(sub_git_dir)) { > + if (!(flags & OPT_FORCE)) > + die(_("Submodule work tree '%s' contains a " > + ".git directory.\nUse --force if you want " > + "to move its contents to superproject's " > + "module folder and convert .git to a file " > + "and then proceed with deinit."), > + displaypath); > + > + if (!(flags & OPT_QUIET)) > + warning(_("Submodule work tree '%s' contains a .git " > + "directory. This will be replaced with a " > + ".git file by using absorbgitdirs."), > + displaypath); > + > + absorb_git_dir_into_superproject(displaypath, flags); > + > + } Looks okay to me. The quiet flag suppresses the warning, which is nice. > if (!(flags & OPT_FORCE)) { > struct child_process cp_rm = CHILD_PROCESS_INIT; > diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh > index cb1b8e35dbf..3df71478d06 100755 > --- a/t/t7400-submodule-basic.sh > +++ b/t/t7400-submodule-basic.sh > @@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su > rmdir init example2 > ' > > -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' ' > +test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' ' > git submodule update --init && > ( > cd init && > rm .git && > - cp -R ../.git/modules/example .git && > + mv ../.git/modules/example .git && The original test case was written with the expectation that the deinit won't happen. But now that it will, I suppose an mv was necessary so that absorbgitdirs can write the gitdir back to the original place successfully. > GIT_WORK_TREE=. git config --unset core.worktree > ) && > test_must_fail git submodule deinit init && > - test_must_fail git submodule deinit -f init && > - test -d init/.git && > - test -n "$(git config --get-regexp "submodule\.example\.")" > + git submodule deinit -f init && > + ! test -d init/.git && > + test -z "$(git config --get-regexp "submodule\.example\.")" > ' Likewise here. The test case has been flipped to reflect the new behaviour. > test_expect_success 'submodule with UTF-8 name' ' > > base-commit: c4203212e360b25a1c69467b5a8437d45a373cac BTW, I see you linked downthread the documentation[1] which says: When [old-form submodules] deinitialized or deleted (see below), the submodule’s Git directory is automatically moved to $GIT_DIR/modules/<name>/ of the superproject. This was not what Git was doing before this patch, it used to die() out instead. So I think this actually is a bug fix (although I am not sure why the test suite also did not agree with the documentation). Either way, I think this is a welcome change. Thanks for the patch! [1] https://git-scm.com/docs/gitsubmodules#_forms
Atharva Raykar <raykar.ath@gmail.com> writes: > "Mugdha Pattnaik via GitGitGadget" <gitgitgadget@gmail.com> writes: > >> From: mugdha <mugdhapattnaik@gmail.com> >> >> Currently, running 'git submodule deinit' on repos where the >> submodule's '.git' is a folder aborts with a message that is not >> exactly user friendly. Let's change this to instead warn the user >> to rerun the command with '--force'. > > This makes sense, a lot of commands seem to follow this pattern of > requiring a user to '--force' when the behaviour might not be different > from what is normally expected. > >> This internally calls 'absorb_git_dir_into_superproject()', which >> moves the '.git' folder into the superproject and replaces it with >> a '.git' file. The rest of the deinit function can operate as it >> already does with new-style submodules. > > Nice. Just like what the NEEDSWORK comment suggested. Indeed. Many use of the word "folder", especially when the thing is called "git dir" in the title, irritates me, though. We use files and directories, not folders. > BTW, I see you linked downthread the documentation[1] which says: > > When [old-form submodules] deinitialized or deleted (see below), the > submodule’s Git directory is automatically moved to > $GIT_DIR/modules/<name>/ of the superproject. > > This was not what Git was doing before this patch, it used to die() out > instead. So I think this actually is a bug fix (although I am not sure > why the test suite also did not agree with the documentation). Curious. I see the description was added by d4803455 (submodules: overhaul documentation, 2017-06-22), which claimed to "detangle" by "move" various existing pieces of documentation into a new file. I guess we added a wish without marking it as such ;-)
diff --git a/builtin/submodule--helper.c b/builtin/submodule--helper.c index ef2776a9e45..4730dc141d4 100644 --- a/builtin/submodule--helper.c +++ b/builtin/submodule--helper.c @@ -1539,16 +1539,24 @@ static void deinit_submodule(const char *path, const char *prefix, struct strbuf sb_rm = STRBUF_INIT; const char *format; - /* - * protect submodules containing a .git directory - * NEEDSWORK: instead of dying, automatically call - * absorbgitdirs and (possibly) warn. - */ - if (is_directory(sub_git_dir)) - die(_("Submodule work tree '%s' contains a .git " - "directory (use 'rm -rf' if you really want " - "to remove it including all of its history)"), - displaypath); + if (is_directory(sub_git_dir)) { + if (!(flags & OPT_FORCE)) + die(_("Submodule work tree '%s' contains a " + ".git directory.\nUse --force if you want " + "to move its contents to superproject's " + "module folder and convert .git to a file " + "and then proceed with deinit."), + displaypath); + + if (!(flags & OPT_QUIET)) + warning(_("Submodule work tree '%s' contains a .git " + "directory. This will be replaced with a " + ".git file by using absorbgitdirs."), + displaypath); + + absorb_git_dir_into_superproject(displaypath, flags); + + } if (!(flags & OPT_FORCE)) { struct child_process cp_rm = CHILD_PROCESS_INIT; diff --git a/t/t7400-submodule-basic.sh b/t/t7400-submodule-basic.sh index cb1b8e35dbf..3df71478d06 100755 --- a/t/t7400-submodule-basic.sh +++ b/t/t7400-submodule-basic.sh @@ -1182,18 +1182,18 @@ test_expect_success 'submodule deinit is silent when used on an uninitialized su rmdir init example2 ' -test_expect_success 'submodule deinit fails when submodule has a .git directory even when forced' ' +test_expect_success 'submodule deinit fails when submodule has a .git directory unless forced' ' git submodule update --init && ( cd init && rm .git && - cp -R ../.git/modules/example .git && + mv ../.git/modules/example .git && GIT_WORK_TREE=. git config --unset core.worktree ) && test_must_fail git submodule deinit init && - test_must_fail git submodule deinit -f init && - test -d init/.git && - test -n "$(git config --get-regexp "submodule\.example\.")" + git submodule deinit -f init && + ! test -d init/.git && + test -z "$(git config --get-regexp "submodule\.example\.")" ' test_expect_success 'submodule with UTF-8 name' '