| Message ID | b31c46a8-380b-3528-27a5-a2dddacaf837@web.de (mailing list archive) |
|---|---|
| State | New, archived |
| Headers | show |
| Series | [v2] strbuf: add and use strbuf_insertstr() | expand |
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. Digging deeper, though, I have to wonder why this bothers inserting "Content-Type: " at all. None of the other cases handled by check_header() bother re-inserting the header, so why this one? I thought it might be because handle_content_type() depends upon the header being present, but from my reading, this does not appear to be the case. handle_content_type() calls has_attr_value() and slurp_attr() to examine the incoming line, but neither of those seem to expect any sort of "<Header>: " either. Thus, it appears that the insertion of "Content-Type: " is superfluous. If this is indeed the case, then rather than converting this to strbuf_insertstr(), I could see it being pulled out into a separate patch which merely removes the strbuf_insert() call.
Am 09.02.20 um 18:36 schrieb Eric Sunshine: > 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. Well, yes, but it would be more sensible if we'd have only a single string here. At the source code level we have two string constants that happen to have the same contents. Handling them separately is reasonable, I think. The compiler is merging those two, and resolves the other strlen() call at compile time, so the generated code is the same. > Thus, I'm not convinced that this change is an improvement. The improvement is to untangle the handling of those two string constants and to use a C string without having to pass along its length. That doesn't make the code clean, yet, admittedly. > Digging deeper, though, I have to wonder why this bothers inserting > "Content-Type: " at all. None of the other cases handled by > check_header() bother re-inserting the header, so why this one? I > thought it might be because handle_content_type() depends upon the > header being present, but from my reading, this does not appear to be > the case. handle_content_type() calls has_attr_value() and > slurp_attr() to examine the incoming line, but neither of those seem > to expect any sort of "<Header>: " either. Thus, it appears that the > insertion of "Content-Type: " is superfluous. If this is indeed the > case, then rather than converting this to strbuf_insertstr(), I could > see it being pulled out into a separate patch which merely removes the > strbuf_insert() call. Interesting. It makes sense that handle_content_type() wouldn't need such a header prefix -- it's only called if its existence in the line has been confirmed. And I also don't see a hint in the code that would justify the insertion. Do you care to send a follow-up patch (or one against master if you're not convinced by my reasoning given above)? Thanks, René
On Sun, Feb 9, 2020 at 1:28 PM René Scharfe <l.s.r@web.de> wrote: > Am 09.02.20 um 18:36 schrieb Eric Sunshine: > > Digging deeper, though, I have to wonder why this bothers inserting > > "Content-Type: " at all. [...] > > Do you care to send a follow-up patch (or one against master if you're > not convinced by my reasoning given above)? It's unlikely I will get to it any time soon. I'm still sitting on unfinished patches I started 1.5 years ago and have a queue of "git worktree"-related tasks on my To-Do list.
On Sun, Feb 09, 2020 at 07:28:31PM +0100, René Scharfe wrote: > Am 09.02.20 um 18:36 schrieb Eric Sunshine: > > 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. > > Well, yes, but it would be more sensible if we'd have only a single > string here. At the source code level we have two string constants that > happen to have the same contents. Handling them separately is > reasonable, I think. > > The compiler is merging those two, and resolves the other strlen() call > at compile time, so the generated code is the same. Yes, if 'strbuf_insertstr' weren't inlined, I'd be less eager to make this suggestion, but since it *is* inlined, I don't think that the compiler will generate substantially different instructions whether we use one or the other here. > > Thus, I'm not convinced that this change is an improvement. > > The improvement is to untangle the handling of those two string > constants and to use a C string without having to pass along its > length. That doesn't make the code clean, yet, admittedly. Agreed. > > Digging deeper, though, I have to wonder why this bothers inserting > > "Content-Type: " at all. None of the other cases handled by > > check_header() bother re-inserting the header, so why this one? I > > thought it might be because handle_content_type() depends upon the > > header being present, but from my reading, this does not appear to be > > the case. handle_content_type() calls has_attr_value() and > > slurp_attr() to examine the incoming line, but neither of those seem > > to expect any sort of "<Header>: " either. Thus, it appears that the > > insertion of "Content-Type: " is superfluous. If this is indeed the > > case, then rather than converting this to strbuf_insertstr(), I could > > see it being pulled out into a separate patch which merely removes the > > strbuf_insert() call. > > Interesting. It makes sense that handle_content_type() wouldn't need > such a header prefix -- it's only called if its existence in the line > has been confirmed. And I also don't see a hint in the code that > would justify the insertion. > > Do you care to send a follow-up patch (or one against master if you're > not convinced by my reasoning given above)? I certainly can't speak for Eric, but for my $.02 I don't think that it's worth holding this series up. This seems like a separate issue to me, and I'd rather it not get get in the way of a perfectly good patch in the meantime. For now, this increases the churn a little bit, but that is the price we have to pay for the new 'strbuf_insertstr' to be applied/used consistently. I'd be happy to see this go further, but I'd be just as happy to stop where we're at. > Thanks, > René Thanks, Taylor
On Sun, Feb 09, 2020 at 12:36:22PM -0500, Eric Sunshine wrote: > 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; }
Jeff King <peff@peff.net> writes: > 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? The result does look cleaner. I do not think we've seen much polishing in this area of the code for quite a long time, which might indicate that what we have may be good enough, but at the same time it would mean it is quiescent time for the code and it is safe to clean it up.
Am 11.02.20 um 00:44 schrieb Jeff King: > On Sun, Feb 09, 2020 at 12:36:22PM -0500, Eric Sunshine wrote: > >> 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? Sure. > --- > 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; > } And you could rename it to skip_header() to fix the issue that its name starts with cmp but its return value is the inverse of a cmp-style function. And it could take a char pointer instead of a strbuf, to reduce its dependencies and make it more widely useful -- but that might also be a case of YAGNI. > > 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); That assumes the header value never contains NULs. Valid? > 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); The repeated sequence cmp_header()+strbuf_add{,str}()+decode_header() makes me itchy. > @@ -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; > } >
On Tue, Feb 11, 2020 at 05:18:02PM +0100, René Scharfe wrote: > And you could rename it to skip_header() to fix the issue that its name > starts with cmp but its return value is the inverse of a cmp-style > function. Good suggestion. > And it could take a char pointer instead of a strbuf, to reduce its > dependencies and make it more widely useful -- but that might also be > a case of YAGNI. I didn't take this one because all of the callers have strbuf, and it saves them from dereferencing. > > - strbuf_add(&sb, line->buf + len + 2, line->len - len - 2); > > + strbuf_addstr(&sb, val); > > That assumes the header value never contains NULs. Valid? I think so, but I pulled it out to a separate patch so that it could be argued for explicitly. > The repeated sequence cmp_header()+strbuf_add{,str}()+decode_header() > makes me itchy. Yeah, it's ugly and I factored it out in a new patch. But the boilerplate and docstring ends up longer than the savings. ;) -Peff
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..543962d40c 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; @@ -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; diff --git a/notes-utils.c b/notes-utils.c index a819410698..4bf4888d8c 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: "); 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.
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. Helped-by: Taylor Blau <me@ttaylorr.com> Helped-by: Eric Sunshine <sunshine@sunshineco.com> Signed-off-by: René Scharfe <l.s.r@web.de> --- Changes: - Added second hunk in mailinfo.c. - Removed outdated comment in notes-util.c. builtin/checkout.c | 2 +- builtin/notes.c | 4 ++-- builtin/sparse-checkout.c | 2 +- commit.c | 2 +- config.c | 2 +- http.c | 4 ++-- mailinfo.c | 4 ++-- notes-utils.c | 2 +- notes.c | 4 ++-- pretty.c | 4 ++-- strbuf.h | 12 ++++++++++++ 11 files changed, 27 insertions(+), 15 deletions(-) -- 2.25.0