{"thread":{"id":"36008","subject":"[PATCH] implemented strbuf_write_or_die()","startedAt":"2014-03-01T11:21:11Z","lastAt":"2014-03-04T18:45:46Z","messageCount":23,"participants":["Faiz Kothari","He Sun","Johannes Sixt","Michael Haggerty","Eric Sunshine","Junio C Hamano","David Kastrup"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"235712","messageId":"1393672871-28281-1-git-send-email-faiz.off93@gmail.com","threadId":"36008","inReplyTo":null,"subject":"[PATCH] implemented strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-01T11:21:11Z","receivedAt":"2014-03-01T11:21:11Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n---\nImplemented write_or_die.c:strbuf_write_or_die() and used in relevant places\nto substitute write_or_die(). I spotted other places where strbuf can be used\nin place of buf[MAX_PATH] but that would require a change in prototype of a \nlot of functions and functions calling them and so on....\nI'll look for more places where strbuf can be used safely.\n\nThanks.\n\n builtin/cat-file.c | 2 +-\n builtin/notes.c | 4 ++--\n builtin/receive-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n builtin/stripspace.c | 2 +-\n builtin/tag.c | 2 +-\n bundle.c | 2 +-\n cache.h | 1 +\n credential-store.c | 2 +-\n fetch-pack.c | 2 +-\n http-backend.c | 2 +-\n remote-curl.c | 8 +++++---\n write_or_die.c | 9 +++++++++\n 13 files changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d5a93e0..c756cd5 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2b24d05..ef40183 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n \tif (strbuf_read(&buf, show.out, 0) < 0)\n \t\tdie_errno(_(\"could not read 'show' output\"));\n \tstrbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n-\twrite_or_die(fd, cbuf.buf, cbuf.len);\n+\tstrbuf_write_or_die(fd, &cbuf);\n \n \tstrbuf_release(&cbuf);\n \tstrbuf_release(&buf);\n@@ -174,7 +174,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n \t\tstrbuf_addch(&buf, '\\n');\n-\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(fd, &buf);\n \n \t\twrite_commented_object(fd, object);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..9434516 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n \tif (use_sideband)\n \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n \telse\n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n }\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..d053f0a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n \t\t}\n \t\tstrbuf_addch(&buf, '\\n');\n \n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(1, &buf);\n \t}\n \tstrbuf_release(&buf);\n }\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1259ed7..cf5c876 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \telse\n \t\tcomment_lines(&buf);\n \n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 74d3780..5af6ea3 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n \t\t\telse\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n-\t\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(fd, &buf);\n \t\t\tstrbuf_release(&buf);\n \t\t}\n \t\tclose(fd);\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..435505d 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(bundle_fd, &buf);\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\ndiff --git a/cache.h b/cache.h\nindex db223e8..8f953a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n extern void write_or_die(int fd, const void *buf, size_t count);\n+extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);\n extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n extern void fsync_or_die(int fd, const char *);\ndiff --git a/credential-store.c b/credential-store.c\nindex f9146e5..5d2f211 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)\n static void print_line(struct strbuf *buf)\n {\n \tstrbuf_addch(buf, '\\n');\n-\twrite_or_die(credential_lock.fd, buf->buf, buf->len);\n+\tstrbuf_write_or_die(credential_lock.fd, buf);\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f061f1f..a2bf881 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t\tstrbuf_write_or_die(fd, buf);\n }\n \n static void insert_one_alternate_ref(const struct ref *ref, void *unused)\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..0204529 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \thdr_int(content_length, buf->len);\n \thdr_str(content_type, type);\n \tend_headers();\n-\twrite_or_die(1, buf->buf, buf->len);\n+\tstrbuf_write_or_die(1, buf);\n }\n \n static void send_local_file(const char *the_type, const char *name)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 10cb011..dee8716 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\t\tstrbuf_write_or_die(client.in, preamble);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(1, &(rpc.result.buf));\n+\t\n \tstrbuf_release(&rpc.result);\n \tstrbuf_release(&preamble);\n \tfree(depth_arg);\n@@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(1, &(rpc.result.buf));\n+\t\n \tstrbuf_release(&rpc.result);\n \targv_array_clear(&args);\n \treturn err;\ndiff --git a/write_or_die.c b/write_or_die.c\nindex b50f99a..5fb309b 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"strbuf.h\"\n \n static void check_pipe(int err)\n {\n@@ -64,6 +65,14 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t}\n }\n \n+void strbuf_write_or_die(int fd, const struct strbuf *sbuf)\n+{\n+\tif(write_in_full(fd, sbuf->buf, sbuf->len) < 0){\n+\t\tcheck_pipe(errno);\n+\t\tdie_errno(\"write error\");\n+\t}\n+}\n+\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-- \n1.7.9.5\n"},{"id":"235746","messageId":"CAJr59C0e22OuDWU5Xc0A=cc+zY32nfum6SXTDU3wLCPyFPF70A@mail.gmail.com","threadId":"36008","inReplyTo":"1393672871-28281-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"He Sun","fromEmail":"sunheehnus@gmail.com","sentAt":"2014-03-01T12:51:58Z","receivedAt":"2014-03-01T12:51:58Z","isPatch":true,"sender":{"key":"sunheehnus@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2889804?v=4"},"body":"2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n> Signed-off-by: Faiz Kothari \n> ---\n> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places\n> to substitute write_or_die(). I spotted other places where strbuf can be used\n> in place of buf[MAX_PATH] but that would require a change in prototype of a\n> lot of functions and functions calling them and so on....\n> I'll look for more places where strbuf can be used safely.\n>\n> Thanks.\n>\n> builtin/cat-file.c | 2 +-\n> builtin/notes.c | 4 ++--\n> builtin/receive-pack.c | 2 +-\n> builtin/send-pack.c | 2 +-\n> builtin/stripspace.c | 2 +-\n> builtin/tag.c | 2 +-\n> bundle.c | 2 +-\n> cache.h | 1 +\n> credential-store.c | 2 +-\n> fetch-pack.c | 2 +-\n> http-backend.c | 2 +-\n> remote-curl.c | 8 +++++---\n> write_or_die.c | 9 +++++++++\n> 13 files changed, 26 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index d5a93e0..c756cd5 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n>\n> strbuf_expand(&buf, opt->format, expand_format, data);\n> strbuf_addch(&buf, '\\n');\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(1, &buf);\n> strbuf_release(&buf);\n>\n> if (opt->print_contents) {\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 2b24d05..ef40183 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n> if (strbuf_read(&buf, show.out, 0) < 0)\n> die_errno(_(\"could not read 'show' output\"));\n> strbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n> - write_or_die(fd, cbuf.buf, cbuf.len);\n> + strbuf_write_or_die(fd, &cbuf);\n>\n> strbuf_release(&cbuf);\n> strbuf_release(&buf);\n> @@ -174,7 +174,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n> strbuf_addch(&buf, '\\n');\n> strbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n> strbuf_addch(&buf, '\\n');\n> - write_or_die(fd, buf.buf, buf.len);\n> + strbuf_write_or_die(fd, &buf);\n>\n> write_commented_object(fd, object);\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 85bba35..9434516 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n> if (use_sideband)\n> send_sideband(1, 1, buf.buf, buf.len, use_sideband);\n> else\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(1, &buf);\n> strbuf_release(&buf);\n> }\n>\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index f420b74..d053f0a 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n> }\n> strbuf_addch(&buf, '\\n');\n>\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(1, &buf);\n> }\n> strbuf_release(&buf);\n> }\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 1259ed7..cf5c876 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> else\n> comment_lines(&buf);\n>\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(1, &buf);\n> strbuf_release(&buf);\n> return 0;\n> }\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 74d3780..5af6ea3 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n> strbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n> else\n> strbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n> - write_or_die(fd, buf.buf, buf.len);\n> + strbuf_write_or_die(fd, &buf);\n> strbuf_release(&buf);\n> }\n> close(fd);\n> diff --git a/bundle.c b/bundle.c\n> index e99065c..435505d 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n> while (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n> unsigned char sha1[20];\n> if (buf.len > 0 && buf.buf[0] == '-') {\n> - write_or_die(bundle_fd, buf.buf, buf.len);\n> + strbuf_write_or_die(bundle_fd, &buf);\n> if (!get_sha1_hex(buf.buf + 1, sha1)) {\n> struct object *object = parse_object_or_die(sha1, buf.buf);\n> object->flags |= UNINTERESTING;\n> diff --git a/cache.h b/cache.h\n> index db223e8..8f953a2 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);\n> extern int copy_file(const char *dst, const char *src, int mode);\n> extern int copy_file_with_time(const char *dst, const char *src, int mode);\n> extern void write_or_die(int fd, const void *buf, size_t count);\n> +extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);\n> extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n> extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n> extern void fsync_or_die(int fd, const char *);\n> diff --git a/credential-store.c b/credential-store.c\n> index f9146e5..5d2f211 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -48,7 +48,7 @@ static void print_entry(struct credential *c)\n> static void print_line(struct strbuf *buf)\n> {\n> strbuf_addch(buf, '\\n');\n> - write_or_die(credential_lock.fd, buf->buf, buf->len);\n> + strbuf_write_or_die(credential_lock.fd, buf);\n> }\n>\n> static void rewrite_credential_file(const char *fn, struct credential *c,\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f061f1f..a2bf881 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n> send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n> packet_flush(fd);\n> } else\n> - write_or_die(fd, buf->buf, buf->len);\n> + strbuf_write_or_die(fd, buf);\n> }\n>\n> static void insert_one_alternate_ref(const struct ref *ref, void *unused)\n> diff --git a/http-backend.c b/http-backend.c\n> index d2c0a62..0204529 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n> hdr_int(content_length, buf->len);\n> hdr_str(content_type, type);\n> end_headers();\n> - write_or_die(1, buf->buf, buf->len);\n> + strbuf_write_or_die(1, buf);\n> }\n>\n> static void send_local_file(const char *the_type, const char *name)\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 10cb011..dee8716 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n> if (start_command(&client))\n> exit(1);\n> if (preamble)\n> - write_or_die(client.in, preamble->buf, preamble->len);\n> + strbuf_write_or_die(client.in, preamble);\n> if (heads)\n> write_or_die(client.in, heads->buf, heads->len);\n\nThis should be changed. May be you can use Ctrl-F to search write_or_die().\nOr if you are using vim, use \"/ and n\" to find all.\n\n>\n> @@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,\n>\n> err = rpc_service(&rpc, heads);\n> if (rpc.result.len)\n> - write_or_die(1, rpc.result.buf, rpc.result.len);\n> + strbuf_write_or_die(1, &(rpc.result.buf));\n\nMay be this should be\nstrbuf_write_or_die(1, &(rpc.result));\n\n> +\n> strbuf_release(&rpc.result);\n> strbuf_release(&preamble);\n> free(depth_arg);\n> @@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n>\n> err = rpc_service(&rpc, heads);\n> if (rpc.result.len)\n> - write_or_die(1, rpc.result.buf, rpc.result.len);\n> + strbuf_write_or_die(1, &(rpc.result.buf));\n\nSame as above\n\n> +\n> strbuf_release(&rpc.result);\n> argv_array_clear(&args);\n> return err;\n> diff --git a/write_or_die.c b/write_or_die.c\n> index b50f99a..5fb309b 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -1,4 +1,5 @@\n> #include \"cache.h\"\n> +#include \"strbuf.h\"\n>\n> static void check_pipe(int err)\n> {\n> @@ -64,6 +65,14 @@ void write_or_die(int fd, const void *buf, size_t count)\n> }\n> }\n>\n> +void strbuf_write_or_die(int fd, const struct strbuf *sbuf)\n> +{\n> + if(write_in_full(fd, sbuf->buf, sbuf->len) < 0){\n> + check_pipe(errno);\n> + die_errno(\"write error\");\n> + }\n> +}\n> +\n\nMaybe we just call write_or_die() in strbuf_write_or_die(), in case that if we\nwanna change something in write_or_dir(), we don't have to do duplicate jobs.\n\n> int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n> {\n> if (write_in_full(fd, buf, count) < 0) {\n> --\n> 1.7.9.5\n>\n> --\n> To unsubscribe from this list: send the line \"unsubscribe git\" in\n> the body of a message to majordomo@vger.kernel.org\n> More majordomo info at http://vger.kernel.org/majordomo-info.html\n"},{"id":"235747","messageId":"1393680567-2166-1-git-send-email-faiz.off93@gmail.com","threadId":"36008","inReplyTo":"CAJr59C0e22OuDWU5Xc0A=cc+zY32nfum6SXTDU3wLCPyFPF70A@mail.gmail.com","subject":"[PATCH] implemented strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-01T13:29:27Z","receivedAt":"2014-03-01T13:29:27Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n---\n> > - write_or_die(1, rpc.result.buf, rpc.result.len);\n> > + strbuf_write_or_die(1, &(rpc.result.buf));\n\n> May be this should be\n> strbuf_write_or_die(1, &(rpc.result));\n\nYes, I changed that :-) Thanks again.\n\n> Maybe we just call write_or_die() in strbuf_write_or_die(), in case that if we\n> wanna change something in write_or_dir(), we don't have to do duplicate jobs.\n\nYes I changed it. It was unnecessary to reimplement it.\n\nThanks :)\n\n builtin/cat-file.c | 2 +-\n builtin/notes.c | 4 ++--\n builtin/receive-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n builtin/stripspace.c | 2 +-\n builtin/tag.c | 2 +-\n bundle.c | 2 +-\n cache.h | 1 +\n credential-store.c | 2 +-\n fetch-pack.c | 2 +-\n http-backend.c | 2 +-\n remote-curl.c | 8 +++++---\n write_or_die.c | 6 ++++++\n 13 files changed, 23 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d5a93e0..c756cd5 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2b24d05..ef40183 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n \tif (strbuf_read(&buf, show.out, 0) < 0)\n \t\tdie_errno(_(\"could not read 'show' output\"));\n \tstrbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n-\twrite_or_die(fd, cbuf.buf, cbuf.len);\n+\tstrbuf_write_or_die(fd, &cbuf);\n \n \tstrbuf_release(&cbuf);\n \tstrbuf_release(&buf);\n@@ -174,7 +174,7 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n \t\tstrbuf_addch(&buf, '\\n');\n-\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(fd, &buf);\n \n \t\twrite_commented_object(fd, object);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..9434516 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n \tif (use_sideband)\n \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n \telse\n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n }\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..d053f0a 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n \t\t}\n \t\tstrbuf_addch(&buf, '\\n');\n \n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(1, &buf);\n \t}\n \tstrbuf_release(&buf);\n }\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1259ed7..cf5c876 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \telse\n \t\tcomment_lines(&buf);\n \n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(1, &buf);\n \tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 74d3780..5af6ea3 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n \t\t\telse\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n-\t\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(fd, &buf);\n \t\t\tstrbuf_release(&buf);\n \t\t}\n \t\tclose(fd);\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..435505d 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(bundle_fd, &buf);\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\ndiff --git a/cache.h b/cache.h\nindex db223e8..8f953a2 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n extern void write_or_die(int fd, const void *buf, size_t count);\n+extern void strbuf_write_or_die(int fd, const struct strbuf *sbuf);\n extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n extern void fsync_or_die(int fd, const char *);\ndiff --git a/credential-store.c b/credential-store.c\nindex f9146e5..5d2f211 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)\n static void print_line(struct strbuf *buf)\n {\n \tstrbuf_addch(buf, '\\n');\n-\twrite_or_die(credential_lock.fd, buf->buf, buf->len);\n+\tstrbuf_write_or_die(credential_lock.fd, buf);\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f061f1f..a2bf881 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t\tstrbuf_write_or_die(fd, buf);\n }\n \n static void insert_one_alternate_ref(const struct ref *ref, void *unused)\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..0204529 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \thdr_int(content_length, buf->len);\n \thdr_str(content_type, type);\n \tend_headers();\n-\twrite_or_die(1, buf->buf, buf->len);\n+\tstrbuf_write_or_die(1, buf);\n }\n \n static void send_local_file(const char *the_type, const char *name)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 10cb011..7d45e5e 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\t\tstrbuf_write_or_die(client.in, preamble);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -767,7 +767,8 @@ static int fetch_git(struct discovery *heads,\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(1, &(rpc.result));\n+\t\n \tstrbuf_release(&rpc.result);\n \tstrbuf_release(&preamble);\n \tfree(depth_arg);\n@@ -889,7 +890,8 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(1, &(rpc.result));\n+\t\n \tstrbuf_release(&rpc.result);\n \targv_array_clear(&args);\n \treturn err;\ndiff --git a/write_or_die.c b/write_or_die.c\nindex b50f99a..7ed9821 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"strbuf.h\"\n \n static void check_pipe(int err)\n {\n@@ -64,6 +65,11 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t}\n }\n \n+void strbuf_write_or_die(int fd, const struct strbuf *sbuf)\n+{\n+\twrite_or_die(fd, sbuf->buf, sbuf->len);\n+}\n+\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-- \n1.7.9.5\n"},{"id":"235762","messageId":"53125266.8040007@kdbg.org","threadId":"36008","inReplyTo":"1393672871-28281-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2014-03-01T21:34:30Z","receivedAt":"2014-03-01T21:34:30Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 01.03.2014 12:21, schrieb Faiz Kothari:\n> Signed-off-by: Faiz Kothari \n> ---\n> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places\n> to substitute write_or_die(). I spotted other places where strbuf can be used\n> in place of buf[MAX_PATH] but that would require a change in prototype of a \n> lot of functions and functions calling them and so on....\n> I'll look for more places where strbuf can be used safely.\n\nYou haven't given a justifiction of the change (why is this change good?)\n\n> diff --git a/write_or_die.c b/write_or_die.c\n> index b50f99a..5fb309b 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -1,4 +1,5 @@\n> #include \"cache.h\"\n> +#include \"strbuf.h\"\n\nI think you have the layering backwards here: strbuf_write_or_die should\nbe part of the (higher-level) strbuf API, and not an extension of the\nlow-level write_or_die function.\n\n> \n> static void check_pipe(int err)\n> {\n> @@ -64,6 +65,14 @@ void write_or_die(int fd, const void *buf, size_t count)\n> \t}\n> }\n> \n> +void strbuf_write_or_die(int fd, const struct strbuf *sbuf)\n\nAnd when you make the function a strbuf API, the prototype should be\n\nvoid strbuf_write_or_die(const struct strbuf *sbuf, int fd)\n\nas in \"hey, strbuf object, write your content to this file descriptor!\"\n\n> +{\n> +\tif(write_in_full(fd, sbuf->buf, sbuf->len) < 0){\n> +\t\tcheck_pipe(errno);\n> +\t\tdie_errno(\"write error\");\n> +\t}\n> +}\n> +\n> int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n> {\n> \tif (write_in_full(fd, buf, count) < 0) {\n> \n\n-- Hannes\n"},{"id":"235763","messageId":"53126051.7030904@alum.mit.edu","threadId":"36008","inReplyTo":"1393680567-2166-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-01T22:33:53Z","receivedAt":"2014-03-01T22:33:53Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Please leave a little more time for people to give feedback between\nversions of a patch series (unless the first version was so broken that\nit would be pointless for any other reviewer to waste time on it. And\nplease label the versions of a single patch series \"[PATCH]\" then\n\"[PATCH v2]\", \"[PATCH v3]\", etc.\n\nI agree with Johannes's advice that the function is in the wrong place\nand has the wrong parameter order.\n\nOn 03/01/2014 02:29 PM, Faiz Kothari wrote:\n> Signed-off-by: Faiz Kothari \n> ---\n>>> - write_or_die(1, rpc.result.buf, rpc.result.len);\n>>> + strbuf_write_or_die(1, &(rpc.result.buf));\n> \n>> May be this should be\n>> strbuf_write_or_die(1, &(rpc.result));\n> \n> Yes, I changed that :-) Thanks again.\n\nI find it alarming that either the compiler didn't emit warnings for the\nold version or that you ignored the compiler warnings. Git should\ncompile without warnings even with with quite strict compiler settings;\nI use gcc with the following options\n\n -Wall -Werror \\\n -Wdeclaration-after-statement \\\n -Wno-format-zero-length \\\n -Wno-format-security\n\nMaybe you weren't including the header file that declares\nstrbuf_write_or_die() in the file containing this invocation?\n\nAlso, the parentheses in \"&(rpc.result)\" are unnecessary.\n\nAnd I think that some of the blank lines that you added contained\ninvisible whitespace. Please check your whitespace! You can run \"git\ndiff --check\" to detect some obvious whitespace problems.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"235766","messageId":"1393719527-24221-1-git-send-email-faiz.off93@gmail.com","threadId":"36008","inReplyTo":"53126051.7030904@alum.mit.edu","subject":"[PATCH v2] implemented strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-02T00:18:47Z","receivedAt":"2014-03-02T00:18:47Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n---\nThanks for the feedback.\nImplemented write_or_dir.c:strbuf_write_or_die() again.\nChecks if NULL is passed to prevent segmentation fault, I was not sure \nwhat error message to print so for now its \"write error\".\nChanged the prototype as suggested.\nImplementing this clearly distinguishes between writing a normal buffer\nand writing a strbuf. Also, it provides an interface to write strbuf\ndirectly without knowing the private members of strbuf, making strbuf \ncompletely opaque. Also, makes the code more readable.\nI hope its proper now.\n\nThanks.\n\n builtin/cat-file.c | 2 +-\n builtin/notes.c | 6 +++---\n builtin/receive-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n builtin/stripspace.c | 2 +-\n builtin/tag.c | 2 +-\n bundle.c | 2 +-\n cache.h | 1 +\n fetch-pack.c | 2 +-\n http-backend.c | 2 +-\n remote-curl.c | 6 +++---\n write_or_die.c | 10 ++++++++++\n 12 files changed, 25 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d5a93e0..d07a0be 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2b24d05..a208d56 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n \tif (strbuf_read(&buf, show.out, 0) < 0)\n \t\tdie_errno(_(\"could not read 'show' output\"));\n \tstrbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n-\twrite_or_die(fd, cbuf.buf, cbuf.len);\n+\tstrbuf_write_or_die(&cbuf, fd);\n \n \tstrbuf_release(&cbuf);\n \tstrbuf_release(&buf);\n@@ -167,14 +167,14 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\t\tdie_errno(_(\"could not create file '%s'\"), path);\n \n \t\tif (msg->given)\n-\t\t\twrite_or_die(fd, msg->buf.buf, msg->buf.len);\n+\t\t\tstrbuf_write_or_die(&(msg->buf), fd);\n \t\telse if (prev && !append_only)\n \t\t\twrite_note_data(fd, prev);\n \n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n \t\tstrbuf_addch(&buf, '\\n');\n-\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, fd);\n \n \t\twrite_commented_object(fd, object);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..d590993 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n \tif (use_sideband)\n \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n \telse\n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n }\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..f26ba21 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n \t\t}\n \t\tstrbuf_addch(&buf, '\\n');\n \n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, 1);\n \t}\n \tstrbuf_release(&buf);\n }\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1259ed7..33b7f85 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \telse\n \t\tcomment_lines(&buf);\n \n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 74d3780..53ab280 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n \t\t\telse\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n-\t\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(&buf, fd);\n \t\t\tstrbuf_release(&buf);\n \t\t}\n \t\tclose(fd);\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..c8bddd8 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(&buf, bundle_fd);\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\ndiff --git a/cache.h b/cache.h\nindex db223e8..8898bf4 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n extern int copy_file_with_time(const char *dst, const char *src, int mode);\n extern void write_or_die(int fd, const void *buf, size_t count);\n+extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);\n extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n extern void fsync_or_die(int fd, const char *);\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f061f1f..af28bd2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t\tstrbuf_write_or_die(buf, fd);\n }\n \n static void insert_one_alternate_ref(const struct ref *ref, void *unused)\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..bed9341 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \thdr_int(content_length, buf->len);\n \thdr_str(content_type, type);\n \tend_headers();\n-\twrite_or_die(1, buf->buf, buf->len);\n+\tstrbuf_write_or_die(buf, 1);\n }\n \n static void send_local_file(const char *the_type, const char *name)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 10cb011..677df24 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\t\tstrbuf_write_or_die(preamble, client.in);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(&rpc.result, 1);\n \tstrbuf_release(&rpc.result);\n \tstrbuf_release(&preamble);\n \tfree(depth_arg);\n@@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(&rpc.result, 1);\n \tstrbuf_release(&rpc.result);\n \targv_array_clear(&args);\n \treturn err;\ndiff --git a/write_or_die.c b/write_or_die.c\nindex b50f99a..373450e 100644\n--- a/write_or_die.c\n+++ b/write_or_die.c\n@@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)\n \t}\n }\n \n+void strbuf_write_or_die(const struct strbuf *sbuf, int fd)\n+{\n+\tif(!sbuf)\n+\t\tfprintf(stderr, \"write error\\n\");\n+\telse if (write_in_full(fd, sbuf->buf, sbuf->len) < 0) {\n+\t\tcheck_pipe(errno);\n+\t\tdie_errno(\"write error\");\n+\t}\n+}\n+\n int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg)\n {\n \tif (write_in_full(fd, buf, count) < 0) {\n-- \n1.7.9.5\n"},{"id":"235771","messageId":"CAPig+cR0gTeB1oUUPQ=W_wWGPMDaY4gkG7+531HT-ktnwn5L-Q@mail.gmail.com","threadId":"36008","inReplyTo":"1393719527-24221-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH v2] implemented strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T02:47:27Z","receivedAt":"2014-03-02T02:47:27Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari wrote:\n> Subject: implemented strbuf_write_or_die()\n\nImperative tone is preferred. The commit message tells what it is\ndoing, not what it did.\n\n Subject: introduce strbuf_write_or_die()\n\n> Signed-off-by: Faiz Kothari \n> ---\n> Implementing this clearly distinguishes between writing a normal buffer\n> and writing a strbuf. Also, it provides an interface to write strbuf\n> directly without knowing the private members of strbuf, making strbuf\n> completely opaque. Also, makes the code more readable.\n\nThese three sentences which justify the change should go above the\n\"---\" line so they are recorded in the project history for future\nreaders to understand why the change was made. As for their actual\nvalue, the first and third sentences are nebulous; the second sentence\nmay be suitable (although strbuf's buf and len are actually considered\npublic, so the justification not be supportable).\n\nA couple other comments:\n\nJustification is important because many topics are in-flight at any\ngiven time. Changes like this one which touch an arbitrary number of\nfiles may conflict with other in-flight topics, which places extra\nburden on the project maintainer (Junio). The justification needs to\nbe strong enough to outweigh that added burden.\n\nIntroduction of a new function is conceptually distinct from the act\nof updating existing callers to invoke it, thus this submission should\nprobably be decomposed into two or more patches where the first patch\nintroduces the function, and following patches convert existing\ncallers.\n\nMore below.\n\n> diff --git a/cache.h b/cache.h\n> index db223e8..8898bf4 100644\n> --- a/cache.h\n> +++ b/cache.h\n> @@ -1227,6 +1227,7 @@ extern int copy_fd(int ifd, int ofd);\n> extern int copy_file(const char *dst, const char *src, int mode);\n> extern int copy_file_with_time(const char *dst, const char *src, int mode);\n> extern void write_or_die(int fd, const void *buf, size_t count);\n> +extern void strbuf_write_or_die(const struct strbuf *sbuf, int fd);\n\nYou still have the layering violation here mentioned by both Johannes\nand Michael. This declaration belongs in strbuf.h, not cache.h\n\nAlso, for bonus points, you should document this new function in\nDocumentation/technical/api-strbuf.txt.\n\n> extern int write_or_whine(int fd, const void *buf, size_t count, const char *msg);\n> extern int write_or_whine_pipe(int fd, const void *buf, size_t count, const char *msg);\n> extern void fsync_or_die(int fd, const char *);\n> diff --git a/write_or_die.c b/write_or_die.c\n> index b50f99a..373450e 100644\n> --- a/write_or_die.c\n> +++ b/write_or_die.c\n> @@ -64,6 +64,16 @@ void write_or_die(int fd, const void *buf, size_t count)\n> }\n> }\n>\n> +void strbuf_write_or_die(const struct strbuf *sbuf, int fd)\n\nDitto regarding the layering violation. This implementation belongs in strbuf.c.\n\n> +{\n> + if(!sbuf)\n> + fprintf(stderr, \"write error\\n\");\n\nThis is a programmer error, rather than a run-time I/O error. As such,\nan assertion would be appropriate:\n\n assert(sbuf);\n\n> + else if (write_in_full(fd, sbuf->buf, sbuf->len) < 0) {\n> + check_pipe(errno);\n> + die_errno(\"write error\");\n\nHe Sun correctly pointed out in his review that you should simply\ninvoke the lower-level write_or_die() here rather than copying/pasting\nits functionality. (You fixed it in an earlier version of the patch\nbut somehow reverted that fix when sending this version.)\n\n> + }\n> +}\n"},{"id":"235776","messageId":"CAPig+cRXSQkC6eG+TAZQoZ5kJNjuv9Rq5QYjpFbEqDSeuVWU3g@mail.gmail.com","threadId":"36008","inReplyTo":"CAPig+cR0gTeB1oUUPQ=W_wWGPMDaY4gkG7+531HT-ktnwn5L-Q@mail.gmail.com","subject":"Re: [PATCH v2] implemented strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T02:55:52Z","receivedAt":"2014-03-02T02:55:52Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 1, 2014 at 9:47 PM, Eric Sunshine wrote:\n> On Sat, Mar 1, 2014 at 7:18 PM, Faiz Kothari wrote:\n>> Implementing this clearly distinguishes between writing a normal buffer\n>> and writing a strbuf. Also, it provides an interface to write strbuf\n>> directly without knowing the private members of strbuf, making strbuf\n>> completely opaque. Also, makes the code more readable.\n>\n> These three sentences which justify the change should go above the\n> \"---\" line so they are recorded in the project history for future\n> readers to understand why the change was made. As for their actual\n> value, the first and third sentences are nebulous; the second sentence\n> may be suitable (although strbuf's buf and len are actually considered\n> public, so the justification not be supportable).\n\n...justification *may* not be...\n"},{"id":"235777","messageId":"CAPig+cRgc4UtmJMieS9Mdrz7vjUNiu7QFu1PSBppKo22Ln5G-A@mail.gmail.com","threadId":"36008","inReplyTo":"CAJr59C0e22OuDWU5Xc0A=cc+zY32nfum6SXTDU3wLCPyFPF70A@mail.gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T03:08:23Z","receivedAt":"2014-03-02T03:08:23Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>> diff --git a/remote-curl.c b/remote-curl.c\n>> index 10cb011..dee8716 100644\n>> --- a/remote-curl.c\n>> +++ b/remote-curl.c\n>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>> if (start_command(&client))\n>> exit(1);\n>> if (preamble)\n>> - write_or_die(client.in, preamble->buf, preamble->len);\n>> + strbuf_write_or_die(client.in, preamble);\n>> if (heads)\n>> write_or_die(client.in, heads->buf, heads->len);\n>\n> This should be changed. May be you can use Ctrl-F to search write_or_die().\n> Or if you are using vim, use \"/ and n\" to find all.\n\nIt's not obvious from the patch fragment, but 'heads' is not a strbuf,\nso Faiz correctly left this invocation alone.\n"},{"id":"235786","messageId":"1393745664-21077-1-git-send-email-faiz.off93@gmail.com","threadId":"36008","inReplyTo":"CAPig+cR0gTeB1oUUPQ=W_wWGPMDaY4gkG7+531HT-ktnwn5L-Q@mail.gmail.com","subject":"[PATCH v3 1/2] Introduce strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-02T07:34:23Z","receivedAt":"2014-03-02T07:34:23Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n\nIntroduced a new function strbuf.c:strbuf_write_or_die()\nto the strbuf family of functions. Now use this API instead\nof write_or_die.c:write_or_die()\n---\nHi,\nThanks for the suggestions and feedbacks.\nAs Johannes Sixt pointed out, the function is now defined\nin strbuf.c and prototype added to strbuf.h\nAlso, replaced if(!sbuf) with assert(sbuf) and split the patch into two \nas pointed out by Eric Sunshine.\n\nAs far as justification is concerned, I am not able to come up with\na satisfactory justification. Apart from, that it makes life of the\nprogrammer a little easier and if we add a few more functions\nto thestrbuf API, we can make strbuf completely opaque. I am open\nto views and since I haven't used this API extensively, I cannot\ncomment for what is missing and what is required. But I am going through it.\nAlso, once this patch is OK, I'll add documentation for the API.\n\nThanks again for the feedback.\n\n strbuf.c | 6 ++++++\n strbuf.h | 1 +\n 2 files changed, 7 insertions(+)\n\ndiff --git a/strbuf.c b/strbuf.c\nindex 83caf4a..337a70c 100644\n--- a/strbuf.c\n+++ b/strbuf.c\n@@ -477,6 +477,12 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n \treturn len;\n }\n \n+void strbuf_write_or_die(const struct strbuf *sb, int fd)\n+{\n+\tassert(sb);\n+\twrite_or_die(fd, sb->buf, sb->len);\n+}\n+\n void strbuf_add_lines(struct strbuf *out, const char *prefix,\n \t\t const char *buf, size_t size)\n {\ndiff --git a/strbuf.h b/strbuf.h\nindex 73e80ce..6aadb6d 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -156,6 +156,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n /* XXX: if read fails, any partial read is undone */\n extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n+extern void strbuf_write_or_die(const struct strbuf *sb, int fd);\n extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n \n extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n-- \n1.7.9.5\n"},{"id":"235787","messageId":"1393745664-21077-2-git-send-email-faiz.off93@gmail.com","threadId":"36008","inReplyTo":"1393745664-21077-1-git-send-email-faiz.off93@gmail.com","subject":"[PATCH v3 2/2] use strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-02T07:34:24Z","receivedAt":"2014-03-02T07:34:24Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"Signed-off-by: Faiz Kothari \n\nUsed strbuf.c:strbuf_write_or_die() instead of\nwrite_or_die.c:write_or_die() at relevant places.\n---\n builtin/cat-file.c | 2 +-\n builtin/notes.c | 6 +++---\n builtin/receive-pack.c | 2 +-\n builtin/send-pack.c | 2 +-\n builtin/stripspace.c | 2 +-\n builtin/tag.c | 2 +-\n bundle.c | 2 +-\n credential-store.c | 2 +-\n fetch-pack.c | 2 +-\n http-backend.c | 2 +-\n remote-curl.c | 6 +++---\n 11 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/cat-file.c b/builtin/cat-file.c\nindex d5a93e0..d07a0be 100644\n--- a/builtin/cat-file.c\n+++ b/builtin/cat-file.c\n@@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n \n \tstrbuf_expand(&buf, opt->format, expand_format, data);\n \tstrbuf_addch(&buf, '\\n');\n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n \n \tif (opt->print_contents) {\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 2b24d05..a208d56 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n \tif (strbuf_read(&buf, show.out, 0) < 0)\n \t\tdie_errno(_(\"could not read 'show' output\"));\n \tstrbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n-\twrite_or_die(fd, cbuf.buf, cbuf.len);\n+\tstrbuf_write_or_die(&cbuf, fd);\n \n \tstrbuf_release(&cbuf);\n \tstrbuf_release(&buf);\n@@ -167,14 +167,14 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n \t\t\tdie_errno(_(\"could not create file '%s'\"), path);\n \n \t\tif (msg->given)\n-\t\t\twrite_or_die(fd, msg->buf.buf, msg->buf.len);\n+\t\t\tstrbuf_write_or_die(&(msg->buf), fd);\n \t\telse if (prev && !append_only)\n \t\t\twrite_note_data(fd, prev);\n \n \t\tstrbuf_addch(&buf, '\\n');\n \t\tstrbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n \t\tstrbuf_addch(&buf, '\\n');\n-\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, fd);\n \n \t\twrite_commented_object(fd, object);\n \ndiff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\nindex 85bba35..d590993 100644\n--- a/builtin/receive-pack.c\n+++ b/builtin/receive-pack.c\n@@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n \tif (use_sideband)\n \t\tsend_sideband(1, 1, buf.buf, buf.len, use_sideband);\n \telse\n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n }\n \ndiff --git a/builtin/send-pack.c b/builtin/send-pack.c\nindex f420b74..f26ba21 100644\n--- a/builtin/send-pack.c\n+++ b/builtin/send-pack.c\n@@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n \t\t}\n \t\tstrbuf_addch(&buf, '\\n');\n \n-\t\twrite_or_die(1, buf.buf, buf.len);\n+\t\tstrbuf_write_or_die(&buf, 1);\n \t}\n \tstrbuf_release(&buf);\n }\ndiff --git a/builtin/stripspace.c b/builtin/stripspace.c\nindex 1259ed7..33b7f85 100644\n--- a/builtin/stripspace.c\n+++ b/builtin/stripspace.c\n@@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n \telse\n \t\tcomment_lines(&buf);\n \n-\twrite_or_die(1, buf.buf, buf.len);\n+\tstrbuf_write_or_die(&buf, 1);\n \tstrbuf_release(&buf);\n \treturn 0;\n }\ndiff --git a/builtin/tag.c b/builtin/tag.c\nindex 74d3780..53ab280 100644\n--- a/builtin/tag.c\n+++ b/builtin/tag.c\n@@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n \t\t\telse\n \t\t\t\tstrbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n-\t\t\twrite_or_die(fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(&buf, fd);\n \t\t\tstrbuf_release(&buf);\n \t\t}\n \t\tclose(fd);\ndiff --git a/bundle.c b/bundle.c\nindex e99065c..c8bddd8 100644\n--- a/bundle.c\n+++ b/bundle.c\n@@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n \twhile (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n \t\tunsigned char sha1[20];\n \t\tif (buf.len > 0 && buf.buf[0] == '-') {\n-\t\t\twrite_or_die(bundle_fd, buf.buf, buf.len);\n+\t\t\tstrbuf_write_or_die(&buf, bundle_fd);\n \t\t\tif (!get_sha1_hex(buf.buf + 1, sha1)) {\n \t\t\t\tstruct object *object = parse_object_or_die(sha1, buf.buf);\n \t\t\t\tobject->flags |= UNINTERESTING;\ndiff --git a/credential-store.c b/credential-store.c\nindex f9146e5..6cb5fcb 100644\n--- a/credential-store.c\n+++ b/credential-store.c\n@@ -48,7 +48,7 @@ static void print_entry(struct credential *c)\n static void print_line(struct strbuf *buf)\n {\n \tstrbuf_addch(buf, '\\n');\n-\twrite_or_die(credential_lock.fd, buf->buf, buf->len);\n+\tstrbuf_write_or_die(buf, credential_lock.fd);\n }\n \n static void rewrite_credential_file(const char *fn, struct credential *c,\ndiff --git a/fetch-pack.c b/fetch-pack.c\nindex f061f1f..af28bd2 100644\n--- a/fetch-pack.c\n+++ b/fetch-pack.c\n@@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n \t\tsend_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n \t\tpacket_flush(fd);\n \t} else\n-\t\twrite_or_die(fd, buf->buf, buf->len);\n+\t\tstrbuf_write_or_die(buf, fd);\n }\n \n static void insert_one_alternate_ref(const struct ref *ref, void *unused)\ndiff --git a/http-backend.c b/http-backend.c\nindex d2c0a62..bed9341 100644\n--- a/http-backend.c\n+++ b/http-backend.c\n@@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n \thdr_int(content_length, buf->len);\n \thdr_str(content_type, type);\n \tend_headers();\n-\twrite_or_die(1, buf->buf, buf->len);\n+\tstrbuf_write_or_die(buf, 1);\n }\n \n static void send_local_file(const char *the_type, const char *name)\ndiff --git a/remote-curl.c b/remote-curl.c\nindex 10cb011..677df24 100644\n--- a/remote-curl.c\n+++ b/remote-curl.c\n@@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n \tif (start_command(&client))\n \t\texit(1);\n \tif (preamble)\n-\t\twrite_or_die(client.in, preamble->buf, preamble->len);\n+\t\tstrbuf_write_or_die(preamble, client.in);\n \tif (heads)\n \t\twrite_or_die(client.in, heads->buf, heads->len);\n \n@@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(&rpc.result, 1);\n \tstrbuf_release(&rpc.result);\n \tstrbuf_release(&preamble);\n \tfree(depth_arg);\n@@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n \n \terr = rpc_service(&rpc, heads);\n \tif (rpc.result.len)\n-\t\twrite_or_die(1, rpc.result.buf, rpc.result.len);\n+\t\tstrbuf_write_or_die(&rpc.result, 1);\n \tstrbuf_release(&rpc.result);\n \targv_array_clear(&args);\n \treturn err;\n-- \n1.7.9.5\n"},{"id":"235810","messageId":"CAPig+cQfbxX0oH_tfaQt5PT0DP29hRZ8seZdJ-ssLDQbH=iJGQ@mail.gmail.com","threadId":"36008","inReplyTo":"1393745664-21077-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH v3 1/2] Introduce strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T20:05:39Z","receivedAt":"2014-03-02T20:05:39Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari wrote:\n> Signed-off-by: Faiz Kothari \n\nPlace your sign off below the commit message.\n\n> Introduced a new function strbuf.c:strbuf_write_or_die()\n> to the strbuf family of functions. Now use this API instead\n> of write_or_die.c:write_or_die()\n\nYou want to explain what this patch is doing in imperative tone. Use\n\"Introduce\" rather than \"Introduced\". The first sentence correctly\nstates what the patch is doing, however, the second sentence explains\nwhat the next patch is doing, so it doesn't belong here. So, your\ncommit message for this patch might become:\n\n Subject: strbuf: introduce strbuf_write_or_die()\n\n Add strbuf convenience wrapper around lower-level write_or_die().\n\n> ---\n> Hi,\n> Thanks for the suggestions and feedbacks.\n> As Johannes Sixt pointed out, the function is now defined\n> in strbuf.c and prototype added to strbuf.h\n> Also, replaced if(!sbuf) with assert(sbuf) and split the patch into two\n> as pointed out by Eric Sunshine.\n\nGood explanation of what changed since the last attempt.\n\n> As far as justification is concerned, I am not able to come up with\n> a satisfactory justification. Apart from, that it makes life of the\n> programmer a little easier and if we add a few more functions\n> to thestrbuf API, we can make strbuf completely opaque. I am open\n> to views and since I haven't used this API extensively, I cannot\n> comment for what is missing and what is required. But I am going through it.\n> Also, once this patch is OK, I'll add documentation for the API.\n\nIt's a good idea to add documentation when you add the function\nitself, otherwise reviewers will have to wait yet another round to\nreview that addition. In this case, the documentation will likely be\none line, so it shouldn't be a particular burden to write it.\n\n> Thanks again for the feedback.\n>\n> strbuf.c | 6 ++++++\n> strbuf.h | 1 +\n> 2 files changed, 7 insertions(+)\n>\n> diff --git a/strbuf.c b/strbuf.c\n> index 83caf4a..337a70c 100644\n> --- a/strbuf.c\n> +++ b/strbuf.c\n> @@ -477,6 +477,12 @@ int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint)\n> return len;\n> }\n>\n> +void strbuf_write_or_die(const struct strbuf *sb, int fd)\n> +{\n> + assert(sb);\n> + write_or_die(fd, sb->buf, sb->len);\n> +}\n\nNice. Much better than previous versions of the patch.\n\n> void strbuf_add_lines(struct strbuf *out, const char *prefix,\n> const char *buf, size_t size)\n> {\n> diff --git a/strbuf.h b/strbuf.h\n> index 73e80ce..6aadb6d 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -156,6 +156,7 @@ extern size_t strbuf_fread(struct strbuf *, size_t, FILE *);\n> /* XXX: if read fails, any partial read is undone */\n> extern ssize_t strbuf_read(struct strbuf *, int fd, size_t hint);\n> extern int strbuf_read_file(struct strbuf *sb, const char *path, size_t hint);\n> +extern void strbuf_write_or_die(const struct strbuf *sb, int fd);\n> extern int strbuf_readlink(struct strbuf *sb, const char *path, size_t hint);\n>\n> extern int strbuf_getwholeline(struct strbuf *, FILE *, int);\n> --\n> 1.7.9.5\n"},{"id":"235811","messageId":"CAPig+cRVaNa=Gi5gck0RDKHUkjLtQgMO8KaQiUZ=oL0pimw0bQ@mail.gmail.com","threadId":"36008","inReplyTo":"1393745664-21077-2-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH v3 2/2] use strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-02T22:04:12Z","receivedAt":"2014-03-02T22:04:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Mar 2, 2014 at 2:34 AM, Faiz Kothari wrote:\n> Signed-off-by: Faiz Kothari \n\nSign off below commit message.\n\n> Used strbuf.c:strbuf_write_or_die() instead of\n> write_or_die.c:write_or_die() at relevant places.\n\nImperative: \"Use strbuf...\"\n\nOtherwise, the patch looks okay.\n\n> ---\n> builtin/cat-file.c | 2 +-\n> builtin/notes.c | 6 +++---\n> builtin/receive-pack.c | 2 +-\n> builtin/send-pack.c | 2 +-\n> builtin/stripspace.c | 2 +-\n> builtin/tag.c | 2 +-\n> bundle.c | 2 +-\n> credential-store.c | 2 +-\n> fetch-pack.c | 2 +-\n> http-backend.c | 2 +-\n> remote-curl.c | 6 +++---\n> 11 files changed, 15 insertions(+), 15 deletions(-)\n>\n> diff --git a/builtin/cat-file.c b/builtin/cat-file.c\n> index d5a93e0..d07a0be 100644\n> --- a/builtin/cat-file.c\n> +++ b/builtin/cat-file.c\n> @@ -255,7 +255,7 @@ static int batch_one_object(const char *obj_name, struct batch_options *opt,\n>\n> strbuf_expand(&buf, opt->format, expand_format, data);\n> strbuf_addch(&buf, '\\n');\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, 1);\n> strbuf_release(&buf);\n>\n> if (opt->print_contents) {\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 2b24d05..a208d56 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -140,7 +140,7 @@ static void write_commented_object(int fd, const unsigned char *object)\n> if (strbuf_read(&buf, show.out, 0) < 0)\n> die_errno(_(\"could not read 'show' output\"));\n> strbuf_add_commented_lines(&cbuf, buf.buf, buf.len);\n> - write_or_die(fd, cbuf.buf, cbuf.len);\n> + strbuf_write_or_die(&cbuf, fd);\n>\n> strbuf_release(&cbuf);\n> strbuf_release(&buf);\n> @@ -167,14 +167,14 @@ static void create_note(const unsigned char *object, struct msg_arg *msg,\n> die_errno(_(\"could not create file '%s'\"), path);\n>\n> if (msg->given)\n> - write_or_die(fd, msg->buf.buf, msg->buf.len);\n> + strbuf_write_or_die(&(msg->buf), fd);\n> else if (prev && !append_only)\n> write_note_data(fd, prev);\n>\n> strbuf_addch(&buf, '\\n');\n> strbuf_add_commented_lines(&buf, note_template, strlen(note_template));\n> strbuf_addch(&buf, '\\n');\n> - write_or_die(fd, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, fd);\n>\n> write_commented_object(fd, object);\n>\n> diff --git a/builtin/receive-pack.c b/builtin/receive-pack.c\n> index 85bba35..d590993 100644\n> --- a/builtin/receive-pack.c\n> +++ b/builtin/receive-pack.c\n> @@ -1114,7 +1114,7 @@ static void report(struct command *commands, const char *unpack_status)\n> if (use_sideband)\n> send_sideband(1, 1, buf.buf, buf.len, use_sideband);\n> else\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, 1);\n> strbuf_release(&buf);\n> }\n>\n> diff --git a/builtin/send-pack.c b/builtin/send-pack.c\n> index f420b74..f26ba21 100644\n> --- a/builtin/send-pack.c\n> +++ b/builtin/send-pack.c\n> @@ -86,7 +86,7 @@ static void print_helper_status(struct ref *ref)\n> }\n> strbuf_addch(&buf, '\\n');\n>\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, 1);\n> }\n> strbuf_release(&buf);\n> }\n> diff --git a/builtin/stripspace.c b/builtin/stripspace.c\n> index 1259ed7..33b7f85 100644\n> --- a/builtin/stripspace.c\n> +++ b/builtin/stripspace.c\n> @@ -115,7 +115,7 @@ int cmd_stripspace(int argc, const char **argv, const char *prefix)\n> else\n> comment_lines(&buf);\n>\n> - write_or_die(1, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, 1);\n> strbuf_release(&buf);\n> return 0;\n> }\n> diff --git a/builtin/tag.c b/builtin/tag.c\n> index 74d3780..53ab280 100644\n> --- a/builtin/tag.c\n> +++ b/builtin/tag.c\n> @@ -349,7 +349,7 @@ static void create_tag(const unsigned char *object, const char *tag,\n> strbuf_commented_addf(&buf, _(tag_template), comment_line_char);\n> else\n> strbuf_commented_addf(&buf, _(tag_template_nocleanup), comment_line_char);\n> - write_or_die(fd, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, fd);\n> strbuf_release(&buf);\n> }\n> close(fd);\n> diff --git a/bundle.c b/bundle.c\n> index e99065c..c8bddd8 100644\n> --- a/bundle.c\n> +++ b/bundle.c\n> @@ -279,7 +279,7 @@ int create_bundle(struct bundle_header *header, const char *path,\n> while (strbuf_getwholeline(&buf, rls_fout, '\\n') != EOF) {\n> unsigned char sha1[20];\n> if (buf.len > 0 && buf.buf[0] == '-') {\n> - write_or_die(bundle_fd, buf.buf, buf.len);\n> + strbuf_write_or_die(&buf, bundle_fd);\n> if (!get_sha1_hex(buf.buf + 1, sha1)) {\n> struct object *object = parse_object_or_die(sha1, buf.buf);\n> object->flags |= UNINTERESTING;\n> diff --git a/credential-store.c b/credential-store.c\n> index f9146e5..6cb5fcb 100644\n> --- a/credential-store.c\n> +++ b/credential-store.c\n> @@ -48,7 +48,7 @@ static void print_entry(struct credential *c)\n> static void print_line(struct strbuf *buf)\n> {\n> strbuf_addch(buf, '\\n');\n> - write_or_die(credential_lock.fd, buf->buf, buf->len);\n> + strbuf_write_or_die(buf, credential_lock.fd);\n> }\n>\n> static void rewrite_credential_file(const char *fn, struct credential *c,\n> diff --git a/fetch-pack.c b/fetch-pack.c\n> index f061f1f..af28bd2 100644\n> --- a/fetch-pack.c\n> +++ b/fetch-pack.c\n> @@ -216,7 +216,7 @@ static void send_request(struct fetch_pack_args *args,\n> send_sideband(fd, -1, buf->buf, buf->len, LARGE_PACKET_MAX);\n> packet_flush(fd);\n> } else\n> - write_or_die(fd, buf->buf, buf->len);\n> + strbuf_write_or_die(buf, fd);\n> }\n>\n> static void insert_one_alternate_ref(const struct ref *ref, void *unused)\n> diff --git a/http-backend.c b/http-backend.c\n> index d2c0a62..bed9341 100644\n> --- a/http-backend.c\n> +++ b/http-backend.c\n> @@ -157,7 +157,7 @@ static void send_strbuf(const char *type, struct strbuf *buf)\n> hdr_int(content_length, buf->len);\n> hdr_str(content_type, type);\n> end_headers();\n> - write_or_die(1, buf->buf, buf->len);\n> + strbuf_write_or_die(buf, 1);\n> }\n>\n> static void send_local_file(const char *the_type, const char *name)\n> diff --git a/remote-curl.c b/remote-curl.c\n> index 10cb011..677df24 100644\n> --- a/remote-curl.c\n> +++ b/remote-curl.c\n> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n> if (start_command(&client))\n> exit(1);\n> if (preamble)\n> - write_or_die(client.in, preamble->buf, preamble->len);\n> + strbuf_write_or_die(preamble, client.in);\n> if (heads)\n> write_or_die(client.in, heads->buf, heads->len);\n>\n> @@ -767,7 +767,7 @@ static int fetch_git(struct discovery *heads,\n>\n> err = rpc_service(&rpc, heads);\n> if (rpc.result.len)\n> - write_or_die(1, rpc.result.buf, rpc.result.len);\n> + strbuf_write_or_die(&rpc.result, 1);\n> strbuf_release(&rpc.result);\n> strbuf_release(&preamble);\n> free(depth_arg);\n> @@ -889,7 +889,7 @@ static int push_git(struct discovery *heads, int nr_spec, char **specs)\n>\n> err = rpc_service(&rpc, heads);\n> if (rpc.result.len)\n> - write_or_die(1, rpc.result.buf, rpc.result.len);\n> + strbuf_write_or_die(&rpc.result, 1);\n> strbuf_release(&rpc.result);\n> argv_array_clear(&args);\n> return err;\n> --\n> 1.7.9.5\n>\n"},{"id":"235896","messageId":"xmqqzjl7qgsd.fsf@gitster.dls.corp.google.com","threadId":"36008","inReplyTo":"1393672871-28281-1-git-send-email-faiz.off93@gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T18:27:14Z","receivedAt":"2014-03-03T18:27:14Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Faiz Kothari writes:\n\n> Signed-off-by: Faiz Kothari \n> ---\n> Implemented write_or_die.c:strbuf_write_or_die() and used in relevant places\n> to substitute write_or_die(). I spotted other places where strbuf can be used\n> in place of buf[MAX_PATH] but that would require a change in prototype of a \n> lot of functions and functions calling them and so on....\n> I'll look for more places where strbuf can be used safely.\n>\n> Thanks.\n>\n> builtin/cat-file.c | 2 +-\n> builtin/notes.c | 4 ++--\n> builtin/receive-pack.c | 2 +-\n> builtin/send-pack.c | 2 +-\n> builtin/stripspace.c | 2 +-\n> builtin/tag.c | 2 +-\n> bundle.c | 2 +-\n> cache.h | 1 +\n> credential-store.c | 2 +-\n> fetch-pack.c | 2 +-\n> http-backend.c | 2 +-\n> remote-curl.c | 8 +++++---\n> write_or_die.c | 9 +++++++++\n> 13 files changed, 26 insertions(+), 14 deletions(-)\n\nIt does not reduce the code, it does not make the resulting code\nread any easier.\n\nWhat is the benefit of this change?\n"},{"id":"235897","messageId":"xmqqvbvvqglc.fsf@gitster.dls.corp.google.com","threadId":"36008","inReplyTo":"CAPig+cRgc4UtmJMieS9Mdrz7vjUNiu7QFu1PSBppKo22Ln5G-A@mail.gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T18:31:27Z","receivedAt":"2014-03-03T18:31:27Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>>> diff --git a/remote-curl.c b/remote-curl.c\n>>> index 10cb011..dee8716 100644\n>>> --- a/remote-curl.c\n>>> +++ b/remote-curl.c\n>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>>> if (start_command(&client))\n>>> exit(1);\n>>> if (preamble)\n>>> - write_or_die(client.in, preamble->buf, preamble->len);\n>>> + strbuf_write_or_die(client.in, preamble);\n>>> if (heads)\n>>> write_or_die(client.in, heads->buf, heads->len);\n>>\n>> This should be changed. May be you can use Ctrl-F to search write_or_die().\n>> Or if you are using vim, use \"/ and n\" to find all.\n>\n> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n> so Faiz correctly left this invocation alone.\n\nThat is a very good sign why this change is merely a code-churn and\nnot an improvement, isn't it? We know (and any strbuf user should\nknow) that ->buf and ->len are the ways to learn the pointer and the\nlength the strbuf holds. Why anybody thinks it is benefitial to\nintroduce another function that is _only_ for writing out strbuf and\ncannot be used to write out a plain buffer is simply beyond me.\n"},{"id":"235902","messageId":"CAFbjVck=yfRfhFtmLedD9A+ddeo7VcJ-km=6NBgmYXtmSfBRmQ@mail.gmail.com","threadId":"36008","inReplyTo":"CAFbjVckhU7NHzLjqPo5WkoBwVLrOLg=CS6mHSKkQstUxB31_eA@mail.gmail.com","subject":"Fwd: [PATCH] implemented strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-03T18:48:29Z","receivedAt":"2014-03-03T18:48:29Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"On Tue, Mar 4, 2014 at 12:01 AM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>>>> diff --git a/remote-curl.c b/remote-curl.c\n>>>> index 10cb011..dee8716 100644\n>>>> --- a/remote-curl.c\n>>>> +++ b/remote-curl.c\n>>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>>>> if (start_command(&client))\n>>>> exit(1);\n>>>> if (preamble)\n>>>> - write_or_die(client.in, preamble->buf, preamble->len);\n>>>> + strbuf_write_or_die(client.in, preamble);\n>>>> if (heads)\n>>>> write_or_die(client.in, heads->buf, heads->len);\n>>>\n>>> This should be changed. May be you can use Ctrl-F to search write_or_die().\n>>> Or if you are using vim, use \"/ and n\" to find all.\n>>\n>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n>> so Faiz correctly left this invocation alone.\n>\n> That is a very good sign why this change is merely a code-churn and\n> not an improvement, isn't it? We know (and any strbuf user should\n> know) that ->buf and ->len are the ways to learn the pointer and the\n> length the strbuf holds. Why anybody thinks it is benefitial to\n> introduce another function that is _only_ for writing out strbuf and\n> cannot be used to write out a plain buffer is simply beyond me.\n>\nHi,\nThanks for the feedback. Yes, I do realize, its kind of a code churn.\nI didn't realize it until I looked at the sign you pointed out.\nBut it was a good exercise to go through the code as this is one of\nthe GSoC microprojects.\nSorry, it didn't turn out to be a beneficial one. My bad.\n\nThanks a lot again for the suggestions and feedback.\n\n-Faiz\n"},{"id":"235910","messageId":"CAPig+cTmejtWXRzr6qk-kd+P8j4b6xMJSUVnNnqObqNXc-S9UA@mail.gmail.com","threadId":"36008","inReplyTo":"xmqqvbvvqglc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-03T19:46:12Z","receivedAt":"2014-03-03T19:46:12Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>\n>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>>>> diff --git a/remote-curl.c b/remote-curl.c\n>>>> index 10cb011..dee8716 100644\n>>>> --- a/remote-curl.c\n>>>> +++ b/remote-curl.c\n>>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>>>> if (start_command(&client))\n>>>> exit(1);\n>>>> if (preamble)\n>>>> - write_or_die(client.in, preamble->buf, preamble->len);\n>>>> + strbuf_write_or_die(client.in, preamble);\n>>>> if (heads)\n>>>> write_or_die(client.in, heads->buf, heads->len);\n>>>\n>>> This should be changed. May be you can use Ctrl-F to search write_or_die().\n>>> Or if you are using vim, use \"/ and n\" to find all.\n>>\n>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n>> so Faiz correctly left this invocation alone.\n>\n> That is a very good sign why this change is merely a code-churn and\n> not an improvement, isn't it? We know (and any strbuf user should\n> know) that ->buf and ->len are the ways to learn the pointer and the\n> length the strbuf holds. Why anybody thinks it is benefitial to\n> introduce another function that is _only_ for writing out strbuf and\n> cannot be used to write out a plain buffer is simply beyond me.\n\nAs a potential GSoC student and newcomer to the project, Faiz would\nnot have known that this would be considered unwanted churn when he\nchose the task from the GSoC microproject page [1]. Perhaps it would\nbe a good idea to retire this item from the list?\n\nOn the other hand, it did expose Faiz to the iterative code review\nprocess on this project and gave him a taste of what would be expected\nof him as a GSoC student, so the microproject achieved that important\ngoal, and thus wasn't an utter failure.\n\n[1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n"},{"id":"235912","messageId":"87siqzjc1p.fsf@fencepost.gnu.org","threadId":"36008","inReplyTo":"CAPig+cTmejtWXRzr6qk-kd+P8j4b6xMJSUVnNnqObqNXc-S9UA@mail.gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"David Kastrup","fromEmail":"dak@gnu.org","sentAt":"2014-03-03T19:51:30Z","receivedAt":"2014-03-03T19:51:30Z","isPatch":true,"sender":{"key":"dak@gnu.org","avatar":"https://avatars.githubusercontent.com/u/52141349?v=4"},"body":"Eric Sunshine writes:\n\n> As a potential GSoC student and newcomer to the project, Faiz would\n> not have known that this would be considered unwanted churn when he\n> chose the task from the GSoC microproject page [1]. Perhaps it would\n> be a good idea to retire this item from the list?\n>\n> On the other hand, it did expose Faiz to the iterative code review\n> process on this project and gave him a taste of what would be expected\n> of him as a GSoC student, so the microproject achieved that important\n> goal, and thus wasn't an utter failure.\n\nAnd the microproject has the fabulous property that we can use it over\nand over again to have a newcomer try committing patches: the previously\nreported problem that we were running out of microprojects will not\noccur when every patch is eventually going to be rejected.\n\nJoking aside, this is a motivational disaster. It should be retired.\n\n-- \nDavid Kastrup\n"},{"id":"235919","messageId":"xmqqsiqznhpb.fsf@gitster.dls.corp.google.com","threadId":"36008","inReplyTo":"CAPig+cTmejtWXRzr6qk-kd+P8j4b6xMJSUVnNnqObqNXc-S9UA@mail.gmail.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-03T20:35:44Z","receivedAt":"2014-03-03T20:35:44Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Eric Sunshine writes:\n\n> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano wrote:\n>> Eric Sunshine writes:\n>>\n>>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n>>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>>>>> diff --git a/remote-curl.c b/remote-curl.c\n>>>>> index 10cb011..dee8716 100644\n>>>>> --- a/remote-curl.c\n>>>>> +++ b/remote-curl.c\n>>>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>>>>> if (start_command(&client))\n>>>>> exit(1);\n>>>>> if (preamble)\n>>>>> - write_or_die(client.in, preamble->buf, preamble->len);\n>>>>> + strbuf_write_or_die(client.in, preamble);\n>>>>> if (heads)\n>>>>> write_or_die(client.in, heads->buf, heads->len);\n>>>>\n>>>> This should be changed. May be you can use Ctrl-F to search write_or_die().\n>>>> Or if you are using vim, use \"/ and n\" to find all.\n>>>\n>>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n>>> so Faiz correctly left this invocation alone.\n>>\n>> That is a very good sign why this change is merely a code-churn and\n>> not an improvement, isn't it? We know (and any strbuf user should\n>> know) that ->buf and ->len are the ways to learn the pointer and the\n>> length the strbuf holds. Why anybody thinks it is benefitial to\n>> introduce another function that is _only_ for writing out strbuf and\n>> cannot be used to write out a plain buffer is simply beyond me.\n>\n> As a potential GSoC student and newcomer to the project, Faiz would\n> not have known that this would be considered unwanted churn when he\n> chose the task from the GSoC microproject page [1]. Perhaps it would\n> be a good idea to retire this item from the list?\n\nI don't think I saw this on the microproject suggestion page when I\nlast looked at it, and assumed that this was on the student's own\ninitiative.\n\n> On the other hand, it did expose Faiz to the iterative code review\n> process on this project and gave him a taste of what would be expected\n> of him as a GSoC student, so the microproject achieved that important\n> goal, and thus wasn't an utter failure.\n>\n> [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n\nSurely.\n\nI would have to say that this is not a good sample exercise to\nsuggest to new students and I'd encourage dropping it from the list.\nYou could argue that it is an effective way to cull people with bad\ndesign taste to mix suggestions to make the codebase worse and see\nwho picks them, but I do not think it is very fair ;-)\n"},{"id":"235923","messageId":"CAPig+cTm-JXXC7raFmHWrG5fc54a7u4V9E5Zj71DTwWxZbb9OA@mail.gmail.com","threadId":"36008","inReplyTo":"xmqqsiqznhpb.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2014-03-03T21:29:32Z","receivedAt":"2014-03-03T21:29:32Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Mon, Mar 3, 2014 at 3:35 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n>> On Mon, Mar 3, 2014 at 1:31 PM, Junio C Hamano wrote:\n>>> Eric Sunshine writes:\n>>>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n>>>> so Faiz correctly left this invocation alone.\n>>>\n>>> That is a very good sign why this change is merely a code-churn and\n>>> not an improvement, isn't it? We know (and any strbuf user should\n>>> know) that ->buf and ->len are the ways to learn the pointer and the\n>>> length the strbuf holds. Why anybody thinks it is benefitial to\n>>> introduce another function that is _only_ for writing out strbuf and\n>>> cannot be used to write out a plain buffer is simply beyond me.\n>>\n>> As a potential GSoC student and newcomer to the project, Faiz would\n>> not have known that this would be considered unwanted churn when he\n>> chose the task from the GSoC microproject page [1]. Perhaps it would\n>> be a good idea to retire this item from the list?\n>\n> I don't think I saw this on the microproject suggestion page when I\n> last looked at it, and assumed that this was on the student's own\n> initiative.\n\nI also had not seen it earlier on the microprojects page and had the\nsame reaction until I re-checked the page and found that it had been\nadded [1].\n\nThe microprojects page already instructs students to indicate that a\nsubmission is for GSoC [2] (and many have followed the advice), but\nperhaps we can avoid this sort of misunderstanding in the future by\nmaking it more explicit: for instance, tell them to add [GSoC] to the\nSubject:.\n\n[1]: https://github.com/git/git.github.io/commit/f314120a2b5e831459673c612a3630ad953d9954\n[2]: https://github.com/git/git.github.io/blame/master/SoC-2014-Microprojects.md#L83\n\n>> On the other hand, it did expose Faiz to the iterative code review\n>> process on this project and gave him a taste of what would be expected\n>> of him as a GSoC student, so the microproject achieved that important\n>> goal, and thus wasn't an utter failure.\n>>\n>> [1]: https://github.com/git/git.github.io/blob/master/SoC-2014-Microprojects.md\n>\n> Surely.\n>\n> I would have to say that this is not a good sample exercise to\n> suggest to new students and I'd encourage dropping it from the list.\n> You could argue that it is an effective way to cull people with bad\n> design taste to mix suggestions to make the codebase worse and see\n> who picks them, but I do not think it is very fair ;-)\n\nAgreed. The item should be dropped from the list.\n"},{"id":"235983","messageId":"53159A81.4050905@alum.mit.edu","threadId":"36008","inReplyTo":"xmqqvbvvqglc.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2014-03-04T09:18:57Z","receivedAt":"2014-03-04T09:18:57Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 03/03/2014 07:31 PM, Junio C Hamano wrote:\n> Eric Sunshine writes:\n> \n>> On Sat, Mar 1, 2014 at 7:51 AM, He Sun wrote:\n>>> 2014-03-01 19:21 GMT+08:00 Faiz Kothari :\n>>>> diff --git a/remote-curl.c b/remote-curl.c\n>>>> index 10cb011..dee8716 100644\n>>>> --- a/remote-curl.c\n>>>> +++ b/remote-curl.c\n>>>> @@ -634,7 +634,7 @@ static int rpc_service(struct rpc_state *rpc, struct discovery *heads)\n>>>> if (start_command(&client))\n>>>> exit(1);\n>>>> if (preamble)\n>>>> - write_or_die(client.in, preamble->buf, preamble->len);\n>>>> + strbuf_write_or_die(client.in, preamble);\n>>>> if (heads)\n>>>> write_or_die(client.in, heads->buf, heads->len);\n>>>\n>>> This should be changed. May be you can use Ctrl-F to search write_or_die().\n>>> Or if you are using vim, use \"/ and n\" to find all.\n>>\n>> It's not obvious from the patch fragment, but 'heads' is not a strbuf,\n>> so Faiz correctly left this invocation alone.\n> \n> That is a very good sign why this change is merely a code-churn and\n> not an improvement, isn't it? We know (and any strbuf user should\n> know) that ->buf and ->len are the ways to learn the pointer and the\n> length the strbuf holds. Why anybody thinks it is benefitial to\n> introduce another function that is _only_ for writing out strbuf and\n> cannot be used to write out a plain buffer is simply beyond me.\n\nI'm the guilty one. I like the change (obviously, since I suggested\nit). Writing strbufs comes up frequently and will hopefully increase in\nusage and I think it is a positive thing to encourage the use of strbufs\nby making them increasingly first-class citizens.\n\nBut I can see your points too, and I humbly defer to the wisdom of the\nlist. I will remove this suggestion from the list of microprojects.\n\nFaiz, this is the way things go on the Git mailing list. It would be\nboring if everybody agreed all the time :-)\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"236009","messageId":"CAFbjVcnnUeYwB=3R8JWuCnSRTRkKtkPMpqmgSXtoa4ut-OO__Q@mail.gmail.com","threadId":"36008","inReplyTo":"53159A81.4050905@alum.mit.edu","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Faiz Kothari","fromEmail":"faiz.off93@gmail.com","sentAt":"2014-03-04T17:01:19Z","receivedAt":"2014-03-04T17:01:19Z","isPatch":true,"sender":{"key":"faiz.off93@gmail.com","avatar":null},"body":"> I'm the guilty one. I like the change (obviously, since I suggested\n> it). Writing strbufs comes up frequently and will hopefully increase in\n> usage and I think it is a positive thing to encourage the use of strbufs\n> by making them increasingly first-class citizens.\n>\n> But I can see your points too, and I humbly defer to the wisdom of the\n> list. I will remove this suggestion from the list of microprojects.\n>\n> Faiz, this is the way things go on the Git mailing list. It would be\n> boring if everybody agreed all the time :-)\n>\n> Michael\n\nHi,\nThank you all. Even I like the strbuf_write_or_die() but again its a\ncode churn as pointed out. But if we want to use strbuf instead of\nstatic buffers we might need this function very often (Its just my\nopinion).\nAnyways, implementing it was an exercise and I enjoyed it. I agree\nwith Michael Haggerty that it would be boring if everybody agreed all\nthe time :D\nI enjoyed it and learnt from the exercise, so I don't think it was a\nwaste or a bad exercise. At least it exposed me to practices of good\nsoftware design and importance of layers in software.\n\nThanks a lot.\n\n-Faiz\n"},{"id":"236019","messageId":"xmqq61ntls4l.fsf@gitster.dls.corp.google.com","threadId":"36008","inReplyTo":"53159A81.4050905@alum.mit.edu","subject":"Re: [PATCH] implemented strbuf_write_or_die()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2014-03-04T18:45:46Z","receivedAt":"2014-03-04T18:45:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 03/03/2014 07:31 PM, Junio C Hamano wrote:\n>> That is a very good sign why this change is merely a code-churn and\n>> not an improvement, isn't it? We know (and any strbuf user should\n>> know) that ->buf and ->len are the ways to learn the pointer and the\n>> length the strbuf holds.\n> ...\n> ... Writing strbufs comes up frequently and will hopefully increase in\n> usage and I think it is a positive thing to encourage the use of strbufs\n> by making them increasingly first-class citizens.\n\nYeah, I understand that. I suspect that the conclusion would have\nbeen very different if we were a C++ project; most likely it would\nbe an excellent idea to add an often-used write_or_die() method to\nthe strbuf class. But we are writing C.\n\n> Faiz, this is the way things go on the Git mailing list. It would be\n> boring if everybody agreed all the time :-)\n\nSurely, and thanks ;-)\n"}]}