Re: [PATCH 07/67] strbuf: make strbuf_complete_line more generic

On Tue, Sep 15, 2015 at 11:25 AM, Jeff King [off-list ref] wrote:

The strbuf_complete_line function make sure that a buffer

s/make/makes/

ends in a newline. But we may want to do this for any
character (e.g., "/" on the end of a path). Let's factor out
a generic version, and keep strbuf_complete_line as a thin
wrapper.

Signed-off-by: Jeff King <redacted>
---
+/**
+ * Ensure that `sb` ends with the character `term`, if it does not
+ * already.
+ */
+static inline void strbuf_complete(struct strbuf *sb, char term)
+{
+       if (sb->len && sb->buf[sb->len - 1] != term)
+               strbuf_addch(sb, term);
+}

Hmm, so this only adds 'term' if not already present *and* if 'sb' is
not empty, which doesn't seem to match the documentation which says
that it "ensures" termination.

But, is that reasonable behavior? Intuitively, I'd expect 'term' to be
added when 'sb' is empty:

    if (!sb->len || sb->buf[sb->len - 1] != term)
        strbuf_addch(sb, term);

strbuf_complete_line()'s existing behavior of not adding '\n' to an
empty string may have been intentional, but actually smells like a
bug.

+
+/**
+ * Ensure that `sb` ends with a newline.
+ */
 static inline void strbuf_complete_line(struct strbuf *sb)
 {
-       if (sb->len && sb->buf[sb->len - 1] != '\n')
-               strbuf_addch(sb, '\n');
+       strbuf_complete(sb, '\n');
 }