Re: [PATCHv2 4/5] ok_to_remove_submodule: absorb the submodule git dir

Stefan Beller [off-list ref] writes:

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

On Wed, Dec 14, 2016 at 10:46 AM, Junio C Hamano [off-list ref] wrote:

Stefan Beller [off-list ref] writes:

quoted

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

Another approach would be to return more than yes/no but the reason
why it is a no. And then the caller would call absorb_git_dir_into_superproject.

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?

I don't know about the double check as I think we should really be sure before
deleting that repo, but absorb_git_dir_into_superproject would fail/die
if it would not actually absorb it, so maybe it is too much caution.

So I'd omit the double check in a reroll.

quoted

      argv_array_pushl(&cp.args, "status", "--porcelain",
                                 "--ignore-submodules=none", NULL);