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

[Junio C Hamano <gitster@pobox.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.

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