Re: [PATCH 1/3] strbuf: add strbuf_add_cwd()

["René Scharfe" <l.s.r@web.de>]

Am 20.07.2014 14:33, schrieb Duy Nguyen:
> On Sun, Jul 20, 2014 at 6:21 PM, René Scharfe <l.s.r@web.de> wrote:
>> +int strbuf_add_cwd(struct strbuf *sb)
>> +{
>> +       size_t oldalloc = sb->alloc;
>> +       size_t guessed_len = 32;
>
> For Linux, I think this is enough to succesfully get cwd in the first
> pass. Windows' $HOME is usually deep in C:\Users\Blahblah. Maybe
> increase this to 128? And we probably want to keep the guessed value,
> so if the first strbuf_add_cwd needs a few rounds to get cwd, the next
> strbuf_add_cwd() call does not.

The initial number is debatable, sure.  I just copied the 32 from 
strbuf_readline().

"C:\Users\John Doe\Documents\Projects\foo" (or whatever) isn't thaaat 
long either, though.  FWIW, the longest (non-hidden) path in my home on 
Windows 8.1 is 139 characters long (as reported by dir -r | %{ 
$_.FullName.Length } | sort -desc | select -f 1), but there are no git 
projects in that directory.  Not sure if that means 128 would be a 
better start value, but I don't mind changing it in any case.

Before adding performance optimizations like remembering the last cwd 
length I'd rather see measurements that demonstrate their use.  I doubt 
we'll see getcwd() show up high on a profile (but didn't measure, except 
for a run of t/perf).

>> +
>> +       for (;; guessed_len *= 2) {
>> +               char *cwd;
>> +
>> +               strbuf_grow(sb, guessed_len);
>> +               cwd = getcwd(sb->buf + sb->len, sb->alloc - sb->len);
>> +               if (cwd) {
>> +                       strbuf_setlen(sb, sb->len + strlen(cwd));
>> +                       return 0;
>> +               }
>> +               if (errno != ERANGE)
>> +                       break;
>> +       }
>> +       if (oldalloc == 0)
>> +               strbuf_release(sb);
>> +       return -1;
>> +}
>
> The loop and the following strbuf_release() are correct. But I wonder
> if it's easier to read if you save getcwd in a separate/local strbuf
> variable and only concat it to "sb" when you successfully get cwd..

Adding an extra allocation and string copy sound more complicated.  But 
you are right that the function is more complicated than necessary. 
Reroll coming..

René