Re: [PATCH] dir: avoid allocation in fill_directory()
- From
Duy Nguyen <pclouds@gmail.com>
- Date
- Feb 8, 2017, 06:22 UTC
- Message-ID
- <CACsJy8CE-cyTZHZZhvhdsNau7iSqBci1BdUqDYvxoE5odV2SBA@mail.gmail.com>
- In-Reply-To
- <73ec9cc7-8a86-8ba9-90fd-a954fa031820@web.de>
On Wed, Feb 8, 2017 at 5:04 AM, René Scharfe <l.s.r@web.de> wrote:
> Pass the match member of the first pathspec item directly to > read_directory() instead of using common_prefix() to duplicate it first, > thus avoiding memory duplication, strlen(3) and free(3).
How about killing common_prefix()? There are two other callers in ls-files.c and commit.c and it looks safe to do (but I didn't look very hard).
Show 20 quoted lines
> diff --git a/dir.c b/dir.c
> index 65c3e681b8..4541f9e146 100644
> --- a/dir.c
> +++ b/dir.c
> @@ -174,20 +174,19 @@ char *common_prefix(const struct pathspec *pathspec)
>
> int fill_directory(struct dir_struct *dir, const struct pathspec *pathspec)
> {
> - char *prefix;
> + const char *prefix;
> size_t prefix_len;
>
> /*
> * Calculate common prefix for the pathspec, and
> * use that to optimize the directory walk
> */
> - prefix = common_prefix(pathspec);
> - prefix_len = prefix ? strlen(prefix) : 0;
> + prefix_len = common_prefix_len(pathspec);
> + prefix = prefix_len ? pathspec->items[0].match : "";There's a subtle difference. Before the patch, prefix[prefix_len] is NUL. After the patch, it's not always true. If some code (incorrectly) depends on that, this patch exposes it. I had a look inside read_directory() though and it looks like no such code exists. So, all good.
Show 7 quoted lines
> > /* Read the directory and prune it */ > read_directory(dir, prefix, prefix_len, pathspec); > > - free(prefix); > return prefix_len; > }
-- Duy