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

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

From
Jeff King <peff@peff.net>
Date
Feb 10, 2020, 23:44 UTC
Message-ID
<20200210234427.GA632160@coredump.intra.peff.net>
In-Reply-To
<CAPig+cQdJ0NJSWZN-2ckeLB7RfU9GZ7LGvVX4y+Q1daPnW8WsA@mail.gmail.com>
On Sun, Feb 09, 2020 at 12:36:22PM -0500, Eric Sunshine wrote:
Show 23 quoted lines
> On Sun, Feb 9, 2020 at 8:45 AM René Scharfe <l.s.r@web.de> wrote:
> > 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.
> >
> > Signed-off-by: René Scharfe <l.s.r@web.de>
> > ---
> > diff --git 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);
> 
> Meh. We've already computed the length of "Content-Type: " a few lines
> earlier, so taking advantage of that value when inserting the string
> literal is perfectly sensible. Thus, I'm not convinced that this
> change is an improvement.

I had a similar thought. I kind of wonder if all of these "len" bits (and their repeated strings) could go away if cmp_header() just used skip_iprefix() under the hood and had a pointer out-parameter.

Something like the (largely untested) patch below. That would also make it easy to support arbitrary amounts of whitespace after the header, which I think are allowed by the standard (whereas now we'd parse "Content-type: text/plain" as " text/plain", which is silly).

Worth doing?
---
diff --git a/mailinfo.c b/mailinfo.c
index b395adbdf2..bbb5b429f8 100644
--- a/mailinfo.c
+++ b/mailinfo.c
@@ -346,11 +346,16 @@ static const char *header[MAX_HDR_PARSED] = {
 	"From","Subject","Date",
 };
 
-static inline int cmp_header(const struct strbuf *line, const char *hdr)
+static inline int cmp_header(const struct strbuf *line, const char *hdr,
+			     const char **outval)
 {
-	int len = strlen(hdr);
-	return !strncasecmp(line->buf, hdr, len) && line->len > len &&
-			line->buf[len] == ':' && isspace(line->buf[len + 1]);
+	const char *val;
+	if (!skip_iprefix(line->buf, hdr, &val) ||
+	    *val++ != ':' ||
+	    !isspace(*val++))
+		return 0;
+	*outval = val;
+	return 1;
 }
 
 static int is_format_patch_separator(const char *line, int len)
@@ -547,17 +552,17 @@ static int check_header(struct mailinfo *mi,
 			const struct strbuf *line,
 			struct strbuf *hdr_data[], int overwrite)
 {
-	int i, ret = 0, len;
+	int i, ret = 0;
 	struct strbuf sb = STRBUF_INIT;
+	const char *val;
 
 	/* search for the interesting parts */
 	for (i = 0; header[i]; i++) {
-		int len = strlen(header[i]);
-		if ((!hdr_data[i] || overwrite) && cmp_header(line, header[i])) {
+		if ((!hdr_data[i] || overwrite) && cmp_header(line, header[i], &val)) {
 			/* Unwrap inline B and Q encoding, and optionally
 			 * normalize the meta information to utf8.
 			 */
-			strbuf_add(&sb, line->buf + len + 2, line->len - len - 2);
+			strbuf_addstr(&sb, val);
 			decode_header(mi, &sb);
 			handle_header(&hdr_data[i], &sb);
 			ret = 1;
@@ -566,26 +571,22 @@ static int check_header(struct mailinfo *mi,
 	}
 
 	/* Content stuff */
-	if (cmp_header(line, "Content-Type")) {
-		len = strlen("Content-Type: ");
-		strbuf_add(&sb, line->buf + len, line->len - len);
+	if (cmp_header(line, "Content-Type", &val)) {
+		strbuf_addstr(&sb, val);
 		decode_header(mi, &sb);
-		strbuf_insert(&sb, 0, "Content-Type: ", len);
 		handle_content_type(mi, &sb);
 		ret = 1;
 		goto check_header_out;
 	}
-	if (cmp_header(line, "Content-Transfer-Encoding")) {
-		len = strlen("Content-Transfer-Encoding: ");
-		strbuf_add(&sb, line->buf + len, line->len - len);
+	if (cmp_header(line, "Content-Transfer-Encoding", &val)) {
+		strbuf_addstr(&sb, val);
 		decode_header(mi, &sb);
 		handle_content_transfer_encoding(mi, &sb);
 		ret = 1;
 		goto check_header_out;
 	}
-	if (cmp_header(line, "Message-Id")) {
-		len = strlen("Message-Id: ");
-		strbuf_add(&sb, line->buf + len, line->len - len);
+	if (cmp_header(line, "Message-Id", &val)) {
+		strbuf_addstr(&sb, val);
 		decode_header(mi, &sb);
 		if (mi->add_message_id)
 			mi->message_id = strbuf_detach(&sb, NULL);
@@ -607,8 +608,9 @@ static int is_inbody_header(const struct mailinfo *mi,
 			    const struct strbuf *line)
 {
 	int i;
+	const char *val;
 	for (i = 0; header[i]; i++)
-		if (!mi->s_hdr_data[i] && cmp_header(line, header[i]))
+		if (!mi->s_hdr_data[i] && cmp_header(line, header[i], &val))
 			return 1;
 	return 0;
 }
Previous: Taylor BlauNext: Junio C Hamano
Message 11 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.