On Wed, Oct 10 2018, Junio C Hamano wrote: > Brandon Casey <drafnel@xxxxxxxxx> writes: > >> ... Again, I don't feel strongly about it, but I'm not >> sure this change actually improves the code. > > Yeah, in the context of the current caller, this is a safe change > that does not break anybody and reduces the number of instructions > executed in this codepath. A mistaken caller may be added in the > future that fails to check auto-threashold beforehand, but that > won't lead to anything bad like looping for a large number of times, > so as long as the API contract into this helper function is clear > that callers are responsible to check beforehand, it is still not > too bad. > > So, I'd throw this into "Meh - I won't regret applying it, but it is > not the end of the world if I forget to apply it, either" pile. > > I _think_ a change that actually improves the code would be to > restructure so that there is a helper that is responsible for > guestimating the number of loose objects, and another that uses the > helper to see if there are too many loose objects. The latter is > the only one tha needs to know about auto-threashold. But we are > not in immdiate need for such a clean-up, I guess, unless somebody > is actively looking into revamping how auto-gc works and doing a > preparatory clean-up. Yeah, that's me :) I have some WIP gc cleanup, but want to sit on it a bit before I submit it to think about the best way to do things. So in the meantime I was sending out a few WIP bits that I expected could be reviewed stand-alone. So I'd prefer to have this applied. It's easy enough to understand that shouldn't take long to prove to be correct & trickle down to "master", and will make those subsequent patches easier to follow.
