On Mon, 20 Apr 2020 at 21:39, Junio C Hamano <gitster@xxxxxxxxx> wrote:
>
> Martin Ågren <martin.agren@xxxxxxxxx> writes:
>
> > +/**
> > + * 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?
Yeah, that's a good point. I'll expand on this to try to better get
through that there are things to consider here.
> > @@ -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.
For the other patches in this series, I spent some time and effort
investigating where strings came from, "do I really feel certain that
they're NUL-terminated?". But for this patch, I more or less went "we've
been using strlen on this all this time, surely if it wasn't guaranteed
to be NUL-terminated we'd have messed up already". And I don't think I'm
making anything worse. But yeah, I didn't really step back to look at
what these sites are really doing, and how, as much as I did for the
others.
Martin
