Re: [PATCH] real_path: make real_path thread-safe

From: Stefan Beller <sbeller@google.com>
Date: Dec 5, 2016, 20:38 UTC
Message-ID: <CAGZ79kbNMS-Hy=6y6EupcnwL58bhM=pesA8Mk_G+tjkL4yNNVA@mail.gmail.com>
In-Reply-To: <20161205201237.GD68588@google.com>

On Mon, Dec 5, 2016 at 12:12 PM, Brandon Williams <bmwill@google.com> wrote:

Show 7 quoted lines

>> > +       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 '/'

Oh, sure, that makes sense. When porting/running this on Windows, does the assumption still hold?

Show 9 quoted 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.

Show 10 quoted lines

>> > +               } 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.

>  And yes, lots of string manipulation
> requires lots of strbufs :)