Re: [PATCH v2] submodule: absorb git dir instead of dying on deinit
- From
Atharva Raykar <raykar.ath@gmail.com>
- Date
- Aug 27, 2021, 13:20 UTC
- Message-ID
- <m28s0nc8gv.fsf@gmail.com>
- In-Reply-To
- <pull.1078.v2.git.git.1630044219145.gitgitgadget@gmail.com>
"Mugdha Pattnaik via GitGitGadget" <gitgitgadget@gmail.com> writes:
Show 6 quoted lines
> 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.
Show 110 quoted lines
> 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.
Show 18 quoted lines
> 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.
Show 10 quoted lines
> 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