am: ignore return value of write_file()

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.

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

index d5da5fe..339e360 100644
@@ -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

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.

---
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(-)

index 2ecde53..15232c4 100644
@@ -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);

index f1dc289..3f6c53f 100644
@@ -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);

2.9.0.393.g704e522

Hi Peff,

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

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

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

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.

---
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(-)

index 1d7c6ef..a991a53 100644
@@ -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

From: René Scharfe <[email protected]>

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.

---
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(-)

index d5da5fe..339e360 100644
@@ -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

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

---
 builtin/branch.c | 5 +----
 1 file changed, 1 insertion(+), 4 deletions(-)

index 2ecde53..15232c4 100644
@@ -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

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

---
 cache.h   |  3 +--
 wrapper.c | 54 +++++++++++-------------------------------------------
 2 files changed, 12 insertions(+), 45 deletions(-)

index f1dc289..3f6c53f 100644
@@ -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);

2.9.0.393.g704e522

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.

---
 wrapper.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

index 0349441..7c126b8 100644
@@ -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

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

---
 cache.h   |  6 ++++++
 wrapper.c | 16 +++++++++++-----
 2 files changed, 17 insertions(+), 5 deletions(-)

index 3f6c53f..9d6ad4f 100644
@@ -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 */

index 7c126b8..b827206 100644
@@ -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

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.

---
 cache.h | 8 ++++++++
 1 file changed, 8 insertions(+)

index 9d6ad4f..c6a282c 100644
@@ -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

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

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.

---
 walker.c | 10 +++++++---
 walker.h |  3 ++-
 2 files changed, 9 insertions(+), 4 deletions(-)

index d95b007..2c86e40 100644
@@ -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)

index 95e5765..a869013 100644
@@ -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

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

---
 builtin/worktree.c | 2 +-
 commit.c           | 2 +-
 2 files changed, 2 insertions(+), 2 deletions(-)

index e866844..cce555c 100644
@@ -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, "../..");

index 3f4f371..9603379 100644
@@ -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

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.

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.

---
 bisect.c |  5 ++++-
 commit.c | 10 ----------
 commit.h |  4 ----
 3 files changed, 4 insertions(+), 15 deletions(-)

index dc13319..02f76f0 100644
@@ -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");

index 3f4f371..bf27972 100644
@@ -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);

index 78ed513..71693ce 100644
@@ -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

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

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.

---
 builtin/am.c    |  7 +------
 builtin/merge.c | 45 +++++----------------------------------------
 2 files changed, 6 insertions(+), 46 deletions(-)

index 339e360..3ac2448 100644
@@ -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);
 }
 
 /**

2.9.0.393.g704e522

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.

---
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(-)

index 15232c4..1d41251 100644
@@ -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

Am 08.07.2016 um 11:04 schrieb Jeff King:

Haha, feels like Candy Crush. :)

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

Thanks,
René

Hi Peff,

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

Ciao,
Dscho

Hi Peff,

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

Hi René,

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

Hi Dscho,

Am 08.07.2016 um 08:33 schrieb Johannes Schindelin:

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é

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