[PATCH] am: ignore return value of write_file()

[PATCH] am: ignore return value of write_file()

[René Scharfe]

write_file() either returns 0 or dies, so there is no point in checking
its return value.  The callers of the wrappers write_state_text(),
write_state_count() and write_state_bool() consequently already ignore
their return values.  Stop pretenting we care and make them void.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
---
 builtin/am.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d5da5fe..339e360 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -184,22 +184,22 @@ static inline const char *am_path(const struct am_state *state, const char *path
 /**
  * For convenience to call write_file()
  */
-static int write_state_text(const struct am_state *state,
-			    const char *name, const char *string)
+static void write_state_text(const struct am_state *state,
+			     const char *name, const char *string)
 {
-	return write_file(am_path(state, name), "%s", string);
+	write_file(am_path(state, name), "%s", string);
 }
 
-static int write_state_count(const struct am_state *state,
-			     const char *name, int value)
+static void write_state_count(const struct am_state *state,
+			      const char *name, int value)
 {
-	return write_file(am_path(state, name), "%d", value);
+	write_file(am_path(state, name), "%d", value);
 }
 
-static int write_state_bool(const struct am_state *state,
-			    const char *name, int value)
+static void write_state_bool(const struct am_state *state,
+			     const char *name, int value)
 {
-	return write_state_text(state, name, value ? "t" : "f");
+	write_state_text(state, name, value ? "t" : "f");
 }
 
 /**
-- 
2.9.0

Re: [PATCH] am: ignore return value of write_file()

[Jeff King]

On Thu, Jul 07, 2016 at 10:02:14PM +0200, René Scharfe wrote:

> write_file() either returns 0 or dies, so there is no point in checking
> its return value.  The callers of the wrappers write_state_text(),
> write_state_count() and write_state_bool() consequently already ignore
> their return values.  Stop pretenting we care and make them void.

Makes sense. Originally it took "fatal" as a parameter, but it was split
into two functions in 12d6ce1 (write_file(): drop "fatal" parameter,
2015-08-24). The return value on the non-gentle version could have been
dropped at that point.

Arguably we could get rid of the gentle form entirely, as below. The
diffstat is certainly pleasing, but maybe we would eventually want it
for another caller. I dunno. I won't be offended if we drop this as
churn.

-- >8 --
Subject: [PATCH] write_file: drop "gently" form

We have two forms of write_file(): one that dies, and one
that returns an error. However, the latter has only a single
caller, which immediately dies anyway (after producing a
message that is not really any more informative than
write_file's generic die(), and arguably worse because it
does not give the actual filename).

Let's convert that site to use the non-gentle form. At that
point the gentle form has no callers, and we can simplify
the implementation of write_file.

Signed-off-by: Jeff King <peff@peff.net>
---
As a fun aside, this patch was generated using "--patience",
which gives a much closer to result to what I actually
changed than Myers diff (not meaningful to the patch, but
I'm just always on the lookout for cases where the
algorithms produce meaningfully different results).

 builtin/branch.c |  5 +----
 cache.h          |  3 +--
 wrapper.c        | 56 ++++++++++++--------------------------------------------
 3 files changed, 14 insertions(+), 50 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2ecde53..15232c4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)
 		    "  %s\n"
 		    "Lines starting with '%c' will be stripped.\n",
 		    branch_name, comment_line_char);
-	if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
-		strbuf_release(&buf);
-		return error_errno(_("could not write branch description template"));
-	}
+	write_file(git_path(edit_description), "%s", buf.buf);
 	strbuf_reset(&buf);
 	if (launch_editor(git_path(edit_description), &buf, NULL)) {
 		strbuf_release(&buf);
diff --git a/cache.h b/cache.h
index f1dc289..3f6c53f 100644
--- a/cache.h
+++ b/cache.h
@@ -1745,8 +1745,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 	return write_in_full(fd, str, strlen(str));
 }
 
-extern int write_file(const char *path, const char *fmt, ...);
-extern int write_file_gently(const char *path, const char *fmt, ...);
+extern void write_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..0349441 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 	return len;
 }
 
-static int write_file_v(const char *path, int fatal,
-			const char *fmt, va_list params)
+void write_file(const char *path, const char *fmt, ...)
 {
+	va_list params;
 	struct strbuf sb = STRBUF_INIT;
 	int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
-	if (fd < 0) {
-		if (fatal)
-			die_errno(_("could not open %s for writing"), path);
-		return -1;
-	}
+	if (fd < 0)
+		die_errno(_("could not open %s for writing"), path);
+
+	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
+	va_end(params);
+
 	strbuf_complete_line(&sb);
-	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
-		int err = errno;
-		close(fd);
-		strbuf_release(&sb);
-		errno = err;
-		if (fatal)
-			die_errno(_("could not write to %s"), path);
-		return -1;
-	}
+	if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		die_errno(_("could not write to %s"), path);
 	strbuf_release(&sb);
-	if (close(fd)) {
-		if (fatal)
-			die_errno(_("could not close %s"), path);
-		return -1;
-	}
-	return 0;
-}
-
-int write_file(const char *path, const char *fmt, ...)
-{
-	int status;
-	va_list params;
-
-	va_start(params, fmt);
-	status = write_file_v(path, 1, fmt, params);
-	va_end(params);
-	return status;
-}
-
-int write_file_gently(const char *path, const char *fmt, ...)
-{
-	int status;
-	va_list params;
-
-	va_start(params, fmt);
-	status = write_file_v(path, 0, fmt, params);
-	va_end(params);
-	return status;
+	if (close(fd))
+		die_errno(_("could not close %s"), path);
 }
 
 void sleep_millisec(int millisec)
-- 
2.9.0.393.g704e522

Re: [PATCH] am: ignore return value of write_file()

[Johannes Schindelin]

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

Hi René,

On Thu, 7 Jul 2016, René Scharfe wrote:

> write_file() either returns 0 or dies, so there is no point in checking
> its return value.

The question is whether it makes sense for write_file() to die(). It is a
library function and not every caller can be happy with that function to
exit the program when some file could not be written, without a chance to
tell the user what to do about the situation.

If write_file() was defined in builtin/am.c, as a static function, I would
grudgingly acquiesce, but it is not.

IMO it would be better to fix write_file() to *not* die() but return
error() instead.

Ciao,
Dscho

Re: [PATCH] am: ignore return value of write_file()

[Johannes Schindelin]

Hi Peff,

On Thu, 7 Jul 2016, Jeff King wrote:

> We have two forms of write_file(): one that dies, and one
> that returns an error. However, the latter has only a single
> caller, which immediately dies anyway (after producing a
> message that is not really any more informative than
> write_file's generic die(), and arguably worse because it
> does not give the actual filename).

This is more an illustration of unnecessarily duplicated code, isn't it?
There are *tons* of instances in Git's code where writing to a file is
implemented separately (and differently).

It would make tons of sense to consolidate all of these instances,
methinks. The diffstat should look *very* pleasing.

Ciao,
Dscho

Re: [PATCH] am: ignore return value of write_file()

[Jeff King]

On Fri, Jul 08, 2016 at 08:37:35AM +0200, Johannes Schindelin wrote:

> > We have two forms of write_file(): one that dies, and one
> > that returns an error. However, the latter has only a single
> > caller, which immediately dies anyway (after producing a
> > message that is not really any more informative than
> > write_file's generic die(), and arguably worse because it
> > does not give the actual filename).
> 
> This is more an illustration of unnecessarily duplicated code, isn't it?
> There are *tons* of instances in Git's code where writing to a file is
> implemented separately (and differently).
> 
> It would make tons of sense to consolidate all of these instances,
> methinks. The diffstat should look *very* pleasing.

I grepped for O_WRONLY, and there are fewer instances than I would have
thought. Most of the obvious write_file() candidates are in the merge
code, which is probably why you saw so many of them. :)

I started at converting a few sites, but it's actually a little awkward
because they all have strbufs (with a ptr/len combo that _could_ contain
NULs, but probably doesn't), and write_file() wants to take a format
string.

I think we can clean that up, though. I'll hopefully have a series in a
few minutes.

-Peff

[PATCH 0/8] write_file cleanups

[Jeff King]

On Fri, Jul 08, 2016 at 02:56:50AM -0400, Jeff King wrote:

> > This is more an illustration of unnecessarily duplicated code, isn't it?
> > There are *tons* of instances in Git's code where writing to a file is
> > implemented separately (and differently).
> > 
> > It would make tons of sense to consolidate all of these instances,
> > methinks. The diffstat should look *very* pleasing.
> 
> I grepped for O_WRONLY, and there are fewer instances than I would have
> thought. Most of the obvious write_file() candidates are in the merge
> code, which is probably why you saw so many of them. :)
> 
> I started at converting a few sites, but it's actually a little awkward
> because they all have strbufs (with a ptr/len combo that _could_ contain
> NULs, but probably doesn't), and write_file() wants to take a format
> string.
> 
> I think we can clean that up, though. I'll hopefully have a series in a
> few minutes.

Here it is. There actually weren't that many spots to clean up, as quite
a few of them have a "twist" where they want to do something clever,
like open the file and feed the descriptor to a sub-function, or open
with funny things like O_EXCL.

But still, the diffstat is pleasing:

 builtin/am.c     | 25 +++++++----------
 builtin/branch.c |  5 +---
 builtin/config.c |  2 +-
 builtin/merge.c  | 45 ++++--------------------------
 cache.h          | 17 ++++++++++--
 wrapper.c        | 52 ++++++++---------------------------
 6 files changed, 44 insertions(+), 102 deletions(-)

and that even includes adding some function documentation.

The most interesting thing is that I also found a real bug, albeit a
fairly minor one. I floated that up to the front of the series.

  [1/8]: config: fix bogus fd check when setting up default config
  [2/8]: am: ignore return value of write_file()
  [3/8]: branch: use non-gentle write_file for branch description
  [4/8]: write_file: drop "gently" form
  [5/8]: write_file: use xopen
  [6/8]: write_file: add pointer+len variant
  [7/8]: write_file: add format attribute
  [8/8]: use write_file_buf where applicable

-Peff

[PATCH 1/8] config: fix bogus fd check when setting up default config

[Jeff King]

Since 9830534 (config --global --edit: create a template
file if needed, 2014-07-25), an edit of the global config
file will try to open() it with O_EXCL, and wants to handle
three cases:

  1. We succeeded; the user has no config file, and we
     should fill in the default template.

  2. We got EEXIST; they have a file already, proceed as usual.

  3. We got another error; we should complain.

However, the check for case 1 does "if (fd)", which will
generally _always_ be true (except for the oddball case that
somehow our stdin got closed and opening really did give us
a new descriptor 0).

So in the EEXIST case, we tried to write the default config
anyway! Fortunately, this turns out to be a noop, since we
just end up writing to and closing "-1", which does nothing.

But in case 3, we would fail to notice any other errors, and
just silently continue (given that we don't actually notice
write errors for the template either, it's probably not that
big a deal; we're about to spawn the editor, so it would
notice any problems. But the code is clearly _trying_ to hit
cover this case and failing).

We can fix it easily by using "fd >= 0" for case 1.

Signed-off-by: Jeff King <peff@peff.net>
---
I was looking at this to see whether it could be converted to
write_file(). However, the O_EXCL and the error-handling make things
too tricky.

 builtin/config.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/config.c b/builtin/config.c
index 1d7c6ef..a991a53 100644
--- a/builtin/config.c
+++ b/builtin/config.c
@@ -604,7 +604,7 @@ int cmd_config(int argc, const char **argv, const char *prefix)
 				      given_config_source.file : git_path("config"));
 		if (use_global_config) {
 			int fd = open(config_file, O_CREAT | O_EXCL | O_WRONLY, 0666);
-			if (fd) {
+			if (fd >= 0) {
 				char *content = default_user_config();
 				write_str_in_full(fd, content);
 				free(content);
-- 
2.9.0.393.g704e522

[PATCH 2/8] am: ignore return value of write_file()

[Jeff King]

From: René Scharfe <l.s.r@web.de>

write_file() either returns 0 or dies, so there is no point in checking
its return value.  The callers of the wrappers write_state_text(),
write_state_count() and write_state_bool() consequently already ignore
their return values.  Stop pretending we care and make them void.

Signed-off-by: Rene Scharfe <l.s.r@web.de>
Signed-off-by: Jeff King <peff@peff.net>
---
I included this just to make it clear that I am building on top. You
can also just apply on top of the existing branch (though note that I
fixed a typo s/pretenting/pretending/ in the commit message).

 builtin/am.c | 18 +++++++++---------
 1 file changed, 9 insertions(+), 9 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index d5da5fe..339e360 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -184,22 +184,22 @@ static inline const char *am_path(const struct am_state *state, const char *path
 /**
  * For convenience to call write_file()
  */
-static int write_state_text(const struct am_state *state,
-			    const char *name, const char *string)
+static void write_state_text(const struct am_state *state,
+			     const char *name, const char *string)
 {
-	return write_file(am_path(state, name), "%s", string);
+	write_file(am_path(state, name), "%s", string);
 }
 
-static int write_state_count(const struct am_state *state,
-			     const char *name, int value)
+static void write_state_count(const struct am_state *state,
+			      const char *name, int value)
 {
-	return write_file(am_path(state, name), "%d", value);
+	write_file(am_path(state, name), "%d", value);
 }
 
-static int write_state_bool(const struct am_state *state,
-			    const char *name, int value)
+static void write_state_bool(const struct am_state *state,
+			     const char *name, int value)
 {
-	return write_state_text(state, name, value ? "t" : "f");
+	write_state_text(state, name, value ? "t" : "f");
 }
 
 /**
-- 
2.9.0.393.g704e522

[PATCH 3/8] branch: use non-gentle write_file for branch description

[Jeff King]

We use write_file_gently() to do this job currently.
However, if we see an error, we simply complain via
error_errno() and then end up exiting with an error code.

By switching to the non-gentle form, the function will die
for us, with a better error. It is more specific about which
syscall caused the error, and that mentions the
actual filename we're trying to write.

Our exit code for the error case does switch from "1" to
"128", but that's OK; it wasn't a meaningful documented code
(and in fact it was odd that it was a different exit code
than most other error conditions).

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/branch.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 2ecde53..15232c4 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,10 +618,7 @@ static int edit_branch_description(const char *branch_name)
 		    "  %s\n"
 		    "Lines starting with '%c' will be stripped.\n",
 		    branch_name, comment_line_char);
-	if (write_file_gently(git_path(edit_description), "%s", buf.buf)) {
-		strbuf_release(&buf);
-		return error_errno(_("could not write branch description template"));
-	}
+	write_file(git_path(edit_description), "%s", buf.buf);
 	strbuf_reset(&buf);
 	if (launch_editor(git_path(edit_description), &buf, NULL)) {
 		strbuf_release(&buf);
-- 
2.9.0.393.g704e522

[PATCH 4/8] write_file: drop "gently" form

[Jeff King]

There are no callers left of write_file_gently(). Let's drop
it, as it doesn't seem likely for new callers to be added
(since its inception, the only callers who wanted the gentle
form generally just died immediately themselves, and have
since been converted).

While we're there, let's also drop the "int" return from
write_file, as it is never meaningful (in the non-gentle
form, we always either die or return 0).

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  3 +--
 wrapper.c | 54 +++++++++++-------------------------------------------
 2 files changed, 12 insertions(+), 45 deletions(-)

diff --git a/cache.h b/cache.h
index f1dc289..3f6c53f 100644
--- a/cache.h
+++ b/cache.h
@@ -1745,8 +1745,7 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 	return write_in_full(fd, str, strlen(str));
 }
 
-extern int write_file(const char *path, const char *fmt, ...);
-extern int write_file_gently(const char *path, const char *fmt, ...);
+extern void write_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
 extern void setup_pager(void);
diff --git a/wrapper.c b/wrapper.c
index 5dc4e15..0349441 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,56 +640,24 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 	return len;
 }
 
-static int write_file_v(const char *path, int fatal,
-			const char *fmt, va_list params)
+void write_file(const char *path, const char *fmt, ...)
 {
+	va_list params;
 	struct strbuf sb = STRBUF_INIT;
 	int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
-	if (fd < 0) {
-		if (fatal)
-			die_errno(_("could not open %s for writing"), path);
-		return -1;
-	}
-	strbuf_vaddf(&sb, fmt, params);
-	strbuf_complete_line(&sb);
-	if (write_in_full(fd, sb.buf, sb.len) != sb.len) {
-		int err = errno;
-		close(fd);
-		strbuf_release(&sb);
-		errno = err;
-		if (fatal)
-			die_errno(_("could not write to %s"), path);
-		return -1;
-	}
-	strbuf_release(&sb);
-	if (close(fd)) {
-		if (fatal)
-			die_errno(_("could not close %s"), path);
-		return -1;
-	}
-	return 0;
-}
-
-int write_file(const char *path, const char *fmt, ...)
-{
-	int status;
-	va_list params;
+	if (fd < 0)
+		die_errno(_("could not open %s for writing"), path);
 
 	va_start(params, fmt);
-	status = write_file_v(path, 1, fmt, params);
+	strbuf_vaddf(&sb, fmt, params);
 	va_end(params);
-	return status;
-}
-
-int write_file_gently(const char *path, const char *fmt, ...)
-{
-	int status;
-	va_list params;
 
-	va_start(params, fmt);
-	status = write_file_v(path, 0, fmt, params);
-	va_end(params);
-	return status;
+	strbuf_complete_line(&sb);
+	if (write_in_full(fd, sb.buf, sb.len) != sb.len)
+		die_errno(_("could not write to %s"), path);
+	strbuf_release(&sb);
+	if (close(fd))
+		die_errno(_("could not close %s"), path);
 }
 
 void sleep_millisec(int millisec)
-- 
2.9.0.393.g704e522

[PATCH 5/8] write_file: use xopen

[Jeff King]

This simplifies the code a tiny bit, and provides consistent
error messages with other users of xopen().

While we're here, let's also switch to using O_WRONLY. We
know we're only going to open/write/close the file, so
there's no point in asking for O_RDWR.

Signed-off-by: Jeff King <peff@peff.net>
---
 wrapper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

diff --git a/wrapper.c b/wrapper.c
index 0349441..7c126b8 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -644,9 +644,7 @@ void write_file(const char *path, const char *fmt, ...)
 {
 	va_list params;
 	struct strbuf sb = STRBUF_INIT;
-	int fd = open(path, O_RDWR | O_CREAT | O_TRUNC, 0666);
-	if (fd < 0)
-		die_errno(_("could not open %s for writing"), path);
+	int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 
 	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
-- 
2.9.0.393.g704e522

[PATCH 6/8] write_file: add pointer+len variant

[Jeff King]

There are many callsites which could use write_file, but for
which it is a little awkward because they have a strbuf or
other pointer/len combo. Specifically:

 1. write_file() takes a format string, so we have to use
    "%s" or "%.*s", which are ugly.

 2. Using any form of "%s" does not handle embedded NULs in
    the output. That probably doesn't matter for our
    call-sites, but it's nicer not to have to worry.

 3. It's less efficient; we format into another strbuf
    just to do the write. That's probably not measurably
    slow for our uses, but it's simply inelegant.

We can fix this by providing a helper to write out the
formatted buffer, and just calling it from write_file().

Note that we don't do the usual "complete with a newline"
that write_file does. If the caller has their own buffer,
there's a reasonable chance they're doing something more
complicated than a single line, and they can call
strbuf_complete_line() themselves.

We could go even further and add strbuf_write_file(), but it
doesn't save much:

  -  write_file_buf(path, sb.buf, sb.len);
  +  strbuf_write_file(&sb, path);

It would also be somewhat asymmetric with strbuf_read_file,
which actually returns errors rather than dying (and the
error handling is most of the benefit of write_file() in the
first place).

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h   |  6 ++++++
 wrapper.c | 16 +++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/cache.h b/cache.h
index 3f6c53f..9d6ad4f 100644
--- a/cache.h
+++ b/cache.h
@@ -1745,6 +1745,12 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
 	return write_in_full(fd, str, strlen(str));
 }
 
+/**
+ * Open (and truncate) the file at path, write the contents of buf to it,
+ * and close it. Dies if any errors are encountered.
+ */
+extern void write_file_buf(const char *path, const char *buf, size_t len);
+
 extern void write_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
diff --git a/wrapper.c b/wrapper.c
index 7c126b8..b827206 100644
--- a/wrapper.c
+++ b/wrapper.c
@@ -640,22 +640,28 @@ int xsnprintf(char *dst, size_t max, const char *fmt, ...)
 	return len;
 }
 
+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) != len)
+		die_errno(_("could not write to %s"), path);
+	if (close(fd))
+		die_errno(_("could not close %s"), path);
+}
+
 void write_file(const char *path, const char *fmt, ...)
 {
 	va_list params;
 	struct strbuf sb = STRBUF_INIT;
-	int fd = xopen(path, O_WRONLY | O_CREAT | O_TRUNC, 0666);
 
 	va_start(params, fmt);
 	strbuf_vaddf(&sb, fmt, params);
 	va_end(params);
 
 	strbuf_complete_line(&sb);
-	if (write_in_full(fd, sb.buf, sb.len) != sb.len)
-		die_errno(_("could not write to %s"), path);
+
+	write_file_buf(path, sb.buf, sb.len);
 	strbuf_release(&sb);
-	if (close(fd))
-		die_errno(_("could not close %s"), path);
 }
 
 void sleep_millisec(int millisec)
-- 
2.9.0.393.g704e522

[PATCH 7/8] write_file: add format attribute

[Jeff King]

This gives us compile-time checking of our format strings,
which is a good thing.

I had also hoped it would help with confusing write_file()
and write_file_buf(), since the former's "..." can make it
match the signature of the latter. But given that the buffer
for write_file_buf() is generally not a string literal, the
compiler won't complain unless -Wformat-nonliteral is on,
and that creates a ton of false positives elsewhere in the
code base.

While we're there, let's also give the function a docstring,
which it never had.

Signed-off-by: Jeff King <peff@peff.net>
---
 cache.h | 8 ++++++++
 1 file changed, 8 insertions(+)

diff --git a/cache.h b/cache.h
index 9d6ad4f..c6a282c 100644
--- a/cache.h
+++ b/cache.h
@@ -1751,6 +1751,14 @@ static inline ssize_t write_str_in_full(int fd, const char *str)
  */
 extern void write_file_buf(const char *path, const char *buf, size_t len);
 
+/**
+ * Like write_file_buf(), but format the contents into a buffer first.
+ * Additionally, write_file() will append a newline if one is not already
+ * present, making it convenient to write text files:
+ *
+ *   write_file(path, "counter: %d", ctr);
+ */
+__attribute__((format (printf, 2, 3)))
 extern void write_file(const char *path, const char *fmt, ...);
 
 /* pager.c */
-- 
2.9.0.393.g704e522

[PATCH 8/8] use write_file_buf where applicable

[Jeff King]

There are several places where we open a file, write some
content from a strbuf, and close it. These can be simplified
with write_file_buf(). As a bonus, many of these did not
catch write problems at close() time.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/am.c    |  7 +------
 builtin/merge.c | 45 +++++----------------------------------------
 2 files changed, 6 insertions(+), 46 deletions(-)

diff --git a/builtin/am.c b/builtin/am.c
index 339e360..3ac2448 100644
--- a/builtin/am.c
+++ b/builtin/am.c
@@ -403,13 +403,8 @@ static int read_commit_msg(struct am_state *state)
  */
 static void write_commit_msg(const struct am_state *state)
 {
-	int fd;
 	const char *filename = am_path(state, "final-commit");
-
-	fd = xopen(filename, O_WRONLY | O_CREAT, 0666);
-	if (write_in_full(fd, state->msg, state->msg_len) < 0)
-		die_errno(_("could not write to %s"), filename);
-	close(fd);
+	write_file_buf(filename, state->msg, state->msg_len);
 }
 
 /**
diff --git a/builtin/merge.c b/builtin/merge.c
index b555a1b..2e782db 100644
--- a/builtin/merge.c
+++ b/builtin/merge.c
@@ -336,15 +336,9 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
 	struct rev_info rev;
 	struct strbuf out = STRBUF_INIT;
 	struct commit_list *j;
-	const char *filename;
-	int fd;
 	struct pretty_print_context ctx = {0};
 
 	printf(_("Squash commit -- not updating HEAD\n"));
-	filename = git_path_squash_msg();
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not write to '%s'"), filename);
 
 	init_revisions(&rev, NULL);
 	rev.ignore_merges = 1;
@@ -371,10 +365,7 @@ static void squash_message(struct commit *commit, struct commit_list *remotehead
 			oid_to_hex(&commit->object.oid));
 		pretty_print_commit(&ctx, commit, &out);
 	}
-	if (write_in_full(fd, out.buf, out.len) != out.len)
-		die_errno(_("Writing SQUASH_MSG"));
-	if (close(fd))
-		die_errno(_("Finishing SQUASH_MSG"));
+	write_file_buf(git_path_squash_msg(), out.buf, out.len);
 	strbuf_release(&out);
 }
 
@@ -756,18 +747,6 @@ static void add_strategies(const char *string, unsigned attr)
 
 }
 
-static void write_merge_msg(struct strbuf *msg)
-{
-	const char *filename = git_path_merge_msg();
-	int fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"),
-			  filename);
-	if (write_in_full(fd, msg->buf, msg->len) != msg->len)
-		die_errno(_("Could not write to '%s'"), filename);
-	close(fd);
-}
-
 static void read_merge_msg(struct strbuf *msg)
 {
 	const char *filename = git_path_merge_msg();
@@ -801,7 +780,7 @@ static void prepare_to_commit(struct commit_list *remoteheads)
 	strbuf_addch(&msg, '\n');
 	if (0 < option_edit)
 		strbuf_commented_addf(&msg, _(merge_editor_comment), comment_line_char);
-	write_merge_msg(&msg);
+	write_file_buf(git_path_merge_msg(), msg.buf, msg.len);
 	if (run_commit_hook(0 < option_edit, get_index_file(), "prepare-commit-msg",
 			    git_path_merge_msg(), "merge", NULL))
 		abort_commit(remoteheads, NULL);
@@ -964,8 +943,6 @@ static int setup_with_upstream(const char ***argv)
 
 static void write_merge_state(struct commit_list *remoteheads)
 {
-	const char *filename;
-	int fd;
 	struct commit_list *j;
 	struct strbuf buf = STRBUF_INIT;
 
@@ -979,26 +956,14 @@ static void write_merge_state(struct commit_list *remoteheads)
 		}
 		strbuf_addf(&buf, "%s\n", oid_to_hex(oid));
 	}
-	filename = git_path_merge_head();
-	fd = open(filename, O_WRONLY | O_CREAT, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-		die_errno(_("Could not write to '%s'"), filename);
-	close(fd);
+	write_file_buf(git_path_merge_head(), buf.buf, buf.len);
 	strbuf_addch(&merge_msg, '\n');
-	write_merge_msg(&merge_msg);
+	write_file_buf(git_path_merge_msg(), merge_msg.buf, merge_msg.len);
 
-	filename = git_path_merge_mode();
-	fd = open(filename, O_WRONLY | O_CREAT | O_TRUNC, 0666);
-	if (fd < 0)
-		die_errno(_("Could not open '%s' for writing"), filename);
 	strbuf_reset(&buf);
 	if (fast_forward == FF_NO)
 		strbuf_addf(&buf, "no-ff");
-	if (write_in_full(fd, buf.buf, buf.len) != buf.len)
-		die_errno(_("Could not write to '%s'"), filename);
-	close(fd);
+	write_file_buf(git_path_merge_mode(), buf.buf, buf.len);
 }
 
 static int default_edit_option(void)
-- 
2.9.0.393.g704e522

[PATCH 9/8] branch: use write_file_buf instead of write_file

[Jeff King]

If we already have a strbuf, then using write_file_buf is a
little nicer to read (no wondering whether "%s" will eat
your NULs), and it's more efficient (no extra formatting
step).

We don't care about the newline magic of write_file(), as we
have our own multi-line content.

Signed-off-by: Jeff King <peff@peff.net>
---
Almost forgot this one. I had originally converted to write_file_buf
directly, but later reshuffled the patches to make the refactoring more
clear.

 builtin/branch.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/branch.c b/builtin/branch.c
index 15232c4..1d41251 100644
--- a/builtin/branch.c
+++ b/builtin/branch.c
@@ -618,7 +618,7 @@ static int edit_branch_description(const char *branch_name)
 		    "  %s\n"
 		    "Lines starting with '%c' will be stripped.\n",
 		    branch_name, comment_line_char);
-	write_file(git_path(edit_description), "%s", buf.buf);
+	write_file_buf(git_path(edit_description), buf.buf, buf.len);
 	strbuf_reset(&buf);
 	if (launch_editor(git_path(edit_description), &buf, NULL)) {
 		strbuf_release(&buf);
-- 
2.9.0.393.g704e522

Re: [PATCH 7/8] write_file: add format attribute

[Jeff King]

On Fri, Jul 08, 2016 at 05:12:41AM -0400, Jeff King wrote:

> I had also hoped it would help with confusing write_file()
> and write_file_buf(), since the former's "..." can make it
> match the signature of the latter. But given that the buffer
> for write_file_buf() is generally not a string literal, the
> compiler won't complain unless -Wformat-nonliteral is on,
> and that creates a ton of false positives elsewhere in the
> code base.

I poked around at the results of compiling with -Wformat-nonliteral, but
gave up at trying to make it work. There are a number of clever uses of
formats that would be hard to do otherwise. There are also several cases
where a format string is used multiple times and we want to avoid
repeating it. But using #define doesn't work, because we want to be able
to translate it.

I did find a little bit of low-hanging fruit, though.

  [1/2]: walker: let walker_say take arbitrary formats
  [2/2]: avoid using sha1_to_hex output as printf format

These are totally independent of the main series, so they can be a
separate topic, go on top, or just get dropped entirely if it's not
worth the trouble.

-Peff

[PATCH 1/2] walker: let walker_say take arbitrary formats

[Jeff King]

We take a printf-style format and a single "char *"
parameter, and the format must therefore have at most one
"%s" in it. Besides being error-prone (and tickling
-Wformat-nonliteral), this is unnecessarily restrictive. We
can just provide the usual varargs interface.

Signed-off-by: Jeff King <peff@peff.net>
---
 walker.c | 10 +++++++---
 walker.h |  3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)

diff --git a/walker.c b/walker.c
index d95b007..2c86e40 100644
--- a/walker.c
+++ b/walker.c
@@ -9,10 +9,14 @@
 
 static unsigned char current_commit_sha1[20];
 
-void walker_say(struct walker *walker, const char *fmt, const char *hex)
+void walker_say(struct walker *walker, const char *fmt, ...)
 {
-	if (walker->get_verbosely)
-		fprintf(stderr, fmt, hex);
+	if (walker->get_verbosely) {
+		va_list ap;
+		va_start(ap, fmt);
+		vfprintf(stderr, fmt, ap);
+		va_end(ap);
+	}
 }
 
 static void report_missing(const struct object *obj)
diff --git a/walker.h b/walker.h
index 95e5765..a869013 100644
--- a/walker.h
+++ b/walker.h
@@ -19,7 +19,8 @@ struct walker {
 };
 
 /* Report what we got under get_verbosely */
-void walker_say(struct walker *walker, const char *, const char *);
+__attribute__((format (printf, 2, 3)))
+void walker_say(struct walker *walker, const char *fmt, ...);
 
 /* Load pull targets from stdin */
 int walker_targets_stdin(char ***target, const char ***write_ref);
-- 
2.9.0.393.g704e522

[PATCH 2/2] avoid using sha1_to_hex output as printf format

[Jeff King]

We know that it should not contain any percent-signs, but
it's a good habit not to feed non-literals to printf
formatters.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 2 +-
 commit.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e866844..cce555c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,
 	 */
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, sha1_to_hex(null_sha1));
+	write_file(sb.buf, "%s", sha1_to_hex(null_sha1));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
diff --git a/commit.c b/commit.c
index 3f4f371..9603379 100644
--- a/commit.c
+++ b/commit.c
@@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,
 {
 	for ( ; list; list = list->next) {
 		const char *format = list->next ? format_cur : format_last;
-		printf(format, oid_to_hex(&list->item->object.oid));
+		printf(format, "%s", oid_to_hex(&list->item->object.oid));
 	}
 }
 
-- 
2.9.0.393.g704e522

Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format

[Jeff King]

On Fri, Jul 08, 2016 at 05:25:26AM -0400, Jeff King wrote:

> diff --git a/commit.c b/commit.c
> index 3f4f371..9603379 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,
>  {
>  	for ( ; list; list = list->next) {
>  		const char *format = list->next ? format_cur : format_last;
> -		printf(format, oid_to_hex(&list->item->object.oid));
> +		printf(format, "%s", oid_to_hex(&list->item->object.oid));

Urgh, this second hunk is clearly bogus. This is a -Wformat-nonliteral
problem, but not because of oid_to_hex(), but rather because of
"format". :-/

Here's a corrected patch. But as this has demonstrated the dangers of
churn, and as it doesn't really get us meaningfully closer to being able
to use -Wformat-nonliteral, perhaps the best course of action is to just
drop it (I do think the "walker_say" patch has more inherent value as a
cleanup, though).

-- >8 --
Subject: [PATCH] avoid using sha1_to_hex output as printf format

We know that it should not contain any percent-signs, but
it's a good habit not to feed non-literals to printf
formatters.

Signed-off-by: Jeff King <peff@peff.net>
---
 builtin/worktree.c | 2 +-
 1 file changed, 1 insertion(+), 1 deletion(-)

diff --git a/builtin/worktree.c b/builtin/worktree.c
index e866844..cce555c 100644
--- a/builtin/worktree.c
+++ b/builtin/worktree.c
@@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,
 	 */
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
-	write_file(sb.buf, sha1_to_hex(null_sha1));
+	write_file(sb.buf, "%s", sha1_to_hex(null_sha1));
 	strbuf_reset(&sb);
 	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
 	write_file(sb.buf, "../..");
-- 
2.9.0.393.g704e522

Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Fri, Jul 08, 2016 at 05:25:26AM -0400, Jeff King wrote:
>
>> diff --git a/commit.c b/commit.c
>> index 3f4f371..9603379 100644
>> --- a/commit.c
>> +++ b/commit.c
>> @@ -1623,7 +1623,7 @@ void print_commit_list(struct commit_list *list,
>>  {
>>  	for ( ; list; list = list->next) {
>>  		const char *format = list->next ? format_cur : format_last;
>> -		printf(format, oid_to_hex(&list->item->object.oid));
>> +		printf(format, "%s", oid_to_hex(&list->item->object.oid));
>
> Urgh, this second hunk is clearly bogus. This is a -Wformat-nonliteral
> problem, but not because of oid_to_hex(), but rather because of
> "format". :-/
>
> Here's a corrected patch. But as this has demonstrated the dangers of
> churn, and as it doesn't really get us meaningfully closer to being able
> to use -Wformat-nonliteral, perhaps the best course of action is to just
> drop it (I do think the "walker_say" patch has more inherent value as a
> cleanup, though).

Hmm.  While both do look correct, and it is a no-brainer to take
this (corrected) patch, I am not sure how much we care about walkers
these days.

As to the hunk to commit.c that was dropped in this round, the only
caller of print_commit_list() is bisect.c, and it passes "%s\n" to
format_cur and format_last, it seems, so that suggests a more
obvious direction for cleaning things up, I would say.

> -- >8 --
> Subject: [PATCH] avoid using sha1_to_hex output as printf format
>
> We know that it should not contain any percent-signs, but
> it's a good habit not to feed non-literals to printf
> formatters.
>
> Signed-off-by: Jeff King <peff@peff.net>
> ---
>  builtin/worktree.c | 2 +-
>  1 file changed, 1 insertion(+), 1 deletion(-)
>
> diff --git a/builtin/worktree.c b/builtin/worktree.c
> index e866844..cce555c 100644
> --- a/builtin/worktree.c
> +++ b/builtin/worktree.c
> @@ -262,7 +262,7 @@ static int add_worktree(const char *path, const char *refname,
>  	 */
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/HEAD", sb_repo.buf);
> -	write_file(sb.buf, sha1_to_hex(null_sha1));
> +	write_file(sb.buf, "%s", sha1_to_hex(null_sha1));
>  	strbuf_reset(&sb);
>  	strbuf_addf(&sb, "%s/commondir", sb_repo.buf);
>  	write_file(sb.buf, "../..");

Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format

[Junio C Hamano]

Junio C Hamano <gitster@pobox.com> writes:

> As to the hunk to commit.c that was dropped in this round, the only
> caller of print_commit_list() is bisect.c, and it passes "%s\n" to
> format_cur and format_last, it seems, so that suggests a more
> obvious direction for cleaning things up, I would say.

And the result is a pleasing diffstat.

-- >8 --
Subject: commit.c: remove print_commit_list()

The helper function tries to offer a way to conveniently show the
last one differently from others, presumably to allow you to say
something like

	A, B, and C.

while iterating over a list that has these three elements.

However, there is only one caller, and it passes the same format
string "%s\n" for both the last one and the other ones.  Retire the
helper function and update the caller with a simplified version.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 bisect.c |  5 ++++-
 commit.c | 10 ----------
 commit.h |  4 ----
 3 files changed, 4 insertions(+), 15 deletions(-)

diff --git a/bisect.c b/bisect.c
index dc13319..02f76f0 100644
--- a/bisect.c
+++ b/bisect.c
@@ -646,7 +646,10 @@ static void exit_if_skipped_commits(struct commit_list *tried,
 
 	printf("There are only 'skip'ped commits left to test.\n"
 	       "The first %s commit could be any of:\n", term_bad);
-	print_commit_list(tried, "%s\n", "%s\n");
+
+	for ( ; tried; tried = tried->next)
+		printf("%s\n", oid_to_hex(&tried->item->object.oid));
+
 	if (bad)
 		printf("%s\n", oid_to_hex(bad));
 	printf("We cannot bisect more!\n");
diff --git a/commit.c b/commit.c
index 3f4f371..bf27972 100644
--- a/commit.c
+++ b/commit.c
@@ -1617,16 +1617,6 @@ struct commit_list **commit_list_append(struct commit *commit,
 	return &new->next;
 }
 
-void print_commit_list(struct commit_list *list,
-		       const char *format_cur,
-		       const char *format_last)
-{
-	for ( ; list; list = list->next) {
-		const char *format = list->next ? format_cur : format_last;
-		printf(format, oid_to_hex(&list->item->object.oid));
-	}
-}
-
 const char *find_commit_header(const char *msg, const char *key, size_t *out_len)
 {
 	int key_len = strlen(key);
diff --git a/commit.h b/commit.h
index 78ed513..71693ce 100644
--- a/commit.h
+++ b/commit.h
@@ -376,10 +376,6 @@ extern int parse_signed_commit(const struct commit *commit,
 			       struct strbuf *message, struct strbuf *signature);
 extern int remove_signature(struct strbuf *buf);
 
-extern void print_commit_list(struct commit_list *list,
-			      const char *format_cur,
-			      const char *format_last);
-
 /*
  * Check the signature of the given commit. The result of the check is stored
  * in sig->check_result, 'G' for a good signature, 'U' for a good signature

Re: [PATCH] am: ignore return value of write_file()

[René Scharfe]

Hi Dscho,

Am 08.07.2016 um 08:33 schrieb Johannes Schindelin:
> On Thu, 7 Jul 2016, René Scharfe wrote:
>> write_file() either returns 0 or dies, so there is no point in checking
>> its return value.
>
> The question is whether it makes sense for write_file() to die(). It is a
> library function and not every caller can be happy with that function to
> exit the program when some file could not be written, without a chance to
> tell the user what to do about the situation.
>
> If write_file() was defined in builtin/am.c, as a static function, I would
> grudgingly acquiesce, but it is not.
>
> IMO it would be better to fix write_file() to *not* die() but return
> error() instead.

there is write_file_gently() for that purpose, but it's used only by a 
single caller that exits on failure after all, and in fact Peff's series 
drops it.

So I think write_file() is fine, and it's rather a question of whether 
am should use write_file_gently() instead.  I don't see why, but perhaps 
that's because it's Friday..

René

Re: [PATCH 0/8] write_file cleanups

[René Scharfe]

Am 08.07.2016 um 11:04 schrieb Jeff King:
> Here it is. There actually weren't that many spots to clean up, as quite
> a few of them have a "twist" where they want to do something clever,
> like open the file and feed the descriptor to a sub-function, or open
> with funny things like O_EXCL.
>
> But still, the diffstat is pleasing:
>
>   builtin/am.c     | 25 +++++++----------
>   builtin/branch.c |  5 +---
>   builtin/config.c |  2 +-
>   builtin/merge.c  | 45 ++++--------------------------
>   cache.h          | 17 ++++++++++--
>   wrapper.c        | 52 ++++++++---------------------------
>   6 files changed, 44 insertions(+), 102 deletions(-)
>
> and that even includes adding some function documentation.

Haha, feels like Candy Crush. :)

(Sent a single small patch, lots of places get patched as if by magic.)

Thanks,
René

Re: [PATCH 2/2] avoid using sha1_to_hex output as printf format

[Jeff King]

On Fri, Jul 08, 2016 at 10:09:28AM -0700, Junio C Hamano wrote:

> Junio C Hamano <gitster@pobox.com> writes:
> 
> > As to the hunk to commit.c that was dropped in this round, the only
> > caller of print_commit_list() is bisect.c, and it passes "%s\n" to
> > format_cur and format_last, it seems, so that suggests a more
> > obvious direction for cleaning things up, I would say.
> 
> And the result is a pleasing diffstat.
> 
> -- >8 --
> Subject: commit.c: remove print_commit_list()

Yeah, I agree this is a nice diffstat.

As for the "walker" thing, I also agree that we don't really care much
about it these days.

I really wish we could just kill it off; it would simplify the http code
significantly (e.g., the entire reason we have the whole async
multi-fetch setup is to support it). But every time I think that,
somebody mentions on the list that they are still using it. :-/

I do suspect we could kill off the http push-over-dav, though. It's less
likely to be useful than the fetch code.

-Peff

Re: [PATCH] am: ignore return value of write_file()

[Jeff King]

On Fri, Jul 08, 2016 at 08:44:28PM +0200, René Scharfe wrote:

> > The question is whether it makes sense for write_file() to die(). It is a
> > library function and not every caller can be happy with that function to
> > exit the program when some file could not be written, without a chance to
> > tell the user what to do about the situation.
> > 
> > If write_file() was defined in builtin/am.c, as a static function, I would
> > grudgingly acquiesce, but it is not.
> > 
> > IMO it would be better to fix write_file() to *not* die() but return
> > error() instead.
> 
> there is write_file_gently() for that purpose, but it's used only by a
> single caller that exits on failure after all, and in fact Peff's series
> drops it.

Yeah, I always feel funny going in the opposite direction of
libification, as in this case. But having looked at the set of current
and potential-to-convert callers, I couldn't find a single one which
would want the gentle behavior. Any site which doesn't die also wanted
something else more complex (e.g., different open options).

So I think rather than loading down write_file() with options that will
make the simple callers harder to read, we are better off to keep its
implementation simple, and let people call its building blocks easily.

And I think we already have that; in my final version it really is just
xopen/write_in_full/close. So the non-simple sites can still make use of
those components.

-Peff

Re: [PATCH] am: ignore return value of write_file()

[Johannes Schindelin]

Hi Peff,

On Fri, 8 Jul 2016, Jeff King wrote:

> I think we can clean that up, though. I'll hopefully have a series in a
> few minutes.

You caught me at busy times... I'll review it tomorrow, promise!

Ciao,
Dscho

Re: [PATCH] am: ignore return value of write_file()

[Johannes Schindelin]

Hi Peff,

On Sat, 9 Jul 2016, Johannes Schindelin wrote:

> On Fri, 8 Jul 2016, Jeff King wrote:
> 
> > I think we can clean that up, though. I'll hopefully have a series in a
> > few minutes.
> 
> You caught me at busy times... I'll review it tomorrow, promise!

And so I did. Looks good to me, I was surprised that there were no
possible callers is the non-builtin code. But I guess they all use the
lockfile mechanism now.

Ciao,
Dscho