{"thread":{"id":"52763","subject":"[PATCH] strbuf: add and use strbuf_insertstr()","startedAt":"2020-02-08T19:57:09Z","lastAt":"2020-02-11T17:27:14Z","messageCount":23,"participants":["René Scharfe","Taylor Blau","Eric Sunshine","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"391376","messageId":"019be197-e0aa-1234-e16f-6561d8340023@web.de","threadId":"52763","inReplyTo":null,"subject":"[PATCH] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-08T19:56:43Z","receivedAt":"2020-02-08T19:57:09Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function for inserting a C string into a strbuf.  Use it\nthroughout the source to get rid of magic string length constants and\nexplicit strlen() calls.\n\nLike strbuf_addstr(), implement it as an inline function to avoid the\nimplicit strlen() calls to cause runtime overhead.\n\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n builtin/checkout.c        |  2 +-\n builtin/notes.c           |  4 ++--\n builtin/sparse-checkout.c |  2 +-\n commit.c                  |  2 +-\n config.c                  |  2 +-\n http.c                    |  4 ++--\n mailinfo.c                |  2 +-\n notes-utils.c             |  2 +-\n notes.c                   |  4 ++--\n pretty.c                  |  4 ++--\n strbuf.h                  | 12 ++++++++++++\n 11 files changed, 26 insertions(+), 14 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex fc2eb1befc..d6773818b8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -865,7 +865,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n \t\t\told_desc ? old_desc : \"(invalid)\", new_branch_info->name);\n \telse\n-\t\tstrbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));\n+\t\tstrbuf_insertstr(&msg, 0, reflog_msg);\n\n \tif (!strcmp(new_branch_info->name, \"HEAD\") && !new_branch_info->path && !opts->force_detach) {\n \t\t/* Nothing to do. */\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 95456f3165..35e468ea2d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -622,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n\n \t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n-\t\t\tstrbuf_insert(&d.buf, 0, \"\\n\", 1);\n+\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n \t\tfree(prev_buf);\n@@ -745,7 +745,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tmemset(&pretty_ctx, 0, sizeof(pretty_ctx));\n \tformat_commit_message(partial, \"%s\", &msg, &pretty_ctx);\n \tstrbuf_trim(&msg);\n-\tstrbuf_insert(&msg, 0, \"notes: \", 7);\n+\tstrbuf_insertstr(&msg, 0, \"notes: \");\n \tupdate_ref(msg.buf, o->local_ref, &oid,\n \t\t   is_null_oid(&parent_oid) ? NULL : &parent_oid,\n \t\t   0, UPDATE_REFS_DIE_ON_ERR);\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex b3bed891cb..38d0d944b3 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -373,7 +373,7 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)\n \t\treturn;\n\n \tif (line->buf[0] != '/')\n-\t\tstrbuf_insert(line, 0, \"/\", 1);\n+\t\tstrbuf_insertstr(line, 0, \"/\");\n\n \tinsert_recursive_pattern(pl, line);\n }\ndiff --git a/commit.c b/commit.c\nindex 3f91d3efc5..a6cfa41a4e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -993,7 +993,7 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n \t\t\tstrbuf_insert(buf, inspos, gpg_sig_header, gpg_sig_header_len);\n \t\t\tinspos += gpg_sig_header_len;\n \t\t}\n-\t\tstrbuf_insert(buf, inspos++, \" \", 1);\n+\t\tstrbuf_insertstr(buf, inspos++, \" \");\n \t\tstrbuf_insert(buf, inspos, bol, len);\n \t\tinspos += len;\n \t\tcopypos += len;\ndiff --git a/config.c b/config.c\nindex d75f88ca0c..b386c0c628 100644\n--- a/config.c\n+++ b/config.c\n@@ -204,7 +204,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \t\tstrbuf_splice(pat, 0, 1, path.buf, slash - path.buf);\n \t\tprefix = slash - path.buf + 1 /* slash */;\n \t} else if (!is_absolute_path(pat->buf))\n-\t\tstrbuf_insert(pat, 0, \"**/\", 3);\n+\t\tstrbuf_insertstr(pat, 0, \"**/\");\n\n \tadd_trailing_starstar_for_dir(pat);\n\ndiff --git a/http.c b/http.c\nindex 5f348169c3..00a0e50763 100644\n--- a/http.c\n+++ b/http.c\n@@ -680,8 +680,8 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n \tfor (header = headers; *header; header++) {\n \t\tif (hide_sensitive_header)\n \t\t\tredact_sensitive_header(*header);\n-\t\tstrbuf_insert((*header), 0, text, strlen(text));\n-\t\tstrbuf_insert((*header), strlen(text), \": \", 2);\n+\t\tstrbuf_insertstr((*header), 0, text);\n+\t\tstrbuf_insertstr((*header), strlen(text), \": \");\n \t\tstrbuf_rtrim((*header));\n \t\tstrbuf_addch((*header), '\\n');\n \t\ttrace_strbuf(&trace_curl, (*header));\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..c535dec2e9 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -254,7 +254,7 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n \tmi->delsp = has_attr_value(line->buf, \"delsp=\", \"yes\");\n\n \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n-\t\tstrbuf_insert(boundary, 0, \"--\", 2);\n+\t\tstrbuf_insertstr(boundary, 0, \"--\");\n \t\tif (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) {\n \t\t\terror(\"Too many boundaries to handle\");\n \t\t\tmi->input_error = -1;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex a819410698..5f94c513d4 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)\n \tstrbuf_complete_line(&buf);\n\n \tcreate_notes_commit(r, t, NULL, buf.buf, buf.len, &commit_oid);\n-\tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n+\tstrbuf_insertstr(&buf, 0, \"notes: \"); /* commit message starts at index 7 */\n \tupdate_ref(buf.buf, t->update_ref, &commit_oid, NULL, 0,\n \t\t   UPDATE_REFS_DIE_ON_ERR);\n\ndiff --git a/notes.c b/notes.c\nindex 0c79964c26..a24af53de6 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1332,9 +1332,9 @@ void expand_notes_ref(struct strbuf *sb)\n \tif (starts_with(sb->buf, \"refs/notes/\"))\n \t\treturn; /* we're happy */\n \telse if (starts_with(sb->buf, \"notes/\"))\n-\t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n+\t\tstrbuf_insertstr(sb, 0, \"refs/\");\n \telse\n-\t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n+\t\tstrbuf_insertstr(sb, 0, \"refs/notes/\");\n }\n\n void expand_loose_notes_ref(struct strbuf *sb)\ndiff --git a/pretty.c b/pretty.c\nindex f5fbbc5ffb..28afc701b6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1609,9 +1609,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\t\tstrbuf_setlen(sb, sb->len - 1);\n \t} else if (orig_len != sb->len) {\n \t\tif (magic == ADD_LF_BEFORE_NON_EMPTY)\n-\t\t\tstrbuf_insert(sb, orig_len, \"\\n\", 1);\n+\t\t\tstrbuf_insertstr(sb, orig_len, \"\\n\");\n \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n-\t\t\tstrbuf_insert(sb, orig_len, \" \", 1);\n+\t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n \t}\n \treturn consumed + 1;\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex bfa66569a4..aae7ac3a82 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -244,6 +244,18 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n  */\n void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);\n\n+/**\n+ * Insert a NUL-terminated string to the given position of the buffer.\n+ * The remaining contents will be shifted, not overwritten.  It's an\n+ * inline function to allow the compiler to resolve strlen() calls on\n+ * constants at compile time.\n+ */\n+static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,\n+\t\t\t\t    const char *s)\n+{\n+\tstrbuf_insert(sb, pos, s, strlen(s));\n+}\n+\n /**\n  * Insert data to the given position of the buffer giving a printf format\n  * string. The contents will be shifted, not overwritten.\n--\n2.25.0\n\n"},{"id":"391381","messageId":"20200208230801.GA33529@syl.local","threadId":"52763","inReplyTo":"019be197-e0aa-1234-e16f-6561d8340023@web.de","subject":"Re: [PATCH] strbuf: add and use strbuf_insertstr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-08T23:08:01Z","receivedAt":"2020-02-08T23:08:06Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi René,\n\nOn Sat, Feb 08, 2020 at 08:56:43PM +0100, René Scharfe wrote:\n> Add a function for inserting a C string into a strbuf.  Use it\n> throughout the source to get rid of magic string length constants and\n> explicit strlen() calls.\n>\n> Like strbuf_addstr(), implement it as an inline function to avoid the\n> implicit strlen() calls to cause runtime overhead.\n\nThis all looks quite reasonable to me. Did you have a specific\nmotivation in mind when writing this patch, other than to get rid of\nhaving to call strlen on the same argument like:\n\n  strbuf_insert(&buf, pos, x, strlen(x))\n\n? Not that this needs more motivation than that, I'm just curious.\n\nI looked through 'git grep strbuf_insert(', and only noticed one spot\nthat I could be updated with 'strbuf_insertstr' in mailinfo.c:\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex c535dec2e9..543962d40c 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n    len = strlen(\"Content-Type: \");\n    strbuf_add(&sb, line->buf + len, line->len - len);\n    decode_header(mi, &sb);\n-\t\tstrbuf_insert(&sb, 0, \"Content-Type: \", len);\n+\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n    handle_content_type(mi, &sb);\n    ret = 1;\n    goto check_header_out;\n\nOtherwise this looks quite good to me.\n\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  builtin/checkout.c        |  2 +-\n>  builtin/notes.c           |  4 ++--\n>  builtin/sparse-checkout.c |  2 +-\n>  commit.c                  |  2 +-\n>  config.c                  |  2 +-\n>  http.c                    |  4 ++--\n>  mailinfo.c                |  2 +-\n>  notes-utils.c             |  2 +-\n>  notes.c                   |  4 ++--\n>  pretty.c                  |  4 ++--\n>  strbuf.h                  | 12 ++++++++++++\n>  11 files changed, 26 insertions(+), 14 deletions(-)\n>\n> diff --git a/builtin/checkout.c b/builtin/checkout.c\n> index fc2eb1befc..d6773818b8 100644\n> --- a/builtin/checkout.c\n> +++ b/builtin/checkout.c\n> @@ -865,7 +865,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n>  \t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n>  \t\t\told_desc ? old_desc : \"(invalid)\", new_branch_info->name);\n>  \telse\n> -\t\tstrbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));\n> +\t\tstrbuf_insertstr(&msg, 0, reflog_msg);\n>\n>  \tif (!strcmp(new_branch_info->name, \"HEAD\") && !new_branch_info->path && !opts->force_detach) {\n>  \t\t/* Nothing to do. */\n> diff --git a/builtin/notes.c b/builtin/notes.c\n> index 95456f3165..35e468ea2d 100644\n> --- a/builtin/notes.c\n> +++ b/builtin/notes.c\n> @@ -622,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n>\n>  \t\tstrbuf_grow(&d.buf, size + 1);\n>  \t\tif (d.buf.len && prev_buf && size)\n> -\t\t\tstrbuf_insert(&d.buf, 0, \"\\n\", 1);\n> +\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n>  \t\tif (prev_buf && size)\n>  \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n>  \t\tfree(prev_buf);\n> @@ -745,7 +745,7 @@ static int merge_commit(struct notes_merge_options *o)\n>  \tmemset(&pretty_ctx, 0, sizeof(pretty_ctx));\n>  \tformat_commit_message(partial, \"%s\", &msg, &pretty_ctx);\n>  \tstrbuf_trim(&msg);\n> -\tstrbuf_insert(&msg, 0, \"notes: \", 7);\n> +\tstrbuf_insertstr(&msg, 0, \"notes: \");\n>  \tupdate_ref(msg.buf, o->local_ref, &oid,\n>  \t\t   is_null_oid(&parent_oid) ? NULL : &parent_oid,\n>  \t\t   0, UPDATE_REFS_DIE_ON_ERR);\n> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\n> index b3bed891cb..38d0d944b3 100644\n> --- a/builtin/sparse-checkout.c\n> +++ b/builtin/sparse-checkout.c\n> @@ -373,7 +373,7 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)\n>  \t\treturn;\n>\n>  \tif (line->buf[0] != '/')\n> -\t\tstrbuf_insert(line, 0, \"/\", 1);\n> +\t\tstrbuf_insertstr(line, 0, \"/\");\n>\n>  \tinsert_recursive_pattern(pl, line);\n>  }\n> diff --git a/commit.c b/commit.c\n> index 3f91d3efc5..a6cfa41a4e 100644\n> --- a/commit.c\n> +++ b/commit.c\n> @@ -993,7 +993,7 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n>  \t\t\tstrbuf_insert(buf, inspos, gpg_sig_header, gpg_sig_header_len);\n>  \t\t\tinspos += gpg_sig_header_len;\n>  \t\t}\n> -\t\tstrbuf_insert(buf, inspos++, \" \", 1);\n> +\t\tstrbuf_insertstr(buf, inspos++, \" \");\n>  \t\tstrbuf_insert(buf, inspos, bol, len);\n>  \t\tinspos += len;\n>  \t\tcopypos += len;\n> diff --git a/config.c b/config.c\n> index d75f88ca0c..b386c0c628 100644\n> --- a/config.c\n> +++ b/config.c\n> @@ -204,7 +204,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n>  \t\tstrbuf_splice(pat, 0, 1, path.buf, slash - path.buf);\n>  \t\tprefix = slash - path.buf + 1 /* slash */;\n>  \t} else if (!is_absolute_path(pat->buf))\n> -\t\tstrbuf_insert(pat, 0, \"**/\", 3);\n> +\t\tstrbuf_insertstr(pat, 0, \"**/\");\n>\n>  \tadd_trailing_starstar_for_dir(pat);\n>\n> diff --git a/http.c b/http.c\n> index 5f348169c3..00a0e50763 100644\n> --- a/http.c\n> +++ b/http.c\n> @@ -680,8 +680,8 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n>  \tfor (header = headers; *header; header++) {\n>  \t\tif (hide_sensitive_header)\n>  \t\t\tredact_sensitive_header(*header);\n> -\t\tstrbuf_insert((*header), 0, text, strlen(text));\n> -\t\tstrbuf_insert((*header), strlen(text), \": \", 2);\n> +\t\tstrbuf_insertstr((*header), 0, text);\n> +\t\tstrbuf_insertstr((*header), strlen(text), \": \");\n>  \t\tstrbuf_rtrim((*header));\n>  \t\tstrbuf_addch((*header), '\\n');\n>  \t\ttrace_strbuf(&trace_curl, (*header));\n> diff --git a/mailinfo.c b/mailinfo.c\n> index b395adbdf2..c535dec2e9 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -254,7 +254,7 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n>  \tmi->delsp = has_attr_value(line->buf, \"delsp=\", \"yes\");\n>\n>  \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n> -\t\tstrbuf_insert(boundary, 0, \"--\", 2);\n> +\t\tstrbuf_insertstr(boundary, 0, \"--\");\n>  \t\tif (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) {\n>  \t\t\terror(\"Too many boundaries to handle\");\n>  \t\t\tmi->input_error = -1;\n> diff --git a/notes-utils.c b/notes-utils.c\n> index a819410698..5f94c513d4 100644\n> --- a/notes-utils.c\n> +++ b/notes-utils.c\n> @@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)\n>  \tstrbuf_complete_line(&buf);\n>\n>  \tcreate_notes_commit(r, t, NULL, buf.buf, buf.len, &commit_oid);\n> -\tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n> +\tstrbuf_insertstr(&buf, 0, \"notes: \"); /* commit message starts at index 7 */\n>  \tupdate_ref(buf.buf, t->update_ref, &commit_oid, NULL, 0,\n>  \t\t   UPDATE_REFS_DIE_ON_ERR);\n>\n> diff --git a/notes.c b/notes.c\n> index 0c79964c26..a24af53de6 100644\n> --- a/notes.c\n> +++ b/notes.c\n> @@ -1332,9 +1332,9 @@ void expand_notes_ref(struct strbuf *sb)\n>  \tif (starts_with(sb->buf, \"refs/notes/\"))\n>  \t\treturn; /* we're happy */\n>  \telse if (starts_with(sb->buf, \"notes/\"))\n> -\t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n> +\t\tstrbuf_insertstr(sb, 0, \"refs/\");\n>  \telse\n> -\t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n> +\t\tstrbuf_insertstr(sb, 0, \"refs/notes/\");\n>  }\n>\n>  void expand_loose_notes_ref(struct strbuf *sb)\n> diff --git a/pretty.c b/pretty.c\n> index f5fbbc5ffb..28afc701b6 100644\n> --- a/pretty.c\n> +++ b/pretty.c\n> @@ -1609,9 +1609,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n>  \t\t\tstrbuf_setlen(sb, sb->len - 1);\n>  \t} else if (orig_len != sb->len) {\n>  \t\tif (magic == ADD_LF_BEFORE_NON_EMPTY)\n> -\t\t\tstrbuf_insert(sb, orig_len, \"\\n\", 1);\n> +\t\t\tstrbuf_insertstr(sb, orig_len, \"\\n\");\n>  \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n> -\t\t\tstrbuf_insert(sb, orig_len, \" \", 1);\n> +\t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n>  \t}\n>  \treturn consumed + 1;\n>  }\n> diff --git a/strbuf.h b/strbuf.h\n> index bfa66569a4..aae7ac3a82 100644\n> --- a/strbuf.h\n> +++ b/strbuf.h\n> @@ -244,6 +244,18 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n>   */\n>  void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);\n>\n> +/**\n> + * Insert a NUL-terminated string to the given position of the buffer.\n> + * The remaining contents will be shifted, not overwritten.  It's an\n> + * inline function to allow the compiler to resolve strlen() calls on\n> + * constants at compile time.\n> + */\n> +static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,\n> +\t\t\t\t    const char *s)\n> +{\n> +\tstrbuf_insert(sb, pos, s, strlen(s));\n> +}\n> +\n>  /**\n>   * Insert data to the given position of the buffer giving a printf format\n>   * string. The contents will be shifted, not overwritten.\n> --\n> 2.25.0\n>\n\nThanks,\nTaylor\n"},{"id":"391384","messageId":"CAPig+cRmzHEQPhTzHrqqS9So63pJqQVGa1kGoWARmGU_0tn0_A@mail.gmail.com","threadId":"52763","inReplyTo":"019be197-e0aa-1234-e16f-6561d8340023@web.de","subject":"Re: [PATCH] strbuf: add and use strbuf_insertstr()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-09T00:53:03Z","receivedAt":"2020-02-09T00:54:10Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sat, Feb 8, 2020 at 2:57 PM René Scharfe <l.s.r@web.de> wrote:\n> Add a function for inserting a C string into a strbuf.  Use it\n> throughout the source to get rid of magic string length constants and\n> explicit strlen() calls.\n>\n> Like strbuf_addstr(), implement it as an inline function to avoid the\n> implicit strlen() calls to cause runtime overhead.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> diff --git a/notes-utils.c b/notes-utils.c\n> @@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)\n> -       strbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n> +       strbuf_insertstr(&buf, 0, \"notes: \"); /* commit message starts at index 7 */\n\nIs there a reason to retain the comment which talks about magic number 7?\n"},{"id":"391387","messageId":"5566a63d-f713-7580-6fc1-83286bc8955d@web.de","threadId":"52763","inReplyTo":"CAPig+cRmzHEQPhTzHrqqS9So63pJqQVGa1kGoWARmGU_0tn0_A@mail.gmail.com","subject":"Re: [PATCH] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T10:23:32Z","receivedAt":"2020-02-09T10:23:38Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.02.20 um 01:53 schrieb Eric Sunshine:\n> On Sat, Feb 8, 2020 at 2:57 PM René Scharfe <l.s.r@web.de> wrote:\n>> Add a function for inserting a C string into a strbuf.  Use it\n>> throughout the source to get rid of magic string length constants and\n>> explicit strlen() calls.\n>>\n>> Like strbuf_addstr(), implement it as an inline function to avoid the\n>> implicit strlen() calls to cause runtime overhead.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> diff --git a/notes-utils.c b/notes-utils.c\n>> @@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)\n>> -       strbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n>> +       strbuf_insertstr(&buf, 0, \"notes: \"); /* commit message starts at index 7 */\n>\n> Is there a reason to retain the comment which talks about magic number 7?\n\nI didn't understand its usefulness, so I didn't dare touch it.  Looking\ndeeper, I think it already became obsolete with 13f8b72d8c (Convert\ncommit_tree() to take strbuf as message, 2011-12-15), which included:\n\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex f8e437db01..5e32548cdd 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -301,12 +301,12 @@ void commit_notes(struct notes_tree *t, const char *msg)\n \t\treturn; /* don't have to commit an unchanged tree */\n\n \t/* Prepare commit message and reflog message */\n-\tstrbuf_addstr(&buf, \"notes: \"); /* commit message starts at index 7 */\n \tstrbuf_addstr(&buf, msg);\n \tif (buf.buf[buf.len - 1] != '\\n')\n \t\tstrbuf_addch(&buf, '\\n'); /* Make sure msg ends with newline */\n\n-\tcreate_notes_commit(t, NULL, buf.buf + 7, commit_sha1);\n+\tcreate_notes_commit(t, NULL, &buf, commit_sha1);\n+\tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n \tupdate_ref(buf.buf, t->ref, commit_sha1, NULL, 0, DIE_ON_ERR);\n\n \tstrbuf_release(&buf);\n\nRené\n"},{"id":"391388","messageId":"b0fc55fd-2ed5-25c4-850e-4c69199e0db3@web.de","threadId":"52763","inReplyTo":"20200208230801.GA33529@syl.local","subject":"Re: [PATCH] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T10:23:26Z","receivedAt":"2020-02-09T10:23:39Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.02.20 um 00:08 schrieb Taylor Blau:\n> Hi René,\n>\n> On Sat, Feb 08, 2020 at 08:56:43PM +0100, René Scharfe wrote:\n>> Add a function for inserting a C string into a strbuf.  Use it\n>> throughout the source to get rid of magic string length constants and\n>> explicit strlen() calls.\n>>\n>> Like strbuf_addstr(), implement it as an inline function to avoid the\n>> implicit strlen() calls to cause runtime overhead.\n>\n> This all looks quite reasonable to me. Did you have a specific\n> motivation in mind when writing this patch, other than to get rid of\n> having to call strlen on the same argument like:\n>\n>   strbuf_insert(&buf, pos, x, strlen(x))\n>\n> ? Not that this needs more motivation than that, I'm just curious.\n\nNo, that's it.\n\n> I looked through 'git grep strbuf_insert(', and only noticed one spot\n> that I could be updated with 'strbuf_insertstr' in mailinfo.c:\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index c535dec2e9..543962d40c 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n>     len = strlen(\"Content-Type: \");\n>     strbuf_add(&sb, line->buf + len, line->len - len);\n>     decode_header(mi, &sb);\n> -\t\tstrbuf_insert(&sb, 0, \"Content-Type: \", len);\n> +\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n>     handle_content_type(mi, &sb);\n>     ret = 1;\n>     goto check_header_out;\n\nRight.  Missed that one because the strlen() call is not in side the\nstrbuf_insert() invocation.\n\nRené\n"},{"id":"391397","messageId":"b31c46a8-380b-3528-27a5-a2dddacaf837@web.de","threadId":"52763","inReplyTo":"019be197-e0aa-1234-e16f-6561d8340023@web.de","subject":"[PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T13:44:23Z","receivedAt":"2020-02-09T13:44:33Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Add a function for inserting a C string into a strbuf.  Use it\nthroughout the source to get rid of magic string length constants and\nexplicit strlen() calls.\n\nLike strbuf_addstr(), implement it as an inline function to avoid the\nimplicit strlen() calls to cause runtime overhead.\n\nHelped-by: Taylor Blau <me@ttaylorr.com>\nHelped-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\nChanges:\n- Added second hunk in mailinfo.c.\n- Removed outdated comment in notes-util.c.\n\n builtin/checkout.c        |  2 +-\n builtin/notes.c           |  4 ++--\n builtin/sparse-checkout.c |  2 +-\n commit.c                  |  2 +-\n config.c                  |  2 +-\n http.c                    |  4 ++--\n mailinfo.c                |  4 ++--\n notes-utils.c             |  2 +-\n notes.c                   |  4 ++--\n pretty.c                  |  4 ++--\n strbuf.h                  | 12 ++++++++++++\n 11 files changed, 27 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/checkout.c b/builtin/checkout.c\nindex fc2eb1befc..d6773818b8 100644\n--- a/builtin/checkout.c\n+++ b/builtin/checkout.c\n@@ -865,7 +865,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,\n \t\tstrbuf_addf(&msg, \"checkout: moving from %s to %s\",\n \t\t\told_desc ? old_desc : \"(invalid)\", new_branch_info->name);\n \telse\n-\t\tstrbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));\n+\t\tstrbuf_insertstr(&msg, 0, reflog_msg);\n\n \tif (!strcmp(new_branch_info->name, \"HEAD\") && !new_branch_info->path && !opts->force_detach) {\n \t\t/* Nothing to do. */\ndiff --git a/builtin/notes.c b/builtin/notes.c\nindex 95456f3165..35e468ea2d 100644\n--- a/builtin/notes.c\n+++ b/builtin/notes.c\n@@ -622,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)\n\n \t\tstrbuf_grow(&d.buf, size + 1);\n \t\tif (d.buf.len && prev_buf && size)\n-\t\t\tstrbuf_insert(&d.buf, 0, \"\\n\", 1);\n+\t\t\tstrbuf_insertstr(&d.buf, 0, \"\\n\");\n \t\tif (prev_buf && size)\n \t\t\tstrbuf_insert(&d.buf, 0, prev_buf, size);\n \t\tfree(prev_buf);\n@@ -745,7 +745,7 @@ static int merge_commit(struct notes_merge_options *o)\n \tmemset(&pretty_ctx, 0, sizeof(pretty_ctx));\n \tformat_commit_message(partial, \"%s\", &msg, &pretty_ctx);\n \tstrbuf_trim(&msg);\n-\tstrbuf_insert(&msg, 0, \"notes: \", 7);\n+\tstrbuf_insertstr(&msg, 0, \"notes: \");\n \tupdate_ref(msg.buf, o->local_ref, &oid,\n \t\t   is_null_oid(&parent_oid) ? NULL : &parent_oid,\n \t\t   0, UPDATE_REFS_DIE_ON_ERR);\ndiff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c\nindex b3bed891cb..38d0d944b3 100644\n--- a/builtin/sparse-checkout.c\n+++ b/builtin/sparse-checkout.c\n@@ -373,7 +373,7 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)\n \t\treturn;\n\n \tif (line->buf[0] != '/')\n-\t\tstrbuf_insert(line, 0, \"/\", 1);\n+\t\tstrbuf_insertstr(line, 0, \"/\");\n\n \tinsert_recursive_pattern(pl, line);\n }\ndiff --git a/commit.c b/commit.c\nindex 3f91d3efc5..a6cfa41a4e 100644\n--- a/commit.c\n+++ b/commit.c\n@@ -993,7 +993,7 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)\n \t\t\tstrbuf_insert(buf, inspos, gpg_sig_header, gpg_sig_header_len);\n \t\t\tinspos += gpg_sig_header_len;\n \t\t}\n-\t\tstrbuf_insert(buf, inspos++, \" \", 1);\n+\t\tstrbuf_insertstr(buf, inspos++, \" \");\n \t\tstrbuf_insert(buf, inspos, bol, len);\n \t\tinspos += len;\n \t\tcopypos += len;\ndiff --git a/config.c b/config.c\nindex d75f88ca0c..b386c0c628 100644\n--- a/config.c\n+++ b/config.c\n@@ -204,7 +204,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)\n \t\tstrbuf_splice(pat, 0, 1, path.buf, slash - path.buf);\n \t\tprefix = slash - path.buf + 1 /* slash */;\n \t} else if (!is_absolute_path(pat->buf))\n-\t\tstrbuf_insert(pat, 0, \"**/\", 3);\n+\t\tstrbuf_insertstr(pat, 0, \"**/\");\n\n \tadd_trailing_starstar_for_dir(pat);\n\ndiff --git a/http.c b/http.c\nindex 5f348169c3..00a0e50763 100644\n--- a/http.c\n+++ b/http.c\n@@ -680,8 +680,8 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,\n \tfor (header = headers; *header; header++) {\n \t\tif (hide_sensitive_header)\n \t\t\tredact_sensitive_header(*header);\n-\t\tstrbuf_insert((*header), 0, text, strlen(text));\n-\t\tstrbuf_insert((*header), strlen(text), \": \", 2);\n+\t\tstrbuf_insertstr((*header), 0, text);\n+\t\tstrbuf_insertstr((*header), strlen(text), \": \");\n \t\tstrbuf_rtrim((*header));\n \t\tstrbuf_addch((*header), '\\n');\n \t\ttrace_strbuf(&trace_curl, (*header));\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..543962d40c 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -254,7 +254,7 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)\n \tmi->delsp = has_attr_value(line->buf, \"delsp=\", \"yes\");\n\n \tif (slurp_attr(line->buf, \"boundary=\", boundary)) {\n-\t\tstrbuf_insert(boundary, 0, \"--\", 2);\n+\t\tstrbuf_insertstr(boundary, 0, \"--\");\n \t\tif (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) {\n \t\t\terror(\"Too many boundaries to handle\");\n \t\t\tmi->input_error = -1;\n@@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n \t\tlen = strlen(\"Content-Type: \");\n \t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n \t\tdecode_header(mi, &sb);\n-\t\tstrbuf_insert(&sb, 0, \"Content-Type: \", len);\n+\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\ndiff --git a/notes-utils.c b/notes-utils.c\nindex a819410698..4bf4888d8c 100644\n--- a/notes-utils.c\n+++ b/notes-utils.c\n@@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)\n \tstrbuf_complete_line(&buf);\n\n \tcreate_notes_commit(r, t, NULL, buf.buf, buf.len, &commit_oid);\n-\tstrbuf_insert(&buf, 0, \"notes: \", 7); /* commit message starts at index 7 */\n+\tstrbuf_insertstr(&buf, 0, \"notes: \");\n \tupdate_ref(buf.buf, t->update_ref, &commit_oid, NULL, 0,\n \t\t   UPDATE_REFS_DIE_ON_ERR);\n\ndiff --git a/notes.c b/notes.c\nindex 0c79964c26..a24af53de6 100644\n--- a/notes.c\n+++ b/notes.c\n@@ -1332,9 +1332,9 @@ void expand_notes_ref(struct strbuf *sb)\n \tif (starts_with(sb->buf, \"refs/notes/\"))\n \t\treturn; /* we're happy */\n \telse if (starts_with(sb->buf, \"notes/\"))\n-\t\tstrbuf_insert(sb, 0, \"refs/\", 5);\n+\t\tstrbuf_insertstr(sb, 0, \"refs/\");\n \telse\n-\t\tstrbuf_insert(sb, 0, \"refs/notes/\", 11);\n+\t\tstrbuf_insertstr(sb, 0, \"refs/notes/\");\n }\n\n void expand_loose_notes_ref(struct strbuf *sb)\ndiff --git a/pretty.c b/pretty.c\nindex f5fbbc5ffb..28afc701b6 100644\n--- a/pretty.c\n+++ b/pretty.c\n@@ -1609,9 +1609,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */\n \t\t\tstrbuf_setlen(sb, sb->len - 1);\n \t} else if (orig_len != sb->len) {\n \t\tif (magic == ADD_LF_BEFORE_NON_EMPTY)\n-\t\t\tstrbuf_insert(sb, orig_len, \"\\n\", 1);\n+\t\t\tstrbuf_insertstr(sb, orig_len, \"\\n\");\n \t\telse if (magic == ADD_SP_BEFORE_NON_EMPTY)\n-\t\t\tstrbuf_insert(sb, orig_len, \" \", 1);\n+\t\t\tstrbuf_insertstr(sb, orig_len, \" \");\n \t}\n \treturn consumed + 1;\n }\ndiff --git a/strbuf.h b/strbuf.h\nindex bfa66569a4..aae7ac3a82 100644\n--- a/strbuf.h\n+++ b/strbuf.h\n@@ -244,6 +244,18 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);\n  */\n void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);\n\n+/**\n+ * Insert a NUL-terminated string to the given position of the buffer.\n+ * The remaining contents will be shifted, not overwritten.  It's an\n+ * inline function to allow the compiler to resolve strlen() calls on\n+ * constants at compile time.\n+ */\n+static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,\n+\t\t\t\t    const char *s)\n+{\n+\tstrbuf_insert(sb, pos, s, strlen(s));\n+}\n+\n /**\n  * Insert data to the given position of the buffer giving a printf format\n  * string. The contents will be shifted, not overwritten.\n--\n2.25.0\n"},{"id":"391409","messageId":"CAPig+cQdJ0NJSWZN-2ckeLB7RfU9GZ7LGvVX4y+Q1daPnW8WsA@mail.gmail.com","threadId":"52763","inReplyTo":"b31c46a8-380b-3528-27a5-a2dddacaf837@web.de","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-09T17:36:22Z","receivedAt":"2020-02-09T17:36:46Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:\n> Add a function for inserting a C string into a strbuf.  Use it\n> throughout the source to get rid of magic string length constants and\n> explicit strlen() calls.\n>\n> Like strbuf_addstr(), implement it as an inline function to avoid the\n> implicit strlen() calls to cause runtime overhead.\n>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n> diff --git a/mailinfo.c b/mailinfo.c\n> @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n>                 len = strlen(\"Content-Type: \");\n>                 strbuf_add(&sb, line->buf + len, line->len - len);\n>                 decode_header(mi, &sb);\n> -               strbuf_insert(&sb, 0, \"Content-Type: \", len);\n> +               strbuf_insertstr(&sb, 0, \"Content-Type: \");\n>                 handle_content_type(mi, &sb);\n\nMeh. We've already computed the length of \"Content-Type: \" a few lines\nearlier, so taking advantage of that value when inserting the string\nliteral is perfectly sensible. Thus, I'm not convinced that this\nchange is an improvement.\n\nDigging deeper, though, I have to wonder why this bothers inserting\n\"Content-Type: \" at all. None of the other cases handled by\ncheck_header() bother re-inserting the header, so why this one? I\nthought it might be because handle_content_type() depends upon the\nheader being present, but from my reading, this does not appear to be\nthe case. handle_content_type() calls has_attr_value() and\nslurp_attr() to examine the incoming line, but neither of those seem\nto expect any sort of \"<Header>: \" either. Thus, it appears that the\ninsertion of \"Content-Type: \" is superfluous. If this is indeed the\ncase, then rather than converting this to strbuf_insertstr(), I could\nsee it being pulled out into a separate patch which merely removes the\nstrbuf_insert() call.\n"},{"id":"391412","messageId":"60b491a1-2b71-d5a5-398f-e6743e2c617a@web.de","threadId":"52763","inReplyTo":"CAPig+cQdJ0NJSWZN-2ckeLB7RfU9GZ7LGvVX4y+Q1daPnW8WsA@mail.gmail.com","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-09T18:28:31Z","receivedAt":"2020-02-09T18:28:40Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 09.02.20 um 18:36 schrieb Eric Sunshine:\n> On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:\n>> Add a function for inserting a C string into a strbuf.  Use it\n>> throughout the source to get rid of magic string length constants and\n>> explicit strlen() calls.\n>>\n>> Like strbuf_addstr(), implement it as an inline function to avoid the\n>> implicit strlen() calls to cause runtime overhead.\n>>\n>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>> ---\n>> diff --git a/mailinfo.c b/mailinfo.c\n>> @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n>>                 len = strlen(\"Content-Type: \");\n>>                 strbuf_add(&sb, line->buf + len, line->len - len);\n>>                 decode_header(mi, &sb);\n>> -               strbuf_insert(&sb, 0, \"Content-Type: \", len);\n>> +               strbuf_insertstr(&sb, 0, \"Content-Type: \");\n>>                 handle_content_type(mi, &sb);\n>\n> Meh. We've already computed the length of \"Content-Type: \" a few lines\n> earlier, so taking advantage of that value when inserting the string\n> literal is perfectly sensible.\n\nWell, yes, but it would be more sensible if we'd have only a single\nstring here.  At the source code level we have two string constants that\nhappen to have the same contents.  Handling them separately is\nreasonable, I think.\n\nThe compiler is merging those two, and resolves the other strlen() call\nat compile time, so the generated code is the same.\n\n> Thus, I'm not convinced that this change is an improvement.\n\nThe improvement is to untangle the handling of those two string\nconstants and to use a C string without having to pass along its\nlength.  That doesn't make the code clean, yet, admittedly.\n\n> Digging deeper, though, I have to wonder why this bothers inserting\n> \"Content-Type: \" at all. None of the other cases handled by\n> check_header() bother re-inserting the header, so why this one? I\n> thought it might be because handle_content_type() depends upon the\n> header being present, but from my reading, this does not appear to be\n> the case. handle_content_type() calls has_attr_value() and\n> slurp_attr() to examine the incoming line, but neither of those seem\n> to expect any sort of \"<Header>: \" either. Thus, it appears that the\n> insertion of \"Content-Type: \" is superfluous. If this is indeed the\n> case, then rather than converting this to strbuf_insertstr(), I could\n> see it being pulled out into a separate patch which merely removes the\n> strbuf_insert() call.\n\nInteresting.  It makes sense that handle_content_type() wouldn't need\nsuch a header prefix -- it's only called if its existence in the line\nhas been confirmed.  And I also don't see a hint in the code that\nwould justify the insertion.\n\nDo you care to send a follow-up patch (or one against master if you're\nnot convinced by my reasoning given above)?\n\nThanks,\nRené\n"},{"id":"391414","messageId":"CAPig+cQCwxopnwC+VYgS39QpaSW10JWW4HZGYzeq+psvB1bLMA@mail.gmail.com","threadId":"52763","inReplyTo":"60b491a1-2b71-d5a5-398f-e6743e2c617a@web.de","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-09T21:09:47Z","receivedAt":"2020-02-09T21:10:03Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Sun, Feb 9, 2020 at 1:28 PM René Scharfe <l.s.r@web.de> wrote:\n> Am 09.02.20 um 18:36 schrieb Eric Sunshine:\n> > Digging deeper, though, I have to wonder why this bothers inserting\n> > \"Content-Type: \" at all. [...]\n>\n> Do you care to send a follow-up patch (or one against master if you're\n> not convinced by my reasoning given above)?\n\nIt's unlikely I will get to it any time soon. I'm still sitting on\nunfinished patches I started 1.5 years ago and have a queue of \"git\nworktree\"-related tasks on my To-Do list.\n"},{"id":"391421","messageId":"20200209231040.GB4530@syl.local","threadId":"52763","inReplyTo":"60b491a1-2b71-d5a5-398f-e6743e2c617a@web.de","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-09T23:10:40Z","receivedAt":"2020-02-09T23:10:44Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"On Sun, Feb 09, 2020 at 07:28:31PM +0100, René Scharfe wrote:\n> Am 09.02.20 um 18:36 schrieb Eric Sunshine:\n> > On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:\n> >> Add a function for inserting a C string into a strbuf.  Use it\n> >> throughout the source to get rid of magic string length constants and\n> >> explicit strlen() calls.\n> >>\n> >> Like strbuf_addstr(), implement it as an inline function to avoid the\n> >> implicit strlen() calls to cause runtime overhead.\n> >>\n> >> Signed-off-by: René Scharfe <l.s.r@web.de>\n> >> ---\n> >> diff --git a/mailinfo.c b/mailinfo.c\n> >> @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n> >>                 len = strlen(\"Content-Type: \");\n> >>                 strbuf_add(&sb, line->buf + len, line->len - len);\n> >>                 decode_header(mi, &sb);\n> >> -               strbuf_insert(&sb, 0, \"Content-Type: \", len);\n> >> +               strbuf_insertstr(&sb, 0, \"Content-Type: \");\n> >>                 handle_content_type(mi, &sb);\n> >\n> > Meh. We've already computed the length of \"Content-Type: \" a few lines\n> > earlier, so taking advantage of that value when inserting the string\n> > literal is perfectly sensible.\n>\n> Well, yes, but it would be more sensible if we'd have only a single\n> string here.  At the source code level we have two string constants that\n> happen to have the same contents.  Handling them separately is\n> reasonable, I think.\n>\n> The compiler is merging those two, and resolves the other strlen() call\n> at compile time, so the generated code is the same.\n\nYes, if 'strbuf_insertstr' weren't inlined, I'd be less eager to make\nthis suggestion, but since it *is* inlined, I don't think that the\ncompiler will generate substantially different instructions whether we\nuse one or the other here.\n\n> > Thus, I'm not convinced that this change is an improvement.\n>\n> The improvement is to untangle the handling of those two string\n> constants and to use a C string without having to pass along its\n> length.  That doesn't make the code clean, yet, admittedly.\n\nAgreed.\n\n> > Digging deeper, though, I have to wonder why this bothers inserting\n> > \"Content-Type: \" at all. None of the other cases handled by\n> > check_header() bother re-inserting the header, so why this one? I\n> > thought it might be because handle_content_type() depends upon the\n> > header being present, but from my reading, this does not appear to be\n> > the case. handle_content_type() calls has_attr_value() and\n> > slurp_attr() to examine the incoming line, but neither of those seem\n> > to expect any sort of \"<Header>: \" either. Thus, it appears that the\n> > insertion of \"Content-Type: \" is superfluous. If this is indeed the\n> > case, then rather than converting this to strbuf_insertstr(), I could\n> > see it being pulled out into a separate patch which merely removes the\n> > strbuf_insert() call.\n>\n> Interesting.  It makes sense that handle_content_type() wouldn't need\n> such a header prefix -- it's only called if its existence in the line\n> has been confirmed.  And I also don't see a hint in the code that\n> would justify the insertion.\n>\n> Do you care to send a follow-up patch (or one against master if you're\n> not convinced by my reasoning given above)?\n\nI certainly can't speak for Eric, but for my $.02 I don't think that\nit's worth holding this series up. This seems like a separate issue to\nme, and I'd rather it not get get in the way of a perfectly good patch\nin the meantime.\n\nFor now, this increases the churn a little bit, but that is the price\nwe have to pay for the new 'strbuf_insertstr' to be applied/used\nconsistently.\n\nI'd be happy to see this go further, but I'd be just as happy to stop\nwhere we're at.\n\n> Thanks,\n> René\n\nThanks,\nTaylor\n"},{"id":"391440","messageId":"35ef5149-2da9-a147-fb5b-8f175ff617e7@web.de","threadId":"52763","inReplyTo":"b31c46a8-380b-3528-27a5-a2dddacaf837@web.de","subject":"[PATCH 2/1] mailinfo: don't insert header prefix for handle_content_type()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-10T07:15:19Z","receivedAt":"2020-02-10T07:16:00Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"handle_content_type() only cares about the value after \"Content-Type: \";\nthere is no need to insert that string for it.\n\nSuggested-by: Eric Sunshine <sunshine@sunshineco.com>\nSigned-off-by: René Scharfe <l.s.r@web.de>\n---\n mailinfo.c | 1 -\n 1 file changed, 1 deletion(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 543962d40c..402ef04dd1 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -570,7 +570,6 @@ static int check_header(struct mailinfo *mi,\n \t\tlen = strlen(\"Content-Type: \");\n \t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n \t\tdecode_header(mi, &sb);\n-\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n--\n2.25.0\n"},{"id":"391473","messageId":"xmqqlfpa1hg5.fsf@gitster-ct.c.googlers.com","threadId":"52763","inReplyTo":"35ef5149-2da9-a147-fb5b-8f175ff617e7@web.de","subject":"Re: [PATCH 2/1] mailinfo: don't insert header prefix for handle_content_type()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-10T17:27:06Z","receivedAt":"2020-02-10T17:27:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"René Scharfe <l.s.r@web.de> writes:\n\n> handle_content_type() only cares about the value after \"Content-Type: \";\n> there is no need to insert that string for it.\n>\n> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  mailinfo.c | 1 -\n>  1 file changed, 1 deletion(-)\n\nOK.  There is only one caller that just saw that the line it is\nlooking at begins with \"Content-Type:\", and the only thing the\nhelper function does is to look for a handful of attr=value pairs\n(without even looking at the primary thing the header wants to\nconvey---the type itself, like text/plain etc.).  The helper may\nwant to learn a bit more careful than the current implementation\nin doing what it does, but it does not need to see the header name\nto do so.  It's not like we will be accepting more than one kind of\ncontent-type like header with this helper and make it capable of\ntailoring its behaviour based on which exact header type it is.\n\nWill queue.  Thanks.\n\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 543962d40c..402ef04dd1 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -570,7 +570,6 @@ static int check_header(struct mailinfo *mi,\n>  \t\tlen = strlen(\"Content-Type: \");\n>  \t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n>  \t\tdecode_header(mi, &sb);\n> -\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n>  \t\thandle_content_type(mi, &sb);\n>  \t\tret = 1;\n>  \t\tgoto check_header_out;\n> --\n> 2.25.0\n"},{"id":"391487","messageId":"20200210195508.GB46461@syl.local","threadId":"52763","inReplyTo":"35ef5149-2da9-a147-fb5b-8f175ff617e7@web.de","subject":"Re: [PATCH 2/1] mailinfo: don't insert header prefix for handle_content_type()","fromName":"Taylor Blau","fromEmail":"me@ttaylorr.com","sentAt":"2020-02-10T19:55:08Z","receivedAt":"2020-02-10T19:55:13Z","isPatch":true,"sender":{"key":"me@ttaylorr.com","avatar":"https://avatars.githubusercontent.com/u/301000140?v=4"},"body":"Hi René,\n\nOn Mon, Feb 10, 2020 at 08:15:19AM +0100, René Scharfe wrote:\n> handle_content_type() only cares about the value after \"Content-Type: \";\n> there is no need to insert that string for it.\n>\n> Suggested-by: Eric Sunshine <sunshine@sunshineco.com>\n> Signed-off-by: René Scharfe <l.s.r@web.de>\n> ---\n>  mailinfo.c | 1 -\n>  1 file changed, 1 deletion(-)\n>\n> diff --git a/mailinfo.c b/mailinfo.c\n> index 543962d40c..402ef04dd1 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -570,7 +570,6 @@ static int check_header(struct mailinfo *mi,\n>  \t\tlen = strlen(\"Content-Type: \");\n>  \t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n>  \t\tdecode_header(mi, &sb);\n> -\t\tstrbuf_insertstr(&sb, 0, \"Content-Type: \");\n\nThis looks quite good to me. When I first looked, I was wondering if we\nshould have used 'skip_prefix' and passed that result into\n'strbuf_insertstr', but I think that's a bigger change than we need for\nnow.\n\nThis looks all right to me.\n\n>  \t\thandle_content_type(mi, &sb);\n>  \t\tret = 1;\n>  \t\tgoto check_header_out;\n> --\n> 2.25.0\n\nHere is my:\n\n  Reviewed-by: Taylor Blau <me@ttaylorr.com>\n\nThanks,\nTaylor\n"},{"id":"391512","messageId":"20200210234427.GA632160@coredump.intra.peff.net","threadId":"52763","inReplyTo":"CAPig+cQdJ0NJSWZN-2ckeLB7RfU9GZ7LGvVX4y+Q1daPnW8WsA@mail.gmail.com","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-10T23:44:27Z","receivedAt":"2020-02-10T23:44:30Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sun, Feb 09, 2020 at 12:36:22PM -0500, Eric Sunshine wrote:\n\n> On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:\n> > Add a function for inserting a C string into a strbuf.  Use it\n> > throughout the source to get rid of magic string length constants and\n> > explicit strlen() calls.\n> >\n> > Like strbuf_addstr(), implement it as an inline function to avoid the\n> > implicit strlen() calls to cause runtime overhead.\n> >\n> > Signed-off-by: René Scharfe <l.s.r@web.de>\n> > ---\n> > diff --git a/mailinfo.c b/mailinfo.c\n> > @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n> >                 len = strlen(\"Content-Type: \");\n> >                 strbuf_add(&sb, line->buf + len, line->len - len);\n> >                 decode_header(mi, &sb);\n> > -               strbuf_insert(&sb, 0, \"Content-Type: \", len);\n> > +               strbuf_insertstr(&sb, 0, \"Content-Type: \");\n> >                 handle_content_type(mi, &sb);\n> \n> Meh. We've already computed the length of \"Content-Type: \" a few lines\n> earlier, so taking advantage of that value when inserting the string\n> literal is perfectly sensible. Thus, I'm not convinced that this\n> change is an improvement.\n\nI had a similar thought. I kind of wonder if all of these \"len\" bits\n(and their repeated strings) could go away if cmp_header() just used\nskip_iprefix() under the hood and had a pointer out-parameter.\n\nSomething like the (largely untested) patch below. That would also make\nit easy to support arbitrary amounts of whitespace after the header,\nwhich I think are allowed by the standard (whereas now we'd parse\n\"Content-type:    text/plain\" as \"    text/plain\", which is silly).\n\nWorth doing?\n\n---\ndiff --git a/mailinfo.c b/mailinfo.c\nindex b395adbdf2..bbb5b429f8 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -346,11 +346,16 @@ static const char *header[MAX_HDR_PARSED] = {\n \t\"From\",\"Subject\",\"Date\",\n };\n \n-static inline int cmp_header(const struct strbuf *line, const char *hdr)\n+static inline int cmp_header(const struct strbuf *line, const char *hdr,\n+\t\t\t     const char **outval)\n {\n-\tint len = strlen(hdr);\n-\treturn !strncasecmp(line->buf, hdr, len) && line->len > len &&\n-\t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n+\tconst char *val;\n+\tif (!skip_iprefix(line->buf, hdr, &val) ||\n+\t    *val++ != ':' ||\n+\t    !isspace(*val++))\n+\t\treturn 0;\n+\t*outval = val;\n+\treturn 1;\n }\n \n static int is_format_patch_separator(const char *line, int len)\n@@ -547,17 +552,17 @@ static int check_header(struct mailinfo *mi,\n \t\t\tconst struct strbuf *line,\n \t\t\tstruct strbuf *hdr_data[], int overwrite)\n {\n-\tint i, ret = 0, len;\n+\tint i, ret = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *val;\n \n \t/* search for the interesting parts */\n \tfor (i = 0; header[i]; i++) {\n-\t\tint len = strlen(header[i]);\n-\t\tif ((!hdr_data[i] || overwrite) && cmp_header(line, header[i])) {\n+\t\tif ((!hdr_data[i] || overwrite) && cmp_header(line, header[i], &val)) {\n \t\t\t/* Unwrap inline B and Q encoding, and optionally\n \t\t\t * normalize the meta information to utf8.\n \t\t\t */\n-\t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n+\t\t\tstrbuf_addstr(&sb, val);\n \t\t\tdecode_header(mi, &sb);\n \t\t\thandle_header(&hdr_data[i], &sb);\n \t\t\tret = 1;\n@@ -566,26 +571,22 @@ static int check_header(struct mailinfo *mi,\n \t}\n \n \t/* Content stuff */\n-\tif (cmp_header(line, \"Content-Type\")) {\n-\t\tlen = strlen(\"Content-Type: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\tif (cmp_header(line, \"Content-Type\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n-\t\tstrbuf_insert(&sb, 0, \"Content-Type: \", len);\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (cmp_header(line, \"Content-Transfer-Encoding\")) {\n-\t\tlen = strlen(\"Content-Transfer-Encoding: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\tif (cmp_header(line, \"Content-Transfer-Encoding\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n \t\thandle_content_transfer_encoding(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (cmp_header(line, \"Message-Id\")) {\n-\t\tlen = strlen(\"Message-Id: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\tif (cmp_header(line, \"Message-Id\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n \t\tif (mi->add_message_id)\n \t\t\tmi->message_id = strbuf_detach(&sb, NULL);\n@@ -607,8 +608,9 @@ static int is_inbody_header(const struct mailinfo *mi,\n \t\t\t    const struct strbuf *line)\n {\n \tint i;\n+\tconst char *val;\n \tfor (i = 0; header[i]; i++)\n-\t\tif (!mi->s_hdr_data[i] && cmp_header(line, header[i]))\n+\t\tif (!mi->s_hdr_data[i] && cmp_header(line, header[i], &val))\n \t\t\treturn 1;\n \treturn 0;\n }\n"},{"id":"391535","messageId":"xmqq1rr1xfmt.fsf@gitster-ct.c.googlers.com","threadId":"52763","inReplyTo":"20200210234427.GA632160@coredump.intra.peff.net","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2020-02-11T16:17:30Z","receivedAt":"2020-02-11T16:17:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King <peff@peff.net> writes:\n\n> Something like the (largely untested) patch below. That would also make\n> it easy to support arbitrary amounts of whitespace after the header,\n> which I think are allowed by the standard (whereas now we'd parse\n> \"Content-type:    text/plain\" as \"    text/plain\", which is silly).\n>\n> Worth doing?\n\nThe result does look cleaner.  I do not think we've seen much\npolishing in this area of the code for quite a long time, which\nmight indicate that what we have may be good enough, but at the same\ntime it would mean it is quiescent time for the code and it is safe\nto clean it up.\n\n"},{"id":"391536","messageId":"a5622196-1978-2fe5-144f-99edc5516dd6@web.de","threadId":"52763","inReplyTo":"20200210234427.GA632160@coredump.intra.peff.net","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"René Scharfe","fromEmail":"l.s.r@web.de","sentAt":"2020-02-11T16:18:02Z","receivedAt":"2020-02-11T16:18:34Z","isPatch":true,"sender":{"key":"l.s.r@web.de","avatar":"https://avatars.githubusercontent.com/u/26122331?v=4"},"body":"Am 11.02.20 um 00:44 schrieb Jeff King:\n> On Sun, Feb 09, 2020 at 12:36:22PM -0500, Eric Sunshine wrote:\n>\n>> On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:\n>>> Add a function for inserting a C string into a strbuf.  Use it\n>>> throughout the source to get rid of magic string length constants and\n>>> explicit strlen() calls.\n>>>\n>>> Like strbuf_addstr(), implement it as an inline function to avoid the\n>>> implicit strlen() calls to cause runtime overhead.\n>>>\n>>> Signed-off-by: René Scharfe <l.s.r@web.de>\n>>> ---\n>>> diff --git a/mailinfo.c b/mailinfo.c\n>>> @@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,\n>>>                 len = strlen(\"Content-Type: \");\n>>>                 strbuf_add(&sb, line->buf + len, line->len - len);\n>>>                 decode_header(mi, &sb);\n>>> -               strbuf_insert(&sb, 0, \"Content-Type: \", len);\n>>> +               strbuf_insertstr(&sb, 0, \"Content-Type: \");\n>>>                 handle_content_type(mi, &sb);\n>>\n>> Meh. We've already computed the length of \"Content-Type: \" a few lines\n>> earlier, so taking advantage of that value when inserting the string\n>> literal is perfectly sensible. Thus, I'm not convinced that this\n>> change is an improvement.\n>\n> I had a similar thought. I kind of wonder if all of these \"len\" bits\n> (and their repeated strings) could go away if cmp_header() just used\n> skip_iprefix() under the hood and had a pointer out-parameter.\n>\n> Something like the (largely untested) patch below. That would also make\n> it easy to support arbitrary amounts of whitespace after the header,\n> which I think are allowed by the standard (whereas now we'd parse\n> \"Content-type:    text/plain\" as \"    text/plain\", which is silly).\n>\n> Worth doing?\n\nSure.\n\n> ---\n> diff --git a/mailinfo.c b/mailinfo.c\n> index b395adbdf2..bbb5b429f8 100644\n> --- a/mailinfo.c\n> +++ b/mailinfo.c\n> @@ -346,11 +346,16 @@ static const char *header[MAX_HDR_PARSED] = {\n>  \t\"From\",\"Subject\",\"Date\",\n>  };\n>\n> -static inline int cmp_header(const struct strbuf *line, const char *hdr)\n> +static inline int cmp_header(const struct strbuf *line, const char *hdr,\n> +\t\t\t     const char **outval)\n>  {\n> -\tint len = strlen(hdr);\n> -\treturn !strncasecmp(line->buf, hdr, len) && line->len > len &&\n> -\t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n> +\tconst char *val;\n> +\tif (!skip_iprefix(line->buf, hdr, &val) ||\n> +\t    *val++ != ':' ||\n> +\t    !isspace(*val++))\n> +\t\treturn 0;\n> +\t*outval = val;\n> +\treturn 1;\n>  }\n\nAnd you could rename it to skip_header() to fix the issue that its name\nstarts with cmp but its return value is the inverse of a cmp-style\nfunction.\n\nAnd it could take a char pointer instead of a strbuf, to reduce its\ndependencies and make it more widely useful -- but that might also be\na case of YAGNI.\n\n>\n>  static int is_format_patch_separator(const char *line, int len)\n> @@ -547,17 +552,17 @@ static int check_header(struct mailinfo *mi,\n>  \t\t\tconst struct strbuf *line,\n>  \t\t\tstruct strbuf *hdr_data[], int overwrite)\n>  {\n> -\tint i, ret = 0, len;\n> +\tint i, ret = 0;\n>  \tstruct strbuf sb = STRBUF_INIT;\n> +\tconst char *val;\n>\n>  \t/* search for the interesting parts */\n>  \tfor (i = 0; header[i]; i++) {\n> -\t\tint len = strlen(header[i]);\n> -\t\tif ((!hdr_data[i] || overwrite) && cmp_header(line, header[i])) {\n> +\t\tif ((!hdr_data[i] || overwrite) && cmp_header(line, header[i], &val)) {\n>  \t\t\t/* Unwrap inline B and Q encoding, and optionally\n>  \t\t\t * normalize the meta information to utf8.\n>  \t\t\t */\n> -\t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n> +\t\t\tstrbuf_addstr(&sb, val);\n\nThat assumes the header value never contains NULs.  Valid?\n\n>  \t\t\tdecode_header(mi, &sb);\n>  \t\t\thandle_header(&hdr_data[i], &sb);\n>  \t\t\tret = 1;\n> @@ -566,26 +571,22 @@ static int check_header(struct mailinfo *mi,\n>  \t}\n>\n>  \t/* Content stuff */\n> -\tif (cmp_header(line, \"Content-Type\")) {\n> -\t\tlen = strlen(\"Content-Type: \");\n> -\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n> +\tif (cmp_header(line, \"Content-Type\", &val)) {\n> +\t\tstrbuf_addstr(&sb, val);\n>  \t\tdecode_header(mi, &sb);\n> -\t\tstrbuf_insert(&sb, 0, \"Content-Type: \", len);\n>  \t\thandle_content_type(mi, &sb);\n>  \t\tret = 1;\n>  \t\tgoto check_header_out;\n>  \t}\n> -\tif (cmp_header(line, \"Content-Transfer-Encoding\")) {\n> -\t\tlen = strlen(\"Content-Transfer-Encoding: \");\n> -\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n> +\tif (cmp_header(line, \"Content-Transfer-Encoding\", &val)) {\n> +\t\tstrbuf_addstr(&sb, val);\n>  \t\tdecode_header(mi, &sb);\n>  \t\thandle_content_transfer_encoding(mi, &sb);\n>  \t\tret = 1;\n>  \t\tgoto check_header_out;\n>  \t}\n> -\tif (cmp_header(line, \"Message-Id\")) {\n> -\t\tlen = strlen(\"Message-Id: \");\n> -\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n> +\tif (cmp_header(line, \"Message-Id\", &val)) {\n> +\t\tstrbuf_addstr(&sb, val);\n>  \t\tdecode_header(mi, &sb);\n>  \t\tif (mi->add_message_id)\n>  \t\t\tmi->message_id = strbuf_detach(&sb, NULL);\n\nThe repeated sequence cmp_header()+strbuf_add{,str}()+decode_header()\nmakes me itchy.\n\n> @@ -607,8 +608,9 @@ static int is_inbody_header(const struct mailinfo *mi,\n>  \t\t\t    const struct strbuf *line)\n>  {\n>  \tint i;\n> +\tconst char *val;\n>  \tfor (i = 0; header[i]; i++)\n> -\t\tif (!mi->s_hdr_data[i] && cmp_header(line, header[i]))\n> +\t\tif (!mi->s_hdr_data[i] && cmp_header(line, header[i], &val))\n>  \t\t\treturn 1;\n>  \treturn 0;\n>  }\n>\n\n"},{"id":"391545","messageId":"20200211171357.GA2118476@coredump.intra.peff.net","threadId":"52763","inReplyTo":"a5622196-1978-2fe5-144f-99edc5516dd6@web.de","subject":"Re: [PATCH v2] strbuf: add and use strbuf_insertstr()","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:13:57Z","receivedAt":"2020-02-11T17:14:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2020 at 05:18:02PM +0100, René Scharfe wrote:\n\n> And you could rename it to skip_header() to fix the issue that its name\n> starts with cmp but its return value is the inverse of a cmp-style\n> function.\n\nGood suggestion.\n\n> And it could take a char pointer instead of a strbuf, to reduce its\n> dependencies and make it more widely useful -- but that might also be\n> a case of YAGNI.\n\nI didn't take this one because all of the callers have strbuf, and it\nsaves them from dereferencing.\n\n> > -\t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n> > +\t\t\tstrbuf_addstr(&sb, val);\n> \n> That assumes the header value never contains NULs.  Valid?\n\nI think so, but I pulled it out to a separate patch so that it could be\nargued for explicitly.\n\n> The repeated sequence cmp_header()+strbuf_add{,str}()+decode_header()\n> makes me itchy.\n\nYeah, it's ugly and I factored it out in a new patch. But the\nboilerplate and docstring ends up longer than the savings. ;)\n\n-Peff\n"},{"id":"391546","messageId":"20200211171649.GB2118476@coredump.intra.peff.net","threadId":"52763","inReplyTo":"xmqq1rr1xfmt.fsf@gitster-ct.c.googlers.com","subject":"[PATCH 0/4] some more mailinfo cleanups","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:16:49Z","receivedAt":"2020-02-11T17:16:52Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Feb 11, 2020 at 08:17:30AM -0800, Junio C Hamano wrote:\n\n> Jeff King <peff@peff.net> writes:\n> \n> > Something like the (largely untested) patch below. That would also make\n> > it easy to support arbitrary amounts of whitespace after the header,\n> > which I think are allowed by the standard (whereas now we'd parse\n> > \"Content-type:    text/plain\" as \"    text/plain\", which is silly).\n> >\n> > Worth doing?\n> \n> The result does look cleaner.  I do not think we've seen much\n> polishing in this area of the code for quite a long time, which\n> might indicate that what we have may be good enough, but at the same\n> time it would mean it is quiescent time for the code and it is safe\n> to clean it up.\n\nYeah, it feels a little like code churn. It's not like we ever add new\nheaders to match. But I doodled the series below while I was in a\nmeeting, and it does fix one tiny user-visible bug (that I don't think\nanybody has ever noticed or complained about), so maybe it's worth\ndoing. ;)\n\nThis is on top of rs/strbuf-insertstr, since it relies on 517b60564e\n(mailinfo: don't insert header prefix for handle_content_type(),\n2020-02-10).\n\n  [1/4]: mailinfo: treat header values as C strings\n  [2/4]: mailinfo: simplify parsing of header values\n  [3/4]: mailinfo: be more liberal with header whitespace\n  [4/4]: mailinfo: factor out some repeated header handling\n\n mailinfo.c          | 62 +++++++++++++++++++++++++++------------------\n t/t5100-mailinfo.sh | 15 +++++++++++\n 2 files changed, 52 insertions(+), 25 deletions(-)\n\n-Peff\n"},{"id":"391547","messageId":"20200211171852.GA2119034@coredump.intra.peff.net","threadId":"52763","inReplyTo":"20200211171649.GB2118476@coredump.intra.peff.net","subject":"[PATCH 1/4] mailinfo: treat header values as C strings","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:18:52Z","receivedAt":"2020-02-11T17:18:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We read each header line into a strbuf, which means that we could\nin theory handle header values with embedded NUL bytes. But in practice,\nthe values we parse out are passed to decode_header(), which uses\nstrstr(), strchr(), etc. And we would not expect such bytes anyway; they\nare forbidden by RFC822, etc and any non-ASCII characters should be\nencoded with RFC2047 encoding.\n\nSo let's switch to using strbuf_addstr(), which saves us some length\ncomputations (and will enable further cleanups in this code).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\nWe _could_ skip this and compute the length later as:\n\n  line->len - (val - line->buf)\n\nbut I like the simplification.\n\n mailinfo.c | 8 ++++----\n 1 file changed, 4 insertions(+), 4 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 402ef04dd1..59d5a8b8f3 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -557,7 +557,7 @@ static int check_header(struct mailinfo *mi,\n \t\t\t/* Unwrap inline B and Q encoding, and optionally\n \t\t\t * normalize the meta information to utf8.\n \t\t\t */\n-\t\t\tstrbuf_add(&sb, line->buf + len + 2, line->len - len - 2);\n+\t\t\tstrbuf_addstr(&sb, line->buf + len + 2);\n \t\t\tdecode_header(mi, &sb);\n \t\t\thandle_header(&hdr_data[i], &sb);\n \t\t\tret = 1;\n@@ -568,23 +568,23 @@ static int check_header(struct mailinfo *mi,\n \t/* Content stuff */\n \tif (cmp_header(line, \"Content-Type\")) {\n \t\tlen = strlen(\"Content-Type: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\t\tstrbuf_addstr(&sb, line->buf + len);\n \t\tdecode_header(mi, &sb);\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n \tif (cmp_header(line, \"Content-Transfer-Encoding\")) {\n \t\tlen = strlen(\"Content-Transfer-Encoding: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\t\tstrbuf_addstr(&sb, line->buf + len);\n \t\tdecode_header(mi, &sb);\n \t\thandle_content_transfer_encoding(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n \tif (cmp_header(line, \"Message-Id\")) {\n \t\tlen = strlen(\"Message-Id: \");\n-\t\tstrbuf_add(&sb, line->buf + len, line->len - len);\n+\t\tstrbuf_addstr(&sb, line->buf + len);\n \t\tdecode_header(mi, &sb);\n \t\tif (mi->add_message_id)\n \t\t\tmi->message_id = strbuf_detach(&sb, NULL);\n-- \n2.25.0.708.g4c6f45973e\n\n"},{"id":"391548","messageId":"20200211171923.GB2119034@coredump.intra.peff.net","threadId":"52763","inReplyTo":"20200211171649.GB2118476@coredump.intra.peff.net","subject":"[PATCH 2/4] mailinfo: simplify parsing of header values","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:19:23Z","receivedAt":"2020-02-11T17:19:25Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"Our code to parse header values first checks to see if a line starts\nwith a header, and then manually skips past the matched string to find\nthe value. We can do this all in one step by modeling after\nskip_prefix(), which returns a pointer into the string after the\nparsing.\n\nThis lets us remove some repeated strings, and will also enable us to\nparse more flexibly in a future patch.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n mailinfo.c | 40 ++++++++++++++++++++++------------------\n 1 file changed, 22 insertions(+), 18 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 59d5a8b8f3..ee8d05e239 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -346,11 +346,16 @@ static const char *header[MAX_HDR_PARSED] = {\n \t\"From\",\"Subject\",\"Date\",\n };\n \n-static inline int cmp_header(const struct strbuf *line, const char *hdr)\n+static inline int skip_header(const struct strbuf *line, const char *hdr,\n+\t\t\t      const char **outval)\n {\n-\tint len = strlen(hdr);\n-\treturn !strncasecmp(line->buf, hdr, len) && line->len > len &&\n-\t\t\tline->buf[len] == ':' && isspace(line->buf[len + 1]);\n+\tconst char *val;\n+\tif (!skip_iprefix(line->buf, hdr, &val) ||\n+\t    *val++ != ':' ||\n+\t    !isspace(*val++))\n+\t\treturn 0;\n+\t*outval = val;\n+\treturn 1;\n }\n \n static int is_format_patch_separator(const char *line, int len)\n@@ -547,17 +552,18 @@ static int check_header(struct mailinfo *mi,\n \t\t\tconst struct strbuf *line,\n \t\t\tstruct strbuf *hdr_data[], int overwrite)\n {\n-\tint i, ret = 0, len;\n+\tint i, ret = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n+\tconst char *val;\n \n \t/* search for the interesting parts */\n \tfor (i = 0; header[i]; i++) {\n-\t\tint len = strlen(header[i]);\n-\t\tif ((!hdr_data[i] || overwrite) && cmp_header(line, header[i])) {\n+\t\tif ((!hdr_data[i] || overwrite) &&\n+\t\t    skip_header(line, header[i], &val)) {\n \t\t\t/* Unwrap inline B and Q encoding, and optionally\n \t\t\t * normalize the meta information to utf8.\n \t\t\t */\n-\t\t\tstrbuf_addstr(&sb, line->buf + len + 2);\n+\t\t\tstrbuf_addstr(&sb, val);\n \t\t\tdecode_header(mi, &sb);\n \t\t\thandle_header(&hdr_data[i], &sb);\n \t\t\tret = 1;\n@@ -566,25 +572,22 @@ static int check_header(struct mailinfo *mi,\n \t}\n \n \t/* Content stuff */\n-\tif (cmp_header(line, \"Content-Type\")) {\n-\t\tlen = strlen(\"Content-Type: \");\n-\t\tstrbuf_addstr(&sb, line->buf + len);\n+\tif (skip_header(line, \"Content-Type\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (cmp_header(line, \"Content-Transfer-Encoding\")) {\n-\t\tlen = strlen(\"Content-Transfer-Encoding: \");\n-\t\tstrbuf_addstr(&sb, line->buf + len);\n+\tif (skip_header(line, \"Content-Transfer-Encoding\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n \t\thandle_content_transfer_encoding(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (cmp_header(line, \"Message-Id\")) {\n-\t\tlen = strlen(\"Message-Id: \");\n-\t\tstrbuf_addstr(&sb, line->buf + len);\n+\tif (skip_header(line, \"Message-Id\", &val)) {\n+\t\tstrbuf_addstr(&sb, val);\n \t\tdecode_header(mi, &sb);\n \t\tif (mi->add_message_id)\n \t\t\tmi->message_id = strbuf_detach(&sb, NULL);\n@@ -606,8 +609,9 @@ static int is_inbody_header(const struct mailinfo *mi,\n \t\t\t    const struct strbuf *line)\n {\n \tint i;\n+\tconst char *val;\n \tfor (i = 0; header[i]; i++)\n-\t\tif (!mi->s_hdr_data[i] && cmp_header(line, header[i]))\n+\t\tif (!mi->s_hdr_data[i] && skip_header(line, header[i], &val))\n \t\t\treturn 1;\n \treturn 0;\n }\n-- \n2.25.0.708.g4c6f45973e\n\n"},{"id":"391549","messageId":"20200211171953.GC2119034@coredump.intra.peff.net","threadId":"52763","inReplyTo":"20200211171649.GB2118476@coredump.intra.peff.net","subject":"[PATCH 3/4] mailinfo: be more liberal with header whitespace","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:19:53Z","receivedAt":"2020-02-11T17:19:55Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"RFC822 and friends allow arbitrary whitespace after the colon of a\nheader and before the values. I.e.:\n\n  Subject:foo\n  Subject: foo\n  Subject:  foo\n\nall have the subject \"foo\". But mailinfo requires exactly one space.\nThis doesn't seem to be bothering anybody, but it is pickier than the\nstandard specifies. And we can easily just soak up arbitrary whitespace\nthere in our parser, so let's do so.\n\nNote that the test covers both too little and too much whitespace, but\nthe \"too much\" case already works fine (because we later eat leading and\ntrailing whitespace from the values).\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n mailinfo.c          |  5 +++--\n t/t5100-mailinfo.sh | 15 +++++++++++++++\n 2 files changed, 18 insertions(+), 2 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex ee8d05e239..78f06da524 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -351,9 +351,10 @@ static inline int skip_header(const struct strbuf *line, const char *hdr,\n {\n \tconst char *val;\n \tif (!skip_iprefix(line->buf, hdr, &val) ||\n-\t    *val++ != ':' ||\n-\t    !isspace(*val++))\n+\t    *val++ != ':')\n \t\treturn 0;\n+\twhile (isspace(*val))\n+\t\tval++;\n \t*outval = val;\n \treturn 1;\n }\ndiff --git a/t/t5100-mailinfo.sh b/t/t5100-mailinfo.sh\nindex 9690dcad4f..147e616533 100755\n--- a/t/t5100-mailinfo.sh\n+++ b/t/t5100-mailinfo.sh\n@@ -213,4 +213,19 @@ test_expect_failure 'mailinfo -b separated double [PATCH]' '\n \ttest z\"$subj\" = z\"Subject: [other] message\"\n '\n \n+test_expect_success 'mailinfo handles unusual header whitespace' '\n+\tgit mailinfo /dev/null /dev/null >actual <<-\\EOF &&\n+\tFrom:Real Name <user@example.com>\n+\tSubject:    extra spaces\n+\tEOF\n+\n+\tcat >expect <<-\\EOF &&\n+\tAuthor: Real Name\n+\tEmail: user@example.com\n+\tSubject: extra spaces\n+\n+\tEOF\n+\ttest_cmp expect actual\n+'\n+\n test_done\n-- \n2.25.0.708.g4c6f45973e\n\n"},{"id":"391550","messageId":"20200211172005.GD2119034@coredump.intra.peff.net","threadId":"52763","inReplyTo":"20200211171649.GB2118476@coredump.intra.peff.net","subject":"[PATCH 4/4] mailinfo: factor out some repeated header handling","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2020-02-11T17:20:05Z","receivedAt":"2020-02-11T17:20:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"We do the same thing for each header: match it, copy it to a strbuf, and\ndecode it. Let's put that in a helper function to avoid repetition.\n\nSigned-off-by: Jeff King <peff@peff.net>\n---\n mailinfo.c | 39 +++++++++++++++++++++++----------------\n 1 file changed, 23 insertions(+), 16 deletions(-)\n\ndiff --git a/mailinfo.c b/mailinfo.c\nindex 78f06da524..cf92255515 100644\n--- a/mailinfo.c\n+++ b/mailinfo.c\n@@ -549,47 +549,54 @@ static void decode_header(struct mailinfo *mi, struct strbuf *it)\n \t\tmi->input_error = -1;\n }\n \n+/*\n+ * Returns true if \"line\" contains a header matching \"hdr\", in which case \"val\"\n+ * will contain the value of the header with any RFC2047 B and Q encoding\n+ * unwrapped, and optionally normalize the meta information to utf8.\n+ */\n+static int parse_header(const struct strbuf *line,\n+\t\t\tconst char *hdr,\n+\t\t\tstruct mailinfo *mi,\n+\t\t\tstruct strbuf *val)\n+{\n+\tconst char *val_str;\n+\n+\tif (!skip_header(line, hdr, &val_str))\n+\t\treturn 0;\n+\tstrbuf_addstr(val, val_str);\n+\tdecode_header(mi, val);\n+\treturn 1;\n+}\n+\n static int check_header(struct mailinfo *mi,\n \t\t\tconst struct strbuf *line,\n \t\t\tstruct strbuf *hdr_data[], int overwrite)\n {\n \tint i, ret = 0;\n \tstruct strbuf sb = STRBUF_INIT;\n-\tconst char *val;\n \n \t/* search for the interesting parts */\n \tfor (i = 0; header[i]; i++) {\n \t\tif ((!hdr_data[i] || overwrite) &&\n-\t\t    skip_header(line, header[i], &val)) {\n-\t\t\t/* Unwrap inline B and Q encoding, and optionally\n-\t\t\t * normalize the meta information to utf8.\n-\t\t\t */\n-\t\t\tstrbuf_addstr(&sb, val);\n-\t\t\tdecode_header(mi, &sb);\n+\t\t    parse_header(line, header[i], mi, &sb)) {\n \t\t\thandle_header(&hdr_data[i], &sb);\n \t\t\tret = 1;\n \t\t\tgoto check_header_out;\n \t\t}\n \t}\n \n \t/* Content stuff */\n-\tif (skip_header(line, \"Content-Type\", &val)) {\n-\t\tstrbuf_addstr(&sb, val);\n-\t\tdecode_header(mi, &sb);\n+\tif (parse_header(line, \"Content-Type\", mi, &sb)) {\n \t\thandle_content_type(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (skip_header(line, \"Content-Transfer-Encoding\", &val)) {\n-\t\tstrbuf_addstr(&sb, val);\n-\t\tdecode_header(mi, &sb);\n+\tif (parse_header(line, \"Content-Transfer-Encoding\", mi, &sb)) {\n \t\thandle_content_transfer_encoding(mi, &sb);\n \t\tret = 1;\n \t\tgoto check_header_out;\n \t}\n-\tif (skip_header(line, \"Message-Id\", &val)) {\n-\t\tstrbuf_addstr(&sb, val);\n-\t\tdecode_header(mi, &sb);\n+\tif (parse_header(line, \"Message-Id\", mi, &sb)) {\n \t\tif (mi->add_message_id)\n \t\t\tmi->message_id = strbuf_detach(&sb, NULL);\n \t\tret = 1;\n-- \n2.25.0.708.g4c6f45973e\n"},{"id":"391551","messageId":"CAPig+cSOWaRij1mfC+LO=KGqmF+KNcTMsBQ331q9fkJdMdY-_Q@mail.gmail.com","threadId":"52763","inReplyTo":"20200211171852.GA2119034@coredump.intra.peff.net","subject":"Re: [PATCH 1/4] mailinfo: treat header values as C strings","fromName":"Eric Sunshine","fromEmail":"sunshine@sunshineco.com","sentAt":"2020-02-11T17:26:58Z","receivedAt":"2020-02-11T17:27:14Z","isPatch":true,"sender":{"key":"sunshine@sunshineco.com","avatar":"https://avatars.githubusercontent.com/u/163641?v=4"},"body":"On Tue, Feb 11, 2020 at 12:18 PM Jeff King <peff@peff.net> wrote:\n> We read each header line into a strbuf, which means that we could\n> in theory handle header values with embedded NUL bytes. But in practice,\n> the values we parse out are passed to decode_header(), which uses\n> strstr(), strchr(), etc. And we would not expect such bytes anyway; they\n> are forbidden by RFC822, etc and any non-ASCII characters should be\n\ns/etc/etc./\n\n> encoded with RFC2047 encoding.\n>\n> So let's switch to using strbuf_addstr(), which saves us some length\n> computations (and will enable further cleanups in this code).\n>\n> Signed-off-by: Jeff King <peff@peff.net>\n"}]}