Re: [PATCH 1/2] sequencer: factor out rewrite_file()

[Johannes Schindelin <Johannes.Schindelin@gmx.de>]

[-- Attachment #1: Type: text/plain, Size: 2059 bytes --]

Hi Peff,

On Wed, 1 Nov 2017, Jeff King wrote:

> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> 
> > Reduce code duplication by extracting a function for rewriting an
> > existing file.
> 
> These patches look like an improvement on their own, but I wonder if we
> shouldn't just be using the existing write_file_buf() for this?
> 
> Compared to your new function:
> 
> > +static int rewrite_file(const char *path, const char *buf, size_t len)
> > +{
> > +	int rc = 0;
> > +	int fd = open(path, O_WRONLY);
> > +	if (fd < 0)
> > +		return error_errno(_("could not open '%s' for writing"), path);
> > +	if (write_in_full(fd, buf, len) < 0)
> > +		rc = error_errno(_("could not write to '%s'"), path);
> > +	if (!rc && ftruncate(fd, len) < 0)
> > +		rc = error_errno(_("could not truncate '%s'"), path);
> > +	close(fd);
> > +	return rc;
> > +}
> 
>   - write_file_buf() uses O_TRUNC instead of ftruncate (but you end up
>     there in your second patch)
> 
>   - it uses O_CREAT, which I think would be OK (we do not expect to
>     create the file, but it would work fine when the file does exist).
> 
>   - it calls die() rather than returning an error. Looking at the
>     callsites, I'm inclined to say that would be fine. Failing to write
>     to the todo file is essentially a fatal error for sequencer code.

I spent substantial time on making the sequencer code libified (it was far
from it). That die() call may look okay now, but it is not at all okay if
we want to make Git's source code cleaner and more reusable. And I want
to.

So my suggestion is to clean up write_file_buf() first, to stop behaving
like a drunk lemming, and to return an error value already, and only then
use it in sequencer.c.

Ciao,
Dscho

P.S.: The existing callers of write_file_buf() don't care because they are
builtins, and for some reason we deem it okay for code in builtins to
simply die() deep in the call chains, without any way for callers to give
advice how to get out of the current mess.