Re: [PATCH 3/4] strbuf: introduce `strbuf_attachstr()`
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Apr 20, 2020, 19:39 UTC
- Message-ID
- <xmqqzhb6q74r.fsf@gitster.c.googlers.com>
- In-Reply-To
- <c3012f1da361af354a904f821b83d61f2534ccb2.1587297254.git.martin.agren@gmail.com>
Martin Ågren <martin.agren@gmail.com> writes:
Show 10 quoted lines
> +/**
> + * Attach a string to a buffer similar to `strbuf_attachstr_len()`.
> + * Useful if you do not know the length of the string.
> + */
> +static inline void strbuf_attachstr(struct strbuf *sb, char *str)
> +{
> + size_t len = strlen(str);
> +
> + strbuf_attach(sb, str, len, len + 1);
> +}This is somewhat worrysome in that the interface is _so_ simple that people may fail to see that str must be allocated piece of memory, and it is preferrable if string fully fills the allocation.
We should repeat that (instead of just trusting "similar to ..." would tell them enough) in the doc, perhaps?
Show 13 quoted lines
> diff --git a/trailer.c b/trailer.c
> index 0c414f2fed..56c4027943 100644
> --- a/trailer.c
> +++ b/trailer.c
> @@ -1095,7 +1095,7 @@ void trailer_info_get(struct trailer_info *info, const char *str,
> for (ptr = trailer_lines; *ptr; ptr++) {
> if (last && isspace((*ptr)->buf[0])) {
> struct strbuf sb = STRBUF_INIT;
> - strbuf_attach(&sb, *last, strlen(*last), strlen(*last));
> + strbuf_attachstr(&sb, *last);
> strbuf_addbuf(&sb, *ptr);
> *last = strbuf_detach(&sb, NULL);
> continue;This is not wrong per-se, but it is unclear if use of strbuf_attach* family to avoid an explicit malloc/copy/free is buying much at this callsite. Simplifying the code here of course is not within the scope of this series.