git/list[1] front-page[2] threads[3] people[4] search[5] about
 

Re: [PATCH] strbuf: add and use strbuf_insertstr()

From
Taylor Blau <me@ttaylorr.com>
Date
Feb 8, 2020, 23:08 UTC
Message-ID
<20200208230801.GA33529@syl.local>
In-Reply-To
<019be197-e0aa-1234-e16f-6561d8340023@web.de>
Hi René,
On Sat, Feb 08, 2020 at 08:56:43PM +0100, René Scharfe wrote:
Show 6 quoted lines
> Add a function for inserting a C string into a strbuf.  Use it
> throughout the source to get rid of magic string length constants and
> explicit strlen() calls.
>
> Like strbuf_addstr(), implement it as an inline function to avoid the
> implicit strlen() calls to cause runtime overhead.

This all looks quite reasonable to me. Did you have a specific motivation in mind when writing this patch, other than to get rid of having to call strlen on the same argument like:

  strbuf_insert(&buf, pos, x, strlen(x))
? Not that this needs more motivation than that, I'm just curious.

I looked through 'git grep strbuf_insert(', and only noticed one spot that I could be updated with 'strbuf_insertstr' in mailinfo.c:

diff --git a/mailinfo.c b/mailinfo.c
index c535dec2e9..543962d40c 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -570,7 +570,7 @@ static int check_header(struct mailinfo *mi,
    len = strlen("Content-Type: ");
    strbuf_add(&sb, line->buf + len, line->len - len);
    decode_header(mi, &sb);
-		strbuf_insert(&sb, 0, "Content-Type: ", len);
+		strbuf_insertstr(&sb, 0, "Content-Type: ");
    handle_content_type(mi, &sb);
    ret = 1;
    goto check_header_out;

Otherwise this looks quite good to me.

>
> Signed-off-by: René Scharfe <l.s.r@web.de>
> ---
>  builtin/checkout.c        |  2 +-
>  builtin/notes.c           |  4 ++--
>  builtin/sparse-checkout.c |  2 +-
>  commit.c                  |  2 +-
>  config.c                  |  2 +-
>  http.c                    |  4 ++--
>  mailinfo.c                |  2 +-
>  notes-utils.c             |  2 +-
>  notes.c                   |  4 ++--
>  pretty.c                  |  4 ++--
>  strbuf.h                  | 12 ++++++++++++
>  11 files changed, 26 insertions(+), 14 deletions(-)
>
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index fc2eb1befc..d6773818b8 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -865,7 +865,7 @@ static void update_refs_for_switch(const struct checkout_opts *opts,
>  		strbuf_addf(&msg, "checkout: moving from %s to %s",
>  			old_desc ? old_desc : "(invalid)", new_branch_info->name);
>  	else
> -		strbuf_insert(&msg, 0, reflog_msg, strlen(reflog_msg));
> +		strbuf_insertstr(&msg, 0, reflog_msg);
>
>  	if (!strcmp(new_branch_info->name, "HEAD") && !new_branch_info->path && !opts->force_detach) {
>  		/* Nothing to do. */
> diff --git a/builtin/notes.c b/builtin/notes.c
> index 95456f3165..35e468ea2d 100644
> --- a/builtin/notes.c
> +++ b/builtin/notes.c
> @@ -622,7 +622,7 @@ static int append_edit(int argc, const char **argv, const char *prefix)
>
>  		strbuf_grow(&d.buf, size + 1);
>  		if (d.buf.len && prev_buf && size)
> -			strbuf_insert(&d.buf, 0, "\n", 1);
> +			strbuf_insertstr(&d.buf, 0, "\n");
>  		if (prev_buf && size)
>  			strbuf_insert(&d.buf, 0, prev_buf, size);
>  		free(prev_buf);
> @@ -745,7 +745,7 @@ static int merge_commit(struct notes_merge_options *o)
>  	memset(&pretty_ctx, 0, sizeof(pretty_ctx));
>  	format_commit_message(partial, "%s", &msg, &pretty_ctx);
>  	strbuf_trim(&msg);
> -	strbuf_insert(&msg, 0, "notes: ", 7);
> +	strbuf_insertstr(&msg, 0, "notes: ");
>  	update_ref(msg.buf, o->local_ref, &oid,
>  		   is_null_oid(&parent_oid) ? NULL : &parent_oid,
>  		   0, UPDATE_REFS_DIE_ON_ERR);
> diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
> index b3bed891cb..38d0d944b3 100644
> --- a/builtin/sparse-checkout.c
> +++ b/builtin/sparse-checkout.c
> @@ -373,7 +373,7 @@ static void strbuf_to_cone_pattern(struct strbuf *line, struct pattern_list *pl)
>  		return;
>
>  	if (line->buf[0] != '/')
> -		strbuf_insert(line, 0, "/", 1);
> +		strbuf_insertstr(line, 0, "/");
>
>  	insert_recursive_pattern(pl, line);
>  }
> diff --git a/commit.c b/commit.c
> index 3f91d3efc5..a6cfa41a4e 100644
> --- a/commit.c
> +++ b/commit.c
> @@ -993,7 +993,7 @@ static int do_sign_commit(struct strbuf *buf, const char *keyid)
>  			strbuf_insert(buf, inspos, gpg_sig_header, gpg_sig_header_len);
>  			inspos += gpg_sig_header_len;
>  		}
> -		strbuf_insert(buf, inspos++, " ", 1);
> +		strbuf_insertstr(buf, inspos++, " ");
>  		strbuf_insert(buf, inspos, bol, len);
>  		inspos += len;
>  		copypos += len;
> diff --git a/config.c b/config.c
> index d75f88ca0c..b386c0c628 100644
> --- a/config.c
> +++ b/config.c
> @@ -204,7 +204,7 @@ static int prepare_include_condition_pattern(struct strbuf *pat)
>  		strbuf_splice(pat, 0, 1, path.buf, slash - path.buf);
>  		prefix = slash - path.buf + 1 /* slash */;
>  	} else if (!is_absolute_path(pat->buf))
> -		strbuf_insert(pat, 0, "**/", 3);
> +		strbuf_insertstr(pat, 0, "**/");
>
>  	add_trailing_starstar_for_dir(pat);
>
> diff --git a/http.c b/http.c
> index 5f348169c3..00a0e50763 100644
> --- a/http.c
> +++ b/http.c
> @@ -680,8 +680,8 @@ static void curl_dump_header(const char *text, unsigned char *ptr, size_t size,
>  	for (header = headers; *header; header++) {
>  		if (hide_sensitive_header)
>  			redact_sensitive_header(*header);
> -		strbuf_insert((*header), 0, text, strlen(text));
> -		strbuf_insert((*header), strlen(text), ": ", 2);
> +		strbuf_insertstr((*header), 0, text);
> +		strbuf_insertstr((*header), strlen(text), ": ");
>  		strbuf_rtrim((*header));
>  		strbuf_addch((*header), '\n');
>  		trace_strbuf(&trace_curl, (*header));
> diff --git a/mailinfo.c b/mailinfo.c
> index b395adbdf2..c535dec2e9 100644
> --- a/mailinfo.c
> +++ b/mailinfo.c
> @@ -254,7 +254,7 @@ static void handle_content_type(struct mailinfo *mi, struct strbuf *line)
>  	mi->delsp = has_attr_value(line->buf, "delsp=", "yes");
>
>  	if (slurp_attr(line->buf, "boundary=", boundary)) {
> -		strbuf_insert(boundary, 0, "--", 2);
> +		strbuf_insertstr(boundary, 0, "--");
>  		if (++mi->content_top >= &mi->content[MAX_BOUNDARIES]) {
>  			error("Too many boundaries to handle");
>  			mi->input_error = -1;
> diff --git a/notes-utils.c b/notes-utils.c
> index a819410698..5f94c513d4 100644
> --- a/notes-utils.c
> +++ b/notes-utils.c
> @@ -52,7 +52,7 @@ void commit_notes(struct repository *r, struct notes_tree *t, const char *msg)
>  	strbuf_complete_line(&buf);
>
>  	create_notes_commit(r, t, NULL, buf.buf, buf.len, &commit_oid);
> -	strbuf_insert(&buf, 0, "notes: ", 7); /* commit message starts at index 7 */
> +	strbuf_insertstr(&buf, 0, "notes: "); /* commit message starts at index 7 */
>  	update_ref(buf.buf, t->update_ref, &commit_oid, NULL, 0,
>  		   UPDATE_REFS_DIE_ON_ERR);
>
> diff --git a/notes.c b/notes.c
> index 0c79964c26..a24af53de6 100644
> --- a/notes.c
> +++ b/notes.c
> @@ -1332,9 +1332,9 @@ void expand_notes_ref(struct strbuf *sb)
>  	if (starts_with(sb->buf, "refs/notes/"))
>  		return; /* we're happy */
>  	else if (starts_with(sb->buf, "notes/"))
> -		strbuf_insert(sb, 0, "refs/", 5);
> +		strbuf_insertstr(sb, 0, "refs/");
>  	else
> -		strbuf_insert(sb, 0, "refs/notes/", 11);
> +		strbuf_insertstr(sb, 0, "refs/notes/");
>  }
>
>  void expand_loose_notes_ref(struct strbuf *sb)
> diff --git a/pretty.c b/pretty.c
> index f5fbbc5ffb..28afc701b6 100644
> --- a/pretty.c
> +++ b/pretty.c
> @@ -1609,9 +1609,9 @@ static size_t format_commit_item(struct strbuf *sb, /* in UTF-8 */
>  			strbuf_setlen(sb, sb->len - 1);
>  	} else if (orig_len != sb->len) {
>  		if (magic == ADD_LF_BEFORE_NON_EMPTY)
> -			strbuf_insert(sb, orig_len, "\n", 1);
> +			strbuf_insertstr(sb, orig_len, "\n");
>  		else if (magic == ADD_SP_BEFORE_NON_EMPTY)
> -			strbuf_insert(sb, orig_len, " ", 1);
> +			strbuf_insertstr(sb, orig_len, " ");
>  	}
>  	return consumed + 1;
>  }
> diff --git a/strbuf.h b/strbuf.h
> index bfa66569a4..aae7ac3a82 100644
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -244,6 +244,18 @@ void strbuf_addchars(struct strbuf *sb, int c, size_t n);
>   */
>  void strbuf_insert(struct strbuf *sb, size_t pos, const void *, size_t);
>
> +/**
> + * Insert a NUL-terminated string to the given position of the buffer.
> + * The remaining contents will be shifted, not overwritten.  It's an
> + * inline function to allow the compiler to resolve strlen() calls on
> + * constants at compile time.
> + */
> +static inline void strbuf_insertstr(struct strbuf *sb, size_t pos,
> +				    const char *s)
> +{
> +	strbuf_insert(sb, pos, s, strlen(s));
> +}
> +
>  /**
>   * Insert data to the given position of the buffer giving a printf format
>   * string. The contents will be shifted, not overwritten.
> --
> 2.25.0
>

Thanks,
Taylor
Previous: René ScharfeNext: René Scharfe
Message 2 of 23 in “strbuf: add and use strbuf_insertstr()”
  1. strbuf: add and use strbuf_insertstr()René Scharfe, Feb 8, 2020
  2. Taylor BlauFeb 8, 2020
  3. René ScharfeFeb 9, 2020
  4. Eric SunshineFeb 9, 2020
  5. René ScharfeFeb 9, 2020
  6. strbuf: add and use strbuf_insertstr()René Scharfe, Feb 9, 2020
  7. Eric SunshineFeb 9, 2020
  8. René ScharfeFeb 9, 2020
  9. Eric SunshineFeb 9, 2020
  10. Taylor BlauFeb 9, 2020
  11. Jeff KingFeb 10, 2020
  12. Junio C HamanoFeb 11, 2020
  13. 0/4 some more mailinfo cleanupsJeff King, Feb 11, 2020
  14. 1/4 mailinfo: treat header values as C stringsJeff King, Feb 11, 2020
  15. Eric SunshineFeb 11, 2020
  16. 2/4 mailinfo: simplify parsing of header valuesJeff King, Feb 11, 2020
  17. 3/4 mailinfo: be more liberal with header whitespaceJeff King, Feb 11, 2020
  18. 4/4 mailinfo: factor out some repeated header handlingJeff King, Feb 11, 2020
  19. René ScharfeFeb 11, 2020
  20. Jeff KingFeb 11, 2020
  21. 2/1 mailinfo: don't insert header prefix for handle_content_type()René Scharfe, Feb 10, 2020
  22. Junio C HamanoFeb 10, 2020
  23. Taylor BlauFeb 10, 2020

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.