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

On Thu, Dec 24, 2009 at 11:35:04PM -0800, Junio C Hamano wrote:

Ilari Liusvaara [off-list ref] writes:

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

It would. But I have no clue about WIN32. But there are other people
on this list who have. :-)
 

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

Done (with bit of reformatting to add space, line break and comment.

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

I added explicit set to -1 on failure case (I think failed pipe doesn't touch
those, but you never know about what some oddball OS is going to do).

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

That's hairy code. One really can't print any errors in write path, as those
errors would go to who knows where due to redirections (I took the error
warning out).

That partial read warning is more for detecting 'can't happen' situations
since pipe should be large enough to atomically transfer integer.

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

If error reporting failures are fatal, all of them.

-Ilari