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

From: Junio C Hamano <gitster@pobox.com>
Date: Oct 29, 2018, 02:16 UTC
Message-ID: <xmqqo9bd8pko.fsf@gitster-ct.c.googlers.com>
In-Reply-To: <20181027074443.GE26685@sigill.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 13 quoted lines

> 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.

Show 5 quoted lines

> I dunno. My comment isn't really an objection exactly, but mostly just
> an indication that I'm still thinking through what the best approach is,
> and what end state we want to end up in.
>
> -Peff