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

[Jeff King]

On Sat, Nov 04, 2017 at 07:36:43PM +0100, Simon Ruderich wrote:

> On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:
> > I think we've been gravitating towards error strbufs, which would make
> > it something like:
> 
> I like this approach to store the error in a separate variable
> and let the caller handle it. This provides proper error messages
> and is cleaner than printing the error on the error site (what
> error_errno does).
> 
> However I wouldn't use strbuf directly and instead add a new
> struct error which provides a small set of helper functions.
> Using a separate type also makes it clear to the reader that is
> not a normal string and is more extendable in the future.

Yes, I think what you've written here (and below) is quite close to the
error_context patches I linked elsewhere in the thread. In other
words, I think it's a sane approach.

> We could also store the error condition in the error struct and
> don't use the return value to indicate and error like this:
> 
> void write_file_buf(const char *path, const char *buf, size_t len)
> {
> struct error err = ERROR_INIT;
> write_file_buf_gently2(path, buf, len, );
> if (err.error)
> error_die();
> }

I agree it might be nice for the error context to have a positive "there
was an error" flag. It's probably worth making it redundant with the
return code, though, so callers can use whichever style is most
convenient for them.

> > OTOH, if we went all-in on flexible error handling contexts, you could
> > imagine this function becoming:
> >
> >   void write_file_buf(const char *path, const char *buf, size_t len,
> >   struct error_context *err)
> >   {
> > int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
> > if (fd < 0)
> > return -1;
> > if (write_in_full(fd, buf, len, err) < 0)
> > return -1;
> > if (xclose(fd, err) < 0)
> > return -1;
> > return 0;
> >   }
> 
> This looks interesting as well, but it misses the feature of
> custom error messages which is really useful.

Right, I didn't think that example through. The functions after the
open() don't have enough information to make a good message.

-Peff

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

[Simon Ruderich]

On Fri, Nov 03, 2017 at 03:13:10PM -0400, Jeff King wrote:
> I think we've been gravitating towards error strbufs, which would make
> it something like:

I like this approach to store the error in a separate variable
and let the caller handle it. This provides proper error messages
and is cleaner than printing the error on the error site (what
error_errno does).

However I wouldn't use strbuf directly and instead add a new
struct error which provides a small set of helper functions.
Using a separate type also makes it clear to the reader that is
not a normal string and is more extendable in the future.

> I'm not excited that the amount of error-handling code is now double the
> amount of code that actually does something useful. Maybe this function
> simply isn't large/complex enough to merit flexible error handling, and
> we should simply go with René's original near-duplicate.

A separate struct (and helper functions) would help in this case
and could look like this, which is almost equal (in code size) to
the original solution using error_errno:

int write_file_buf_gently2(const char *path, const char *buf, size_t len, 
struct error *err)
{
int rc = 0;
int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
if (fd < 0)
return error_addf_errno(err, _("could not open '%s' for 
writing"), path);
if (write_in_full(fd, buf, len) < 0)
rc = error_addf_errno(err, _("could not write to '%s'"), 
path);
if (close(fd) && !rc)
rc = error_addf_errno(err, _("could not close '%s'"), path);
return rc;
}

(I didn't touch write_in_full here, but it could also take the
err and then the code would get a little shorter, however would
lose the "path" information, but see below.)

And in the caller:

void write_file_buf(const char *path, const char *buf, size_t len)
{
struct error err = ERROR_INIT;
if (write_file_buf_gently2(path, buf, len, ) < 0)
error_die();
}

For now struct error just contains the strbuf, but one could add
the call location (by using a macro for error_addf_errno) or the
original errno or more information in the future.

error_addf_errno() could also prepend the error the buffer so
that the caller can add more information if necessary and we get
something like: "failed to write file 'foo': write failed: errno
text" in the write_file_buf case (the first error string is from
write_file_buf_gently2, the second from write_in_full). However
I'm not sure how well this works with translations.

We could also store the error condition in the error struct and
don't use the return value to indicate and error like this:

void write_file_buf(const char *path, const char *buf, size_t len)
{
struct error err = ERROR_INIT;
write_file_buf_gently2(path, buf, len, );
if (err.error)
error_die();
}

> OTOH, if we went all-in on flexible error handling contexts, you could
> imagine this function becoming:
>
>   void write_file_buf(const char *path, const char *buf, size_t len,
>   struct error_context *err)
>   {
>   int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
>   if (fd < 0)
>   return -1;
>   if (write_in_full(fd, buf, len, err) < 0)
>   return -1;
>   if (xclose(fd, err) < 0)
>   return -1;
>   return 0;
>   }

This looks interesting as well, but it misses the feature of
custom error messages which is really useful.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

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

[Jeff King]

On Sat, Nov 04, 2017 at 10:05:43AM +0100, René Scharfe wrote:

> >> How about *not* printing the error at the place where you notice the
> >> error, and instead return an error code to the caller to be noticed
> >> which dies with an error message?
> > 
> > That ends up giving less-specific errors.
> 
> Not necessarily.  Function could return different codes for different
> errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and
> the caller could use that to select the message to show.
> 
> Basically all of the messages in wrapper.c consist of some text mixed
> with the affected path path and a strerror(3) string, so they're
> compatible in that way.  A single function (get_path_error_format()?)
> could thus be used and callers would be able to combine its result with
> die(), error(), or warning().

I think we've had this discussion before, a while ago. Yes, returning an
integer error code is nice because you don't have pass in an extra
parameter. But I think there are two pitfalls:

  1. Integers may not be descriptive enough to cover all cases, which is
 how we ended up with the strbuf-passing strategy in the ref code.
 Certainly you could add an integer for every possible bespoke
 message, but then I'm not sure it's buying that much over having
 the function simply fill in a strbuf.

  2. For complex functions there may be multiple errors that need to
 stack. I think the refs code has cases like this, where a syscall
 fails, which causes a fundamental ref operation to fail, which
 causes a higher-level operation to fail. It's only the caller of
 the higher-level operation that knows how to report the error.

Certainly an integer error code would work for _this_ function, but I'd
rather see us grow towards consistent error handling.

-Peff

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

[René Scharfe]

Am 03.11.2017 um 20:13 schrieb Jeff King:
> On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:
> 
>> Simon Ruderich  writes:
>>
>>> I tried looking into this by adding a new write_file_buf_gently()
>>> (or maybe renaming write_file_buf to write_file_buf_or_die) and
>>> using it from write_file_buf() but I don't know the proper way to
>>> handle the error-case in write_file_buf(). Just calling
>>> die("write_file_buf") feels ugly, as the real error was already
>>> printed on screen by error_errno() and I didn't find any function
>>> to just exit without writing a message (which still respects
>>> die_routine). Suggestions welcome.
>>
>> How about *not* printing the error at the place where you notice the
>> error, and instead return an error code to the caller to be noticed
>> which dies with an error message?
> 
> That ends up giving less-specific errors.

Not necessarily.  Function could return different codes for different
errors, e.g. -1 for an open(2) error and -2 for a write(2) error, and
the caller could use that to select the message to show.

Basically all of the messages in wrapper.c consist of some text mixed
with the affected path path and a strerror(3) string, so they're
compatible in that way.  A single function (get_path_error_format()?)
could thus be used and callers would be able to combine its result with
die(), error(), or warning().

René

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

[Jeff King]

On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:

> Simon Ruderich  writes:
> 
> > I tried looking into this by adding a new write_file_buf_gently()
> > (or maybe renaming write_file_buf to write_file_buf_or_die) and
> > using it from write_file_buf() but I don't know the proper way to
> > handle the error-case in write_file_buf(). Just calling
> > die("write_file_buf") feels ugly, as the real error was already
> > printed on screen by error_errno() and I didn't find any function
> > to just exit without writing a message (which still respects
> > die_routine). Suggestions welcome.
> 
> How about *not* printing the error at the place where you notice the
> error, and instead return an error code to the caller to be noticed
> which dies with an error message?

That ends up giving less-specific errors. It might be an OK tradeoff
here.

I think we've been gravitating towards error strbufs, which would make
it something like:

diff --git a/wrapper.c b/wrapper.c
index 61aba0b5c1..08eb5d1cb8 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -649,13 +649,34 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
return len;
 }
 
+int write_file_buf_gently(const char *path, const char *buf, size_t len,
+ struct strbuf *err)
+{
+   int fd = open(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
+   if (fd < 0) {
+   strbuf_addf(err, _("could not open '%s' for writing: %s"),
+   path, strerror(errno));
+   return -1;
+   }
+   if (write_in_full(fd, buf, len) < 0) {
+   strbuf_addf(err, _("could not write to %s: %s"),
+   path, strerror(errno));
+   close(fd);
+   return -1;
+   }
+   if (close(fd)) {
+   strbuf_addf(err, _("could not close %s: %s"),
+   path, strerror(errno));
+   return -1;
+   }
+   return 0;
+}
+
 void write_file_buf(const char *path, const char *buf, size_t len)
 {
-   int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-   if (write_in_full(fd, buf, len) < 0)
-   die_errno(_("could not write to %s"), path);
-   if (close(fd))
-   die_errno(_("could not close %s"), path);
+   struct strbuf err = STRBUF_INIT;
+   if (write_file_buf_gently(path, buf, len, ) < 0)
+   die("%s", err.buf);
 }
 
 void write_file(const char *path, const char *fmt, ...)


I'm not excited that the amount of error-handling code is now double the
amount of code that actually does something useful. Maybe this function
simply isn't large/complex enough to merit flexible error handling, and
we should simply go with René's original near-duplicate.

OTOH, if we went all-in on flexible error handling contexts, you could
imagine this function becoming:

  void write_file_buf(const char *path, const char *buf, size_t len,
  struct error_context *err)
  {
int fd = xopen(path, err, O_WRONLY | O_CREAT | O_TRUNC, 0666);
if (fd < 0)
return -1;
if (write_in_full(fd, buf, len, err) < 0)
return -1;
if (xclose(fd, err) < 0)
return -1;
return 0;
  }

Kind of gross, in that we're adding a layer on top of all system calls.
But if used consistently, it makes error-reporting a lot more pleasant,
and makes all of our "whoops, we forgot to save errno" bugs go away.

-Peff

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

[Jeff King]

On Fri, Nov 03, 2017 at 03:46:44PM +0100, Johannes Schindelin wrote:

> > I tried looking into this by adding a new write_file_buf_gently()
> > (or maybe renaming write_file_buf to write_file_buf_or_die) and
> > using it from write_file_buf() but I don't know the proper way to
> > handle the error-case in write_file_buf(). Just calling
> > die("write_file_buf") feels ugly, as the real error was already
> > printed on screen by error_errno() and I didn't find any function
> > to just exit without writing a message (which still respects
> > die_routine). Suggestions welcome.
> 
> In my ideal world, we could use all those fancy refactoring tools that are
> currently en vogue and simply turn *all* error()/error_errno() calls into
> context-aware functions that can be told to die() right away, or to return
> the error in an error buffer, depending hwhat the caller (or the call
> chain, really) wants.
> 
> This is quite a bit more object-oriented than Git's code base, though, and
> besides, I am not aware of any refactoring tool that would make this
> painless (it's not just a matter of adding a parameter, you also have to
> pass it through all of the call chain, something you get for free when
> working with an object-oriented language).

FWIW, I sketched this out a bit here:

  
https://public-inbox.org/git/20160928085841.aoisson3fnuke...@sigill.intra.peff.net/

And you can see the patches I played with while writing that here:

  
https://github.com/peff/git/compare/cb5918aa0d50f50e83787f65c2ddc3dcb10159fe...4d61927e66dcfdbdb6cc6c88ec4018e2142e826c

(but note they don't quite compile, as some of the conversions are
half-done; it was really just to get a sense of the flavor of the
thing).

One of the complaints was that it makes it harder to see when we are
calling die() (because it's now happening via an error callback). That
maybe confusing for users, but it may also affect generated code since
the code paths that hit the NORETURN function are obscured.

But we could stop short of adding error_die, and just have error_silent,
error_warn, and error_print (and callers can turn error_print into a
die() themselves).

-Peff

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

[Johannes Schindelin]

Hi Simon,

On Fri, 3 Nov 2017, Simon Ruderich wrote:

> On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> > On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
> >> 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.
> >
> > That would be fine with me, too.
> 
> I tried looking into this by adding a new write_file_buf_gently()
> (or maybe renaming write_file_buf to write_file_buf_or_die) and
> using it from write_file_buf() but I don't know the proper way to
> handle the error-case in write_file_buf(). Just calling
> die("write_file_buf") feels ugly, as the real error was already
> printed on screen by error_errno() and I didn't find any function
> to just exit without writing a message (which still respects
> die_routine). Suggestions welcome.

In my ideal world, we could use all those fancy refactoring tools that are
currently en vogue and simply turn *all* error()/error_errno() calls into
context-aware functions that can be told to die() right away, or to return
the error in an error buffer, depending hwhat the caller (or the call
chain, really) wants.

This is quite a bit more object-oriented than Git's code base, though, and
besides, I am not aware of any refactoring tool that would make this
painless (it's not just a matter of adding a parameter, you also have to
pass it through all of the call chain, something you get for free when
working with an object-oriented language).

So what I did in the sequencer when faced with the same conundrum was to
simply return -1 if the function I called returned a negative value. The
top-level builtin (in that case, `rebase--helper`) simply returns !!ret as
exit code (so that `-1` gets translated into the exit code `1`).

BTW I would not use the `_or_die()` convention, as it suggests that that
function will *always* die() in the error case. Instead, what I would
follow is the `, int die_on_error` pattern e.g. of `real_pathdup()`, and
simply *add* that parameter to the signature (and changing the return
value to `int`).

Ciao,
Dscho

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

[Junio C Hamano]

Simon Ruderich  writes:

> I tried looking into this by adding a new write_file_buf_gently()
> (or maybe renaming write_file_buf to write_file_buf_or_die) and
> using it from write_file_buf() but I don't know the proper way to
> handle the error-case in write_file_buf(). Just calling
> die("write_file_buf") feels ugly, as the real error was already
> printed on screen by error_errno() and I didn't find any function
> to just exit without writing a message (which still respects
> die_routine). Suggestions welcome.

How about *not* printing the error at the place where you notice the
error, and instead return an error code to the caller to be noticed
which dies with an error message?

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

[Simon Ruderich]

On Wed, Nov 01, 2017 at 06:16:18PM -0400, Jeff King wrote:
> On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:
>> 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.
>
> That would be fine with me, too.

I tried looking into this by adding a new write_file_buf_gently()
(or maybe renaming write_file_buf to write_file_buf_or_die) and
using it from write_file_buf() but I don't know the proper way to
handle the error-case in write_file_buf(). Just calling
die("write_file_buf") feels ugly, as the real error was already
printed on screen by error_errno() and I didn't find any function
to just exit without writing a message (which still respects
die_routine). Suggestions welcome.

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

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

[Jeff King]

On Wed, Nov 01, 2017 at 10:46:14PM +0100, Johannes Schindelin wrote:

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

That would be fine with me, too.

-Peff

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

[Johannes Schindelin]

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.

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

[Jeff King]

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.

-Peff

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

[Johannes Schindelin]

Hi René,

On Tue, 31 Oct 2017, René Scharfe wrote:

> Reduce code duplication by extracting a function for rewriting an
> existing file.

Fine by me. Thanks,
Dscho

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

[René Scharfe]

Am 01.11.2017 um 12:10 schrieb Simon Ruderich:
> On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
>> +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);
> 
> We might want to check the return value of close() as some file
> systems report write errors only on close. But I'm not sure how
> the rest of Git's code-base handles this.

Most calls are not checked, but that doesn't necessarily mean they need
to (or should) stay that way.  The Linux man-page of close(2) spends
multiple paragraphs recommending to check its return value..  Care to
send a follow-up patch?

René

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

[Simon Ruderich]

On Tue, Oct 31, 2017 at 10:54:21AM +0100, René Scharfe wrote:
> +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);

We might want to check the return value of close() as some file
systems report write errors only on close. But I'm not sure how
the rest of Git's code-base handles this.

> + return rc;
> +}

Regards
Simon
-- 
+ privacy is necessary
+ using gnupg http://gnupg.org
+ public key id: 0x92FEFDB7E44C32F9

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

[Kevin Daudt]

On Tue, Oct 31, 2017 at 05:33:57PM +0100, Kevin Daudt 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.
> > 
> > Signed-off-by: Rene Scharfe 
> > ---
> >  sequencer.c | 46 +-
> >  1 file changed, 17 insertions(+), 29 deletions(-)
> > 
> > diff --git a/sequencer.c b/sequencer.c > > index f2a10cc4f2..17360eb38a 
> > 100644
> > --- a/sequencer.c
> > +++ b/sequencer.c
> > @@ -2665,6 +2665,20 @@ int check_todo_list(void)
> > return res;
> >  }
> >  
> > +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;
> > +}
> > +
> >  /* skip picking commits whose parents are unchanged */
> >  int skip_unnecessary_picks(void)
> >  {
> > @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
> > }
> > close(fd);
> >  
> > -   fd = open(rebase_path_todo(), O_WRONLY, 0666);
> > -   if (fd < 0) {
> > -   error_errno(_("could not open '%s' for writing"),
> > -   rebase_path_todo());
> > +   if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> > +todo_list.buf.len - offset) < 0) {
> > todo_list_release(_list);
> > return -1;
> > }
> > -   if (write_in_full(fd, todo_list.buf.buf + offset,
> > -   todo_list.buf.len - offset) < 0) {
> > -   error_errno(_("could not write to '%s'"),
> > -   rebase_path_todo());
> > -   close(fd);
> > -   todo_list_release(_list);
> 
> Is this missing on purpose in the new situation?
>

I wasn't looking at the context, only the changed lines. After reading
it again, it's clear that nothing is missing (the freeing of todo_list).

Kevin

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

[Kevin Daudt]

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.
> 
> Signed-off-by: Rene Scharfe 
> ---
>  sequencer.c | 46 +-
>  1 file changed, 17 insertions(+), 29 deletions(-)
> 
> diff --git a/sequencer.c b/sequencer.c
> index f2a10cc4f2..17360eb38a 100644
> --- a/sequencer.c
> +++ b/sequencer.c
> @@ -2665,6 +2665,20 @@ int check_todo_list(void)
>   return res;
>  }
>  
> +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;
> +}
> +
>  /* skip picking commits whose parents are unchanged */
>  int skip_unnecessary_picks(void)
>  {
> @@ -2737,29 +2751,11 @@ int skip_unnecessary_picks(void)
>   }
>   close(fd);
>  
> - fd = open(rebase_path_todo(), O_WRONLY, 0666);
> - if (fd < 0) {
> - error_errno(_("could not open '%s' for writing"),
> - rebase_path_todo());
> + if (rewrite_file(rebase_path_todo(), todo_list.buf.buf + offset,
> +  todo_list.buf.len - offset) < 0) {
>   todo_list_release(_list);
>   return -1;
>   }
> - if (write_in_full(fd, todo_list.buf.buf + offset,
> - todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not write to '%s'"),
> - rebase_path_todo());
> - close(fd);
> - todo_list_release(_list);

Is this missing on purpose in the new situation?

> - return -1;
> - }
> - if (ftruncate(fd, todo_list.buf.len - offset) < 0) {
> - error_errno(_("could not truncate '%s'"),
> - rebase_path_todo());
> - todo_list_release(_list);
> - close(fd);
> - return -1;
> - }
> - close(fd);
>  
>   todo_list.current = i;
>   if (is_fixup(peek_command(_list, 0)))
> @@ -2944,15 +2940,7 @@ int rearrange_squash(void)
>   }
>   }
>  
> - fd = open(todo_file, O_WRONLY);
> - if (fd < 0)
> - res = error_errno(_("could not open '%s'"), todo_file);
> - else if (write(fd, buf.buf, buf.len) < 0)
> - res = error_errno(_("could not write to '%s'"), 
> todo_file);
> - else if (ftruncate(fd, buf.len) < 0)
> - res = error_errno(_("could not truncate '%s'"),
> -todo_file);
> - close(fd);
> + res = rewrite_file(todo_file, buf.buf, buf.len);
>   strbuf_release();
>   }
>  
> -- 
> 2.15.0

Except for that question, it looks good to me (as a beginner), it makes
the code better readable.