On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote: > Jeff King <peff@xxxxxxxx> writes: > > > Cases like this are kind of weird. I'd expect to see wait_all() return > > immediately when !HAVE_THREADS. But we don't need to, because later we > > do this: > > > >> - if (num_threads) > >> + if (HAVE_THREADS && num_threads) > >> hit |= wait_all(); > > > > Which I think works, but it feels like we're introducing some subtle > > dependencies about which functions get called in which cases. I'd hoped > > in general that the non-threaded code paths could mostly just collapse > > down into the same as the "threads == 1" cases, and we wouldn't have to > > ask "are threads even supported" in a lot of places. > > True, but the way "grep" code works with threads is already strange, > and I suspect that the subtle strangeness you feel mostly comes from > that. The single threaded code signaled "hit" with return value of > individual functions like grep_file(), but the meaning of return > value from them is changed to "does not matter--we do not have > meaningful result yet at this point" when threading is used. > > In the new world order where "threading is the norm but > single-threaded is still supported", I wonder if we would want to > drive the single threaded case the same way with the add_work(run) > interface and return the "did we see hits?" etc. from the same (or > at lesat "more similar than today's") interface, so that the flow of > data smells the same in both cases. Right, your second paragraph here is a better statement of what I was trying to get at. ;) But if the problem is simply that we are not quite there yet in the grep code, I am OK with taking this as the first pass, and knowing that there is more cleanup to be done later (though that sort of thing is IMHO very useful in a commit message). -Peff
