Re: [PATCH v4] submodule: absorb git dir instead of dying on deinit

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Fri, Aug 27 2021, Mugdha Pattnaik via GitGitGadget wrote:

> From: mugdha <mugdhapattnaik@gmail.com>
>
> Currently, running 'git submodule deinit' on repos where the
> submodule's '.git' is a directory, 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 internally calls 'absorb_git_dir_into_superproject()', which
> moves the git dir 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.
>
> 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
>     
>     Changes since v3:
>     
>      * Replaced 1 instance of the word "folder" with "directory"
>      * Fixed tab spacing
>     
>     Thank you, Mugdha
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-git-1078%2Fmugdhapattnaik%2Fsubmodule-deinit-absorbgitdirs-v4
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-git-1078/mugdhapattnaik/submodule-deinit-absorbgitdirs-v4
> Pull-Request: https://github.com/git/git/pull/1078
>
> Range-diff vs v3:
>
>  1:  1ac65b2458b ! 1:  7460fc0e12a submodule: absorb git dir instead of dying on deinit
>      @@ builtin/submodule--helper.c: static void deinit_submodule(const char *path, cons
>       +		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);
>      ++				      ".git directory.\nUse --force if you want "
>      ++				      "to move its contents to superproject's "
>      ++				      "module directory 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);
>      ++					  "directory. This will be replaced with a "
>      ++					  ".git file by using absorbgitdirs."),
>      ++					displaypath);
>       +
>       +			absorb_git_dir_into_superproject(displaypath, flags);
>       +
>
>
>  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..040b26f149d 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 directory 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\.")"

I see we don't have a "test_dir_is_missing" for the pre-image, but
shouldn't the post-image "! test -d" just be a test_path_is_missing?
I.e. should this pass if .git is a file? Probably not...