On Fri, Nov 03, 2017 at 10:44:08PM +0900, Junio C Hamano wrote:
> Simon Ruderich <[email protected]> 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
