Re: [PATCH 7/7] config: flip return value of store_write_*()

[Jonathan Nieder <jrnieder@gmail.com>]

Hi,

Jeff King wrote:

> The store_write_section() and store_write_pairs() functions
> are basically high-level wrappers around write(). But their
> return values are flipped from our usual convention, using
> "1" for success and "0" for failure.
>
> Let's flip them to follow the usual write() conventions and
> update all callers. As these are local to config.c, it's
> unlikely that we'd have new callers in any topics in flight
> (which would be silently broken by our change). But just to
> be on the safe side, let's rename them to just
> write_section() and write_pairs().  That also accentuates
> their relationship with write().
>
> Signed-off-by: Jeff King <peff@peff.net>

The caller only cares if it succeeded, right?  Could this return
the customary 0 vs -1 instead of the number of bytes written?

That would look like the following (as a patch to squash in):

With or without that tweak,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

diff --git i/config.c w/config.c
index 272a32aac0..8f92d452bf 100644
--- i/config.c
+++ w/config.c
@@ -2297,11 +2297,10 @@ static int write_error(const char *filename)
 	return 4;
 }
 
-static ssize_t write_section(int fd, const char *key)
+static int write_section(int fd, const char *key)
 {
 	const char *dot;
-	int i;
-	ssize_t ret;
+	int i, ret;
 	struct strbuf sb = STRBUF_INIT;
 
 	dot = memchr(key, '.', store.baselen);
@@ -2317,16 +2316,15 @@ static ssize_t write_section(int fd, const char *key)
 		strbuf_addf(&sb, "[%.*s]\n", store.baselen, key);
 	}
 
-	ret = write_in_full(fd, sb.buf, sb.len);
+	ret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;
 	strbuf_release(&sb);
 
 	return ret;
 }
 
-static ssize_t write_pair(int fd, const char *key, const char *value)
+static int write_pair(int fd, const char *key, const char *value)
 {
-	int i;
-	ssize_t ret;
+	int i, ret;
 	int length = strlen(key + store.baselen + 1);
 	const char *quote = "";
 	struct strbuf sb = STRBUF_INIT;
@@ -2366,7 +2364,7 @@ static ssize_t write_pair(int fd, const char *key, const char *value)
 		}
 	strbuf_addf(&sb, "%s\n", quote);
 
-	ret = write_in_full(fd, sb.buf, sb.len);
+	ret = write_in_full(fd, sb.buf, sb.len) < 0 ? -1 : 0;
 	strbuf_release(&sb);
 
 	return ret;
@@ -2499,8 +2497,7 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		}
 
 		store.key = (char *)key;
-		if (write_section(fd, key) < 0 ||
-		    write_pair(fd, key, value) < 0)
+		if (write_section(fd, key) || write_pair(fd, key, value))
 			goto write_err_out;
 	} else {
 		struct stat st;
@@ -2623,10 +2620,10 @@ int git_config_set_multivar_in_file_gently(const char *config_filename,
 		/* write the pair (value == NULL means unset) */
 		if (value != NULL) {
 			if (store.state == START) {
-				if (write_section(fd, key) < 0)
+				if (write_section(fd, key))
 					goto write_err_out;
 			}
-			if (write_pair(fd, key, value) < 0)
+			if (write_pair(fd, key, value))
 				goto write_err_out;
 		}
 
@@ -2820,7 +2817,7 @@ int git_config_rename_section_in_file(const char *config_filename,
 					continue;
 				}
 				store.baselen = strlen(new_name);
-				if (write_section(out_fd, new_name) < 0) {
+				if (write_section(out_fd, new_name)) {
 					ret = write_error(get_lock_file_path(lock));
 					goto out;
 				}