Re: [PATCH 08/16] update submodules: add depopulate_submodule

From: Stefan Beller <sbeller@google.com>
Date: Nov 17, 2016, 22:28 UTC
Message-ID: <CAGZ79kZLqVg=FTiF+tOCSrqca_5zQoZHqGswJ0u+9u_kdmMaig@mail.gmail.com>
In-Reply-To: <20161117111337.GD39230@book.hvoigt.net>

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

Show 14 quoted lines

> On Tue, Nov 15, 2016 at 03:06:43PM -0800, Stefan Beller wrote:
>> diff --git a/cache.h b/cache.h
>> index a50a61a..65c47e4 100644
>> --- a/cache.h
>> +++ b/cache.h
>> @@ -2018,4 +2018,6 @@ void sleep_millisec(int millisec);
>>   */
>>  void safe_create_dir(const char *dir, int share);
>>
>> +void remove_subtree_or_die(const char *path);
>
> It seems that it is called remove_subtree already internally but can we
> maybe change that name? The term 'subtree' refers to something else[1] for
> me.

Doh, right.

> Maybe just: remove_directory() would make it also clear that there
> is no special internal git datatype meant by that but just a directory
> in the filesystem.

I'll go with `remove_directory_or_die` for now.

Show 22 quoted lines

>
>> +
>>  #endif /* CACHE_H */
>> diff --git a/entry.c b/entry.c
>> index c6eea24..019826b 100644
>> --- a/entry.c
>> +++ b/entry.c
>> @@ -73,6 +73,14 @@ static void remove_subtree(struct strbuf *path)
>>               die_errno("cannot rmdir '%s'", path->buf);
>>  }
>>
>> +void remove_subtree_or_die(const char *path)
>> +{
>> +     struct strbuf sb = STRBUF_INIT;
>> +     strbuf_addstr(&sb, path);
>> +     remove_subtree(&sb);
>> +     strbuf_release(&sb);
>> +}
>
> Why are you exposing it with const char * instead of strbuf? We get
> unnecessary conversions in case a caller already has a strbuf ready.
> Just in case later code also wants to use it.

For the cleanliness of API design. (I thought `void remove(char *dir)` is the closest approximation of `rm -rf $1`)

I'll use a strbuf instead.

Thanks! Stefan