Re: [PATCH] strbuf: add strbuf_add_real_path()
- From
- Brandon Williams <bmwill@google.com>
- Date
- Feb 27, 2017, 18:22 UTC
- Message-ID
- <20170227182217.GC153455@google.com>
- In-Reply-To
- <4d191b86-d36c-e3ec-99c6-d15baa6b659a@web.de>
On 02/25, René Scharfe wrote:
Show 10 quoted lines
> Add a function for appending the canonized absolute pathname of a given > path to a strbuf. It keeps the existing contents intact, as expected of > a function of the strbuf_add() family, while avoiding copying the result > if the given strbuf is empty. It's more consistent with the rest of the > strbuf API than strbuf_realpath(), which it's wrapping. > > Also add a semantic patch demonstrating its intended usage and apply it > to the current tree. Using strbuf_add_real_path() instead of calling > strbuf_addstr() and real_path() avoids an extra copy to a static buffer. >
Seems like a reasonable thing to do. When I wrote strbuf_realpath() I think I looked at the strbuf_getcwd() function for what it did (since it handled paths) and it simply uses the provided buffer disregarding what is already stored in it.
Show 52 quoted lines
> Signed-off-by: Rene Scharfe <l.s.r@web.de>
> ---
> contrib/coccinelle/strbuf.cocci | 6 ++++++
> setup.c | 2 +-
> strbuf.c | 11 +++++++++++
> strbuf.h | 14 ++++++++++++++
> 4 files changed, 32 insertions(+), 1 deletion(-)
>
> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci
> index 63995f22ff..1d580e49b0 100644
> --- a/contrib/coccinelle/strbuf.cocci
> +++ b/contrib/coccinelle/strbuf.cocci
> @@ -38,3 +38,9 @@ expression E1, E2, E3;
> @@
> - strbuf_addstr(E1, find_unique_abbrev(E2, E3));
> + strbuf_add_unique_abbrev(E1, E2, E3);
> +
> +@@
> +expression E1, E2;
> +@@
> +- strbuf_addstr(E1, real_path(E2));
> ++ strbuf_add_real_path(E1, E2);
> diff --git a/setup.c b/setup.c
> index 967f289f1e..f14cbcd338 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)
> if (!is_absolute_path(data.buf))
> strbuf_addf(&path, "%s/", gitdir);
> strbuf_addbuf(&path, &data);
> - strbuf_addstr(sb, real_path(path.buf));
> + strbuf_add_real_path(sb, path.buf);
> ret = 1;
> } else {
> strbuf_addstr(sb, gitdir);
> diff --git a/strbuf.c b/strbuf.cq
> index 8fec6579f7..ace58e7367 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const char *path)
> strbuf_addstr(sb, path);
> }
>
> +void strbuf_add_real_path(struct strbuf *sb, const char *path)
> +{
> + if (sb->len) {
> + struct strbuf resolved = STRBUF_INIT;
> + strbuf_realpath(&resolved, path, 1);
> + strbuf_addbuf(sb, &resolved);
> + strbuf_release(&resolved);
> + } else
> + strbuf_realpath(sb, path, 1);I know its not required but I would have braces on the 'else' branch since they were needed on the 'if' branch. But that's up to you and your style :)
Show 33 quoted lines
> +}
> +
> int printf_ln(const char *fmt, ...)
> {
> int ret;
> diff --git a/strbuf.h b/strbuf.h
> index cf1b5409e7..cf8e4bf532 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);
> */
> extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);
>
> +/**
> + * Canonize `path` (make it absolute, resolve symlinks, remove extra
> + * slashes) and append it to `sb`. Die with an informative error
> + * message if there is a problem.
> + *
> + * The directory part of `path` (i.e., everything up to the last
> + * dir_sep) must denote a valid, existing directory, but the last
> + * component need not exist.
> + *
> + * Callers that don't mind links should use the more lightweight
> + * strbuf_add_absolute_path() instead.
> + */
> +extern void strbuf_add_real_path(struct strbuf *sb, const char *path);
> +
>
> /**
> * Normalize in-place the path contained in the strbuf. See
> --
> 2.12.0
> -- Brandon Williams