Re: [PATCH] bundle, fast-import: detect write failure

From: Pierre Habouzit <madcoder@artemis.madism.org>
Date: Jan 10, 2008, 16:25 UTC
Message-ID: <20080110162526.GB27808@artemis.madism.org>
In-Reply-To: <87hchlhm3k.fsf@rho.meyering.net>

On Thu, Jan 10, 2008 at 01:00:15PM +0000, Jim Meyering wrote:

Show 13 quoted lines

> Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:
> > Are you aware what this code does?  It writes a ".keep" file.  Whose
> > purpose is to _exist_, and whose purpose is fulfilled, even if the write
> > or the push-back did not succeed.
> 
> Hi,
> 
> I do see what you mean.
> 
> If the write is not necessary, then perhaps you would prefer a comment
> documenting that failures of the write and following close are ignorable.
> And add a '(void)' stmt prefix, to tell compilers that ignoring the
> return value is deliberate.

  Note that (void) isn't enough with the most recent gcc flavours, which
is a pain. I do use:

#define IGNORE(expr)  do { if (expr) (void)0; } while (0)

for that purpose in my code. I know IGNORE isn't a brilliant name, but it's modeled after the ocaml function doing the same thing.

> However, even if it's not technically required to fail at that point,
> if it were my choice, I'd prefer to know when a .keep file whose
> contents are unimportant just happens to reside on a bad spot on my
> disk.  I/O errors should never be ignored.

  Actually I think .keep files are empty, so the write() should not be
there in the first place, and we should only check for close() right ?
not that it matters that much.

Show 7 quoted lines

> > I could not care less what the manual says.  What is important is if the
> > defensive programming is done mindlessly, and therefore can fail so not
> > gracefully.
> 
> On the other hand, if that write failure is truly ignorable,
> a mindless minimalist :-) might argue that it's best just to
> omit the syscall.

  And leak a file descriptor :)

-- 
·O·  Pierre Habouzit
··O                                                madcoder@debian.org
OOO                                                http://www.madism.org