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

From: René Scharfe <l.s.r@web.de>
Date: Jul 20, 2014, 15:21 UTC
Message-ID: <53CBDE8F.7000804@web.de>
In-Reply-To: <CACsJy8BHJ8UFbnEn6C7FiA2SNvL5hZSZg5iDd6CQPDxt+bUJHQ@mail.gmail.com>

Am 20.07.2014 14:33, schrieb Duy Nguyen:

Show 11 quoted lines

> 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).

Show 21 quoted lines

>> +
>> +       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é