Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Dec 14, 2016, 18:46 UTC
- Message-ID
- <xmqqbmwexs9x.fsf@gitster.mtv.corp.google.com>
- In-Reply-To
- <20161213205622.841-5-sbeller@google.com>
Stefan Beller <sbeller@google.com> writes:
Show 38 quoted lines
> It is a major reason to say no, when deciding if a submodule can be
> deleted, if the git directory of the submodule being contained in the
> submodule's working directory.
>
> Migrate the git directory into the superproject instead of failing,
> and proceed with the other checks.
>
> Signed-off-by: Stefan Beller <sbeller@google.com>
> ---
> submodule.c | 15 +++++++++++++--
> 1 file changed, 13 insertions(+), 2 deletions(-)
>
> diff --git a/submodule.c b/submodule.c
> index 2d13744b06..e42efa2337 100644
> --- a/submodule.c
> +++ b/submodule.c
> @@ -1026,11 +1026,22 @@ int ok_to_remove_submodule(const char *path, unsigned flags)
> struct strbuf buf = STRBUF_INIT;
> int ok_to_remove = 1;
>
> + /* Is it there? */
> if (!file_exists(path) || is_empty_dir(path))
> return 1;
>
> - if (!submodule_uses_gitfile(path))
> - return 0;
> + /* Does it have a .git directory? */
> + if (!submodule_uses_gitfile(path)) {
> + absorb_git_dir_into_superproject("", path,
> + ABSORB_GITDIR_RECURSE_SUBMODULES);
> +
> + /*
> + * We should be using a gitfile by now. Let's double
> + * check as losing the git dir would be fatal.
> + */
> + if (!submodule_uses_gitfile(path))
> + return 0;
> + }It feels a bit funny for a function that answers yes/no question to actually have a side effect, but probably it is OK. It feels dirty, but I dunno.
A brief reading of the above makes us wonder what should happen if the absorb_git_dir_into_superproject() call fails, but a separate "submodule_uses_gitfile()" is needed to see if it failed? Perhaps the function needs to tell the caller if it succeeded?
> > argv_array_pushl(&cp.args, "status", "--porcelain", > "--ignore-submodules=none", NULL);