Re: [PATCH 2/5] strbuf: factor out strbuf_expand_step()

[Taylor Blau <me@ttaylorr.com>]

On Tue, Jun 20, 2023 at 06:25:30PM +0200, René Scharfe wrote:
> Am 19.06.23 um 18:10 schrieb Taylor Blau:
> > On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:
> >> diff --git a/strbuf.c b/strbuf.c
> >> index 08eec8f1d8..a90b597da1 100644
> >> --- a/strbuf.c
> >> +++ b/strbuf.c
> >> @@ -415,19 +415,24 @@ void strbuf_vaddf(struct strbuf *sb, const char *fmt, va_list ap)
> >>  	strbuf_setlen(sb, sb->len + len);
> >>  }
> >>
> >> +int strbuf_expand_step(struct strbuf *sb, const char **formatp)
> >> +{
> >> +	const char *format = *formatp;
> >> +	const char *percent = strchrnul(format, '%');
> >
> > Can format be NULL here? Obviously formatp is never NULL since it is
> > `&s`, but we guarded the while loop in the pre-image with `while (*s)`
> > and I am not sure that **formatp is always non-NULL here.
>
> The "*s" in builtin/branch.c::quote_literal_for_format() that you quote
> dereferences "s", so we'd get a segfault if it was NULL.  A NULL (and
> NUL) check would look like "while (s && *s)".  The old code in strbuf.c
> passes the format to strchrnul(), which can't handle NULL either.  So
> no, format must not be NULL here, and this is not a new requirement.

Ah, thanks for catching: I agree with your reasoning.

> But now I noticed that builtin/branch.c::quote_literal_for_format()
> only ever gets called with "" or "remotes/", none of which needs
> quoting.  We could drop this function entirely, and add it back when
> needed, if ever.  But that's out of the scope of this series.

Yes, agreed.

Thanks,
Taylor