Re: [RFC PATCH 1/2] Report exec errors from run-command

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 25, 2009, 07:35 UTC
Message-ID: <7vr5qjecbb.fsf@alter.siamese.dyndns.org>
In-Reply-To: <1261676971-3285-2-git-send-email-ilari.liusvaara@elisanet.fi>

Ilari Liusvaara <ilari.liusvaara@elisanet.fi> writes:

Show 8 quoted lines

> Previously run-command was unable to report errors happening in exec
> call. Change it to pass errno from failed exec to errno value at
> return.
>
> The errno value passing can be done by opening close-on-exec pipe and
> piping the error code through in case of failure. In case of success,
> close-on-exec closes the pipe on successful exec and parent process
> gets end of file on read.

Thanks; I think overall this is a good idea, even though I have no clue if WIN32 side wants a similar support.

 - At first reading, the "while (close(fd) < 0 && errno != EBADF);"
   pattern was a bit of eyesore.  It might be worth factoring that out to
   a small static helper function that a smart compiler would
   automatically inline (or mark it as a static inline).

 - Is it guaranteed that a failed pipe(2) never touches its int fildes[2]
   parameter, or the values are undefined when it fails?  The approach
   would save one extra variable, compared to an alternative approach to
   keep an explicit variable to record a pipe failure, but It feels iffy
   that the code relies on them being left as their initial -1 values.

 - Should we worry about partial write as well (you seem to warn when you
   get a partial read)?  Is it worth using xread() and friends found in
   wrapper.c instead of goto read/write_again?

 - Shouldn't any of these warning() be die() instead?

 - The two extra file descriptors this patch uses are allocated after all
   the existing pipe flipping is done, and all the dup's done in the child
   process are to use dup2() to set up the known file descriptors at low
   numbers, so I don't think we have to worry about this patch changing
   the behaviour of the process pair by changing the assignment of file
   descriptors (we had a bug or two in the past that made subprocess
   misbehave under some funky condition, e.g. run with fd#0 closed).