On Tue, Jun 20, 2023 at 06:25:30PM +0200, René Scharfe wrote:

Am 19.06.23 um 18:10 schrieb Taylor Blau:

quoted

On Sat, Jun 17, 2023 at 10:41:44PM +0200, René Scharfe wrote:

quoted

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