On Thu, Sep 29, 2016 at 06:18:03PM -0700, Linus Torvalds wrote: > On Thu, Sep 29, 2016 at 5:57 PM, Linus Torvalds > <torvalds@xxxxxxxxxxxxxxxxxxxx> wrote: > > > > Actually, all the other cases seem to be "parse a SHA1 with a known > > length", so they really don't have a negative length. So this seems > > ok, and is easier to verify than the "what all contexts might use > > DEFAULT_ABBREV" thing. There's only a few callers, and it's a static > > function so it's easy to check it locally in sha1_name.c. > > Here's my original patch with just a tiny change that instead of > starting the automatic guessing at 7 each time, it starts at > "default_automatic_abbrev", which is initialized to 7. > > The difference is that if we decide that "oh, that was too small, need > to repeat", we also update that "default_automatic_abbrev" value, so > that we won't start at the number that we now know was too small. > > So it still loops over the abbrev values, but now it only loops a > couple of times. > > I actually verified the performance impact by doing > > time git rev-list --abbrev-commit HEAD > /dev/null > > on the kernel git tree, and it does actually matter. With my original > patch, we wasted a noticeable amount of time on just the extra > looping, with this it's down to the same performance as just doing it > once at init time (it's about 12s vs 9s on my laptop). I agree that this deals with the performance concerns by caching the default_abbrev_len and starting there. I still think it's unnecessarily invasive to touch get_short_sha1() at all, which is otherwise only a reading function. So IMHO, the best combination is the init_default_abbrev() you posted in [1], but initialized at the top of find_unique_abbrev(). And cached there, obviously, in a similar way. -Peff [1] http://public-inbox.org/git/CA+55aFyVEQ+8TBBUm5KG9APtd9wy8cp_mRO=3nj12DXZNLAC9A@xxxxxxxxxxxxxx/