Re: [PATCH 11/16] teach unpack_trees() to remove submodule contents

From: Stefan Beller <sbeller@google.com>
Date: Nov 18, 2016, 19:25 UTC
Message-ID: <CAGZ79kYBWa1yjdRK4GzFo8BtsGJVexxxh0vVW4K5-xYhG9ucEA@mail.gmail.com>
In-Reply-To: <20161117133538.GF39230@book.hvoigt.net>

On Thu, Nov 17, 2016 at 5:35 AM, Heiko Voigt <hvoigt@hvoigt.net> wrote:

Show 33 quoted lines

> On Tue, Nov 15, 2016 at 03:06:46PM -0800, Stefan Beller wrote:
>> Extend rmdir_or_warn() to remove the directories of those submodules which
>> are scheduled for removal. Also teach verify_clean_submodule() to check
>> that a submodule configured to be removed is not modified before scheduling
>> it for removal.
>>
>> Signed-off-by: Stefan Beller <sbeller@google.com>
>> ---
> [...]
>> --- a/wrapper.c
>> +++ b/wrapper.c
>> @@ -2,6 +2,7 @@
>>   * Various trivial helper wrappers around standard functions
>>   */
>>  #include "cache.h"
>> +#include "submodule.h"
>>
>>  static void do_nothing(size_t size)
>>  {
>> @@ -592,6 +593,9 @@ int unlink_or_warn(const char *file)
>>
>>  int rmdir_or_warn(const char *file)
>>  {
>> +     if (submodule_is_interesting(file, null_sha1)
>> +         && depopulate_submodule(file))
>> +             return -1;
>
> How about untracked files inside submodules? Should we not care about
> them? With this code the entire directory will be removed. In general
> git would not remove a directory with untracked files in it even though
> it is removed in the database. I wonder if we should imitate that here
> and just remove files that are tracked by git so that users do not loose
> files that might be precious to them.

Well from the superprojects perspective a submodule looks like a file, so if the checkout involved removing a file it had to either be clean or the checkout failed. So I though we'd go with a similar way here?

We need to stop when untracked files are present, I am not sure if we need to care if the files are ignored, though.

In this version of the series, the test added in patch 14, testing for treating untracked files properly is a failing test. So I agree that we should not just delete them; I'll fix that in the next version of the series.

Thanks, Stefan