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

[Jeff King <peff@peff.net>]

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

> Simon Ruderich <simon@ruderich.org> 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, &err) < 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