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

From: Stefan Beller <sbeller@google.com>
Date: Dec 14, 2016, 18:52 UTC
Message-ID: <CAGZ79kZ=PqTKt5xDobnR5oqwz84i10ZobmQb6QqeA3aSkbrd8Q@mail.gmail.com>
In-Reply-To: <xmqqbmwexs9x.fsf@gitster.mtv.corp.google.com>

On Wed, Dec 14, 2016 at 10:46 AM, Junio C Hamano <gitster@pobox.com> wrote:

Show 44 quoted lines

> Stefan Beller <sbeller@google.com> writes:
>
>> 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.

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.

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