Re: [PATCH 06/10] grep: remove #ifdef NO_PTHREADS

[Jeff King <peff@peff.net>]

On Mon, Oct 29, 2018 at 11:16:39AM +0900, Junio C Hamano wrote:

> Jeff King <peff@peff.net> 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