Re: [PATCH v2 0/4] road to reentrant real_path

[Duy Nguyen]

On Sat, Dec 10, 2016 at 2:42 AM, Brandon Williams  wrote:
> On 12/09, Duy Nguyen wrote:
>> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams  wrote:
>> > diff --git a/setup.c b/setup.c
>> > index fe572b8..0d9fdd0 100644
>> > --- a/setup.c
>> > +++ b/setup.c
>> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const 
>> > char *gitdir)
>> > if (!is_absolute_path(data.buf))
>> > strbuf_addf(, "%s/", gitdir);
>> > strbuf_addbuf(, );
>> > -   strbuf_addstr(sb, real_path(path.buf));
>> > +   strbuf_realpath(sb, path.buf, 1);
>>
>> This is not the same because of this hunk in strbuf_realpath()
>
> Then perhaps I shouldn't make this change (and just leave it as is)
> since the way real_path_internal/strbuf_realpath is written requires
> that the strbuf being used for the resolved path only contains the
> resolved path (see the lstat(resolved->buf ) call).  Sidenote it
> looks like strbuf_getcwd() also does a reset, though more subtlety,
> since it just passes its buffer to getcwd().

Yeah that's ok too (I did not see this subtlety when I suggested the change).
-- 
Duy

Re: [PATCH v2 0/4] road to reentrant real_path

[Brandon Williams]

On 12/09, Duy Nguyen wrote:
> On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams  wrote:
> > diff --git a/setup.c b/setup.c
> > index fe572b8..0d9fdd0 100644
> > --- a/setup.c
> > +++ b/setup.c
> > @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const 
> > char *gitdir)
> > if (!is_absolute_path(data.buf))
> > strbuf_addf(, "%s/", gitdir);
> > strbuf_addbuf(, );
> > -   strbuf_addstr(sb, real_path(path.buf));
> > +   strbuf_realpath(sb, path.buf, 1);
> 
> This is not the same because of this hunk in strbuf_realpath()

Then perhaps I shouldn't make this change (and just leave it as is)
since the way real_path_internal/strbuf_realpath is written requires
that the strbuf being used for the resolved path only contains the
resolved path (see the lstat(resolved->buf ) call).  Sidenote it
looks like strbuf_getcwd() also does a reset, though more subtlety,
since it just passes its buffer to getcwd().

> 
> > @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, 
> > int die_on_error)
> > goto error_out;
> > }
> >
> > -   strbuf_reset();
> > +   strbuf_reset(resolved);
> >
> > if (is_absolute_path(path)) {
> 
> But if you you remove that, then all (old) callers of
> strbuf_realpath() must do a strbuf_reset() in advance if needed
> (probably just real_path does) which sounds reasonable to me. You're
> probably want to be careful about the strbuf_reset() at the end of the
> function too.
> 
> Other than that, I think this diff looks nice.
> -- 
> Duy

-- 
Brandon Williams

Re: [PATCH v2 0/4] road to reentrant real_path

[Duy Nguyen]

On Fri, Dec 9, 2016 at 6:58 AM, Brandon Williams  wrote:
> diff --git a/setup.c b/setup.c
> index fe572b8..0d9fdd0 100644
> --- a/setup.c
> +++ b/setup.c
> @@ -254,10 +254,12 @@ int get_common_dir_noenv(struct strbuf *sb, const char 
> *gitdir)
> if (!is_absolute_path(data.buf))
> strbuf_addf(, "%s/", gitdir);
> strbuf_addbuf(, );
> -   strbuf_addstr(sb, real_path(path.buf));
> +   strbuf_realpath(sb, path.buf, 1);

This is not the same because of this hunk in strbuf_realpath()

> @@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, 
> int die_on_error)
> goto error_out;
> }
>
> -   strbuf_reset();
> +   strbuf_reset(resolved);
>
> if (is_absolute_path(path)) {

But if you you remove that, then all (old) callers of
strbuf_realpath() must do a strbuf_reset() in advance if needed
(probably just real_path does) which sounds reasonable to me. You're
probably want to be careful about the strbuf_reset() at the end of the
function too.

Other than that, I think this diff looks nice.
-- 
Duy

[PATCH v2 0/4] road to reentrant real_path

[Brandon Williams]

Thanks for all of the comments on v1 of the series.  Hopefully this series
addresses the issues with windows and actually passes the first test :)

Some changes in v2:
* the 1st component of a path should now be handled correctly on windows as well
  as unix.
* Pushed the static strbuf to the callers of real_path_internal
* renamed real_path_internal to strbuf_realpath
* added real_pathdup
* migrated some callers of real_path to real_pathdup and strbuf_realpath

Brandon Williams (4):
  real_path: resolve symlinks by hand
  real_path: convert real_path_internal to strbuf_realpath
  real_path: create real_pathdup
  real_path: have callers use real_pathdup and strbuf_realpath

 abspath.c | 215 --
 builtin/init-db.c |   6 +-
 cache.h   |   3 +
 environment.c |   2 +-
 setup.c   |  15 ++--
 sha1_file.c   |   2 +-
 submodule.c   |   2 +-
 transport.c   |   2 +-
 worktree.c|   2 +-
 9 files changed, 163 insertions(+), 86 deletions(-)

--- interdiff from v1

diff --git a/abspath.c b/abspath.c
index 6f546e0..df37356 100644
--- a/abspath.c
+++ b/abspath.c
@@ -14,13 +14,13 @@ int is_directory(const char *path)
 /* removes the last path component from 'path' except if 'path' is root */
 static void strip_last_component(struct strbuf *path)
 {
-   if (path->len > 1) {
+   if (path->len > offset_1st_component(path->buf)) {
char *last_slash = find_last_dir_sep(path->buf);
strbuf_setlen(path, last_slash - path->buf);
}
 }
 
-/* gets the next component in 'remaining' and places it in 'next' */
+/* get (and remove) the next component in 'remaining' and place it in 'next' */
 static void get_next_component(struct strbuf *next, struct strbuf *remaining)
 {
char *start = NULL;
@@ -31,10 +31,10 @@ static void get_next_component(struct strbuf *next, struct 
strbuf *remaining)
/* look for the next component */
/* Skip sequences of multiple path-separators */
for (start = remaining->buf; is_dir_sep(*start); start++)
-   /* nothing */;
+   ; /* nothing */
/* Find end of the path component */
for (end = start; *end && !is_dir_sep(*end); end++)
-   /* nothing */;
+   ; /* nothing */
 
strbuf_add(next, start, end - start);
/* remove the component from 'remaining' */
@@ -48,21 +48,17 @@ static void get_next_component(struct strbuf *next, struct 
strbuf *remaining)
  * Return the real path (i.e., absolute path, with symlinks resolved
  * and extra slashes removed) equivalent to the specified path.  (If
  * you want an absolute path but don't mind links, use
- * absolute_path().)  The return value is a pointer to a static
- * buffer.
+ * absolute_path().)  Places the resolved realpath in the provided strbuf.
  *
  * 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.  If die_on_error is set, then die with an
  * informative error message if there is a problem.  Otherwise, return
  * NULL on errors (without generating any output).
- *
- * If path is our buffer, then return path, as it's already what the
- * user wants.
  */
-static const char *real_path_internal(const char *path, int die_on_error)
+char *strbuf_realpath(struct strbuf *resolved, const char *path,
+ int die_on_error)
 {
-   static struct strbuf resolved = STRBUF_INIT;
struct strbuf remaining = STRBUF_INIT;
struct strbuf next = STRBUF_INIT;
struct strbuf symlink = STRBUF_INIT;
@@ -70,10 +66,6 @@ static const char *real_path_internal(const char *path, int 
die_on_error)
int num_symlinks = 0;
struct stat st;
 
-   /* We've already done it */
-   if (path == resolved.buf)
-   return path;
-
if (!*path) {
if (die_on_error)
die("The empty string is not a valid path");
@@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int 
die_on_error)
goto error_out;
}
 
-   strbuf_reset();
+   strbuf_reset(resolved);
 
if (is_absolute_path(path)) {
/* absolute path; start with only root as being resolved */
-   strbuf_addch(, '/');
-   strbuf_addstr(, path + 1);
+   int offset = offset_1st_component(path);
+   strbuf_add(resolved, path, offset);
+   strbuf_addstr(, path + offset);
} else {
/* relative path; can use CWD as the initial resolved path */
-   if (strbuf_getcwd()) {
+   if (strbuf_getcwd(resolved)) {
if (die_on_error)
-   die_errno("Could not get current working 
directory");
+   die_errno("unable to get current working