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@xxxxxxxx> 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@xxxxxxxxx> 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; }