[3/5] sparse-index: use strbuf in path_found()

Commit Message

Derrick Stolee June 20, 2024, 4:11 p.m. UTC

From: Derrick Stolee <[e-mail]>

The path_found() method previously reused strings from the cache entries
the calling methods were using. This prevents string manipulation in
place and causes some odd reallocation before the final lstat() call in
the method.

Refactor the method to use strbufs and copy the path into the strbuf,
but also only the parent directory and not the whole path. This looks
like extra copying when assigning the path to the strbuf, but we save an
allocation by dropping the 'tmp' string, and we are "reusing" the copy
from 'tmp' to put the data in the strbuf.

Signed-off-by: Derrick Stolee <[e-mail]>
---
 sparse-index.c | 21 +++++++++------------
 1 file changed, 9 insertions(+), 12 deletions(-)

Comments

Elijah Newren June 24, 2024, 10:13 p.m. UTC | #1

On Thu, Jun 20, 2024 at 10:11 AM Derrick Stolee via GitGitGadget
<[e-mail]> wrote:
>
> From: Derrick Stolee <[e-mail]>
>
> The path_found() method previously reused strings from the cache entries
> the calling methods were using. This prevents string manipulation in
> place and causes some odd reallocation before the final lstat() call in
> the method.
>
> Refactor the method to use strbufs and copy the path into the strbuf,
> but also only the parent directory and not the whole path. This looks
> like extra copying when assigning the path to the strbuf, but we save an
> allocation by dropping the 'tmp' string, and we are "reusing" the copy
> from 'tmp' to put the data in the strbuf.
>
> Signed-off-by: Derrick Stolee <[e-mail]>
> ---
>  sparse-index.c | 21 +++++++++------------
>  1 file changed, 9 insertions(+), 12 deletions(-)
>
> diff --git a/sparse-index.c b/sparse-index.c
> index de6e727f5c1..fec4f393360 100644
> --- a/sparse-index.c
> +++ b/sparse-index.c
> @@ -440,31 +440,30 @@ void ensure_correct_sparsity(struct index_state *istate)
>  }
>
>  struct path_found_data {
> -       const char *dirname;
> -       size_t dir_len;
> +       struct strbuf dir;
>         int dir_found;
>  };
>
>  #define PATH_FOUND_DATA_INIT { \
> +       .dir = STRBUF_INIT, \
>         .dir_found = 1 \
>  }
>
>  static void clear_path_found_data(struct path_found_data *data)
>  {
> -       return;
> +       strbuf_release(&data->dir);
>  }
>
>  static int path_found(const char *path, struct path_found_data *data)
>  {
>         struct stat st;
>         char *newdir;
> -       char *tmp;
>
>         /*
>          * If dirname corresponds to a directory that doesn't exist, and this
>          * path starts with dirname, then path can't exist.
>          */
> -       if (!data->dir_found && !memcmp(path, data->dirname, data->dir_len))
> +       if (!data->dir_found && !memcmp(path, data->dir.buf, data->dir.len))
>                 return 0;
>
>         /*
> @@ -486,17 +485,15 @@ static int path_found(const char *path, struct path_found_data *data)
>          * If path starts with directory (which we already lstat'ed and found),
>          * then no need to lstat parent directory again.
>          */
> -       if (data->dir_found && data->dirname &&
> -           memcmp(path, data->dirname, data->dir_len))
> +       if (data->dir_found && data->dir.buf &&
> +           memcmp(path, data->dir.buf, data->dir.len))
>                 return 0;
>
>         /* Free previous dirname, and cache path's dirname */
> -       data->dirname = path;
> -       data->dir_len = newdir - path + 1;
> +       strbuf_reset(&data->dir);
> +       strbuf_add(&data->dir, path, newdir - path + 1);
>
> -       tmp = xstrndup(path, data->dir_len);
> -       data->dir_found = !lstat(tmp, &st);
> -       free(tmp);
> +       data->dir_found = !lstat(data->dir.buf, &st);
>
>         return 0;
>  }
> --
> gitgitgadget

Another simple translation; the series looks good so far...

Patch

diff --git a/sparse-index.c b/sparse-index.c
index de6e727f5c1..fec4f393360 100644
--- a/sparse-index.c
+++ b/sparse-index.c
@@ -440,31 +440,30 @@  void ensure_correct_sparsity(struct index_state *istate)
 }
 
 struct path_found_data {
-	const char *dirname;
-	size_t dir_len;
+	struct strbuf dir;
 	int dir_found;
 };
 
 #define PATH_FOUND_DATA_INIT { \
+	.dir = STRBUF_INIT, \
 	.dir_found = 1 \
 }
 
 static void clear_path_found_data(struct path_found_data *data)
 {
-	return;
+	strbuf_release(&data->dir);
 }
 
 static int path_found(const char *path, struct path_found_data *data)
 {
 	struct stat st;
 	char *newdir;
-	char *tmp;
 
 	/*
 	 * If dirname corresponds to a directory that doesn't exist, and this
 	 * path starts with dirname, then path can't exist.
 	 */
-	if (!data->dir_found && !memcmp(path, data->dirname, data->dir_len))
+	if (!data->dir_found && !memcmp(path, data->dir.buf, data->dir.len))
 		return 0;
 
 	/*
@@ -486,17 +485,15 @@  static int path_found(const char *path, struct path_found_data *data)
 	 * If path starts with directory (which we already lstat'ed and found),
 	 * then no need to lstat parent directory again.
 	 */
-	if (data->dir_found && data->dirname &&
-	    memcmp(path, data->dirname, data->dir_len))
+	if (data->dir_found && data->dir.buf &&
+	    memcmp(path, data->dir.buf, data->dir.len))
 		return 0;
 
 	/* Free previous dirname, and cache path's dirname */
-	data->dirname = path;
-	data->dir_len = newdir - path + 1;
+	strbuf_reset(&data->dir);
+	strbuf_add(&data->dir, path, newdir - path + 1);
 
-	tmp = xstrndup(path, data->dir_len);
-	data->dir_found = !lstat(tmp, &st);
-	free(tmp);
+	data->dir_found = !lstat(data->dir.buf, &st);
 
 	return 0;
 }