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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 7, 2021, 19:41 UTC
Message-ID: <xmqqwnmopqqk.fsf@gitster.g>
In-Reply-To: <pull.1078.v6.git.git.1633523057369.gitgitgadget@gmail.com>

"Mugdha Pattnaik via GitGitGadget" <gitgitgadget@gmail.com> writes:

> From: mugdha <mugdhapattnaik@gmail.com>

This line is called "in-body header" that is used to override the author name that is automatically obtained from the e-mail's "From:" header (which is set to "Mugdha Pattnaik via GitGitGadget" by GGG, which is obviously not your name, hence we use an in-body header to override it). It should match what you use to sign off your patches, the one we see below.

I'll hand-edit so that "git show" will say that the author is "Mugdha Pattnaik", not "mugdha", while applying, but please make sure your further submissions will not have this problem.

> 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'.

OK. That sounds like an improvement, albeit possibly an overly cautious one, as a casual "deinit" user will get an error as before without "--force", which may or may not be a good thing. Requiring "--force" means it is safer by default by not changing the on-disk data. But requiring "--force" also means we end up training users to say "--force" when it shouldn't have to be.

A plausible alternative is to always absorb but with a warning "we absorbed it for you", without requiring "--force". If we didn't have "git submodule deinit" command now and were designing it from scratch, would we design it to fail because the submodule's git directory is not absorbed? I doubt it, as I do not think of a good reason to do so offhand.

Does "git submodule" currently reject a "deinit" request due to some _other_ conditions or safety concerns and require the "--force" option to continue? Requiring the "--force" option to resolve ".git is a directory, and the user wants to make it absorbed" means that the user will be _forced_ to bypass these _other_ safety valves only to save the submodule repository from destruction when running "deinit", which may not be a good trade-off between the safety requirements of these _other_ conditions, if exists, and the one we are dealing with.

> 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.

This is not wrong per-se, but such an implementation detail is something best left for the patch.

> We also edit a test case such that it matches the new behaviour of
> deinit.

"match the new behaviour" in what way?

    In one test, we used to require "git submodule deinit" to fail
    even with the "--force" option when the submodule's .git/
    directory is not absorbed.  Adjust it to expect the operation to
    pass.

would be a description at the right level of detail, I think.

Show 34 quoted lines

> 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);

Shouldn't the first argument to this call be "path" not "displaypath"? The paths in messages may want to have the path from the top to the submodule location prefixed for human consumption, but the called function only cares about the path to the submodule from the current directory, no?

The second parameter of this call seems totally bogus to me. What is the vocabulary of bits the called function takes? Is that from the same set the flags this function takes? Does the called function even understand OPT_QUIET, or does the bitpattern that happens to correspond to OPT_QUIET have a totally different meaning to the called function and we will trigger a behaviour that this caller does not expect at all?

Thanks.