{"thread":{"id":"20907","subject":"[PATCH] use write_str_in_full helper to avoid literal string lengths","startedAt":"2009-09-12T08:54:32Z","lastAt":"2009-09-13T07:32:46Z","messageCount":3,"participants":["Jim Meyering","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"122936","messageId":"87skes35mf.fsf@meyering.net","threadId":"20907","inReplyTo":null,"subject":"[PATCH] use write_str_in_full helper to avoid literal string lengths","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-12T08:54:32Z","receivedAt":"2009-09-12T08:54:32Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"In next's 2d14d65ce7b136b0fb18dcf27e5caff67829f658,\nI happened to notice two changes like this:\n\n-\twrite_in_full(helper->in, \"list\\n\", 5);\n+\n+\tstrbuf_addstr(&buf, \"list\\n\");\n+\twrite_in_full(helper->in, buf.buf, buf.len);\n+\tstrbuf_reset(&buf);\n\nIMHO, it would be better to define a new function,\n\n    static inline ssize_t write_str_in_full(int fd, const char *str)\n    {\n           return write_in_full(fd, str, strlen(str));\n    }\n\nand then use it like this:\n\n-       strbuf_addstr(&buf, \"list\\n\");\n-       write_in_full(helper->in, buf.buf, buf.len);\n-       strbuf_reset(&buf);\n+       write_str_in_full(helper->in, \"list\\n\");\n\nThus not requiring the added allocation, and still avoiding\nthe maintenance risk of literal string lengths.\nThese days, compilers are good enough that strlen(\"literal\")\nimposes no run-time cost.\n\nTransformed via this:\n\n    perl -pi -e \\\n        's/write_in_full\\((.*?), (\".*?\"), \\d+\\)/write_str_in_full($1, $2)/'\\\n      $(git grep -l 'write_in_full.*\"')\n\n\n>From fe368f8b3720f04c9dfce952711d2fb412b52e3c Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering <meyering@redhat.com>\nDate: Sat, 12 Sep 2009 09:56:13 +0200\nSubject: [PATCH] use write_str_in_full helper to avoid literal string lengths\n\n* cache.h (write_str_in_full): Define function.\n* builtin-fetch.c (quickfetch): Use it.\n* builtin-reflog.c (expire_reflog): Likewise.\n* commit.c (write_shallow_commits): Likewise.\n* config.c (git_config_set_multivar): Likewise.\n* rerere.c (write_rr): Likewise.\n* transport-helper.c (get_helper, disconnect_helper): Likewise.\n(get_refs_list): Likewise.\n* upload-pack.c (receive_needs): Likewise.\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\n builtin-fetch.c    |    2 +-\n builtin-reflog.c   |    2 +-\n cache.h            |    9 +++++++--\n commit.c           |    2 +-\n config.c           |    2 +-\n rerere.c           |    2 +-\n transport-helper.c |   10 +++-------\n upload-pack.c      |    4 ++--\n 8 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 817dd6b..cb48c57 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -454,7 +454,7 @@ static int quickfetch(struct ref *ref_map)\n\n \tfor (ref = ref_map; ref; ref = ref->next) {\n \t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n-\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t    write_str_in_full(revlist.in, \"\\n\") < 0) {\n \t\t\tif (errno != EPIPE && errno != EINVAL)\n \t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n \t\t\terr = -1;\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 95198c5..e23b5ef 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -362,7 +362,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t} else if (cmd->updateref &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_in_full(lock->lock_fd, \"\\n\", 1) != 1 ||\n+\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n \t\t\t\tlock->lk->filename);\ndiff --git a/cache.h b/cache.h\nindex 30a7a16..90bf127 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -924,13 +924,18 @@ extern const char *git_mailmap_file;\n extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n-extern ssize_t read_in_full(int fd, void *buf, size_t count);\n-extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n extern void write_or_die(int fd, const void *buf, size_t count);\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\n+extern ssize_t read_in_full(int fd, void *buf, size_t count);\n+extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n+static inline ssize_t write_str_in_full(int fd, const char *str)\n+{\n+\treturn write_in_full(fd, str, strlen(str));\n+}\n+\n /* pager.c */\n extern void setup_pager(void);\n extern const char *pager_program;\ndiff --git a/commit.c b/commit.c\nindex a6c6f70..fedbd5e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -212,7 +212,7 @@ int write_shallow_commits(int fd, int use_pack_protocol)\n \t\t\telse {\n \t\t\t\tif (write_in_full(fd, hex,  40) != 40)\n \t\t\t\t\tbreak;\n-\t\t\t\tif (write_in_full(fd, \"\\n\", 1) != 1)\n+\t\t\t\tif (write_str_in_full(fd, \"\\n\") != 1)\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/config.c b/config.c\nindex f21530c..c644061 100644\n--- a/config.c\n+++ b/config.c\n@@ -1119,7 +1119,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t\t\t    copy_end - copy_begin)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t\tif (new_line &&\n-\t\t\t\t    write_in_full(fd, \"\\n\", 1) != 1)\n+\t\t\t\t    write_str_in_full(fd, \"\\n\") != 1)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n \t\t\tcopy_begin = store.offset[i];\ndiff --git a/rerere.c b/rerere.c\nindex 87360dc..29f95f6 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -61,7 +61,7 @@ static int write_rr(struct string_list *rr, int out_fd)\n \t\tpath = rr->items[i].string;\n \t\tlength = strlen(path) + 1;\n \t\tif (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||\n-\t\t    write_in_full(out_fd, \"\\t\", 1) != 1 ||\n+\t\t    write_str_in_full(out_fd, \"\\t\") != 1 ||\n \t\t    write_in_full(out_fd, path, length) != length)\n \t\t\tdie(\"unable to write rerere record\");\n \t}\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b1ea7e6..832d81f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -37,9 +37,7 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n \tdata->helper = helper;\n\n-\tstrbuf_addstr(&buf, \"capabilities\\n\");\n-\twrite_in_full(helper->in, buf.buf, buf.len);\n-\tstrbuf_reset(&buf);\n+\twrite_str_in_full(helper->in, \"capabilities\\n\");\n\n \tfile = fdopen(helper->out, \"r\");\n \twhile (1) {\n@@ -58,7 +56,7 @@ static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n \tif (data->helper) {\n-\t\twrite_in_full(data->helper->in, \"\\n\", 1);\n+\t\twrite_str_in_full(data->helper->in, \"\\n\");\n \t\tclose(data->helper->in);\n \t\tfinish_command(data->helper);\n \t\tfree((char *)data->helper->argv[0]);\n@@ -124,9 +122,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n\n \thelper = get_helper(transport);\n\n-\tstrbuf_addstr(&buf, \"list\\n\");\n-\twrite_in_full(helper->in, buf.buf, buf.len);\n-\tstrbuf_reset(&buf);\n+\twrite_str_in_full(helper->in, \"list\\n\");\n\n \tfile = fdopen(helper->out, \"r\");\n \twhile (1) {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c77ab71..b3471e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -553,7 +553,7 @@ static void receive_needs(void)\n\n \tshallow_nr = 0;\n \tif (debug_fd)\n-\t\twrite_in_full(debug_fd, \"#S\\n\", 3);\n+\t\twrite_str_in_full(debug_fd, \"#S\\n\");\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tunsigned char sha1_buf[20];\n@@ -619,7 +619,7 @@ static void receive_needs(void)\n \t\t}\n \t}\n \tif (debug_fd)\n-\t\twrite_in_full(debug_fd, \"#E\\n\", 3);\n+\t\twrite_str_in_full(debug_fd, \"#E\\n\");\n\n \tif (!use_sideband && daemon_mode)\n \t\tno_progress = 1;\n--\n1.6.5.rc0.190.g15871\n"},{"id":"123010","messageId":"7veiqbbttg.fsf@alter.siamese.dyndns.org","threadId":"20907","inReplyTo":"87skes35mf.fsf@meyering.net","subject":"Re: [PATCH] use write_str_in_full helper to avoid literal string lengths","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2009-09-12T23:56:59Z","receivedAt":"2009-09-12T23:56:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jim Meyering <jim@meyering.net> writes:\n\n>  ...Thus not requiring the added allocation, and still avoiding\n> the maintenance risk of literal string lengths.\n> These days, compilers are good enough that strlen(\"literal\")\n> imposes no run-time cost.\n>\n> Transformed via this:\n>\n>     perl -pi -e \\\n>         's/write_in_full\\((.*?), (\".*?\"), \\d+\\)/write_str_in_full($1, $2)/'\\\n>       $(git grep -l 'write_in_full.*\"')\n>\n>\n> From fe368f8b3720f04c9dfce952711d2fb412b52e3c Mon Sep 17 00:00:00 2001\n> From: Jim Meyering <meyering@redhat.com>\n> Date: Sat, 12 Sep 2009 09:56:13 +0200\n> Subject: [PATCH] use write_str_in_full helper to avoid literal string lengths\n>\n> * cache.h (write_str_in_full): Define function.\n> * builtin-fetch.c (quickfetch): Use it.\n> * builtin-reflog.c (expire_reflog): Likewise.\n> * commit.c (write_shallow_commits): Likewise.\n> * config.c (git_config_set_multivar): Likewise.\n> * rerere.c (write_rr): Likewise.\n> * transport-helper.c (get_helper, disconnect_helper): Likewise.\n> (get_refs_list): Likewise.\n> * upload-pack.c (receive_needs): Likewise.\n\nThanks.  I agree with the reasoning you wrote outside the proposed log\nmessage.\n\nWe usually do not write these bullet points (iow, we are not GNU) in our\nlog message.  The names of the functions, call sites and files that are\ninvolved are something anybody can see from the patch text,\n\nI think the GNU convention was useful back when we were trapped in a\nsystem with non-atomic commits, where it was very hard to see what files\nwere affected in a single logical changeset (i.e. CVS).\n\nLuckily, we graduated those dark ages.\n\nInstead, we prefer to have justifications (and methods), like what you\nwrote at the beginning of your message.  These are not something people\ncan find in the patch text and they deserve to be recorded in the commit.\n"},{"id":"123038","messageId":"87ljkjz4dd.fsf@meyering.net","threadId":"20907","inReplyTo":"7veiqbbttg.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] use write_str_in_full helper to avoid literal string lengths","fromName":"Jim Meyering","fromEmail":"jim@meyering.net","sentAt":"2009-09-13T07:32:46Z","receivedAt":"2009-09-13T07:32:46Z","isPatch":true,"sender":{"key":"jim@meyering.net","avatar":"https://avatars.githubusercontent.com/u/710630?v=4"},"body":"Junio C Hamano wrote:\n...\n> Thanks.  I agree with the reasoning you wrote outside the proposed log\n> message.\n>\n> We usually do not write these bullet points (iow, we are not GNU) in our\n> log message.  The names of the functions, call sites and files that are\n> involved are something anybody can see from the patch text,\n>\n> I think the GNU convention was useful back when we were trapped in a\n> system with non-atomic commits, where it was very hard to see what files\n> were affected in a single logical changeset (i.e. CVS).\n>\n> Luckily, we graduated those dark ages.\n>\n> Instead, we prefer to have justifications (and methods), like what you\n> wrote at the beginning of your message.  These are not something people\n> can find in the patch text and they deserve to be recorded in the commit.\n\nHeh ;-)\nOld habits...\nHere's that same commit with a better log message:\n\n>From 7edbf0141c9e2ae2999f8b7919c938b9384240e2 Mon Sep 17 00:00:00 2001\nFrom: Jim Meyering <meyering@redhat.com>\nDate: Sat, 12 Sep 2009 09:56:13 +0200\nSubject: [PATCH] use write_str_in_full helper to avoid literal string lengths\n\nIn 2d14d65ce7b136b0fb18dcf27e5caff67829f658,\nI noticed two changes like this:\n\n-\twrite_in_full(helper->in, \"list\\n\", 5);\n+\n+\tstrbuf_addstr(&buf, \"list\\n\");\n+\twrite_in_full(helper->in, buf.buf, buf.len);\n+\tstrbuf_reset(&buf);\n\nInstead, let's define a new function, in cache.h:\n\n    static inline ssize_t write_str_in_full(int fd, const char *str)\n    {\n           return write_in_full(fd, str, strlen(str));\n    }\n\nand then use it like this:\n\n-       strbuf_addstr(&buf, \"list\\n\");\n-       write_in_full(helper->in, buf.buf, buf.len);\n-       strbuf_reset(&buf);\n+       write_str_in_full(helper->in, \"list\\n\");\n\nThus not requiring the added allocation, and still avoiding\nthe maintenance risk of literal string lengths.\nThese days, compilers are good enough that strlen(\"literal\")\nimposes no run-time cost.\n\nUpdate the two uses in transport-helper.c manually,\nand transform the other uses via this:\n\n    perl -pi -e \\\n        's/write_in_full\\((.*?), (\".*?\"), \\d+\\)/write_str_in_full($1, $2)/'\\\n      $(git grep -l 'write_in_full.*\"')\n\nSigned-off-by: Jim Meyering <meyering@redhat.com>\n---\n builtin-fetch.c    |    2 +-\n builtin-reflog.c   |    2 +-\n cache.h            |    9 +++++++--\n commit.c           |    2 +-\n config.c           |    2 +-\n rerere.c           |    2 +-\n transport-helper.c |   10 +++-------\n upload-pack.c      |    4 ++--\n 8 files changed, 17 insertions(+), 16 deletions(-)\n\ndiff --git a/builtin-fetch.c b/builtin-fetch.c\nindex 817dd6b..cb48c57 100644\n--- a/builtin-fetch.c\n+++ b/builtin-fetch.c\n@@ -454,7 +454,7 @@ static int quickfetch(struct ref *ref_map)\n\n \tfor (ref = ref_map; ref; ref = ref->next) {\n \t\tif (write_in_full(revlist.in, sha1_to_hex(ref->old_sha1), 40) < 0 ||\n-\t\t    write_in_full(revlist.in, \"\\n\", 1) < 0) {\n+\t\t    write_str_in_full(revlist.in, \"\\n\") < 0) {\n \t\t\tif (errno != EPIPE && errno != EINVAL)\n \t\t\t\terror(\"failed write to rev-list: %s\", strerror(errno));\n \t\t\terr = -1;\ndiff --git a/builtin-reflog.c b/builtin-reflog.c\nindex 95198c5..e23b5ef 100644\n--- a/builtin-reflog.c\n+++ b/builtin-reflog.c\n@@ -362,7 +362,7 @@ static int expire_reflog(const char *ref, const unsigned char *sha1, int unused,\n \t\t} else if (cmd->updateref &&\n \t\t\t(write_in_full(lock->lock_fd,\n \t\t\t\tsha1_to_hex(cb.last_kept_sha1), 40) != 40 ||\n-\t\t\t write_in_full(lock->lock_fd, \"\\n\", 1) != 1 ||\n+\t\t\t write_str_in_full(lock->lock_fd, \"\\n\") != 1 ||\n \t\t\t close_ref(lock) < 0)) {\n \t\t\tstatus |= error(\"Couldn't write %s\",\n \t\t\t\tlock->lk->filename);\ndiff --git a/cache.h b/cache.h\nindex 30a7a16..90bf127 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -924,13 +924,18 @@ extern const char *git_mailmap_file;\n extern void maybe_flush_or_die(FILE *, const char *);\n extern int copy_fd(int ifd, int ofd);\n extern int copy_file(const char *dst, const char *src, int mode);\n-extern ssize_t read_in_full(int fd, void *buf, size_t count);\n-extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n extern void write_or_die(int fd, const void *buf, size_t count);\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\n+extern ssize_t read_in_full(int fd, void *buf, size_t count);\n+extern ssize_t write_in_full(int fd, const void *buf, size_t count);\n+static inline ssize_t write_str_in_full(int fd, const char *str)\n+{\n+\treturn write_in_full(fd, str, strlen(str));\n+}\n+\n /* pager.c */\n extern void setup_pager(void);\n extern const char *pager_program;\ndiff --git a/commit.c b/commit.c\nindex a6c6f70..fedbd5e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -212,7 +212,7 @@ int write_shallow_commits(int fd, int use_pack_protocol)\n \t\t\telse {\n \t\t\t\tif (write_in_full(fd, hex,  40) != 40)\n \t\t\t\t\tbreak;\n-\t\t\t\tif (write_in_full(fd, \"\\n\", 1) != 1)\n+\t\t\t\tif (write_str_in_full(fd, \"\\n\") != 1)\n \t\t\t\t\tbreak;\n \t\t\t}\n \t\t}\ndiff --git a/config.c b/config.c\nindex f21530c..c644061 100644\n--- a/config.c\n+++ b/config.c\n@@ -1119,7 +1119,7 @@ int git_config_set_multivar(const char *key, const char *value,\n \t\t\t\t    copy_end - copy_begin)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t\tif (new_line &&\n-\t\t\t\t    write_in_full(fd, \"\\n\", 1) != 1)\n+\t\t\t\t    write_str_in_full(fd, \"\\n\") != 1)\n \t\t\t\t\tgoto write_err_out;\n \t\t\t}\n \t\t\tcopy_begin = store.offset[i];\ndiff --git a/rerere.c b/rerere.c\nindex 87360dc..29f95f6 100644\n--- a/rerere.c\n+++ b/rerere.c\n@@ -61,7 +61,7 @@ static int write_rr(struct string_list *rr, int out_fd)\n \t\tpath = rr->items[i].string;\n \t\tlength = strlen(path) + 1;\n \t\tif (write_in_full(out_fd, rr->items[i].util, 40) != 40 ||\n-\t\t    write_in_full(out_fd, \"\\t\", 1) != 1 ||\n+\t\t    write_str_in_full(out_fd, \"\\t\") != 1 ||\n \t\t    write_in_full(out_fd, path, length) != length)\n \t\t\tdie(\"unable to write rerere record\");\n \t}\ndiff --git a/transport-helper.c b/transport-helper.c\nindex b1ea7e6..832d81f 100644\n--- a/transport-helper.c\n+++ b/transport-helper.c\n@@ -37,9 +37,7 @@ static struct child_process *get_helper(struct transport *transport)\n \t\tdie(\"Unable to run helper: git %s\", helper->argv[0]);\n \tdata->helper = helper;\n\n-\tstrbuf_addstr(&buf, \"capabilities\\n\");\n-\twrite_in_full(helper->in, buf.buf, buf.len);\n-\tstrbuf_reset(&buf);\n+\twrite_str_in_full(helper->in, \"capabilities\\n\");\n\n \tfile = fdopen(helper->out, \"r\");\n \twhile (1) {\n@@ -58,7 +56,7 @@ static int disconnect_helper(struct transport *transport)\n {\n \tstruct helper_data *data = transport->data;\n \tif (data->helper) {\n-\t\twrite_in_full(data->helper->in, \"\\n\", 1);\n+\t\twrite_str_in_full(data->helper->in, \"\\n\");\n \t\tclose(data->helper->in);\n \t\tfinish_command(data->helper);\n \t\tfree((char *)data->helper->argv[0]);\n@@ -124,9 +122,7 @@ static struct ref *get_refs_list(struct transport *transport, int for_push)\n\n \thelper = get_helper(transport);\n\n-\tstrbuf_addstr(&buf, \"list\\n\");\n-\twrite_in_full(helper->in, buf.buf, buf.len);\n-\tstrbuf_reset(&buf);\n+\twrite_str_in_full(helper->in, \"list\\n\");\n\n \tfile = fdopen(helper->out, \"r\");\n \twhile (1) {\ndiff --git a/upload-pack.c b/upload-pack.c\nindex c77ab71..b3471e4 100644\n--- a/upload-pack.c\n+++ b/upload-pack.c\n@@ -553,7 +553,7 @@ static void receive_needs(void)\n\n \tshallow_nr = 0;\n \tif (debug_fd)\n-\t\twrite_in_full(debug_fd, \"#S\\n\", 3);\n+\t\twrite_str_in_full(debug_fd, \"#S\\n\");\n \tfor (;;) {\n \t\tstruct object *o;\n \t\tunsigned char sha1_buf[20];\n@@ -619,7 +619,7 @@ static void receive_needs(void)\n \t\t}\n \t}\n \tif (debug_fd)\n-\t\twrite_in_full(debug_fd, \"#E\\n\", 3);\n+\t\twrite_str_in_full(debug_fd, \"#E\\n\");\n\n \tif (!use_sideband && daemon_mode)\n \t\tno_progress = 1;\n--\n1.6.5.rc0.190.g15871\n"}]}