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

From: René Scharfe <l.s.r@web.de>
Date: Jun 20, 2023, 16:25 UTC
Message-ID: <fe0db8cc-4d1b-3108-1d36-939b01a95244@web.de>
In-Reply-To: <ZJB98Q6NHbK6D1uB@nand.local>

Am 19.06.23 um 18:10 schrieb Taylor Blau:

Show 17 quoted lines

> 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.

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.

René