Re: [PATCH 3/4] strbuf: introduce `strbuf_attachstr()`

["Martin Ågren" <martin.agren@gmail.com>]

On Mon, 20 Apr 2020 at 21:39, Junio C Hamano <gitster@pobox.com> wrote:
>
> Martin Ågren <martin.agren@gmail.com> 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