On Mon, Dec 5, 2016 at 10:58 AM, Brandon Williams <bmwill@xxxxxxxxxx> wrote:
> 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.
>
> Signed-off-by: Brandon Williams <bmwill@xxxxxxxxxx>
Thanks for working on this, some comments below:
>
> +/* 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?
> +/* 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' ?
> + 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.
> + } 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.
> +
> + if (next.len == 0) {
> + continue; /* empty component */
which means we resolve over path//with//double//slashes just fine,
as well as /./ parts. :)
> }
>
> - 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.
> + 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)
> + //strbuf_release(&resolved);
This is why the cover letter toned down expectations ?
(no // as comment, maybe remove that line?)
Thanks,
Stefan