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

[Jeff King]

On Wed, Sep 13, 2017 at 02:25:28PM -0700, Jonathan Nieder wrote:

> > 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 
> 
> The caller only cares if it succeeded, right?  Could this return
> the customary 0 vs -1 instead of the number of bytes written?

Yes, it could. I went with "follow the conventions of write()" because
these are used in a big chain of write() calls (well, really
write_in_full). But given the current callers, it does not matter either
way.

Thanks for reviewing the series, and sorry if my comments have been a
bit terse. I'm trying to clear my pile before going out of town for a
few days (which I admit may have contributed to my desire for you to
prepare patches on top).

But either way, don't expect a re-roll until next week at the earliest.

-Peff

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

[Jonathan Nieder]

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 

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 

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(, "[%.*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();
 
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(, "%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();
 
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;
}

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

[Jeff King]

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 
---
 config.c | 28 +++-
 1 file changed, 15 insertions(+), 13 deletions(-)

diff --git a/config.c b/config.c
index daf093db45..f8b7b81445 100644
--- a/config.c
+++ b/config.c
@@ -2297,10 +2297,11 @@ static int write_error(const char *filename)
return 4;
 }
 
-static int store_write_section(int fd, const char *key)
+static ssize_t write_section(int fd, const char *key)
 {
const char *dot;
-   int i, success;
+   int i;
+   ssize_t ret;
struct strbuf sb = STRBUF_INIT;
 
dot = memchr(key, '.', store.baselen);
@@ -2316,15 +2317,16 @@ static int store_write_section(int fd, const char *key)
strbuf_addf(, "[%.*s]\n", store.baselen, key);
}
 
-   success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+   ret = write_in_full(fd, sb.buf, sb.len);
strbuf_release();
 
-   return success;
+   return ret;
 }
 
-static int store_write_pair(int fd, const char *key, const char *value)
+static ssize_t write_pair(int fd, const char *key, const char *value)
 {
-   int i, success;
+   int i;
+   ssize_t ret;
int length = strlen(key + store.baselen + 1);
const char *quote = "";
struct strbuf sb = STRBUF_INIT;
@@ -2364,10 +2366,10 @@ static int store_write_pair(int fd, const char *key, 
const char *value)
}
strbuf_addf(, "%s\n", quote);
 
-   success = write_in_full(fd, sb.buf, sb.len) == sb.len;
+   ret = write_in_full(fd, sb.buf, sb.len);
strbuf_release();
 
-   return success;
+   return ret;
 }
 
 static ssize_t find_beginning_of_line(const char *contents, size_t size,
@@ -2497,8 +2499,8 @@ int git_config_set_multivar_in_file_gently(const char 
*config_filename,
}
 
store.key = (char *)key;
-   if (!store_write_section(fd, key) ||
-   !store_write_pair(fd, key, value))
+   if (write_section(fd, key) < 0 ||
+   write_pair(fd, key, value) < 0)
goto write_err_out;
} else {
struct stat st;
@@ -2620,10 +2622,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 (!store_write_section(fd, key))
+   if (write_section(fd, key) < 0)
goto write_err_out;
}
-   if (!store_write_pair(fd, key, value))
+   if (write_pair(fd, key, value) < 0)
goto write_err_out;
}
 
@@ -2816,7 +2818,7 @@ int git_config_rename_section_in_file(const char 
*config_filename,
continue;
}
store.baselen = strlen(new_name);
-   if (!store_write_section(out_fd, new_name)) {
+   if (write_section(out_fd, new_name) < 0) {
ret = 
write_error(get_lock_file_path(lock));
goto out;
}
-- 
2.14.1.874.ge7b2e05270