Re: [PATCH 3/3] clean: improve performance when removing lots of directories
On Tue, Apr 7, 2015 at 12:10 AM, Eric Sunshine <sunshine@sunshineco.com> wrote:
Show 52 quoted lines
> On Mon, Apr 6, 2015 at 7:48 AM, Erik Elfström <erik.elfstrom@gmail.com> wrote:
>> Before this change, clean used resolve_gitlink_ref to check for the
>> presence of nested git repositories. This had the drawback of creating
>> a ref_cache entry for every directory that should potentially be
>> cleaned. The linear search through the ref_cache list caused a massive
>> performance hit for large number of directories.
>>
>> Teach clean.c:remove_dirs to use setup.c:is_git_directory
>> instead. is_git_directory will actually open HEAD and parse the HEAD
>> ref but this implies a nested git repository and should be rare when
>> cleaning.
>>
>> Using is_git_directory should give a more standardized check for what
>> is and what isn't a git repository but also gives a slight behavioral
>> change. We will now detect and respect bare and empty nested git
>> repositories (only init run). Update t7300 to reflect this.
>>
>> The time to clean an untracked directory containing 100000 sub
>> directories went from 61s to 1.7s after this change.
>
> Impressive.
>
>> Signed-off-by: Erik Elfström <erik.elfstrom@gmail.com>
>> Helped-by: Jeff King <peff@peff.net>
>
> It is customary for your sign-off to be last.
>
> More below...
>
>> ---
>> diff --git a/builtin/clean.c b/builtin/clean.c
>> index 98c103f..e951bd9 100644
>> --- a/builtin/clean.c
>> +++ b/builtin/clean.c
>> @@ -148,6 +147,24 @@ static int exclude_cb(const struct option *opt, const char *arg, int unset)
>> return 0;
>> }
>>
>> +static int is_git_repository(struct strbuf *path)
>> +{
>> + int ret = 0;
>> + if (is_git_directory(path->buf))
>> + ret = 1;
>> + else {
>> + int orig_path_len = path->len;
>> + if (path->buf[orig_path_len - 1] != '/')
>
> Minor: I don't know how others feel about it, but I always find it a
> bit disturbing to see a potential negative array access without a
> safety check that orig_path_len is not 0, either directly in the
> conditional or as a documenting assert().
>I think I would prefer to accept empty input and return false rather than assert. What to you think about:
static int is_git_repository(struct strbuf *path)
{
int ret = 0;
size_t orig_path_len = path->len;
if (orig_path_len == 0)
ret = 0;
else if (is_git_directory(path->buf))
ret = 1;
else {
if (path->buf[orig_path_len - 1] != '/')
strbuf_addch(path, '/');
strbuf_addstr(path, ".git");
if (is_git_directory(path->buf))
ret = 1;
strbuf_setlen(path, orig_path_len);
} return ret;
}
Also I borrowed this pattern from remove_dirs and it has the same problem. Should I add something like this as a separate commit?
Show changes to builtin/clean.c +3 −1
diff --git a/builtin/clean.c b/builtin/clean.c
index ccffd8a..88850e3 100644
--- a/builtin/clean.c
+++ b/builtin/clean.c
@@ -173,7 +173,8 @@ static int remove_dirs(struct strbuf *path, const
char *prefix, int force_flag,
DIR *dir;
struct strbuf quoted = STRBUF_INIT;
struct dirent *e;
- int res = 0, ret = 0, gone = 1, original_len = path->len, len;
+ int res = 0, ret = 0, gone = 1;
+ size_t original_len = path->len, len;
struct string_list dels = STRING_LIST_INIT_DUP;
*dir_gone = 1;
@@ -201,6 +202,7 @@ static int remove_dirs(struct strbuf *path, const
char *prefix, int force_flag,
return res;
}
+ assert(original_len > 0 && "expects non-empty path");
if (path->buf[original_len - 1] != '/')
strbuf_addch(path, '/');
>> + strbuf_addch(path, '/');
>> + strbuf_addstr(path, ".git");
>> + if (is_git_directory(path->buf))
>> + ret = 1;
>> + strbuf_setlen(path, orig_path_len);
>> + }
>> +
>> + return ret;
>> +}
>> +
>> static int remove_dirs(struct strbuf *path, const char *prefix, int force_flag,
>> int dry_run, int quiet, int *dir_gone)
>> {
Erik Elfström
