{"thread":{"id":"45225","subject":"[PATCH] strbuf: add strbuf_add_real_path()","startedAt":"2017-02-25T16:01:18Z","lastAt":"2017-02-28T21:38:58Z","messageCount":5,"participants":["René Scharfe","Jeff King","Brandon Williams"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"312640","messageId":"4d191b86-d36c-e3ec-99c6-d15baa6b659a@web.de","threadId":"45225","inReplyTo":null,"subject":"[PATCH] strbuf: add strbuf_add_real_path()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-25T16:00:33Z","receivedAt":"2017-02-25T16:01:18Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function for appending the canonized absolute pathname of a given\npath to a strbuf.  It keeps the existing contents intact, as expected of\na function of the strbuf_add() family, while avoiding copying the result\nif the given strbuf is empty.  It's more consistent with the rest of the\nstrbuf API than strbuf_realpath(), which it's wrapping.\n\nAlso add a semantic patch demonstrating its intended usage and apply it\nto the current tree.  Using strbuf_add_real_path() instead of calling\nstrbuf_addstr() and real_path() avoids an extra copy to a static buffer.\n\nSigned-off-by: Rene Scharfe <l.s.r@web.de>\n---\n contrib/coccinelle/strbuf.cocci |  6 ++++++\n setup.c                         |  2 +-\n strbuf.c                        | 11 +++++++++++\n strbuf.h                        | 14 ++++++++++++++\n 4 files changed, 32 insertions(+), 1 deletion(-)\n\ndiff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci\nindex 63995f22ff..1d580e49b0 100644\n--- a/contrib/coccinelle/strbuf.cocci\n+++ b/contrib/coccinelle/strbuf.cocci\n@@ -38,3 +38,9 @@ expression E1, E2, E3;\n @@\n - strbuf_addstr(E1, find_unique_abbrev(E2, E3));\n + strbuf_add_unique_abbrev(E1, E2, E3);\n+\n+@@\n+expression E1, E2;\n+@@\n+- strbuf_addstr(E1, real_path(E2));\n++ strbuf_add_real_path(E1, E2);\ndiff --git a/setup.c b/setup.c\nindex 967f289f1e..f14cbcd338 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n \t\tif (!is_absolute_path(data.buf))\n \t\t\tstrbuf_addf(&path, \"%s/\", gitdir);\n \t\tstrbuf_addbuf(&path, &data);\n-\t\tstrbuf_addstr(sb, real_path(path.buf));\n+\t\tstrbuf_add_real_path(sb, path.buf);\n \t\tret = 1;\n \t} else {\n \t\tstrbuf_addstr(sb, gitdir);\ndiff --git a/strbuf.c b/strbuf.cq\nindex 8fec6579f7..ace58e7367 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const char *path)\n \tstrbuf_addstr(sb, path);\n }\n \n+void strbuf_add_real_path(struct strbuf *sb, const char *path)\n+{\n+\tif (sb->len) {\n+\t\tstruct strbuf resolved = STRBUF_INIT;\n+\t\tstrbuf_realpath(&resolved, path, 1);\n+\t\tstrbuf_addbuf(sb, &resolved);\n+\t\tstrbuf_release(&resolved);\n+\t} else\n+\t\tstrbuf_realpath(sb, path, 1);\n+}\n+\n int printf_ln(const char *fmt, ...)\n {\n \tint ret;\ndiff --git a/strbuf.h b/strbuf.h\nindex cf1b5409e7..cf8e4bf532 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);\n  */\n extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n \n+/**\n+ * Canonize `path` (make it absolute, resolve symlinks, remove extra\n+ * slashes) and append it to `sb`.  Die with an informative error\n+ * message if there is a problem.\n+ *\n+ * The directory part of `path` (i.e., everything up to the last\n+ * dir_sep) must denote a valid, existing directory, but the last\n+ * component need not exist.\n+ *\n+ * Callers that don't mind links should use the more lightweight\n+ * strbuf_add_absolute_path() instead.\n+ */\n+extern void strbuf_add_real_path(struct strbuf *sb, const char *path);\n+\n \n /**\n  * Normalize in-place the path contained in the strbuf. See\n-- \n2.12.0\n\n"},{"id":"312652","messageId":"20170225201123.ozrrfgim2rdctczv@sigill.intra.peff.net","threadId":"45225","inReplyTo":"4d191b86-d36c-e3ec-99c6-d15baa6b659a@web.de","subject":"Re: [PATCH] strbuf: add strbuf_add_real_path()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-02-25T20:11:23Z","receivedAt":"2017-02-25T20:13:13Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Feb 25, 2017 at 05:00:33PM +0100, René Scharfe wrote:\n\n> Add a function for appending the canonized absolute pathname of a given\n> path to a strbuf.  It keeps the existing contents intact, as expected of\n> a function of the strbuf_add() family, while avoiding copying the result\n> if the given strbuf is empty.  It's more consistent with the rest of the\n> strbuf API than strbuf_realpath(), which it's wrapping.\n> \n> Also add a semantic patch demonstrating its intended usage and apply it\n> to the current tree.  Using strbuf_add_real_path() instead of calling\n> strbuf_addstr() and real_path() avoids an extra copy to a static buffer.\n\nIt's also re-entrant, which real_path() is not.\n\n> +void strbuf_add_real_path(struct strbuf *sb, const char *path)\n> +{\n> +\tif (sb->len) {\n> +\t\tstruct strbuf resolved = STRBUF_INIT;\n> +\t\tstrbuf_realpath(&resolved, path, 1);\n> +\t\tstrbuf_addbuf(sb, &resolved);\n> +\t\tstrbuf_release(&resolved);\n> +\t} else\n> +\t\tstrbuf_realpath(sb, path, 1);\n> +}\n\nThe wrapping here seems a little backwards. If strbuf_add_real_path()\nwere the inner one, then we would not need this extra allocation. I know\nthat the reasons are historical, but I don't think it would be\nimpossible to teach the realpath code to do it.\n\nOTOH, it may not be worth the effort. It's not like strbuf_realpath()\ndoesn't allocate secondary strbufs for its work already, so dropping one\nmore is probably not that exciting. And certainly think your patch is an\nincremental improvement.\n\nOut of curiosity, I took a stab at the patch, which is below. Only\nlightly tested by me, and it does make the logic a bit more complicated\nto read, as you have to adjust for the original \"base\" in several\nplaces.\n\ndiff --git a/abspath.c b/abspath.c\nindex 2f0c26e0e..286072f48 100644\n--- a/abspath.c\n+++ b/abspath.c\n@@ -12,9 +12,9 @@ int is_directory(const char *path)\n }\n \n /* removes the last path component from 'path' except if 'path' is root */\n-static void strip_last_component(struct strbuf *path)\n+static void strip_last_component(struct strbuf *path, size_t base)\n {\n-\tsize_t offset = offset_1st_component(path->buf);\n+\tsize_t offset = base + offset_1st_component(path->buf + base);\n \tsize_t len = path->len;\n \n \t/* Find start of the last component */\n@@ -49,14 +49,15 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)\n }\n \n /* copies root part from remaining to resolved, canonicalizing it on the way */\n-static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)\n+static void get_root_part(struct strbuf *resolved, struct strbuf *remaining,\n+\t\t\t  size_t base)\n {\n \tint offset = offset_1st_component(remaining->buf);\n \n-\tstrbuf_reset(resolved);\n+\tstrbuf_setlen(resolved, base);\n \tstrbuf_add(resolved, remaining->buf, offset);\n #ifdef GIT_WINDOWS_NATIVE\n-\tconvert_slashes(resolved->buf);\n+\tconvert_slashes(resolved->buf + base);\n #endif\n \tstrbuf_remove(remaining, 0, offset);\n }\n@@ -78,8 +79,8 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining)\n  * informative error message if there is a problem.  Otherwise, return\n  * NULL on errors (without generating any output).\n  */\n-char *strbuf_realpath(struct strbuf *resolved, const char *path,\n-\t\t      int die_on_error)\n+char *strbuf_add_real_path(struct strbuf *resolved, const char *path,\n+\t\t\t   int die_on_error)\n {\n \tstruct strbuf remaining = STRBUF_INIT;\n \tstruct strbuf next = STRBUF_INIT;\n@@ -87,6 +88,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \tchar *retval = NULL;\n \tint num_symlinks = 0;\n \tstruct stat st;\n+\tsize_t base = resolved->len;\n \n \tif (!*path) {\n \t\tif (die_on_error)\n@@ -96,9 +98,9 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t}\n \n \tstrbuf_addstr(&remaining, path);\n-\tget_root_part(resolved, &remaining);\n+\tget_root_part(resolved, &remaining, base);\n \n-\tif (!resolved->len) {\n+\tif (resolved->len == base) {\n \t\t/* relative path; can use CWD as the initial resolved path */\n \t\tif (strbuf_getcwd(resolved)) {\n \t\t\tif (die_on_error)\n@@ -118,7 +120,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t\tcontinue; /* '.' component */\n \t\t} else if (next.len == 2 && !strcmp(next.buf, \"..\")) {\n \t\t\t/* '..' component; strip the last path component */\n-\t\t\tstrip_last_component(resolved);\n+\t\t\tstrip_last_component(resolved, base);\n \t\t\tcontinue;\n \t\t}\n \n@@ -127,12 +129,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t\tstrbuf_addch(resolved, '/');\n \t\tstrbuf_addbuf(resolved, &next);\n \n-\t\tif (lstat(resolved->buf, &st)) {\n+\t\tif (lstat(resolved->buf + base, &st)) {\n \t\t\t/* error out unless this was the last component */\n \t\t\tif (errno != ENOENT || remaining.len) {\n \t\t\t\tif (die_on_error)\n \t\t\t\t\tdie_errno(\"Invalid path '%s'\",\n-\t\t\t\t\t\t  resolved->buf);\n+\t\t\t\t\t\t  resolved->buf + base);\n \t\t\t\telse\n \t\t\t\t\tgoto error_out;\n \t\t\t}\n@@ -150,7 +152,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \t\t\t\t\tgoto error_out;\n \t\t\t}\n \n-\t\t\tlen = strbuf_readlink(&symlink, resolved->buf,\n+\t\t\tlen = strbuf_readlink(&symlink, resolved->buf + base,\n \t\t\t\t\t      st.st_size);\n \t\t\tif (len < 0) {\n \t\t\t\tif (die_on_error)\n@@ -162,14 +164,14 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \n \t\t\tif (is_absolute_path(symlink.buf)) {\n \t\t\t\t/* absolute symlink; set resolved to root */\n-\t\t\t\tget_root_part(resolved, &symlink);\n+\t\t\t\tget_root_part(resolved, &symlink, base);\n \t\t\t} else {\n \t\t\t\t/*\n \t\t\t\t * relative symlink\n \t\t\t\t * strip off the last component since it will\n \t\t\t\t * be replaced with the contents of the symlink\n \t\t\t\t */\n-\t\t\t\tstrip_last_component(resolved);\n+\t\t\t\tstrip_last_component(resolved, base);\n \t\t\t}\n \n \t\t\t/*\n@@ -202,6 +204,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,\n \treturn retval;\n }\n \n+char *strbuf_realpath(struct strbuf *sb, const char *path, int die_on_error)\n+{\n+\tstrbuf_reset(sb);\n+\treturn strbuf_add_real_path(sb, path, die_on_error);\n+}\n+\n const char *real_path(const char *path)\n {\n \tstatic struct strbuf realpath = STRBUF_INIT;\n"},{"id":"312749","messageId":"20170227182217.GC153455@google.com","threadId":"45225","inReplyTo":"4d191b86-d36c-e3ec-99c6-d15baa6b659a@web.de","subject":"Re: [PATCH] strbuf: add strbuf_add_real_path()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-27T18:22:17Z","receivedAt":"2017-02-27T18:23:16Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/25, René Scharfe wrote:\n> Add a function for appending the canonized absolute pathname of a given\n> path to a strbuf.  It keeps the existing contents intact, as expected of\n> a function of the strbuf_add() family, while avoiding copying the result\n> if the given strbuf is empty.  It's more consistent with the rest of the\n> strbuf API than strbuf_realpath(), which it's wrapping.\n> \n> Also add a semantic patch demonstrating its intended usage and apply it\n> to the current tree.  Using strbuf_add_real_path() instead of calling\n> strbuf_addstr() and real_path() avoids an extra copy to a static buffer.\n> \n\nSeems like a reasonable thing to do.  When I wrote strbuf_realpath() I\nthink I looked at the strbuf_getcwd() function for what it did (since it\nhandled paths) and it simply uses the provided buffer disregarding what\nis already stored in it.\n\n> Signed-off-by: Rene Scharfe <l.s.r@web.de>\n> ---\n>  contrib/coccinelle/strbuf.cocci |  6 ++++++\n>  setup.c                         |  2 +-\n>  strbuf.c                        | 11 +++++++++++\n>  strbuf.h                        | 14 ++++++++++++++\n>  4 files changed, 32 insertions(+), 1 deletion(-)\n> \n> diff --git a/contrib/coccinelle/strbuf.cocci b/contrib/coccinelle/strbuf.cocci\n> index 63995f22ff..1d580e49b0 100644\n> --- a/contrib/coccinelle/strbuf.cocci\n> +++ b/contrib/coccinelle/strbuf.cocci\n> @@ -38,3 +38,9 @@ expression E1, E2, E3;\n>  @@\n>  - strbuf_addstr(E1, find_unique_abbrev(E2, E3));\n>  + strbuf_add_unique_abbrev(E1, E2, E3);\n> +\n> +@@\n> +expression E1, E2;\n> +@@\n> +- strbuf_addstr(E1, real_path(E2));\n> ++ strbuf_add_real_path(E1, E2);\n> diff --git a/setup.c b/setup.c\n> index 967f289f1e..f14cbcd338 100644\n> --- a/setup.c\n> +++ b/setup.c\n> @@ -254,7 +254,7 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)\n>  \t\tif (!is_absolute_path(data.buf))\n>  \t\t\tstrbuf_addf(&path, \"%s/\", gitdir);\n>  \t\tstrbuf_addbuf(&path, &data);\n> -\t\tstrbuf_addstr(sb, real_path(path.buf));\n> +\t\tstrbuf_add_real_path(sb, path.buf);\n>  \t\tret = 1;\n>  \t} else {\n>  \t\tstrbuf_addstr(sb, gitdir);\n> diff --git a/strbuf.c b/strbuf.cq\n> index 8fec6579f7..ace58e7367 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -707,6 +707,17 @@ void strbuf_add_absolute_path(struct strbuf *sb, const char *path)\n>  \tstrbuf_addstr(sb, path);\n>  }\n>  \n> +void strbuf_add_real_path(struct strbuf *sb, const char *path)\n> +{\n> +\tif (sb->len) {\n> +\t\tstruct strbuf resolved = STRBUF_INIT;\n> +\t\tstrbuf_realpath(&resolved, path, 1);\n> +\t\tstrbuf_addbuf(sb, &resolved);\n> +\t\tstrbuf_release(&resolved);\n> +\t} else\n> +\t\tstrbuf_realpath(sb, path, 1);\n\nI know its not required but I would have braces on the 'else' branch\nsince they were needed on the 'if' branch.  But that's up to you and\nyour style :)\n\n> +}\n> +\n>  int printf_ln(const char *fmt, ...)\n>  {\n>  \tint ret;\n> diff --git a/strbuf.h b/strbuf.h\n> index cf1b5409e7..cf8e4bf532 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -441,6 +441,20 @@ extern int strbuf_getcwd(struct strbuf *sb);\n>   */\n>  extern void strbuf_add_absolute_path(struct strbuf *sb, const char *path);\n>  \n> +/**\n> + * Canonize `path` (make it absolute, resolve symlinks, remove extra\n> + * slashes) and append it to `sb`.  Die with an informative error\n> + * message if there is a problem.\n> + *\n> + * The directory part of `path` (i.e., everything up to the last\n> + * dir_sep) must denote a valid, existing directory, but the last\n> + * component need not exist.\n> + *\n> + * Callers that don't mind links should use the more lightweight\n> + * strbuf_add_absolute_path() instead.\n> + */\n> +extern void strbuf_add_real_path(struct strbuf *sb, const char *path);\n> +\n>  \n>  /**\n>   * Normalize in-place the path contained in the strbuf. See\n> -- \n> 2.12.0\n> \n\n-- \nBrandon Williams\n"},{"id":"312811","messageId":"3063b6fb-02aa-703c-0b56-300109cf054d@web.de","threadId":"45225","inReplyTo":"20170227182217.GC153455@google.com","subject":"Re: [PATCH] strbuf: add strbuf_add_real_path()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2017-02-27T22:45:50Z","receivedAt":"2017-02-28T01:15:19Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 27.02.2017 um 19:22 schrieb Brandon Williams:\n> On 02/25, René Scharfe wrote:\n>> +void strbuf_add_real_path(struct strbuf *sb, const char *path)\n>> +{\n>> +\tif (sb->len) {\n>> +\t\tstruct strbuf resolved = STRBUF_INIT;\n>> +\t\tstrbuf_realpath(&resolved, path, 1);\n>> +\t\tstrbuf_addbuf(sb, &resolved);\n>> +\t\tstrbuf_release(&resolved);\n>> +\t} else\n>> +\t\tstrbuf_realpath(sb, path, 1);\n>\n> I know its not required but I would have braces on the 'else' branch\n> since they were needed on the 'if' branch.  But that's up to you and\n> your style :)\n\nPersonally I'd actually prefer them as well, but the project's style has \ntraditionally been to avoid braces on such trailing single-line branches \nto save lines.  The CodingGuidelines for this topic have been clarified \nrecently, though, and seem to require them now.  Interesting.\n\nRené\n"},{"id":"312905","messageId":"20170228204209.GA110257@google.com","threadId":"45225","inReplyTo":"3063b6fb-02aa-703c-0b56-300109cf054d@web.de","subject":"Re: [PATCH] strbuf: add strbuf_add_real_path()","fromName":"Brandon Williams","fromEmail":"bmwill@google.com","sentAt":"2017-02-28T20:42:09Z","receivedAt":"2017-02-28T21:38:58Z","isPatch":true,"sender":{"key":"bwilliams.eng@gmail.com","avatar":null},"body":"On 02/27, René Scharfe wrote:\n> Am 27.02.2017 um 19:22 schrieb Brandon Williams:\n> >On 02/25, René Scharfe wrote:\n> >>+void strbuf_add_real_path(struct strbuf *sb, const char *path)\n> >>+{\n> >>+\tif (sb->len) {\n> >>+\t\tstruct strbuf resolved = STRBUF_INIT;\n> >>+\t\tstrbuf_realpath(&resolved, path, 1);\n> >>+\t\tstrbuf_addbuf(sb, &resolved);\n> >>+\t\tstrbuf_release(&resolved);\n> >>+\t} else\n> >>+\t\tstrbuf_realpath(sb, path, 1);\n> >\n> >I know its not required but I would have braces on the 'else' branch\n> >since they were needed on the 'if' branch.  But that's up to you and\n> >your style :)\n> \n> Personally I'd actually prefer them as well, but the project's style\n> has traditionally been to avoid braces on such trailing single-line\n> branches to save lines.  The CodingGuidelines for this topic have\n> been clarified recently, though, and seem to require them now.\n> Interesting.\n> \n> René\n\nHaving the project's guidelines align with your own preference makes\nthings a bit easier!\n\n-- \nBrandon Williams\n"}]}