From: Jeff King <peff@peff.net>
Date: Thu, 27 Jun 2024 00:33:23 GMT
Subject: Re: Re* [PATCH v5] describe: refresh the index when 'broken' flag is used
Message-ID: <20240627003323.GA1460039@coredump.intra.peff.net>
In-Reply-To: <20240626203415.GA441931@coredump.intra.peff.net>

On Wed, Jun 26, 2024 at 04:34:17PM -0400, Jeff King wrote:

> >  * What is this thing trying to do?  When run_command() fails, it
> >    wants to leave cp.out open, so that the caller this returns to
> >    can write into it???  That cannot be the case, as cp itself is
> >    internal.  So does this "close(cp.out)" really matter?
> 
> I think it's totally broken. Using cp.out, cp.in, etc, with
> run_command() is a deadlock waiting to happen, since it implies opening
> a pipe, not doing anything with our end, and then doing a waitpid() on
> the child.
> 
> You'd always want to use start_command(), and then do something useful
> with the pipe, and then finish_command(). Arguably run_command() should
> bug if cp.out, etc are non-zero.

Oh, I guess I should have looked at the code. We do indeed BUG() in this
case already, courtesy of c29b3962af (run-command: forbid using
run_command with piped output, 2015-03-22).

So all is well there. This caller is still completely wrong to call
close() though.

-Peff