Am 20.07.2014 14:33, schrieb Duy Nguyen:
On Sun, Jul 20, 2014 at 6:21 PM, René Scharfe <l.s.r@xxxxxx> 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é -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html