On Wed, Jan 17, 2018 at 12:54:33PM -0800, Junio C Hamano wrote:
> Jeff Hostetler <g...@jeffhostetler.com> writes:
>
> >> void sha1_file_name(struct strbuf *buf, const unsigned char
> >> *sha1)
> >> {
> >> - strbuf_addf(buf, "%s/", get_object_directory());
> >> + const char *obj_dir = get_object_directory();
> >> + size_t extra = strlen(obj_dir) + 1 + GIT_MAX_HEXSZ;
> >
> > Very minor nit. Should this be "+3" rather than "+1"?
> > One for the slash after obj_dir, one for the slash between
> > "xx/y[38]", and one for the trailing NUL.
> >
> >> + if (extra > strbuf_avail(buf))
> >> + strbuf_grow(buf, extra);
>
> The callers who care use static strbuf with 1/2, which lets them
> grow it to an appropriate size after they make their first call.
>
> On the other hand, the ones to which performance does not matter by
> definition do not care.
>
> I actually think this whole "extra -> grow" business should be
> discarded. With a miscomputed "extra" like this, it does not help
> anybody---everybody may pay cost for one extra realloc due to the
> miscalculation, and the ones that do care also do during their first
> call.
Let me second that. The diffstat here, along with the magic numbers, is
not really encouraging unless we have a demonstrable speedup. In which
case we can then measure and compare other approaches, like pushing a
static strbuf farther up the stack. But without that, it feels like
stumbling around in the dark.
-Peff
