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

[Atharva Raykar <raykar.ath@gmail.com>]

"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