making real_path thread-safe
PATCH 1/1 making real_path thread-safe
The goal of this patch is to make the function real_path thread-safe. To do, the current logic which uses chdir() to do the symlink resolution needed to be rewritten to perform the symlink resolution by hand.
I wanted to get the bulk of the work out there for review quickly so technically this patch doesn't make real_path thread safe yet as it still uses a static buffer to serve up the resultant path. On a future iteration I plan on removing this and pushing ownership of the returned string to the callers.
For more information on why this change is desirable you can see the following threads:
https://public-inbox.org/git/[email protected]/
https://public-inbox.org/git/[email protected]/
Brandon Williams (1):
real_path: make real_path thread-safe
abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 61 deletions(-)
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH 1/1 real_path: make real_path thread-safe
The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls to chdir() can be removed, making real_path reentrant.
--- abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 61 deletions(-)
abspath.c +122 -61
@@ -11,8 +11,38 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode)); } +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >1 ) { +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' */ +static void get_next_component (struct strbuf *next,struct strbuf *remaining) +{ +char *start =NULL ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) +/* nothing */ ; +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) +/* nothing */ ; + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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
@@ -33,22 +62,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else { - last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +strbuf_addch (&resolved,'/' ); +strbuf_addstr (&remaining, path +1 ); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("Could not get current working directory" ); +else + goto error_out; + } +strbuf_addstr (&remaining, path); + } + +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ; } -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* append the next component and resolve resultant path */ +if (resolved.buf[resolved.len -1 ] !='/' ) +strbuf_addch (&resolved,'/' ); +strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); -if (chdir (sb.buf)) { +if (num_symlinks++ >MAXSYMLINKS ) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } - -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb); -strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* +* absolute symlink +* reset resolved path to root +*/ +strbuf_setlen (&resolved,1 ); + }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +//strbuf_release(&resolved); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH] real_path: make real_path thread-safe
Quoted 7 lines · Brandon Williams
The current implementation of
real_pathuseschdir()in order to resolve symlinks. Unfortunately this isn't thread-safe aschdir()affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls tochdir()can be removed, makingreal_pathreentrant.Signed-off-by Brandon Williams <[email protected]>
Thanks for working on this, some comments below:
abspath.c int is_directory(const char *path) · line 18
Quoted 6 lines
abspath.c +5
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >1 ) { +char *last_slash =find_last_dir_sep (path->buf);
What happens when there is no dir_sep?
abspath.c int is_directory(const char *path) · line 25
Quoted 3 lines
abspath.c +3
-0+/* gets the next component in 'remaining' and places it in 'next' */ +static void get_next_component (struct strbuf *next,struct strbuf *remaining) +{
It's more than just getting it, it also chops it off of 'remaining' ?
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 89
Quoted 6 lines
abspath.c +6
-0+strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +strbuf_addch (&resolved,'/' ); +strbuf_addstr (&remaining, path +1 );
This is where we would wait for input of Windows savy people.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 94
Quoted 5 lines
abspath.c +5
-0+ }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("Could not get current working directory" );
I am looking at xgetcwd, which words it slightly differently.
if (strbuf_getcwd(&sb))
die_errno(_("unable to get current working directory"));Not sure if aligning them would be a good idea?
Going by "git grep die_errno" as well as our Coding guidelines, we don't want to see capitalized error messages.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 106
Quoted 3 lines
abspath.c +3
-0+ +if (next.len ==0 ) { +continue ;/* empty component */
which means we resolve over path//with//double//slashes just fine, as well as /./ parts. :)
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 115
Quoted 5 lines
}abspath.c +1
-2-if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* append the next component and resolve resultant path */
"resultant" indicates you have a math background. :) But I had to look it up, I guess it is fine that way, though "resulting" may cause less mental friction for non native speakers.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 129
Quoted 9 lines
abspath.c +4
-1+if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) {
As far as I can tell, we could keep the symlink strbuf at a smaller scope here? (I was surprised how many strbufs are declared at the beginning of the function)
Quoted 1 line
+ //strbuf_release(&resolved);
This is why the cover letter toned down expectations ? (no // as comment, maybe remove that line?)
Thanks,
Stefan
Re: [PATCH] real_path: make real_path thread-safe
Quoted 7 lines · Stefan Beller
abspath.c int is_directory(const char *path) · line 18
Quoted 5 lines
abspath.c +5
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >1 ) { +char *last_slash =find_last_dir_sep (path->buf);What happens when there is no
dir_sep?
There should always be a dir_sep since that only gets run if the passed in path at least contains root '/'
Quoted 6 lines
abspath.c int is_directory(const char *path) · line 25
Quoted 4 lines
abspath.c +3
-0+/* gets the next component in 'remaining' and places it in 'next' */ +static void get_next_component (struct strbuf *next,struct strbuf *remaining) +{It's more than just getting it, it also chops it off of 'remaining' ?
True, I can update the comment to reflect that.
Quoted 8 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 94
Quoted 5 lines
abspath.c +5
-0+ }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("Could not get current working directory" );I am looking at xgetcwd, which words it slightly differently.
Quoted 7 lines
if (strbuf_getcwd(&sb)) die_errno(_("unable to get current working directory"));Not sure if aligning them would be a good idea?
Going by "git grep
die_errno" as well as our Coding guidelines, we don't want to see capitalized error messages.
K I can use the other msg.
Quoted 21 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 115
Quoted 4 lines
abspath.c +1
-2-if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* append the next component and resolve resultant path */ "resultant" indicates you have a math background. :) But I had to look it up, I guess it is fine that way, though "resulting" may cause less mental friction for non native speakers.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 129
Quoted 9 lines
abspath.c +4
-1+if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) {
Quoted 3 lines
As far as I can tell, we could keep the symlink strbuf at a smaller scope here? (I was surprised how many strbufs are declared at the beginning of the function)
Yeah I can push it down in scope. There will be a bit more allocation churn with the smaller scope but multiple symlinks should be rare? Alternatively the 'next' buffer can be reused...I decided against that initially due to readability. And yes, lots of string manipulation requires lots of strbufs :)
Quoted 4 lines
Quoted 1 line
+ //strbuf_release(&resolved);
This is why the cover letter toned down expectations ? (no // as comment, maybe remove that line?)
yep. It will be added back in though once the callers to real_path take ownership of the memory.
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
Quoted 7 lines · Brandon Williams
Quoted 4 lines
abspath.c int is_directory(const char *path) · line 18
Quoted 2 lines
abspath.c +2
-0+if (path->len >1 ) { +char *last_slash =find_last_dir_sep (path->buf);What happens when there is no
dir_sep?There should always be a
dir_sepsince that only gets run if the passed in path at least contains root '/'
Oh, sure, that makes sense. When porting/running this on Windows, does the assumption still hold?
Quoted 9 lines
Quoted 7 lines
if (strbuf_getcwd(&sb)) die_errno(_("unable to get current working directory"));Not sure if aligning them would be a good idea?
Going by "git grep
die_errno" as well as our Coding guidelines, we don't want to see capitalized error messages.K I can use the other msg.
Well this wasn't a rhetorical question, but I was genuine wondering if that was worth it.
When having different error messages in different places, it makes debugging easier, because you have fewer starting points.
But this function is deep down in the stack, such that you would expect other error messages to also show up , so I dunno.
Quoted 10 lines
Quoted 5 lines
Quoted 1 line
+ } else if (
S_ISLNK(st.st_mode)) {As far as I can tell, we could keep the symlink strbuf at a smaller scope here? (I was surprised how many strbufs are declared at the beginning of the function)
Yeah I can push it down in scope. There will be a bit more allocation churn with the smaller scope but multiple symlinks should be rare? Alternatively the 'next' buffer can be reused...I decided against that initially due to readability.
I'd second to not reuse 'next'. :)
I guess we could keep the less churn-y version then.
Quoted 2 lines
And yes, lots of string manipulation
requires lots of strbufs :)
Re: [PATCH] real_path: make real_path thread-safe
In reply to Brandon Williamsabspath.c int is_directory(const char *path) · line 65
Quoted 4 lines
static const char *real_path_internal(const char *path, int
die_on_error) {abspath.c +1
-1-static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ;
Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
abspath.c int is_directory(const char *path) · line 75
Quoted 4 lines
/* We've already done it */abspath.c +1
-1-if (path == sb.buf) +if (path == resolved.buf)return path;
I wonder how often we run into this case; are there some callers explicitly relying on real_path_internal being cheap for repeated calls? (Maybe run the test suite with this early return instrumented? Not sure how to assess the impact of removing the cheap out return optimization)
The long tail (i.e. the actual functionality) should actually be faster, I'd imagine
as we do less than with using chdir.
Thanks,
Stefan
Re: [PATCH] real_path: make real_path thread-safe
Quoted 14 lines · Stefan Beller
abspath.c int is_directory(const char *path) · line 65
Quoted 4 lines
static const char *real_path_internal(const char *path, int
die_on_error) {abspath.c +1
-1-static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ;Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
abspath.c int is_directory(const char *path) · line 75
Quoted 4 lines
/* We've already done it */abspath.c +1
-1-if (path == sb.buf) +if (path == resolved.buf)return path;
Quoted 8 lines
I wonder how often we run into this case; are there some callers explicitly relying on
real_path_internalbeing cheap for repeated calls? (Maybe run the test suite with this early return instrumented? Not sure how to assess the impact of removing the cheap out return optimization)The long tail (i.e. the actual functionality) should actually be faster, I'd imagine
as we do less than with using chdir.
Depends on how expensive the chdir calls were. And I'm working to get rid of the static buffer. Just need have the callers own the memory first.
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
Quoted 20 lines · Brandon Williams
Quoted 19 lines · Stefan Beller
abspath.c int is_directory(const char *path) · line 65
Quoted 4 lines
static const char *real_path_internal(const char *path, int
die_on_error) {abspath.c +1
-1-static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ;Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
abspath.c int is_directory(const char *path) · line 75
Quoted 4 lines
/* We've already done it */abspath.c +1
-1-if (path == sb.buf) +if (path == resolved.buf)return path;I wonder how often we run into this case; are there some callers explicitly relying on
real_path_internalbeing cheap for repeated calls? (Maybe run the test suite with this early return instrumented? Not sure how to assess the impact of removing the cheap out return optimization)
Quoted 7 lines
Quoted 3 lines
The long tail (i.e. the actual functionality) should actually be faster, I'd imagine
as we do less than with using chdir.Depends on how expensive the chdir calls were. And I'm working to get rid of the static buffer. Just need have the callers own the memory first.
I suggest you turn this real_path_internal into strbuf_real_path. In other words, it takes a strbuf and writes the result there, allocating memory if needed.
This function can replace the two strbuf_addstr(..., real_path(..)); we have in setup.c and sha1_file.c. real_path() can own a static strbuf buffer to retain current behavior. We could also have a new wrapper real_pathdup() around strbuf_real_path(), which can replace 9 instances of xstrdup(real_path(...)) (and Stefan is adding a few more; that's what led me back to these mails)
Signature · 1 line
Duy
Re: [PATCH] real_path: make real_path thread-safe
Quoted 33 lines · Duy Nguyen
Quoted 20 lines · Brandon Williams
Quoted 19 lines · Stefan Beller
abspath.c int is_directory(const char *path) · line 65
Quoted 4 lines
static const char *real_path_internal(const char *path, int
die_on_error) {abspath.c +1
-1-static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ;Also by having this static here, it is not quite thread safe, yet.
By removing the static here we cannot do the early cheap check as:
abspath.c int is_directory(const char *path) · line 75
Quoted 4 lines
/* We've already done it */abspath.c +1
-1-if (path == sb.buf) +if (path == resolved.buf)return path;I wonder how often we run into this case; are there some callers explicitly relying on
real_path_internalbeing cheap for repeated calls? (Maybe run the test suite with this early return instrumented? Not sure how to assess the impact of removing the cheap out return optimization)Quoted 7 lines
Quoted 3 lines
The long tail (i.e. the actual functionality) should actually be faster, I'd imagine
as we do less than with using chdir.Depends on how expensive the chdir calls were. And I'm working to get rid of the static buffer. Just need have the callers own the memory first.
I suggest you turn this
real_path_internalintostrbuf_real_path. In other words, it takes a strbuf and writes the result there, allocating memory if needed.
Quoted 6 lines
This function can replace the two
strbuf_addstr(..., real_path(..)); we have insetup.candsha1_file.c.real_path()can own a static strbuf buffer to retain current behavior. We could also have a new wrapperreal_pathdup()aroundstrbuf_real_path(), which can replace 9 instances of xstrdup(real_path(...)) (and Stefan is adding a few more; that's what led me back to these mails)
Sounds like a plan, thanks for the advice.
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
In reply to Brandon Williamsabspath.c int is_directory(const char *path) · line 21
Quoted 8 lines · Brandon Williams
abspath.c +8
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >1 ) { +char *last_slash =find_last_dir_sep (path->buf); +strbuf_setlen (path, last_slash - path->buf); + } +}
You use find_last_dir_sep() which takes care of "Windows uses backslash" issue. Is this function expected to be fed something like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or is that handled by a lot higher level up in the callchain? I am reacting the comparison of path->len and 1 here.
Also is the input expected to be normalized? Are we expected to be fed something like "/a//b/./c/../d/e" and react sensibly, or is that handled by a lot higher level up in the callchain?
abspath.c int is_directory(const char *path) · line 34
Quoted 12 lines
abspath.c +12
-0+/* gets the next component in 'remaining' and places it in 'next' */ +static void get_next_component (struct strbuf *next,struct strbuf *remaining) +{ +char *start =NULL ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) +/* nothing */ ;
Style:
; /* nothing */
abspath.c int is_directory(const char *path) · line 39
Quoted 5 lines
abspath.c +5
-0+/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) +/* nothing */ ; + +strbuf_add (next, start, end - start);
OK, so this was given "///foo/bar" in "remaining" and appended 'foo/' to "next". I.e. deduping of slashes is handled here.
POSIX cares about treating "//" at the very beginning of the path specially. Is that supposed to be handled here, or by a lot higher level up in the callchain?
Quoted 18 lines
abspath.c +5
-1+/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved @@ -21,7 +51,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * * 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
abspath.c int is_directory(const char *path) · line 65
Quoted 4 lines
abspath.c +1
-0@@ -33,22 +62,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { +static struct strbuf resolved =STRBUF_INIT ;
This being 'static' would probably mean that this is not reentrant, which goes against the title of the patch.
Re: [PATCH] real_path: make real_path thread-safe
Quoted 16 lines · Junio C Hamano
abspath.c int is_directory(const char *path) · line 21
Quoted 8 lines · Brandon Williams
abspath.c +8
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >1 ) { +char *last_slash =find_last_dir_sep (path->buf); +strbuf_setlen (path, last_slash - path->buf); + } +}You use
find_last_dir_sep()which takes care of "Windows uses backslash" issue. Is this function expected to be fed something like "C:\My Files\foo.txt" and more importantly "C:\My Files"? Or is that handled by a lot higher level up in the callchain? I am reacting the comparison ofpath->lenand 1 here.
This function should accept both absolute and relative paths, which means it should probably accept "C:\My Files". I wasn't thinking about windows 100% of the time while writing this so I'm hoping that a windows expert will point things like this out to me :). So in reality this check shouldn't be (path->len > 1) but rather some function is_only_root which would check if the strbuf only contains a string which looks like root.
Quoted 4 lines
Also is the input expected to be normalized? Are we expected to be fed something like "/a//b/./c/../d/e" and react sensibly, or is that handled by a lot higher level up in the callchain?
Yes it should handle paths like that sensibly.
Quoted 14 lines
abspath.c int is_directory(const char *path) · line 34
Quoted 12 lines
abspath.c +12
-0+/* gets the next component in 'remaining' and places it in 'next' */ +static void get_next_component (struct strbuf *next,struct strbuf *remaining) +{ +char *start =NULL ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) +/* nothing */ ;Style:
Quoted 1 line
; /* nothing */
k will fix.
Quoted 10 lines
abspath.c int is_directory(const char *path) · line 39
Quoted 6 lines
abspath.c +5
-0+/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) +/* nothing */ ; + +strbuf_add (next, start, end - start);OK, so this was given "///foo/bar" in "remaining" and appended 'foo/' to "next". I.e. deduping of slashes is handled here.
Quoted 3 lines
POSIX cares about treating "//" at the very beginning of the path specially. Is that supposed to be handled here, or by a lot higher level up in the callchain?
What exactly does "//" mean in this context? (I'm just naive in this area)
Quoted 18 lines
Quoted 18 lines
abspath.c +5
-1+/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved @@ -21,7 +51,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * * 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
Quoted 8 lines
abspath.c int is_directory(const char *path) · line 65
Quoted 5 lines
abspath.c +1
-0@@ -33,22 +62,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { +static struct strbuf resolved =STRBUF_INIT ;This being 'static' would probably mean that this is not reentrant, which goes against the title of the patch.
Yes I mentioned in the cover letter that this was the one last thing that needed to be changed to make it reentrant. I wanted to get this out for review sooner since this is the biggest change (getting rid of the chdir() calls) and dropping the static here would just require making the callers own the memory which should hopefully be an easy refactor. It just would have taken a bit more time to actually do that refactor now. I'm slowly working on it while this is being reviewed and could be ready to send out when rerolling this patch. Sorry for having a slightly misleading patch title :)
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
Quoted 7 lines · Brandon Williams
Quoted 3 lines · Junio C Hamano
POSIX cares about treating "//" at the very beginning of the path specially. Is that supposed to be handled here, or by a lot higher level up in the callchain?
What exactly does "//" mean in this context? (I'm just naive in this area)
This refers to a UNC path (ie Universal Naming Convention) which is used to refer to servers, printers and other 'network resources'. Although this started (many moons ago) in unix, it isn't used too much outside of windows networks! (where it is usually denoted by \\servername\path).
You can see the relics of unix UNC paths if you look at the wording
for basename() in the POSIX standard. Note the special treatment of
the path which 'is exactly "//"', see http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.html
ATB,
Ramsay Jones
Re: [PATCH] real_path: make real_path thread-safe
Quoted 17 lines · Ramsay Jones
Quoted 7 lines · Brandon Williams
Quoted 3 lines · Junio C Hamano
POSIX cares about treating "//" at the very beginning of the path specially. Is that supposed to be handled here, or by a lot higher level up in the callchain?
What exactly does "//" mean in this context? (I'm just naive in this area)
This refers to a UNC path (ie Universal Naming Convention) which is used to refer to servers, printers and other 'network resources'. Although this started (many moons ago) in unix, it isn't used too much outside of windows networks! (where it is usually denoted by \\servername\path).
Quoted 6 lines
You can see the relics of unix UNC paths if you look at the wording
forbasename()in the POSIX standard. Note the special treatment of
the path which 'is exactly "//"', see http://pubs.opengroup.org/onlinepubs/009695399/functions/basename.htmlATB,
Ramsay Jones
Please allow one more comment about UNC:
They are used under Windows, and typically wotk together with Git. One breakage between 2.10 and 2.11 has been observed, saying that pushing to \\SERVER\SHARE\DIRECTORY does not work any more.
It has been reported under
"git 2.11.0 error when pushing to remote located on a windows share"
both here and
here:
https://github.com/git-for-windows/git/issues/979#issuecomment-264816175
I don't have a Windows box at the moment, and I don't know if the breakage was introduced by changes in real_patyh().
But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1
and
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1
Re: [PATCH] real_path: make real_path thread-safe
Quoted 9 lines · Torsten Bögershausen
But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1and
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1
Additional questions that may be interesting are:
//A/B/../C is it //A/C? is it an error?
//A/B/../../C/D is it //C/D? is it an error?Re: [PATCH] real_path: make real_path thread-safe
Quoted 12 lines · Junio C Hamano
Quoted 9 lines · Torsten Bögershausen
But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1and
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1
Quoted 5 lines
Additional questions that may be interesting are:
//A/B/../C is it //A/C? is it an error? //A/B/../../C/D is it //C/D? is it an error?
Also is //.. the same as //? I would assume so since /.. is /
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
Quoted 8 lines · Brandon Williams
Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Torsten Bögershausen
But in any case it seems that e.g.
//SEFVER/SHARE/DIR1/DIR2/..
must be converted into
//SEFVER/SHARE/DIR1
Quoted 8 lines
Quoted 8 lines
Quoted 4 lines
and
\\SEFVER\SHARE\DIR1\DIR2\..
must be converted into
\\SEFVER\SHARE\DIR1Additional questions that may be interesting are:
//A/B/../C is it //A/C? is it an error?
Yes, at least under Windows.
If I have e.g. a Raspi with SAMBA, I can put a git Repository here:
//raspi/torsten/projects/git
If I use
git push //raspi/torsten/../junio/projects/git
that should be an error.
Quoted 2 lines
Quoted 2 lines
//A/B/../../C/D is it //C/D? is it an error?
Same for
git push /raspi/../raspi2/torsten//projects/git
Quoted 4 lines
Also is //.. the same as //? I would assume so since /.. is /
Under Windows //.. is simply illegal, I would say. The documentation here
https://msdn.microsoft.com/en-us/library/windows/desktop/aa365247(v=vs.85).aspx
Mentions these 2 examples, what to feed into the WIN32 file API:
a)
\\?\D:\very-long-path
b)
\\server\share\path\file"
c)
"\\?\UNC\server\share\path"
So whatever we do, the ".." resoltion is only allowed to look at "very-long-path" or "path".
Some conversion may be done in mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?
It seams that we may wnat a function get_start_of_path(uncpath), which returns:
get_start_of_path_win("//?/D:/very-long-path") "/very-long-path"
get_start_of_path_win("//server/share/path/file") "/path/file"
get_start_of_path_win("//?/UNC/server/share/path") "/path"(I don't know if we need the variant with '\', but is would'n hurt):get_start_of_path_win("\\\\?\\D:\\very-long-path") "\\very-long-path"get_start_of_path_win("\\\\server\\share\\path\\file") "\\path\\file"get_start_of_path_win("\\\\?\\UNC\\server\\share\\path") "\\path"
Then the non-windows version could simply returnget_start_of_path_non_win(something) something
Does this make sense ?
Re: [PATCH] real_path: make real_path thread-safe
Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
Quoted 3 lines
Some conversion may be done in
mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?
Only if needed, and there are not many places where this is the case. (Actually, I can't find one place where we do.) In particular, typical file accesses does not require the conversion.
Quoted 4 lines
It seams that we may wnat a function
get_start_of_path(uncpath), which returns:
get_start_of_path_win("//?/D:/very-long-path")"/very-long-path"
We have offset_1st_component().
-- Hannes
Re: [PATCH] real_path: make real_path thread-safe
Quoted 8 lines · Johannes Sixt
Am 08.12.2016 um 08:55 schrieb Torsten Bögershausen:
Quoted 3 lines
Some conversion may be done in
mingw.c:
https://github.com/github/git-msysgit/blob/master/compat/mingw.c
So what I understand, '/' in Git are already converted into '\' if needed ?Only if needed, and there are not many places where this is the case. (Actually, I can't find one place where we do.) In particular, typical file accesses does not require the conversion.
That's convenient, that way I don't have to worry about using '\' as a directory separator instead of '/'.
Quoted 7 lines
Quoted 5 lines
It seams that we may wnat a function
get_start_of_path(uncpath), which returns:
get_start_of_path_win("//?/D:/very-long-path")"/very-long-path"We have
offset_1st_component().
Thanks for letting me know this function exists, that makes my job a bit easier.
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
In reply to Brandon WilliamsAm 07.12.2016 um 01:10 schrieb Brandon Williams:
Quoted 4 lines
This function should accept both absolute and relative paths, which means it should probably accept "C:\My Files". I wasn't thinking about windows 100% of the time while writing this so I'm hoping that a windows expert will point things like this out to me :).
;)
With this patch, the test suite fails at the very first git init call:
D:\Src\mingw-git\t>sh t0000-basic.sh -v -i -x
fatal: Invalid path '/:': No such file or directory
error: cannot run git init -- have you built things yet?
FATAL: Unexpected exit with code 1
I haven't dug further, yet.
-- Hannes
Re: [PATCH] real_path: make real_path thread-safe
Quoted 10 lines · Johannes Sixt
Am 07.12.2016 um 01:10 schrieb Brandon Williams:
Quoted 4 lines
This function should accept both absolute and relative paths, which means it should probably accept "C:\My Files". I wasn't thinking about windows 100% of the time while writing this so I'm hoping that a windows expert will point things like this out to me :).
;)
With this patch, the test suite fails at the very first git init call:
Quoted 9 lines
D:\Src\mingw-git\t>sh
t0000-basic.sh-v -i -x
fatal: Invalid path '/:': No such file or directory
error: cannot run git init -- have you built things yet?
FATAL: Unexpected exit with code 1I haven't dug further, yet.
-- Hannes
Thanks for providing me with the error. Instead of assuming root is "/" I'll need to extract what root is from an absolute path. Aside from what root looks like, do most other path constructs behave similarly in unix and windows? (like ".." and "." as examples)
Since I don't really have a windows machine to test things it might be slightly difficult to get everything correct quickly but hopefully we can get this working :)
Signature · 1 line
Brandon Williams
Re: [PATCH] real_path: make real_path thread-safe
Am 07.12.2016 um 23:29 schrieb Brandon Williams:
Quoted 4 lines
Instead of assuming root is "/"
I'll need to extract what root is from an absolute path. Aside from what root looks like, do most other path constructs behave similarly in unix and windows? (like ".." and "." as examples)
Yes, .. and . work the same way, except that they cannot appear in the \\server\share part. I also think that .. does not cancel these parts.
As long as you use is_absolute_path() and do not simplify path components before offset_1st_component(), you should be on the safe side.
Quoted 3 lines
Since I don't really have a windows machine to test things it might be slightly difficult to get everything correct quickly but hopefully we can get this working :)
I'll lend a hand, of course, as time permits.
-- Hannes
Re: [PATCH] real_path: make real_path thread-safe
Quoted 13 lines · Johannes Sixt
Am 07.12.2016 um 23:29 schrieb Brandon Williams:
Quoted 4 lines
Instead of assuming root is "/"
I'll need to extract what root is from an absolute path. Aside from what root looks like, do most other path constructs behave similarly in unix and windows? (like ".." and "." as examples)Yes, .. and . work the same way, except that they cannot appear in the \\server\share part. I also think that .. does not cancel these parts.
As long as you use
is_absolute_path()and do not simplify path components beforeoffset_1st_component(), you should be on the safe side.
Quoted 5 lines
Quoted 3 lines
Since I don't really have a windows machine to test things it might be slightly difficult to get everything correct quickly but hopefully we can get this working :)
I'll lend a hand, of course, as time permits.
Thanks, as always, for working well together ;-).
v2 cover road to reentrant real_path
In reply to Brandon WilliamsThanks 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
abspath.c +48 -38
@@ -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 (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {/* absolute path; start with only root as being resolved */ -strbuf_addch (&resolved,'/' ); -strbuf_addstr (&remaining, path +1 ); +int offset =offset_1st_component (path); +strbuf_add (resolved, path, offset); +strbuf_addstr (&remaining, path + offset); }else {/* relative path; can use CWD as the initial resolved path */ -if (strbuf_getcwd (&resolved)) { +if (strbuf_getcwd (resolved)) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("unable to get current working directory" );else goto error_out; }
@@ -108,21 +101,21 @@ static const char *real_path_internal(const char *path, int die_on_error)continue ;/* '.' component */ }else if (next.len ==2 && !strcmp (next.buf,".." )) {/* '..' component; strip the last path component */ -strip_last_component (&resolved); +strip_last_component (resolved);continue ; }/* append the next component and resolve resultant path */ -if (resolved.buf[resolved.len -1 ] !='/' ) -strbuf_addch (&resolved,'/' ); -strbuf_addbuf (&resolved, &next); +if (!is_dir_sep (resolved->buf[resolved->len -1 ])) +strbuf_addch (resolved,'/' ); +strbuf_addbuf (resolved, &next); -if (lstat (resolved.buf, &st)) { +if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error)die_errno ("Invalid path '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -138,29 +131,29 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } - len =strbuf_readlink (&symlink, resolved.buf, + len =strbuf_readlink (&symlink, resolved->buf, st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }if (is_absolute_path (symlink.buf)) { -/* -* absolute symlink -* reset resolved path to root -*/ -strbuf_setlen (&resolved,1 ); +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (resolved); +strbuf_add (resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset); }else {/* * relative symlink * strip off the last component since it will * be replaced with the contents of the symlink */ -strip_last_component (&resolved); +strip_last_component (resolved); }/*
@@ -180,25 +173,42 @@ static const char *real_path_internal(const char *path, int die_on_error) } } - retval = resolved.buf; + retval = resolved->buf; error_out: -//strbuf_release(&resolved); strbuf_release (&remaining);strbuf_release (&next);strbuf_release (&symlink); +if (!retval) +strbuf_reset (resolved); +return retval; }const char *real_path (const char *path) { -return real_path_internal (path,1 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,1 ); }const char *real_path_if_valid (const char *path) { -return real_path_internal (path,0 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,0 ); +} + +char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; }/*
builtin/init-db.c +3 -3
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir, {int reinit;int exist_ok = flags &INIT_DB_EXIST_OK ; -char *original_git_dir =xstrdup (real_path (git_dir)); +char *original_git_dir =real_pathdup (git_dir);if (real_git_dir) {struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 );if (real_git_dir && !is_absolute_path (real_git_dir)) - real_git_dir =xstrdup (real_path (real_git_dir)); + real_git_dir =real_pathdup (real_git_dir);if (argc ==1 ) {int mkdir_tried =0 ;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir); - git_work_tree_cfg =xstrdup (real_path (rel)); + git_work_tree_cfg =real_pathdup (rel);free (rel); }if (!git_work_tree_cfg)
cache.h +3 -0
@@ -1064,8 +1064,11 @@ static inline int is_absolute_path(const char *path)return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); }int is_directory (const char *); +char *strbuf_realpath (struct strbuf *resolved,const char *path, +int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
environment.c +1 -1
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)return ; } git_work_tree_initialized =1 ; - work_tree =xstrdup (real_path (new_work_tree)); + work_tree =real_pathdup (new_work_tree); }const char *get_git_work_tree (void )
setup.c +9 -6
@@ -254,10 +254,12 @@ 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_realpath (sb, path.buf,1 ); ret =1 ; - }else + }else {strbuf_addstr (sb, gitdir); + } +strbuf_release (&data);strbuf_release (&path);return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* --work-tree is set without --git-dir; use discovered one */ if (getenv (GIT_WORK_TREE_ENVIRONMENT ) || git_work_tree_cfg) {if (offset != cwd->len && !is_absolute_path (gitdir)) - gitdir =xstrdup (real_path (gitdir)); + gitdir =real_pathdup (gitdir);if (chdir (cwd->buf))die_errno ("Could not come back to cwd" );return setup_explicit_git_dir (gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,/* Keep entry but do not canonicalize it */ return 1 ; }else { -const char *real_path =real_path_if_valid (ceil); -if (!real_path) +char *real_path =real_pathdup (ceil); +if (!real_path) {return 0 ; + }free (item->string); - item->string =xstrdup (real_path); + item->string = real_path;return 1 ; } }
sha1_file.c +1 -1
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,struct strbuf pathbuf =STRBUF_INIT ;if (!is_absolute_path (entry) && relative_base) { -strbuf_addstr (&pathbuf,real_path (relative_base)); +strbuf_realpath (&pathbuf, relative_base,1 );strbuf_addch (&pathbuf,'/' ); }strbuf_addstr (&pathbuf, entry);
submodule.c +1 -1
@@ -1227,7 +1227,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) {struct strbuf file_name =STRBUF_INIT ;struct strbuf rel_path =STRBUF_INIT ; -const char *real_work_tree =xstrdup (real_path (work_tree)); +const char *real_work_tree =real_pathdup (work_tree);/* Update gitfile */ strbuf_addf (&file_name,"%s/.git" , work_tree);
transport.c +1 -1
@@ -1130,7 +1130,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,const struct ref *extra;struct alternate_refs_data *cb = data; - other =xstrdup (real_path (e->path)); + other =real_pathdup (e->path); len =strlen (other);while (other[len-1 ] =='/' )
worktree.c +1 -1
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,return wt; arg =prefix_filename (prefix,strlen (prefix), arg); - path =xstrdup (real_path (arg)); + path =real_pathdup (arg);for (; *list; list++)if (!fspathcmp (path,real_path ((*list)->path)))break ;
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH v2 1/4 real_path: resolve symlinks by hand
The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls to chdir() can be removed, making real_path one step closer to being reentrant.
--- abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 61 deletions(-)
abspath.c +122 -61
@@ -11,8 +11,38 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode)); } +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >offset_1st_component (path->buf)) { +char *last_slash =find_last_dir_sep (path->buf); +strbuf_setlen (path, last_slash - path->buf); + } +} + +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +51,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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
@@ -33,22 +62,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {
@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else { - last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; + } +strbuf_addstr (&remaining, path); + } + +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ; } -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* append the next component and resolve resultant path */ +if (!is_dir_sep (resolved.buf[resolved.len -1 ])) +strbuf_addch (&resolved,'/' ); +strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); -if (chdir (sb.buf)) { +if (num_symlinks++ >MAXSYMLINKS ) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } - -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb); -strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset); + }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
Quoted 6 lines · Brandon Williams
The current implementation of
real_pathuseschdir()in order to resolve symlinks. Unfortunately this isn't thread-safe aschdir()affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls tochdir()can be removed, makingreal_pathone step closer to being reentrant.
Better description for this part of the change. I like the improvement, as it clearly indicates what this particular patch is about, and why, but doesn't over-state what we gain here.
The rest of this seems reasonable, though I'm not super familiar with the code, so I didn't have any comments.
Thanks,
Jake
Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
In reply to Brandon WilliamsAm 09.12.2016 um 00:58 schrieb Brandon Williams:
Quoted 17 lines
The current implementation of
real_pathuseschdir()in order to resolve symlinks. Unfortunately this isn't thread-safe aschdir()affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls tochdir()can be removed, makingreal_pathone step closer to being reentrant.Signed-off-by Brandon Williams <[email protected]>--- abspath.c | 183 +++++++++++++++++++++++++++++++++++++++++--------------------- 1 file changed, 122 insertions(+), 61 deletions(-)abspath.c +0
-0@@ -11,8 +11,38 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode));
abspath.c int is_directory(const char *path) · line 21
Quoted 10 lines
abspath.c +8
-0} +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >offset_1st_component (path->buf)) { +char *last_slash =find_last_dir_sep (path->buf); +strbuf_setlen (path, last_slash - path->buf); + } +}
This implementation is not correct because when the input is "/foo", the result is "" when it should be "/". Also, can the input be a non-normalized path? When the input is "foo///bar", should the result be "foo" or would "foo//" be an acceptable result? I think it should be the former. find_last_dir_sep() returns the last of the three slashes, not the first one. Therefore, I've rewritten the function thusly:
static void strip_last_component(struct strbuf *path)
{
size_t offset = offset_1st_component(path->buf);
size_t len = path->len;
while (offset < len && !is_dir_sep(path->buf[len - 1]))
len--;
while (offset < len && is_dir_sep(path->buf[len - 1]))
len--;
strbuf_setlen(path, len);
}Quoted 11 lines
abspath.c +11
-0+ +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */
abspath.c int is_directory(const char *path) · line 42
Quoted 10 lines
abspath.c +10
-0+for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +}
Mental note: This function strips leading slashes and the first path component; post-condition: remaining is empty or has leading slashes.
Quoted 58 lines
abspath.c +8
-24+/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved @@ -21,7 +51,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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 @@ -33,22 +62,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {@@ -58,74 +81,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else {
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 90
Quoted 10 lines
abspath.c +7
-3- last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset);
OK.
Quoted 7 lines
abspath.c +7
-0+ }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out;
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 107
Quoted 10 lines
abspath.c +10
-0+ } +strbuf_addstr (&remaining, path); + } + +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */
Can this happen? I think it can: when 'path' is a root directory, or ends with path separators.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 109
Quoted 2 lines
abspath.c +2
-0+ }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */
Good.
I don't mind strcmp(), but others might point out that a single character comparison should be sufficient.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 113
Quoted 4 lines
abspath.c +4
-0+ }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ;
Good.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 118
Quoted 7 lines
abspath.c +3
-2} -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* append the next component and resolve resultant path */ +if (!is_dir_sep (resolved.buf[resolved.len -1 ])) +strbuf_addch (&resolved,'/' );
Can we access resolved.buf[resolved.len - 1] here? At this point, resolved has been filled with an absolute path or from getcwd(); it cannot be empty in the first iteration. In subsequent iterations, it cannot be empty as long as strip_last_component() does not make it empty. Your original version of strip_last_component() could make it empty; my rewrite should not.
Do we have to check for the path separator at all? Typically, resolved does not end with a slash, but if we went all the way up to the root, there is a trailing slash. Good; the condition is required.
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 123
Quoted 5 lines
abspath.c +5
-0+strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (!(errno ==ENOENT && !remaining.len)) {
Perhaps it was easier to _write_ the condition in this way, but I would have an easier time to _read_ it when it is
if (errno != ENOENT || remaining.len) {Quoted 51 lines
abspath.c +11
-24if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); -if (chdir (sb.buf)) { +if (num_symlinks++ >MAXSYMLINKS ) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } - -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb);
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 157
Quoted 10 lines
abspath.c +7
-3-strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset);
Good. I would have expected some kind of "goto repeat;" because when we encounter a symlink to an absolute path, most, if not all, work done so far is obsoleted. But I haven't thought too deeply about this.
Quoted 13 lines
abspath.c +13
-0+ }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 180
Quoted 10 lines
abspath.c +10
-0+if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining);
Good. This looks very reasonable.
Quoted 18 lines
abspath.c +6
-5+ } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Re: [PATCH v2 1/4] real_path: resolve symlinks by hand
Quoted 38 lines · Johannes Sixt
Am 09.12.2016 um 00:58 schrieb Brandon Williams:
Quoted 17 lines
The current implementation of
real_pathuseschdir()in order to resolve symlinks. Unfortunately this isn't thread-safe aschdir()affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls tochdir()can be removed, makingreal_pathone step closer to being reentrant.Signed-off-by Brandon Williams <[email protected]>---
abspath.c| 183 +++++++++++++++++++++++++++++++++++++++++---------------------1 file changed, 122 insertions(+), 61 deletions(-)abspath.c +0
-0@@ -11,8 +11,38 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode));abspath.c int is_directory(const char *path) · line 21
Quoted 10 lines
abspath.c +8
-0} +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +if (path->len >offset_1st_component (path->buf)) { +char *last_slash =find_last_dir_sep (path->buf); +strbuf_setlen (path, last_slash - path->buf); + } +}This implementation is not correct because when the input is "/foo", the result is "" when it should be "/". Also, can the input be a non-normalized path? When the input is "foo///bar", should the result be "foo" or would "foo//" be an acceptable result? I think it should be the former.
find_last_dir_sep()returns the last of the three slashes, not the first one. Therefore, I've rewritten the function thusly:static void
strip_last_component(struct strbuf *path)
Quoted 10 lines
{
size_t offset = offset_1st_component(path->buf);
size_t len = path->len;
while (offset < len && !is_dir_sep(path->buf[len - 1]))
len--;
while (offset < len && is_dir_sep(path->buf[len - 1]))
len--;
strbuf_setlen(path, len);
}Thanks for that catch. So your rewrite takes the offset of the 1st component and ensures that we don't cut that part off. It first strips all non directory separators and then all directory separators. This way "/foo////bar" becomes "/foo" and as you pointed out "/foo" would become "/". The offset would also take care of windows drive letters and the like. Looks good. Thanks!
Quoted 11 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 123
Quoted 5 lines
abspath.c +5
-0+strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (!(errno ==ENOENT && !remaining.len)) {Perhaps it was easier to _write_ the condition in this way, but I would have an easier time to _read_ it when it is
if (errno != ENOENT || remaining.len) {
Yes I did write it out weird, mostly because it made the most sense for what I was trying to accomplish (add path components must exist, except for the very last one). I'm fine applying DeMorgan's since it looks a little cleaner.
Quoted 4 lines
Quoted 4 lines
abspath.c +4
-0+ +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf);
Quoted 8 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 157
Quoted 3 lines
abspath.c +3
-0+strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset);Good. I would have expected some kind of "goto repeat;" because when we encounter a symlink to an absolute path, most, if not all, work done so far is obsoleted. But I haven't thought too deeply about this.
It made the most sense to just reuse the same looping condition that I already had in place. Resetting the resolved string to be the root component of the absolute symlink made it easy to "throw away" all the old work to allow us to start from scratch again.
Signature · 1 line
Brandon Williams
PATCH v2 2/4 real_path: convert real_path_internal to strbuf_realpath
In reply to Brandon WilliamsChange the name of real_path_internal to strbuf_realpath. In addition push the static strbuf up to its callers and instead take as a parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
--- abspath.c | 53 +++++++++++++++++++++++++---------------------------- cache.h | 2 ++ 2 files changed, 27 insertions(+), 28 deletions(-)
abspath.c +25 -28
@@ -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,16 +73,16 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {/* absolute path; start with only root as being resolved */ int offset =offset_1st_component (path); -strbuf_add (&resolved, path, offset); +strbuf_add (resolved, path, offset);strbuf_addstr (&remaining, path + offset); }else {/* relative path; can use CWD as the initial resolved path */ -if (strbuf_getcwd (&resolved)) { +if (strbuf_getcwd (resolved)) {if (die_on_error)die_errno ("unable to get current working directory" );else
@@ -109,21 +101,21 @@ static const char *real_path_internal(const char *path, int die_on_error)continue ;/* '.' component */ }else if (next.len ==2 && !strcmp (next.buf,".." )) {/* '..' component; strip the last path component */ -strip_last_component (&resolved); +strip_last_component (resolved);continue ; }/* append the next component and resolve resultant path */ -if (!is_dir_sep (resolved.buf[resolved.len -1 ])) -strbuf_addch (&resolved,'/' ); -strbuf_addbuf (&resolved, &next); +if (!is_dir_sep (resolved->buf[resolved->len -1 ])) +strbuf_addch (resolved,'/' ); +strbuf_addbuf (resolved, &next); -if (lstat (resolved.buf, &st)) { +if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ if (!(errno ==ENOENT && !remaining.len)) {if (die_on_error)die_errno ("Invalid path '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -139,12 +131,12 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } - len =strbuf_readlink (&symlink, resolved.buf, + len =strbuf_readlink (&symlink, resolved->buf, st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -152,8 +144,8 @@ static const char *real_path_internal(const char *path, int die_on_error)if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ int offset =offset_1st_component (symlink.buf); -strbuf_reset (&resolved); -strbuf_add (&resolved, symlink.buf, offset); +strbuf_reset (resolved); +strbuf_add (resolved, symlink.buf, offset);strbuf_remove (&symlink,0 , offset); }else {/*
@@ -161,7 +153,7 @@ static const char *real_path_internal(const char *path, int die_on_error)* strip off the last component since it will * be replaced with the contents of the symlink */ -strip_last_component (&resolved); +strip_last_component (resolved); }/*
@@ -181,24 +173,29 @@ static const char *real_path_internal(const char *path, int die_on_error) } } - retval = resolved.buf; + retval = resolved->buf; error_out:strbuf_release (&remaining);strbuf_release (&next);strbuf_release (&symlink); +if (!retval) +strbuf_reset (resolved); +return retval; }const char *real_path (const char *path) { -return real_path_internal (path,1 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,1 ); }const char *real_path_if_valid (const char *path) { -return real_path_internal (path,0 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,0 ); }/*
cache.h +2 -0
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); }int is_directory (const char *); +char *strbuf_realpath (struct strbuf *resolved,const char *path, +int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path);const char *absolute_path (const char *path);
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH v2 3/4 real_path: create real_pathdup
In reply to Brandon WilliamsCreate real_pathdup which returns a caller owned string of the resolved realpath based on the provide path.
--- abspath.c | 13 +++++++++++++ cache.h | 1 + 2 files changed, 14 insertions(+)
abspath.c +13 -0
@@ -198,6 +198,19 @@ const char *real_path_if_valid(const char *path)return strbuf_realpath (&realpath, path,0 ); } +char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; +} +/* * Use this to get an absolute path from a relative one. If you want * to resolve links, you should use real_path.
cache.h +1 -0
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v2 3/4] real_path: create real_pathdup
Am 09.12.2016 um 00:58 schrieb Brandon Williams:
abspath.c const char *real_path_if_valid(const char *path) · line 206
Quoted 6 lines
abspath.c +6
-0+char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 ))
Style nit: blank after if is missing.
-- Hannes
PATCH v2 4/4 real_path: have callers use real_pathdup and strbuf_realpath
In reply to Brandon WilliamsMigrate callers of real_path() who duplicate the retern value to use real_pathdup or strbuf_realpath.
--- builtin/init-db.c | 6 +++--- environment.c | 2 +- setup.c | 15 +++++++++------ sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 7 files changed, 17 insertions(+), 14 deletions(-)
builtin/init-db.c +3 -3
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir, {int reinit;int exist_ok = flags &INIT_DB_EXIST_OK ; -char *original_git_dir =xstrdup (real_path (git_dir)); +char *original_git_dir =real_pathdup (git_dir);if (real_git_dir) {struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 );if (real_git_dir && !is_absolute_path (real_git_dir)) - real_git_dir =xstrdup (real_path (real_git_dir)); + real_git_dir =real_pathdup (real_git_dir);if (argc ==1 ) {int mkdir_tried =0 ;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir); - git_work_tree_cfg =xstrdup (real_path (rel)); + git_work_tree_cfg =real_pathdup (rel);free (rel); }if (!git_work_tree_cfg)
environment.c +1 -1
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)return ; } git_work_tree_initialized =1 ; - work_tree =xstrdup (real_path (new_work_tree)); + work_tree =real_pathdup (new_work_tree); }const char *get_git_work_tree (void )
setup.c +9 -6
@@ -254,10 +254,12 @@ 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_realpath (sb, path.buf,1 ); ret =1 ; - }else + }else {strbuf_addstr (sb, gitdir); + } +strbuf_release (&data);strbuf_release (&path);return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* --work-tree is set without --git-dir; use discovered one */ if (getenv (GIT_WORK_TREE_ENVIRONMENT ) || git_work_tree_cfg) {if (offset != cwd->len && !is_absolute_path (gitdir)) - gitdir =xstrdup (real_path (gitdir)); + gitdir =real_pathdup (gitdir);if (chdir (cwd->buf))die_errno ("Could not come back to cwd" );return setup_explicit_git_dir (gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,/* Keep entry but do not canonicalize it */ return 1 ; }else { -const char *real_path =real_path_if_valid (ceil); -if (!real_path) +char *real_path =real_pathdup (ceil); +if (!real_path) {return 0 ; + }free (item->string); - item->string =xstrdup (real_path); + item->string = real_path;return 1 ; } }
sha1_file.c +1 -1
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,struct strbuf pathbuf =STRBUF_INIT ;if (!is_absolute_path (entry) && relative_base) { -strbuf_addstr (&pathbuf,real_path (relative_base)); +strbuf_realpath (&pathbuf, relative_base,1 );strbuf_addch (&pathbuf,'/' ); }strbuf_addstr (&pathbuf, entry);
submodule.c +1 -1
@@ -1227,7 +1227,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) {struct strbuf file_name =STRBUF_INIT ;struct strbuf rel_path =STRBUF_INIT ; -const char *real_work_tree =xstrdup (real_path (work_tree)); +const char *real_work_tree =real_pathdup (work_tree);/* Update gitfile */ strbuf_addf (&file_name,"%s/.git" , work_tree);
transport.c +1 -1
@@ -1130,7 +1130,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,const struct ref *extra;struct alternate_refs_data *cb = data; - other =xstrdup (real_path (e->path)); + other =real_pathdup (e->path); len =strlen (other);while (other[len-1 ] =='/' )
worktree.c +1 -1
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,return wt; arg =prefix_filename (prefix,strlen (prefix), arg); - path =xstrdup (real_path (arg)); + path =real_pathdup (arg);for (; *list; list++)if (!fspathcmp (path,real_path ((*list)->path)))break ;
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v2 0/4] road to reentrant real_path
In reply to Brandon Williamssetup.c int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) · line 257
Quoted 10 lines · Brandon Williams
setup.c +1
-1@@ -254,10 +254,12 @@ 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_realpath (sb, path.buf,1 );
This is not the same because of this hunk in strbuf_realpath()
setup.c static const char *real_path_internal(const char *path, int die_on_error) · line 78
Quoted 8 lines
setup.c +1
-1@@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +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.
Signature · 1 line
Duy
Re: [PATCH v2 0/4] road to reentrant real_path
Quoted 13 lines · Duy Nguyen
setup.c int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) · line 257
Quoted 10 lines · Brandon Williams
setup.c +1
-1@@ -254,10 +254,12 @@ 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_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 &st) call). Sidenote it looks like strbuf_getcwd() also does a reset, though more subtlety, since it just passes its buffer to getcwd().
Quoted 19 lines
setup.c static const char *real_path_internal(const char *path, int die_on_error) · line 78
Quoted 9 lines
setup.c +1
-1@@ -81,17 +73,18 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {But if you you remove that, then all (old) callers of
strbuf_realpath()must do astrbuf_reset()in advance if needed (probably justreal_pathdoes) which sounds reasonable to me. You're probably want to be careful about thestrbuf_reset()at the end of the function too.Other than that, I think this diff looks nice.
Signature · 1 line
Duy
Signature · 1 line
Brandon Williams
Re: [PATCH v2 0/4] road to reentrant real_path
Quoted 15 lines · Brandon Williams
Quoted 13 lines · Duy Nguyen
setup.c int get_common_dir_noenv(struct strbuf *sb, const char *gitdir) · line 257
Quoted 10 lines · Brandon Williams
setup.c +1
-1@@ -254,10 +254,12 @@ 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_realpath (sb, path.buf,1 );This is not the same because of this hunk in
strbuf_realpath()
Quoted 6 lines
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 &st)call). Sidenote it looks likestrbuf_getcwd()also does a reset, though more subtlety, since it just passes its buffer togetcwd().
Yeah that's ok too (I did not see this subtlety when I suggested the change).
Signature · 1 line
Duy
v3 cover road to reentrant real_path
In reply to Brandon WilliamsChanges in v3:
* Rewrite of strip_last_component() as the v2 verison didn't properly handle inputs like '/foo'. Thanks to Johannes for pointing this out and suggesting a solution.
* Small style changes
* Revert the call in get_common_dir_noenv() to maintain proper functionality.
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 | 222 ++++++++++++++++++++++++++++++++++++------------------ builtin/init-db.c | 6 +- cache.h | 3 + environment.c | 2 +- setup.c | 13 ++-- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 9 files changed, 169 insertions(+), 85 deletions(-)
--- interdiff from v2
abspath.c +13 -6
@@ -14,10 +14,17 @@ 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 >offset_1st_component (path->buf)) { -char *last_slash =find_last_dir_sep (path->buf); -strbuf_setlen (path, last_slash - path->buf); - } +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--; +/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--; + +strbuf_setlen (path, len); }/* get (and remove) the next component in 'remaining' and place it in 'next' */
@@ -112,7 +119,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ -if (!(errno ==ENOENT && !remaining.len)) { +if (errno !=ENOENT || remaining.len) {if (die_on_error)die_errno ("Invalid path '%s'" , resolved->buf);
@@ -203,7 +210,7 @@ char *real_pathdup(const char *path)struct strbuf realpath =STRBUF_INIT ;char *retval =NULL ; -if (strbuf_realpath (&realpath, path,0 )) +if (strbuf_realpath (&realpath, path,0 )) retval =strbuf_detach (&realpath,NULL );strbuf_release (&realpath);
setup.c +1 -1
@@ -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_realpath (sb, path.buf,1 ); +strbuf_addstr (sb,real_path (path.buf)); ret =1 ; }else {strbuf_addstr (sb, gitdir);
Signature · 1 line
2.8.0.rc3.226.g39d4020
PATCH v3 1/4 real_path: resolve symlinks by hand
The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls to chdir() can be removed, making real_path one step closer to being reentrant.
--- abspath.c | 190 ++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 129 insertions(+), 61 deletions(-)
abspath.c +129 -61
@@ -11,8 +11,45 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode)); } +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--; +/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--; + +strbuf_setlen (path, len); +} + +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#define MAXSYMLINKS 5/* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +58,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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
@@ -33,22 +69,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {
@@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else { - last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; } +strbuf_addstr (&remaining, path); + } -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ; + } + +/* append the next component and resolve resultant path */ +if (!is_dir_sep (resolved.buf[resolved.len -1 ])) +strbuf_addch (&resolved,'/' ); +strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (errno !=ENOENT || remaining.len) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); -if (chdir (sb.buf)) { +if (num_symlinks++ >MAXSYMLINKS ) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } - -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb); -strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset); + }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v3 1/4] real_path: resolve symlinks by hand
abspath.c int is_directory(const char *path) · line 22
Quoted 9 lines · Brandon Williams
abspath.c +9
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--;
If somebody at a higher level in the callchain has already normalized path, this is not a problem, but this will behave "unexpectedly" when path ends with a dir_sep byte (or more).
E.g. for input path "foo/bar/", the above loop runs zero times and then ...
abspath.c int is_directory(const char *path) · line 25
Quoted 3 lines
abspath.c +3
-0+/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--;
... the slash at the end is removed, leaving "foo/bar" in path.
Quoted 13 lines
abspath.c +12
-0+strbuf_setlen (path, len); +} +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */
abspath.c int is_directory(const char *path) · line 49
Quoted 10 lines
abspath.c +10
-0+for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +}
Unlike the strip_last_component(), I think this one is more carefully done and avoids getting fooled by //extra//slashes// at the beginning or at the end, which does help in the correctness of the loop we see below.
Quoted 23 lines
abspath.c +17
-0@@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; } +strbuf_addstr (&remaining, path); + } +/* Iterate over the remaining path components */
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 119
Quoted 10 lines
abspath.c +10
-0+while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved);
Wouldn't this let "resolved" eventually run out of the path components to strip for a malformed input e.g. "/a/../../b"?
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 179
Quoted 7 lines
abspath.c +7
-0+ ... +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' );
This can add duplicate dir_sep if readlink(2)'ed contents of the symbolic link already ends with a slash, but I think it (together with the fact that the code does nothing to normalize what is read from the symbolic link) probably does not matter, given the way how get_next_component() is implemented.
Quoted 20 lines
abspath.c +14
-0+strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } + retval = resolved.buf; + error_out: +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Re: [PATCH v3 1/4] real_path: resolve symlinks by hand
Quoted 16 lines · Junio C Hamano
abspath.c int is_directory(const char *path) · line 22
Quoted 9 lines · Brandon Williams
abspath.c +9
-0+/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--;If somebody at a higher level in the callchain has already normalized path, this is not a problem, but this will behave "unexpectedly" when path ends with a
dir_sepbyte (or more).
Quoted 9 lines
E.g. for input path "foo/bar/", the above loop runs zero times and then ...
abspath.c int is_directory(const char *path) · line 25
Quoted 3 lines
abspath.c +3
-0+/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--;... the slash at the end is removed, leaving "foo/bar" in path.
The way this is currently used I don't believe this scenario can happen (since input to this shouldn't have trailing slashes), but if others begin to use this function then yes, that is an implicit assumption. I think it may be an ok assumption though since this is only called on "resolved" which is the ouput and needs to be normalized to begin with. To fix this we could simply add the while loop that strips dir_sep at the beginning as well as at the end, like so:
/* Skip sequences of multiple path-separators */ while (offset < len && is_dir_sep(path->buf[len - 1])) len--; /* Skip sequences of multiple path-separators */ while (offset < len && !is_dir_sep(path->buf[len - 1])) len--; /* Skip sequences of multiple path-separators */ while (offset < len && is_dir_sep(path->buf[len - 1])) len--;
Quoted 51 lines
Quoted 13 lines
abspath.c +12
-0+strbuf_setlen (path, len); +} +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ abspath.c int is_directory(const char *path) · line 49
Quoted 10 lines
abspath.c +10
-0+for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +}Unlike the
strip_last_component(), I think this one is more carefully done and avoids getting fooled by //extra//slashes// at the beginning or at the end, which does help in the correctness of the loop we see below.Quoted 22 lines
abspath.c +16
-0@@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; } +strbuf_addstr (&remaining, path); + }
Quoted 15 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 119
Quoted 11 lines
abspath.c +11
-0+/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved);Wouldn't this let "resolved" eventually run out of the path components to strip for a malformed input e.g. "/a/../../b"?
As I understand it, that path is correct and would resolve to "/b". The strip_last_component function doesn't allow striping the "1st" component or the "root" component. So if resolved is "/" and we encounter a ".." which requires striping of the last component, the result would be "/".
Quoted 5 lines
Quoted 5 lines
abspath.c +5
-0+ ... +/* +* if there are still remaining components to resolve +* then append them to symlink +*/
Quoted 9 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 179
Quoted 2 lines
abspath.c +2
-0+if (remaining.len) { +strbuf_addch (&symlink,'/' );This can add duplicate
dir_sepif readlink(2)'ed contents of the symbolic link already ends with a slash, but I think it (together with the fact that the code does nothing to normalize what is read from the symbolic link) probably does not matter, given the way howget_next_component()is implemented.
Yes, I think the way get_next_component() is written will account for non-normalized symlink contents. This way we only have to worry about normalizing input in one location (maybe two with strip_last_component()).
Quoted 21 lines
Quoted 20 lines
abspath.c +14
-0+strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } + retval = resolved.buf; + error_out: +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
Brandon Williams
Re: [PATCH v3 1/4] real_path: resolve symlinks by hand
Quoted 11 lines · Brandon Williams
On 12/12, Junio C Hamano wrote:
...Quoted 8 lines
E.g. for input path "foo/bar/", the above loop runs zero times and then ...
abspath.c int is_directory(const char *path) · line 25
Quoted 3 lines
abspath.c +3
-0+/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--;... the slash at the end is removed, leaving "foo/bar" in path.
Quoted 3 lines
The way this is currently used I don't believe this scenario can happen (since input to this shouldn't have trailing slashes), but if others begin to use this function then yes, that is an implicit assumption.
OK.
Quoted 15 lines
Quoted 6 lines
Quoted 6 lines
abspath.c +6
-0+/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ Quoted 8 lines
abspath.c static const char *real_path_internal(const char *path, int die_on_error) · line 119
Quoted 5 lines
abspath.c +5
-0+ }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved);Wouldn't this let "resolved" eventually run out of the path components to strip for a malformed input e.g. "/a/../../b"?
Quoted 1 line
As I understand it, that path is correct and would resolve to "/b".
That is OK on the traditional UNIX end.
I am not sure if we want to handle the "//host/share/$remaining" in this codepath, though. If we do, then this is still an issue (IIRC, somebody explained that we do not want to step into //host/share/ part when seeing ".."---we'd at least need to leave a NEEDSWORK comment so that interested parties can later take a look into it.
PATCH v3 2/4 real_path: convert real_path_internal to strbuf_realpath
In reply to Brandon WilliamsChange the name of real_path_internal to strbuf_realpath. In addition push the static strbuf up to its callers and instead take as a parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
--- abspath.c | 53 +++++++++++++++++++++++++---------------------------- cache.h | 2 ++ 2 files changed, 27 insertions(+), 28 deletions(-)
abspath.c +25 -28
@@ -55,21 +55,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 ;
@@ -77,10 +73,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" );
@@ -88,16 +80,16 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {/* absolute path; start with only root as being resolved */ int offset =offset_1st_component (path); -strbuf_add (&resolved, path, offset); +strbuf_add (resolved, path, offset);strbuf_addstr (&remaining, path + offset); }else {/* relative path; can use CWD as the initial resolved path */ -if (strbuf_getcwd (&resolved)) { +if (strbuf_getcwd (resolved)) {if (die_on_error)die_errno ("unable to get current working directory" );else
@@ -116,21 +108,21 @@ static const char *real_path_internal(const char *path, int die_on_error)continue ;/* '.' component */ }else if (next.len ==2 && !strcmp (next.buf,".." )) {/* '..' component; strip the last path component */ -strip_last_component (&resolved); +strip_last_component (resolved);continue ; }/* append the next component and resolve resultant path */ -if (!is_dir_sep (resolved.buf[resolved.len -1 ])) -strbuf_addch (&resolved,'/' ); -strbuf_addbuf (&resolved, &next); +if (!is_dir_sep (resolved->buf[resolved->len -1 ])) +strbuf_addch (resolved,'/' ); +strbuf_addbuf (resolved, &next); -if (lstat (resolved.buf, &st)) { +if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ if (errno !=ENOENT || remaining.len) {if (die_on_error)die_errno ("Invalid path '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -146,12 +138,12 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } - len =strbuf_readlink (&symlink, resolved.buf, + len =strbuf_readlink (&symlink, resolved->buf, st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -159,8 +151,8 @@ static const char *real_path_internal(const char *path, int die_on_error)if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ int offset =offset_1st_component (symlink.buf); -strbuf_reset (&resolved); -strbuf_add (&resolved, symlink.buf, offset); +strbuf_reset (resolved); +strbuf_add (resolved, symlink.buf, offset);strbuf_remove (&symlink,0 , offset); }else {/*
@@ -168,7 +160,7 @@ static const char *real_path_internal(const char *path, int die_on_error)* strip off the last component since it will * be replaced with the contents of the symlink */ -strip_last_component (&resolved); +strip_last_component (resolved); }/*
@@ -188,24 +180,29 @@ static const char *real_path_internal(const char *path, int die_on_error) } } - retval = resolved.buf; + retval = resolved->buf; error_out:strbuf_release (&remaining);strbuf_release (&next);strbuf_release (&symlink); +if (!retval) +strbuf_reset (resolved); +return retval; }const char *real_path (const char *path) { -return real_path_internal (path,1 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,1 ); }const char *real_path_if_valid (const char *path) { -return real_path_internal (path,0 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,0 ); }/*
cache.h +2 -0
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); }int is_directory (const char *); +char *strbuf_realpath (struct strbuf *resolved,const char *path, +int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path);const char *absolute_path (const char *path);
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v3 2/4] real_path: convert real_path_internal to strbuf_realpath
Quoted 5 lines · Brandon Williams
Change the name of
real_path_internaltostrbuf_realpath. In addition push the static strbuf up to its callers and instead take as a parameter a pointer to a strbuf to use for the final result.This change makes
strbuf_realpathreentrant.
Yup, this step makes sense.
PATCH v3 3/4 real_path: create real_pathdup
In reply to Brandon WilliamsCreate real_pathdup which returns a caller owned string of the resolved realpath based on the provide path.
--- abspath.c | 13 +++++++++++++ cache.h | 1 + 2 files changed, 14 insertions(+)
abspath.c +13 -0
@@ -205,6 +205,19 @@ const char *real_path_if_valid(const char *path)return strbuf_realpath (&realpath, path,0 ); } +char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; +} +/* * Use this to get an absolute path from a relative one. If you want * to resolve links, you should use real_path.
cache.h +1 -0
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v3 3/4] real_path: create real_pathdup
Quoted 19 lines · Brandon Williams
Create
real_pathdupwhich returns a caller owned string of the resolved realpath based on the provide path.Signed-off-by Brandon Williams <[email protected]>--- abspath.c | 13 +++++++++++++ cache.h | 1 + 2 files changed, 14 insertions(+)abspath.c +2
-0@@ -205,6 +205,19 @@ const char *real_path_if_valid(const char *path)return strbuf_realpath (&realpath, path,0 ); } +char *real_pathdup (const char *path) +{
abspath.c const char *real_path_if_valid(const char *path) · line 219
Quoted 10 lines
abspath.c +10
-0+struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; +}
OK. Taken alone, the above makes a reader wonder if it still makes sense for strbuf_realpath() to return realpath.buf (or NULL upon error). An obvious alternative is to return 0 on success and -1 on failure.
But other callers of strbuf_realpath() are mimicking the historical "give a path in 'const char *', receive a path in 'const char *' that you have to xstrdup() if you want to keep it" interface, and this interface may be easier for them. I dunno.
Quoted 15 lines
abspath.c +0
-0/* * Use this to get an absolute path from a relative one. If you want * to resolve links, you should use real_path. cache.h +1
-0@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
PATCH v3 4/4 real_path: have callers use real_pathdup and strbuf_realpath
In reply to Brandon WilliamsMigrate callers of real_path() who duplicate the retern value to use real_pathdup or strbuf_realpath.
--- builtin/init-db.c | 6 +++--- environment.c | 2 +- setup.c | 13 ++++++++----- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 7 files changed, 16 insertions(+), 13 deletions(-)
builtin/init-db.c +3 -3
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir, {int reinit;int exist_ok = flags &INIT_DB_EXIST_OK ; -char *original_git_dir =xstrdup (real_path (git_dir)); +char *original_git_dir =real_pathdup (git_dir);if (real_git_dir) {struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 );if (real_git_dir && !is_absolute_path (real_git_dir)) - real_git_dir =xstrdup (real_path (real_git_dir)); + real_git_dir =real_pathdup (real_git_dir);if (argc ==1 ) {int mkdir_tried =0 ;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir); - git_work_tree_cfg =xstrdup (real_path (rel)); + git_work_tree_cfg =real_pathdup (rel);free (rel); }if (!git_work_tree_cfg)
environment.c +1 -1
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)return ; } git_work_tree_initialized =1 ; - work_tree =xstrdup (real_path (new_work_tree)); + work_tree =real_pathdup (new_work_tree); }const char *get_git_work_tree (void )
setup.c +8 -5
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)strbuf_addbuf (&path, &data);strbuf_addstr (sb,real_path (path.buf)); ret =1 ; - }else + }else {strbuf_addstr (sb, gitdir); + } +strbuf_release (&data);strbuf_release (&path);return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* --work-tree is set without --git-dir; use discovered one */ if (getenv (GIT_WORK_TREE_ENVIRONMENT ) || git_work_tree_cfg) {if (offset != cwd->len && !is_absolute_path (gitdir)) - gitdir =xstrdup (real_path (gitdir)); + gitdir =real_pathdup (gitdir);if (chdir (cwd->buf))die_errno ("Could not come back to cwd" );return setup_explicit_git_dir (gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,/* Keep entry but do not canonicalize it */ return 1 ; }else { -const char *real_path =real_path_if_valid (ceil); -if (!real_path) +char *real_path =real_pathdup (ceil); +if (!real_path) {return 0 ; + }free (item->string); - item->string =xstrdup (real_path); + item->string = real_path;return 1 ; } }
sha1_file.c +1 -1
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,struct strbuf pathbuf =STRBUF_INIT ;if (!is_absolute_path (entry) && relative_base) { -strbuf_addstr (&pathbuf,real_path (relative_base)); +strbuf_realpath (&pathbuf, relative_base,1 );strbuf_addch (&pathbuf,'/' ); }strbuf_addstr (&pathbuf, entry);
submodule.c +1 -1
@@ -1227,7 +1227,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) {struct strbuf file_name =STRBUF_INIT ;struct strbuf rel_path =STRBUF_INIT ; -const char *real_work_tree =xstrdup (real_path (work_tree)); +const char *real_work_tree =real_pathdup (work_tree);/* Update gitfile */ strbuf_addf (&file_name,"%s/.git" , work_tree);
transport.c +1 -1
@@ -1130,7 +1130,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,const struct ref *extra;struct alternate_refs_data *cb = data; - other =xstrdup (real_path (e->path)); + other =real_pathdup (e->path); len =strlen (other);while (other[len-1 ] =='/' )
worktree.c +1 -1
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,return wt; arg =prefix_filename (prefix,strlen (prefix), arg); - path =xstrdup (real_path (arg)); + path =real_pathdup (arg);for (; *list; list++)if (!fspathcmp (path,real_path ((*list)->path)))break ;
Signature · 1 line
2.8.0.rc3.226.g39d4020
Re: [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.
Looks good.
Re: [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.Looks good.
This has small non-textual conflicts with Stefan's embed^Wabsorption topic; please holler if you spot my mismerge. Thanks.
Re: [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 4 lines · Junio C Hamano
Quoted 2 lines · Junio C Hamano
Brandon Williams <[email protected]> writes:
Quoted 8 lines
Quoted 4 lines
Quoted 2 lines
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.Looks good.
This has small non-textual conflicts with Stefan's embed^Wabsorption topic; please holler if you spot my mismerge. Thanks.
5f6a003727 (Merge branch 'bw/realpath-wo-chdir' into jch) looks good to me.
Re: [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 8 lines · Stefan Beller
Quoted 7 lines · Junio C Hamano
Quoted 5 lines · Junio C Hamano
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.
Quoted 8 lines
Quoted 5 lines
Quoted 1 line
Looks good.
This has small non-textual conflicts with Stefan's embed^Wabsorption topic; please holler if you spot my mismerge. Thanks.
5f6a003727(Merge branch 'bw/realpath-wo-chdir' into jch) looks good to me.
Looks good to me as well.
Signature · 1 line
Brandon Williams
Re: [PATCH v3 4/4] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 11 lines · Brandon Williams
Quoted 10 lines · Stefan Beller
Quoted 9 lines · Junio C Hamano
Quoted 6 lines · Junio C Hamano
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.Looks good.
Quoted 7 lines
Quoted 5 lines
Quoted 2 lines
This has small non-textual conflicts with Stefan's embed^Wabsorption topic; please holler if you spot my mismerge. Thanks.
5f6a003727(Merge branch 'bw/realpath-wo-chdir' into jch) looks good to me.Looks good to me as well.
Thanks.
PATCH 1/1 real_path: canonicalize directory separators in root parts
In reply to Brandon WilliamsWhen an absolute path is resolved, resolution begins at the first path component after the root part. The root part is just copied verbatim, because it must not be inspected for symbolic links. For POSIX paths, this is just the initial slash, but on Windows, the root part has the forms c:\ or \\server\share. We do want to canonicalize the back-slashes in the root part because these parts are compared to the result of getcwd(), which does return a fully canonicalized path.
Factor out a helper that splits off the root part, and have it canonicalize the copied part.
This change was prompted because t1504-ceiling-dirs.sh caught a breakage in GIT_CEILING_DIRECTORIES handling on Windows.
---
This introduces the second #ifdef GIT_WINDOWS_NATIVE in this file. It could be avoided if convert_slashes were defined as a do-nothing on POSIX, but that would not help the other occurrence. Therefore, I suggest to leave it at this.
abspath.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
abspath.c +17 -12
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)strbuf_remove (remaining,0 , end - remaining->buf); } +/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); + +strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif +strbuf_remove (remaining,0 , offset); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ #define MAXSYMLINKS 5
@@ -80,14 +93,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, goto error_out; } -strbuf_reset (resolved); +strbuf_addstr (&remaining, path); +get_root_part (resolved, &remaining); -if (is_absolute_path (path)) { -/* absolute path; start with only root as being resolved */ -int offset =offset_1st_component (path); -strbuf_add (resolved, path, offset); -strbuf_addstr (&remaining, path + offset); - }else { +if (!resolved->len) {/* relative path; can use CWD as the initial resolved path */ if (strbuf_getcwd (resolved)) {if (die_on_error)
@@ -95,7 +104,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,else goto error_out; } -strbuf_addstr (&remaining, path); }/* Iterate over the remaining path components */
@@ -150,10 +158,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ -int offset =offset_1st_component (symlink.buf); -strbuf_reset (resolved); -strbuf_add (resolved, symlink.buf, offset); -strbuf_remove (&symlink,0 , offset); +get_root_part (resolved, &symlink); }else {/* * relative symlink
Signature · 1 line
2.11.0.79.gf6b77ca
Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
Quoted 32 lines · Johannes Sixt
When an absolute path is resolved, resolution begins at the first path component after the root part. The root part is just copied verbatim, because it must not be inspected for symbolic links. For POSIX paths, this is just the initial slash, but on Windows, the root part has the forms c:\ or \\server\share. We do want to canonicalize the back-slashes in the root part because these parts are compared to the result of
getcwd(), which does return a fully canonicalized path.Factor out a helper that splits off the root part, and have it canonicalize the copied part.
This change was prompted because
t1504-ceiling-dirs.shcaught a breakage inGIT_CEILING_DIRECTORIEShandling on Windows.Signed-off-by Johannes Sixt <[email protected]>---
This introduces the second #ifdefGIT_WINDOWS_NATIVEin this file. It could be avoided ifconvert_slasheswere defined as a do-nothing on POSIX, but that would not help the other occurrence. Therefore, I suggest to leave it at this.abspath.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)abspath.c +0
-0@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)strbuf_remove (remaining,0 , end - remaining->buf); }
abspath.c static void get_next_component(struct strbuf *next, struct strbuf *remaining) · line 60
Quoted 10 lines
abspath.c +10
-0+/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); + +strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif
So then the only extra cononicalization that is happening here is converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
Signature · 1 line
Brandon Williams
Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
Am 21.12.2016 um 23:33 schrieb Brandon Williams:
Quoted 6 lines · Johannes Sixt
Quoted 5 lines
abspath.c +5
-0+/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); +
Quoted 8 lines
abspath.c static void get_next_component(struct strbuf *next, struct strbuf *remaining) · line 60
Quoted 5 lines
abspath.c +5
-0+strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif So then the only extra cononicalization that is happening here is converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
Correct. All other directory separators are canonicalized by the primary function, strbuf_realpath.
-- Hannes
Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
Quoted 16 lines · Johannes Sixt
Am 21.12.2016 um 23:33 schrieb Brandon Williams:
Quoted 6 lines · Johannes Sixt
Quoted 5 lines
abspath.c +5
-0+/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); +Quoted 8 lines
abspath.c static void get_next_component(struct strbuf *next, struct strbuf *remaining) · line 60
Quoted 5 lines
abspath.c +5
-0+strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif So then the only extra cononicalization that is happening here is converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
Quoted 2 lines
Correct. All other directory separators are canonicalized by the primary function,
strbuf_realpath.
Sounds good. Logically everything looks good to me. And I like that setting 'resolved' to the root of an abs path is pulled out into a helper function. It took me a couple extra seconds to realize that offset_1st_component returns 0 with a relative path, which makes causes the call to get_root_part to essentially be a noop (ie nothing is resolved).
Thanks for helping get this to work on windows!
Signature · 1 line
Brandon Williams
Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
Am 22.12.2016 um 18:33 schrieb Brandon Williams:
Quoted 4 lines
It took me a couple extra seconds to realize that
offset_1st_componentreturns 0 with a relative path, which makes causes the call toget_root_partto essentially be a noop (ie nothing is resolved).
Yeah, I am still unsure whether it is a good idea to optimize away the is_absolute_path() call, because we lose the symmetry to the symlink case, where we cannot get rid of the call...
But I think the condition plus comment
if (!resolved->len) {
/* relative path; can use CWD as the initial resolved path */makes things fairly clear.
-- Hannes
Re: [PATCH bw/realpath-wo-chdir] real_path: canonicalize directory separators in root parts
In reply to Brandon WilliamsQuoted 20 lines · Brandon Williams
Quoted 16 lines · Johannes Sixt
Am 21.12.2016 um 23:33 schrieb Brandon Williams:
Quoted 6 lines · Johannes Sixt
Quoted 5 lines
abspath.c +5
-0+/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); +Quoted 8 lines
abspath.c static void get_next_component(struct strbuf *next, struct strbuf *remaining) · line 60
Quoted 5 lines
abspath.c +5
-0+strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif So then the only extra cononicalization that is happening here is converting '\\server\share' to '//server/share'? (or 'c:\' to 'c:/')
Quoted 2 lines
Correct. All other directory separators are canonicalized by the primary function,
strbuf_realpath.
Quoted 8 lines
Sounds good. Logically everything looks good to me. And I like that setting 'resolved' to the root of an abs path is pulled out into a helper function. It took me a couple extra seconds to realize that
offset_1st_componentreturns 0 with a relative path, which makes causes the call toget_root_partto essentially be a noop (ie nothing is resolved).Thanks for helping get this to work on windows!
Thanks, both.
Let's move the topic with this patch to 'next'. Further micro-optimization can be done incrementally if desired.
v4 cover road to reentrant real_path
In reply to Brandon WilliamsOnly change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
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
Johannes Sixt (1):
real_path: canonicalize directory separators in root parts
abspath.c | 225 +++++++++++++++++++++++++++++++++++++----------------- builtin/init-db.c | 6 +- cache.h | 3 + environment.c | 2 +- setup.c | 13 ++-- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 9 files changed, 173 insertions(+), 84 deletions(-)
--- interdiff with v3
abspath.c +3 -3
@@ -62,7 +62,7 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) }/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXSYMLINKS 5 +#define MAXDEPTH 5/* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -138,10 +138,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,ssize_t len;strbuf_reset (&symlink); -if (num_symlinks++ >MAXSYMLINKS ) { +if (num_symlinks++ >MAXDEPTH ) {if (die_on_error)die ("More than %d nested symlinks " -"on path '%s'" ,MAXSYMLINKS , path); +"on path '%s'" ,MAXDEPTH , path);else goto error_out; }
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v4 1/5 real_path: resolve symlinks by hand
The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls to chdir() can be removed, making real_path one step closer to being reentrant.
--- abspath.c | 188 ++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 128 insertions(+), 60 deletions(-)
abspath.c +128 -60
@@ -11,6 +11,43 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode)); } +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--; +/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--; + +strbuf_setlen (path, len); +} + +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ #define MAXDEPTH 5
@@ -21,7 +58,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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
@@ -33,22 +69,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {
@@ -58,74 +88,112 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else { - last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; } +strbuf_addstr (&remaining, path); + } -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ; + } + +/* append the next component and resolve resultant path */ +if (!is_dir_sep (resolved.buf[resolved.len -1 ])) +strbuf_addch (&resolved,'/' ); +strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (errno !=ENOENT || remaining.len) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); -if (chdir (sb.buf)) { +if (num_symlinks++ >MAXDEPTH ) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXDEPTH , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } - -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb); -strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset); + }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v4 2/5 real_path: convert real_path_internal to strbuf_realpath
In reply to Brandon WilliamsChange the name of real_path_internal to strbuf_realpath. In addition push the static strbuf up to its callers and instead take as a parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
--- abspath.c | 53 +++++++++++++++++++++++++---------------------------- cache.h | 2 ++ 2 files changed, 27 insertions(+), 28 deletions(-)
abspath.c +25 -28
@@ -55,21 +55,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 ;
@@ -77,10 +73,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" );
@@ -88,16 +80,16 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {/* absolute path; start with only root as being resolved */ int offset =offset_1st_component (path); -strbuf_add (&resolved, path, offset); +strbuf_add (resolved, path, offset);strbuf_addstr (&remaining, path + offset); }else {/* relative path; can use CWD as the initial resolved path */ -if (strbuf_getcwd (&resolved)) { +if (strbuf_getcwd (resolved)) {if (die_on_error)die_errno ("unable to get current working directory" );else
@@ -116,21 +108,21 @@ static const char *real_path_internal(const char *path, int die_on_error)continue ;/* '.' component */ }else if (next.len ==2 && !strcmp (next.buf,".." )) {/* '..' component; strip the last path component */ -strip_last_component (&resolved); +strip_last_component (resolved);continue ; }/* append the next component and resolve resultant path */ -if (!is_dir_sep (resolved.buf[resolved.len -1 ])) -strbuf_addch (&resolved,'/' ); -strbuf_addbuf (&resolved, &next); +if (!is_dir_sep (resolved->buf[resolved->len -1 ])) +strbuf_addch (resolved,'/' ); +strbuf_addbuf (resolved, &next); -if (lstat (resolved.buf, &st)) { +if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ if (errno !=ENOENT || remaining.len) {if (die_on_error)die_errno ("Invalid path '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -146,12 +138,12 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } - len =strbuf_readlink (&symlink, resolved.buf, + len =strbuf_readlink (&symlink, resolved->buf, st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -159,8 +151,8 @@ static const char *real_path_internal(const char *path, int die_on_error)if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ int offset =offset_1st_component (symlink.buf); -strbuf_reset (&resolved); -strbuf_add (&resolved, symlink.buf, offset); +strbuf_reset (resolved); +strbuf_add (resolved, symlink.buf, offset);strbuf_remove (&symlink,0 , offset); }else {/*
@@ -168,7 +160,7 @@ static const char *real_path_internal(const char *path, int die_on_error)* strip off the last component since it will * be replaced with the contents of the symlink */ -strip_last_component (&resolved); +strip_last_component (resolved); }/*
@@ -188,24 +180,29 @@ static const char *real_path_internal(const char *path, int die_on_error) } } - retval = resolved.buf; + retval = resolved->buf; error_out:strbuf_release (&remaining);strbuf_release (&next);strbuf_release (&symlink); +if (!retval) +strbuf_reset (resolved); +return retval; }const char *real_path (const char *path) { -return real_path_internal (path,1 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,1 ); }const char *real_path_if_valid (const char *path) { -return real_path_internal (path,0 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,0 ); }/*
cache.h +2 -0
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); }int is_directory (const char *); +char *strbuf_realpath (struct strbuf *resolved,const char *path, +int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path);const char *absolute_path (const char *path);
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v4 3/5 real_path: create real_pathdup
In reply to Brandon WilliamsCreate real_pathdup which returns a caller owned string of the resolved realpath based on the provide path.
--- abspath.c | 13 +++++++++++++ cache.h | 1 + 2 files changed, 14 insertions(+)
abspath.c +13 -0
@@ -205,6 +205,19 @@ const char *real_path_if_valid(const char *path)return strbuf_realpath (&realpath, path,0 ); } +char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; +} +/* * Use this to get an absolute path from a relative one. If you want * to resolve links, you should use real_path.
cache.h +1 -0
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v4 4/5 real_path: have callers use real_pathdup and strbuf_realpath
In reply to Brandon WilliamsMigrate callers of real_path() who duplicate the retern value to use real_pathdup or strbuf_realpath.
--- builtin/init-db.c | 6 +++--- environment.c | 2 +- setup.c | 13 ++++++++----- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 7 files changed, 16 insertions(+), 13 deletions(-)
builtin/init-db.c +3 -3
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir, {int reinit;int exist_ok = flags &INIT_DB_EXIST_OK ; -char *original_git_dir =xstrdup (real_path (git_dir)); +char *original_git_dir =real_pathdup (git_dir);if (real_git_dir) {struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 );if (real_git_dir && !is_absolute_path (real_git_dir)) - real_git_dir =xstrdup (real_path (real_git_dir)); + real_git_dir =real_pathdup (real_git_dir);if (argc ==1 ) {int mkdir_tried =0 ;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir); - git_work_tree_cfg =xstrdup (real_path (rel)); + git_work_tree_cfg =real_pathdup (rel);free (rel); }if (!git_work_tree_cfg)
environment.c +1 -1
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)return ; } git_work_tree_initialized =1 ; - work_tree =xstrdup (real_path (new_work_tree)); + work_tree =real_pathdup (new_work_tree); }const char *get_git_work_tree (void )
setup.c +8 -5
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)strbuf_addbuf (&path, &data);strbuf_addstr (sb,real_path (path.buf)); ret =1 ; - }else + }else {strbuf_addstr (sb, gitdir); + } +strbuf_release (&data);strbuf_release (&path);return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* --work-tree is set without --git-dir; use discovered one */ if (getenv (GIT_WORK_TREE_ENVIRONMENT ) || git_work_tree_cfg) {if (offset != cwd->len && !is_absolute_path (gitdir)) - gitdir =xstrdup (real_path (gitdir)); + gitdir =real_pathdup (gitdir);if (chdir (cwd->buf))die_errno ("Could not come back to cwd" );return setup_explicit_git_dir (gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,/* Keep entry but do not canonicalize it */ return 1 ; }else { -const char *real_path =real_path_if_valid (ceil); -if (!real_path) +char *real_path =real_pathdup (ceil); +if (!real_path) {return 0 ; + }free (item->string); - item->string =xstrdup (real_path); + item->string = real_path;return 1 ; } }
sha1_file.c +1 -1
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,struct strbuf pathbuf =STRBUF_INIT ;if (!is_absolute_path (entry) && relative_base) { -strbuf_addstr (&pathbuf,real_path (relative_base)); +strbuf_realpath (&pathbuf, relative_base,1 );strbuf_addch (&pathbuf,'/' ); }strbuf_addstr (&pathbuf, entry);
submodule.c +1 -1
@@ -1301,7 +1301,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) {struct strbuf file_name =STRBUF_INIT ;struct strbuf rel_path =STRBUF_INIT ; -const char *real_work_tree =xstrdup (real_path (work_tree)); +const char *real_work_tree =real_pathdup (work_tree);/* Update gitfile */ strbuf_addf (&file_name,"%s/.git" , work_tree);
transport.c +1 -1
@@ -1146,7 +1146,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,const struct ref *extra;struct alternate_refs_data *cb = data; - other =xstrdup (real_path (e->path)); + other =real_pathdup (e->path); len =strlen (other);while (other[len-1 ] =='/' )
worktree.c +1 -1
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,return wt; arg =prefix_filename (prefix,strlen (prefix), arg); - path =xstrdup (real_path (arg)); + path =real_pathdup (arg);for (; *list; list++)if (!fspathcmp (path,real_path ((*list)->path)))break ;
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
Re: [PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.
Nit: s/retern/return
Re: [PATCH v4 4/5] real_path: have callers use real_pathdup and strbuf_realpath
Quoted 5 lines · Jacob Keller
Quoted 2 lines · Brandon Williams
Migrate callers of
real_path()who duplicate the retern value to usereal_pathduporstrbuf_realpath.Nit: s/retern/return
Thanks for catching that, I'll fix that in the next reroll.
Signature · 1 line
Brandon Williams
PATCH v4 5/5 real_path: canonicalize directory separators in root parts
In reply to Brandon WilliamsFrom: Johannes Sixt <[email protected]>
When an absolute path is resolved, resolution begins at the first path component after the root part. The root part is just copied verbatim, because it must not be inspected for symbolic links. For POSIX paths, this is just the initial slash, but on Windows, the root part has the forms c:\ or \\server\share. We do want to canonicalize the back-slashes in the root part because these parts are compared to the result of getcwd(), which does return a fully canonicalized path.
Factor out a helper that splits off the root part, and have it canonicalize the copied part.
This change was prompted because t1504-ceiling-dirs.sh caught a breakage in GIT_CEILING_DIRECTORIES handling on Windows.
--- abspath.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
abspath.c +17 -12
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)strbuf_remove (remaining,0 , end - remaining->buf); } +/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); + +strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif +strbuf_remove (remaining,0 , offset); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ #define MAXDEPTH 5
@@ -80,14 +93,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, goto error_out; } -strbuf_reset (resolved); +strbuf_addstr (&remaining, path); +get_root_part (resolved, &remaining); -if (is_absolute_path (path)) { -/* absolute path; start with only root as being resolved */ -int offset =offset_1st_component (path); -strbuf_add (resolved, path, offset); -strbuf_addstr (&remaining, path + offset); - }else { +if (!resolved->len) {/* relative path; can use CWD as the initial resolved path */ if (strbuf_getcwd (resolved)) {if (die_on_error)
@@ -95,7 +104,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,else goto error_out; } -strbuf_addstr (&remaining, path); }/* Iterate over the remaining path components */
@@ -150,10 +158,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ -int offset =offset_1st_component (symlink.buf); -strbuf_reset (resolved); -strbuf_add (resolved, symlink.buf, offset); -strbuf_remove (&symlink,0 , offset); +get_root_part (resolved, &symlink); }else {/* * relative symlink
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
Re: [PATCH v4 0/5] road to reentrant real_path
In reply to Brandon WilliamsQuoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endif
would be a good solution?
It looks like the "usual" value is more like 20 or 30 on most systems, though. We should probably also set errno to ELOOP when we hit the limit, which is what other symlink-resolving functions would do.
I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too. We should be picking it up from <sys/param.h>.
-Peff
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 8 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
Quoted 7 lines
The only problem was that we were redefining the macro. So maybe:
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
Quoted 10 lines
It looks like the "usual" value is more like 20 or 30 on most systems, though. We should probably also set errno to ELOOP when we hit the limit, which is what other symlink-resolving functions would do.
I'm surprised we didn't hit this on Linux, which has MAXSYMLINKS, too. We should be picking it up from <
sys/param.h>.-Peff
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 11 lines · Torsten Bögershausen
Quoted 10 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
Quoted 8 lines
Quoted 5 lines
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32
I mentioned "5" because that is the current value of MAXDEPTH. I do think it would be reasonable to bump it to something higher.
Quoted 2 lines
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
I think it's probably more important to match the rest of the OS, so that open("foo") and realpath("foo") behave similarly on the same system. Though I think even that isn't always possible, as the limit is dynamic on some systems.
I think the idea of the 20-30 range is that it's small enough to catch an infinite loop quickly, and large enough that nobody will ever hit it in practice. :)
-Peff
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 28 lines · Jeff King
Quoted 11 lines · Torsten Bögershausen
Quoted 10 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
Quoted 8 lines
Quoted 5 lines
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32I mentioned "5" because that is the current value of MAXDEPTH. I do think it would be reasonable to bump it to something higher.
Quoted 2 lines
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
Quoted 8 lines
I think it's probably more important to match the rest of the OS, so that
open("foo")andrealpath("foo")behave similarly on the same system. Though I think even that isn't always possible, as the limit is dynamic on some systems.I think the idea of the 20-30 range is that it's small enough to catch an infinite loop quickly, and large enough that nobody will ever hit it in practice. :)
I agree that we should have similar guarantees as the OS provides, especially if the OS already has MAXSYMLINKS defined. What then, should the fall back value be if the OS doesn't have this defined? 5 like we have done historically, or something around the 20-30 range as Torsten suggests?
Signature · 1 line
Brandon Williams
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 38 lines · Brandon Williams
Quoted 28 lines · Jeff King
Quoted 11 lines · Torsten Bögershausen
Quoted 10 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
Quoted 8 lines
Quoted 5 lines
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32I mentioned "5" because that is the current value of MAXDEPTH. I do think it would be reasonable to bump it to something higher.
Quoted 2 lines
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
Quoted 8 lines
I think it's probably more important to match the rest of the OS, so that
open("foo")andrealpath("foo")behave similarly on the same system. Though I think even that isn't always possible, as the limit is dynamic on some systems.I think the idea of the 20-30 range is that it's small enough to catch an infinite loop quickly, and large enough that nobody will ever hit it in practice. :)
Quoted 5 lines
I agree that we should have similar guarantees as the OS provides, especially if the OS already has MAXSYMLINKS defined. What then, should the fall back value be if the OS doesn't have this defined? 5 like we have done historically, or something around the 20-30 range as Torsten suggests?
As a fallback I'd rather go for a larger number than too small. The reason for the existence is just to break an infinite loop (and report an error? which the current code doesn't quite do, but your series actually does).
If the number is too large, then it takes a bit longer to generate the error message, but the error path is no big deal w.r.t. performance, so it's fine for it taking a bit longer.
If the number is too low, then we may hinder people from getting actual work done, (i.e. they have to figure out what the problem is and recompile git), so I'd think a larger number is not harmful. So 32?
Quoted 3 lines
--
Brandon Williams
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 50 lines · Stefan Beller
Quoted 38 lines · Brandon Williams
Quoted 28 lines · Jeff King
Quoted 11 lines · Torsten Bögershausen
Quoted 10 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
Quoted 8 lines
Quoted 5 lines
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32I mentioned "5" because that is the current value of MAXDEPTH. I do think it would be reasonable to bump it to something higher.
Quoted 2 lines
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
Quoted 8 lines
I think it's probably more important to match the rest of the OS, so that
open("foo")andrealpath("foo")behave similarly on the same system. Though I think even that isn't always possible, as the limit is dynamic on some systems.I think the idea of the 20-30 range is that it's small enough to catch an infinite loop quickly, and large enough that nobody will ever hit it in practice. :)
Quoted 5 lines
I agree that we should have similar guarantees as the OS provides, especially if the OS already has MAXSYMLINKS defined. What then, should the fall back value be if the OS doesn't have this defined? 5 like we have done historically, or something around the 20-30 range as Torsten suggests?
As a fallback I'd rather go for a larger number than too small. The reason for the existence is just to break an infinite loop (and report an error? which the current code doesn't quite do, but your series actually does).
Quoted 8 lines
If the number is too large, then it takes a bit longer to generate the error message, but the error path is no big deal w.r.t. performance, so it's fine for it taking a bit longer.
If the number is too low, then we may hinder people from getting actual work done, (i.e. they have to figure out what the problem is and recompile git), so I'd think a larger number is not harmful. So 32?
I think I agree as well.
Thanks,
Jake
Quoted 3 lines
Quoted 3 lines
--
Brandon Williams
Re: [PATCH v4 0/5] road to reentrant real_path
Quoted 60 lines · Jacob Keller
Quoted 50 lines · Stefan Beller
Quoted 38 lines · Brandon Williams
Quoted 28 lines · Jeff King
Quoted 11 lines · Torsten Bögershausen
Quoted 10 lines · Jeff King
Quoted 2 lines · Brandon Williams
Only change with v4 is in [1/5] renaming the #define MAXSYMLINKS back to MAXDEPTH due to a naming conflict brought up by Lars Schneider.
Hmm. Isn't MAXSYMLINKS basically what you want here, though? It what's what all other similar functions will be using.
The only problem was that we were redefining the macro. So maybe:
Quoted 8 lines
Quoted 5 lines
#ifndef MAXSYMLINKS #define MAXSYMLINKS 5 #endifwould be a good solution?
Why 5 ? (looking at the 20..30 below)
And why 5 on one system and e.g. on my Mac OS
#define MAXSYMLINKS 32I mentioned "5" because that is the current value of MAXDEPTH. I do think it would be reasonable to bump it to something higher.
Quoted 2 lines
Would the same value value for all Git installations on all platforms make sense? #define GITMAXSYMLINKS 20
Quoted 8 lines
I think it's probably more important to match the rest of the OS, so that
open("foo")andrealpath("foo")behave similarly on the same system. Though I think even that isn't always possible, as the limit is dynamic on some systems.I think the idea of the 20-30 range is that it's small enough to catch an infinite loop quickly, and large enough that nobody will ever hit it in practice. :)
Quoted 5 lines
I agree that we should have similar guarantees as the OS provides, especially if the OS already has MAXSYMLINKS defined. What then, should the fall back value be if the OS doesn't have this defined? 5 like we have done historically, or something around the 20-30 range as Torsten suggests?
As a fallback I'd rather go for a larger number than too small. The reason for the existence is just to break an infinite loop (and report an error? which the current code doesn't quite do, but your series actually does).
Quoted 8 lines
If the number is too large, then it takes a bit longer to generate the error message, but the error path is no big deal w.r.t. performance, so it's fine for it taking a bit longer.
If the number is too low, then we may hinder people from getting actual work done, (i.e. they have to figure out what the problem is and recompile git), so I'd think a larger number is not harmful. So 32?
Quoted 8 lines
I think I agree as well.
Thanks,
JakeQuoted 3 lines
Quoted 3 lines
--
Brandon Williams
That's two people for 32. I'll use that as the fallback and resend the series.
Signature · 1 line
Brandon Williams
v5 cover road to reentrant real_path
In reply to Brandon Williamschanges in v5:
* set errno to ELOOP when MAXSYMLINKS is exceded.
* revert to use MAXSYMLINKS instead of MAXDEPTH.
* If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.
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
Johannes Sixt (1):
real_path: canonicalize directory separators in root parts
abspath.c | 231 +++++++++++++++++++++++++++++++++++++----------------- builtin/init-db.c | 6 +- cache.h | 3 + environment.c | 2 +- setup.c | 13 +-- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 9 files changed, 178 insertions(+), 85 deletions(-)
--- interdiff with v4
abspath.c +7 -3
@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) }/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#ifndef MAXSYMLINKS +#define MAXSYMLINKS 32 +#endif /* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -138,10 +140,12 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,ssize_t len;strbuf_reset (&symlink); -if (num_symlinks++ >MAXDEPTH ) { +if (num_symlinks++ >MAXSYMLINKS ) { + errno =ELOOP ; +if (die_on_error)die ("More than %d nested symlinks " -"on path '%s'" ,MAXDEPTH , path); +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; }
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v5 1/5 real_path: resolve symlinks by hand
The current implementation of real_path uses chdir() in order to resolve symlinks. Unfortunately this isn't thread-safe as chdir() affects a process as a whole and not just an individual thread. Instead perform the symlink resolution by hand so that the calls to chdir() can be removed, making real_path one step closer to being reentrant.
--- abspath.c | 194 ++++++++++++++++++++++++++++++++++++++++++-------------------- 1 file changed, 133 insertions(+), 61 deletions(-)
abspath.c +133 -61
@@ -11,8 +11,47 @@ int is_directory(const char *path)return (!stat (path, &st) &&S_ISDIR (st.st_mode)); } +/* removes the last path component from 'path' except if 'path' is root */ +static void strip_last_component (struct strbuf *path) +{ +size_t offset =offset_1st_component (path->buf); +size_t len = path->len; + +/* Find start of the last component */ +while (offset < len && !is_dir_sep (path->buf[len -1 ])) + len--; +/* Skip sequences of multiple path-separators */ +while (offset < len &&is_dir_sep (path->buf[len -1 ])) + len--; + +strbuf_setlen (path, len); +} + +/* 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 ; +char *end =NULL ; + +strbuf_reset (next); + +/* look for the next component */ +/* Skip sequences of multiple path-separators */ +for (start = remaining->buf;is_dir_sep (*start); start++) + ;/* nothing */ +/* Find end of the path component */ +for (end = start; *end && !is_dir_sep (*end); end++) + ;/* nothing */ + +strbuf_add (next, start, end - start); +/* remove the component from 'remaining' */ +strbuf_remove (remaining,0 , end - remaining->buf); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXDEPTH 5 +#ifndef MAXSYMLINKS +#define MAXSYMLINKS 32 +#endif /* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -21,7 +60,6 @@ int is_directory(const char *path)* absolute_path().) The return value is a pointer to a static * buffer. * -* The input and all intermediate paths must be shorter than MAX_PATH. * 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
@@ -33,22 +71,16 @@ int is_directory(const char *path)*/ static const char *real_path_internal (const char *path,int die_on_error) { -static struct strbuf sb =STRBUF_INIT ; +static struct strbuf resolved =STRBUF_INIT ; +struct strbuf remaining =STRBUF_INIT ; +struct strbuf next =STRBUF_INIT ; +struct strbuf symlink =STRBUF_INIT ;char *retval =NULL ; - -/* -* If we have to temporarily chdir(), store the original CWD -* here so that we can chdir() back to it at the end of the -* function: -*/ -struct strbuf cwd =STRBUF_INIT ; - -int depth =MAXDEPTH ; -char *last_elem =NULL ; +int num_symlinks =0 ;struct stat st;/* We've already done it */ -if (path == sb.buf) +if (path == resolved.buf)return path;if (!*path) {
@@ -58,74 +90,114 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&sb); -strbuf_addstr (&sb, path); - -while (depth--) { -if (!is_directory (sb.buf)) { -char *last_slash =find_last_dir_sep (sb.buf); -if (last_slash) { - last_elem =xstrdup (last_slash +1 ); -strbuf_setlen (&sb, last_slash - sb.buf +1 ); - }else { - last_elem =xmemdupz (sb.buf, sb.len); -strbuf_reset (&sb); - } +strbuf_reset (&resolved); + +if (is_absolute_path (path)) { +/* absolute path; start with only root as being resolved */ +int offset =offset_1st_component (path); +strbuf_add (&resolved, path, offset); +strbuf_addstr (&remaining, path + offset); + }else { +/* relative path; can use CWD as the initial resolved path */ +if (strbuf_getcwd (&resolved)) { +if (die_on_error) +die_errno ("unable to get current working directory" ); +else + goto error_out; } +strbuf_addstr (&remaining, path); + } -if (sb.len) { -if (!cwd.len &&strbuf_getcwd (&cwd)) { +/* Iterate over the remaining path components */ +while (remaining.len >0 ) { +get_next_component (&next, &remaining); + +if (next.len ==0 ) { +continue ;/* empty component */ + }else if (next.len ==1 && !strcmp (next.buf,"." )) { +continue ;/* '.' component */ + }else if (next.len ==2 && !strcmp (next.buf,".." )) { +/* '..' component; strip the last path component */ +strip_last_component (&resolved); +continue ; + } + +/* append the next component and resolve resultant path */ +if (!is_dir_sep (resolved.buf[resolved.len -1 ])) +strbuf_addch (&resolved,'/' ); +strbuf_addbuf (&resolved, &next); + +if (lstat (resolved.buf, &st)) { +/* error out unless this was the last component */ +if (errno !=ENOENT || remaining.len) {if (die_on_error) -die_errno ("Could not get current working directory" ); +die_errno ("Invalid path '%s'" , + resolved.buf);else goto error_out; } + }else if (S_ISLNK (st.st_mode)) { +ssize_t len; +strbuf_reset (&symlink); + +if (num_symlinks++ >MAXSYMLINKS ) { + errno =ELOOP ; -if (chdir (sb.buf)) {if (die_on_error) -die_errno ("Could not switch to '%s'" , - sb.buf); +die ("More than %d nested symlinks " +"on path '%s'" ,MAXSYMLINKS , path);else goto error_out; } - } -if (strbuf_getcwd (&sb)) { -if (die_on_error) -die_errno ("Could not get current working directory" ); -else - goto error_out; - } -if (last_elem) { -if (sb.len && !is_dir_sep (sb.buf[sb.len -1 ])) -strbuf_addch (&sb,'/' ); -strbuf_addstr (&sb, last_elem); -free (last_elem); - last_elem =NULL ; - } - -if (!lstat (sb.buf, &st) &&S_ISLNK (st.st_mode)) { -struct strbuf next_sb =STRBUF_INIT ; -ssize_t len =strbuf_readlink (&next_sb, sb.buf,0 ); + len =strbuf_readlink (&symlink, resolved.buf, + st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - sb.buf); + resolved.buf);else goto error_out; } -strbuf_swap (&sb, &next_sb); -strbuf_release (&next_sb); - }else -break ; + +if (is_absolute_path (symlink.buf)) { +/* absolute symlink; set resolved to root */ +int offset =offset_1st_component (symlink.buf); +strbuf_reset (&resolved); +strbuf_add (&resolved, symlink.buf, offset); +strbuf_remove (&symlink,0 , offset); + }else { +/* +* relative symlink +* strip off the last component since it will +* be replaced with the contents of the symlink +*/ +strip_last_component (&resolved); + } + +/* +* if there are still remaining components to resolve +* then append them to symlink +*/ +if (remaining.len) { +strbuf_addch (&symlink,'/' ); +strbuf_addbuf (&symlink, &remaining); + } + +/* +* use the symlink as the remaining components that +* need to be resloved +*/ +strbuf_swap (&symlink, &remaining); + } } - retval = sb.buf; + retval = resolved.buf; + error_out: -free (last_elem); -if (cwd.len &&chdir (cwd.buf)) -die_errno ("Could not change back to '%s'" , cwd.buf); -strbuf_release (&cwd); +strbuf_release (&remaining); +strbuf_release (&next); +strbuf_release (&symlink);return retval; }
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v5 2/5 real_path: convert real_path_internal to strbuf_realpath
In reply to Brandon WilliamsChange the name of real_path_internal to strbuf_realpath. In addition push the static strbuf up to its callers and instead take as a parameter a pointer to a strbuf to use for the final result.
This change makes strbuf_realpath reentrant.
--- abspath.c | 53 +++++++++++++++++++++++++---------------------------- cache.h | 2 ++ 2 files changed, 27 insertions(+), 28 deletions(-)
abspath.c +25 -28
@@ -57,21 +57,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 ;
@@ -79,10 +75,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" );
@@ -90,16 +82,16 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } -strbuf_reset (&resolved); +strbuf_reset (resolved);if (is_absolute_path (path)) {/* absolute path; start with only root as being resolved */ int offset =offset_1st_component (path); -strbuf_add (&resolved, path, offset); +strbuf_add (resolved, path, offset);strbuf_addstr (&remaining, path + offset); }else {/* relative path; can use CWD as the initial resolved path */ -if (strbuf_getcwd (&resolved)) { +if (strbuf_getcwd (resolved)) {if (die_on_error)die_errno ("unable to get current working directory" );else
@@ -118,21 +110,21 @@ static const char *real_path_internal(const char *path, int die_on_error)continue ;/* '.' component */ }else if (next.len ==2 && !strcmp (next.buf,".." )) {/* '..' component; strip the last path component */ -strip_last_component (&resolved); +strip_last_component (resolved);continue ; }/* append the next component and resolve resultant path */ -if (!is_dir_sep (resolved.buf[resolved.len -1 ])) -strbuf_addch (&resolved,'/' ); -strbuf_addbuf (&resolved, &next); +if (!is_dir_sep (resolved->buf[resolved->len -1 ])) +strbuf_addch (resolved,'/' ); +strbuf_addbuf (resolved, &next); -if (lstat (resolved.buf, &st)) { +if (lstat (resolved->buf, &st)) {/* error out unless this was the last component */ if (errno !=ENOENT || remaining.len) {if (die_on_error)die_errno ("Invalid path '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -150,12 +142,12 @@ static const char *real_path_internal(const char *path, int die_on_error) goto error_out; } - len =strbuf_readlink (&symlink, resolved.buf, + len =strbuf_readlink (&symlink, resolved->buf, st.st_size);if (len <0 ) {if (die_on_error)die_errno ("Invalid symlink '%s'" , - resolved.buf); + resolved->buf);else goto error_out; }
@@ -163,8 +155,8 @@ static const char *real_path_internal(const char *path, int die_on_error)if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ int offset =offset_1st_component (symlink.buf); -strbuf_reset (&resolved); -strbuf_add (&resolved, symlink.buf, offset); +strbuf_reset (resolved); +strbuf_add (resolved, symlink.buf, offset);strbuf_remove (&symlink,0 , offset); }else {/*
@@ -172,7 +164,7 @@ static const char *real_path_internal(const char *path, int die_on_error)* strip off the last component since it will * be replaced with the contents of the symlink */ -strip_last_component (&resolved); +strip_last_component (resolved); }/*
@@ -192,24 +184,29 @@ static const char *real_path_internal(const char *path, int die_on_error) } } - retval = resolved.buf; + retval = resolved->buf; error_out:strbuf_release (&remaining);strbuf_release (&next);strbuf_release (&symlink); +if (!retval) +strbuf_reset (resolved); +return retval; }const char *real_path (const char *path) { -return real_path_internal (path,1 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,1 ); }const char *real_path_if_valid (const char *path) { -return real_path_internal (path,0 ); +static struct strbuf realpath =STRBUF_INIT ; +return strbuf_realpath (&realpath, path,0 ); }/*
cache.h +2 -0
@@ -1064,6 +1064,8 @@ static inline int is_absolute_path(const char *path)return is_dir_sep (path[0 ]) ||has_dos_drive_prefix (path); }int is_directory (const char *); +char *strbuf_realpath (struct strbuf *resolved,const char *path, +int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path);const char *absolute_path (const char *path);
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v5 3/5 real_path: create real_pathdup
In reply to Brandon WilliamsCreate real_pathdup which returns a caller owned string of the resolved realpath based on the provide path.
--- abspath.c | 13 +++++++++++++ cache.h | 1 + 2 files changed, 14 insertions(+)
abspath.c +13 -0
@@ -209,6 +209,19 @@ const char *real_path_if_valid(const char *path)return strbuf_realpath (&realpath, path,0 ); } +char *real_pathdup (const char *path) +{ +struct strbuf realpath =STRBUF_INIT ; +char *retval =NULL ; + +if (strbuf_realpath (&realpath, path,0 )) + retval =strbuf_detach (&realpath,NULL ); + +strbuf_release (&realpath); + +return retval; +} +/* * Use this to get an absolute path from a relative one. If you want * to resolve links, you should use real_path.
cache.h +1 -0
@@ -1068,6 +1068,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, int die_on_error);const char *real_path (const char *path);const char *real_path_if_valid (const char *path); +char *real_pathdup (const char *path);const char *absolute_path (const char *path);const char *remove_leading_path (const char *in,const char *prefix);const char *relative_path (const char *in,const char *prefix,struct strbuf *sb);
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v5 4/5 real_path: have callers use real_pathdup and strbuf_realpath
In reply to Brandon WilliamsMigrate callers of real_path() who duplicate the return value to use real_pathdup or strbuf_realpath.
--- builtin/init-db.c | 6 +++--- environment.c | 2 +- setup.c | 13 ++++++++----- sha1_file.c | 2 +- submodule.c | 2 +- transport.c | 2 +- worktree.c | 2 +- 7 files changed, 16 insertions(+), 13 deletions(-)
builtin/init-db.c +3 -3
@@ -338,7 +338,7 @@ int init_db(const char *git_dir, const char *real_git_dir, {int reinit;int exist_ok = flags &INIT_DB_EXIST_OK ; -char *original_git_dir =xstrdup (real_path (git_dir)); +char *original_git_dir =real_pathdup (git_dir);if (real_git_dir) {struct stat st;
@@ -489,7 +489,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix) argc =parse_options (argc, argv, prefix, init_db_options, init_db_usage,0 );if (real_git_dir && !is_absolute_path (real_git_dir)) - real_git_dir =xstrdup (real_path (real_git_dir)); + real_git_dir =real_pathdup (real_git_dir);if (argc ==1 ) {int mkdir_tried =0 ;
@@ -560,7 +560,7 @@ int cmd_init_db(int argc, const char **argv, const char *prefix)const char *git_dir_parent =strrchr (git_dir,'/' );if (git_dir_parent) {char *rel =xstrndup (git_dir, git_dir_parent - git_dir); - git_work_tree_cfg =xstrdup (real_path (rel)); + git_work_tree_cfg =real_pathdup (rel);free (rel); }if (!git_work_tree_cfg)
environment.c +1 -1
@@ -259,7 +259,7 @@ void set_git_work_tree(const char *new_work_tree)return ; } git_work_tree_initialized =1 ; - work_tree =xstrdup (real_path (new_work_tree)); + work_tree =real_pathdup (new_work_tree); }const char *get_git_work_tree (void )
setup.c +8 -5
@@ -256,8 +256,10 @@ int get_common_dir_noenv(struct strbuf *sb, const char *gitdir)strbuf_addbuf (&path, &data);strbuf_addstr (sb,real_path (path.buf)); ret =1 ; - }else + }else {strbuf_addstr (sb, gitdir); + } +strbuf_release (&data);strbuf_release (&path);return ret;
@@ -692,7 +694,7 @@ static const char *setup_discovered_git_dir(const char *gitdir,/* --work-tree is set without --git-dir; use discovered one */ if (getenv (GIT_WORK_TREE_ENVIRONMENT ) || git_work_tree_cfg) {if (offset != cwd->len && !is_absolute_path (gitdir)) - gitdir =xstrdup (real_path (gitdir)); + gitdir =real_pathdup (gitdir);if (chdir (cwd->buf))die_errno ("Could not come back to cwd" );return setup_explicit_git_dir (gitdir, cwd, nongit_ok);
@@ -800,11 +802,12 @@ static int canonicalize_ceiling_entry(struct string_list_item *item,/* Keep entry but do not canonicalize it */ return 1 ; }else { -const char *real_path =real_path_if_valid (ceil); -if (!real_path) +char *real_path =real_pathdup (ceil); +if (!real_path) {return 0 ; + }free (item->string); - item->string =xstrdup (real_path); + item->string = real_path;return 1 ; } }
sha1_file.c +1 -1
@@ -291,7 +291,7 @@ static int link_alt_odb_entry(const char *entry, const char *relative_base,struct strbuf pathbuf =STRBUF_INIT ;if (!is_absolute_path (entry) && relative_base) { -strbuf_addstr (&pathbuf,real_path (relative_base)); +strbuf_realpath (&pathbuf, relative_base,1 );strbuf_addch (&pathbuf,'/' ); }strbuf_addstr (&pathbuf, entry);
submodule.c +1 -1
@@ -1301,7 +1301,7 @@ void connect_work_tree_and_git_dir(const char *work_tree, const char *git_dir) {struct strbuf file_name =STRBUF_INIT ;struct strbuf rel_path =STRBUF_INIT ; -const char *real_work_tree =xstrdup (real_path (work_tree)); +const char *real_work_tree =real_pathdup (work_tree);/* Update gitfile */ strbuf_addf (&file_name,"%s/.git" , work_tree);
transport.c +1 -1
@@ -1146,7 +1146,7 @@ static int refs_from_alternate_cb(struct alternate_object_database *e,const struct ref *extra;struct alternate_refs_data *cb = data; - other =xstrdup (real_path (e->path)); + other =real_pathdup (e->path); len =strlen (other);while (other[len-1 ] =='/' )
worktree.c +1 -1
@@ -255,7 +255,7 @@ struct worktree *find_worktree(struct worktree **list,return wt; arg =prefix_filename (prefix,strlen (prefix), arg); - path =xstrdup (real_path (arg)); + path =real_pathdup (arg);for (; *list; list++)if (!fspathcmp (path,real_path ((*list)->path)))break ;
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH v5 5/5 real_path: canonicalize directory separators in root parts
In reply to Brandon WilliamsFrom: Johannes Sixt <[email protected]>
When an absolute path is resolved, resolution begins at the first path component after the root part. The root part is just copied verbatim, because it must not be inspected for symbolic links. For POSIX paths, this is just the initial slash, but on Windows, the root part has the forms c:\ or \\server\share. We do want to canonicalize the back-slashes in the root part because these parts are compared to the result of getcwd(), which does return a fully canonicalized path.
Factor out a helper that splits off the root part, and have it canonicalize the copied part.
This change was prompted because t1504-ceiling-dirs.sh caught a breakage in GIT_CEILING_DIRECTORIES handling on Windows.
--- abspath.c | 29 +++++++++++++++++------------ 1 file changed, 17 insertions(+), 12 deletions(-)
abspath.c +17 -12
@@ -48,6 +48,19 @@ static void get_next_component(struct strbuf *next, struct strbuf *remaining)strbuf_remove (remaining,0 , end - remaining->buf); } +/* copies root part from remaining to resolved, canonicalizing it on the way */ +static void get_root_part (struct strbuf *resolved,struct strbuf *remaining) +{ +int offset =offset_1st_component (remaining->buf); + +strbuf_reset (resolved); +strbuf_add (resolved, remaining->buf, offset); +#ifdef GIT_WINDOWS_NATIVE +convert_slashes (resolved->buf); +#endif +strbuf_remove (remaining,0 , offset); +} +/* We allow "recursive" symbolic links. Only within reason, though. */ #ifndef MAXSYMLINKS #define MAXSYMLINKS 32
@@ -82,14 +95,10 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path, goto error_out; } -strbuf_reset (resolved); +strbuf_addstr (&remaining, path); +get_root_part (resolved, &remaining); -if (is_absolute_path (path)) { -/* absolute path; start with only root as being resolved */ -int offset =offset_1st_component (path); -strbuf_add (resolved, path, offset); -strbuf_addstr (&remaining, path + offset); - }else { +if (!resolved->len) {/* relative path; can use CWD as the initial resolved path */ if (strbuf_getcwd (resolved)) {if (die_on_error)
@@ -97,7 +106,6 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,else goto error_out; } -strbuf_addstr (&remaining, path); }/* Iterate over the remaining path components */
@@ -154,10 +162,7 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,if (is_absolute_path (symlink.buf)) {/* absolute symlink; set resolved to root */ -int offset =offset_1st_component (symlink.buf); -strbuf_reset (resolved); -strbuf_add (resolved, symlink.buf, offset); -strbuf_remove (&symlink,0 , offset); +get_root_part (resolved, &symlink); }else {/* * relative symlink
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
Re: [PATCH v5 0/5] road to reentrant real_path
In reply to Brandon WilliamsQuoted 5 lines · Brandon Williams
changes in v5:
* set errno to ELOOP when MAXSYMLINKS is exceded.
* revert to use MAXSYMLINKS instead of MAXDEPTH.
* If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.
Quoted 9 lines
Brandon Williams (4):
real_path: resolve symlinks by hand
real_path: convertreal_path_internaltostrbuf_realpath
real_path: createreal_pathdup
real_path: have callers usereal_pathdupandstrbuf_realpathJohannes Sixt (1):
real_path: canonicalize directory separators in root parts
How does this relate to the 5-patch real_path: series that has been on 'next' since last year?
Re: [PATCH v5 0/5] road to reentrant real_path
Quoted 13 lines · Junio C Hamano
Quoted 11 lines · Brandon Williams
changes in v5:
* set errno to ELOOP when MAXSYMLINKS is exceded.
* revert to use MAXSYMLINKS instead of MAXDEPTH.
* If the OS hasn't defined MAXSYMLINKS, use a fallback value of 32.Brandon Williams (4):
real_path: resolve symlinks by hand
real_path: convertreal_path_internaltostrbuf_realpath
real_path: createreal_pathdup
real_path: have callers usereal_pathdupandstrbuf_realpath
Quoted 6 lines
Quoted 3 lines
Johannes Sixt (1):
real_path: canonicalize directory separators in root partsHow does this relate to the 5-patch
real_path: series that has been on 'next' since last year?
The only difference should be in the first patch of the series which handles the #define a bit differently due to the discussion that happened last week.
Here is the interdiff between v5 and 'origin/bw/realpath-wo-chdir':
abspath.c +5 -1
@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) }/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXSYMLINKS 5 +#ifndef MAXSYMLINKS +#define MAXSYMLINKS 32 +#endif /* * Return the real path (i.e., absolute path, with symlinks resolved
@@ -139,6 +141,8 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,strbuf_reset (&symlink);if (num_symlinks++ >MAXSYMLINKS ) { + errno =ELOOP ; +if (die_on_error)die ("More than %d nested symlinks " "on path '%s'" ,MAXSYMLINKS , path);
Signature · 1 line
Brandon Williams
Re: [PATCH v5 0/5] road to reentrant real_path
Quoted 8 lines · Brandon Williams
Quoted 2 lines
How does this relate to the 5-patch
real_path: series that has been on 'next' since last year?The only difference should be in the first patch of the series which handles the #define a bit differently due to the discussion that happened last week.
Here is the interdiff between v5 and 'origin/bw/realpath-wo-chdir':
Then can you make that an incremental patch (or two) with its own log message instead? It (or they) would look and smell like a bugfix patch that follows up a change that has already landed. As you know, we won't eject and replace patches that are already in 'next' during a development cycle.
Thanks.
Quoted 25 lines
abspath.c +5
-1@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) }/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXSYMLINKS 5 +#ifndef MAXSYMLINKS +#define MAXSYMLINKS 32 +#endif /* * Return the real path (i.e., absolute path, with symlinks resolved @@ -139,6 +141,8 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,strbuf_reset (&symlink);if (num_symlinks++ >MAXSYMLINKS ) { + errno =ELOOP ; +if (die_on_error)die ("More than %d nested symlinks " "on path '%s'" ,MAXSYMLINKS , path);
Re: [PATCH v5 0/5] road to reentrant real_path
Quoted 11 lines · Junio C Hamano
Quoted 8 lines · Brandon Williams
Quoted 2 lines
How does this relate to the 5-patch
real_path: series that has been on 'next' since last year?The only difference should be in the first patch of the series which handles the #define a bit differently due to the discussion that happened last week.
Here is the interdiff between v5 and 'origin/bw/realpath-wo-chdir':
Quoted 7 lines
Then can you make that an incremental patch (or two) with its own log message instead? It (or they) would look and smell like a bugfix patch that follows up a change that has already landed. As you know, we won't eject and replace patches that are already in 'next' during a development cycle.
Thanks.
Yes I'll get right on that.
Signature · 1 line
Brandon Williams
Re: [PATCH v5 0/5] road to reentrant real_path
Quoted 18 lines · Brandon Williams
Quoted 17 lines · Junio C Hamano
Quoted 8 lines · Brandon Williams
Quoted 2 lines
How does this relate to the 5-patch
real_path: series that has been on 'next' since last year?The only difference should be in the first patch of the series which handles the #define a bit differently due to the discussion that happened last week.
Here is the interdiff between v5 and 'origin/bw/realpath-wo-chdir':
Then can you make that an incremental patch (or two) with its own log message instead? It (or they) would look and smell like a bugfix patch that follows up a change that has already landed. As you know, we won't eject and replace patches that are already in 'next' during a development cycle.
Quoted 3 lines
Quoted 1 line
Thanks.
Yes I'll get right on that.
Thanks. Will queue.
PATCH 1/2 real_path: prevent redefinition of MAXSYMLINKS
In reply to Junio C HamanoThe macro 'MAXSYMLINKS' is already defined on macOS and Linux in <sys/param.h>. If 'MAXSYMLINKS' has already been defined, use the value defined by the OS otherwise default to a value of 32 which is more inline with what is allowed by many systems.
--- abspath.c | 4 +++- 1 file changed, 3 insertions(+), 1 deletion(-)
abspath.c +3 -1
@@ -62,7 +62,9 @@ static void get_root_part(struct strbuf *resolved, struct strbuf *remaining) }/* We allow "recursive" symbolic links. Only within reason, though. */ -#define MAXSYMLINKS 5 +#ifndef MAXSYMLINKS +#define MAXSYMLINKS 32 +#endif /* * Return the real path (i.e., absolute path, with symlinks resolved
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog
PATCH 2/2 real_path: set errno when max number of symlinks is exceeded
Set errno to ELOOP when the maximum number of symlinks is exceeded, as would be done by other symlink-resolving functions.
--- abspath.c | 2 ++ 1 file changed, 2 insertions(+)
abspath.c +2 -0
@@ -141,6 +141,8 @@ char *strbuf_realpath(struct strbuf *resolved, const char *path,strbuf_reset (&symlink);if (num_symlinks++ >MAXSYMLINKS ) { + errno =ELOOP ; +if (die_on_error)die ("More than %d nested symlinks " "on path '%s'" ,MAXSYMLINKS , path);
Signature · 1 line
2.11.0.390.gc69c2f50cf-goog