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

From

Jeff King <peff@peff.net>

Date

Sep 16, 2015, 09:57 UTC

Message-ID

<20150916095740.GC13966@sigill.intra.peff.net>

In-Reply-To

<xmqqoah3tap6.fsf@gitster.mtv.corp.google.com>

On Tue, Sep 15, 2015 at 06:27:49PM -0700, Junio C Hamano wrote:

> Eric Sunshine <sunshine@sunshineco.com> writes:
> 
> >> +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.
> [...]
> So to these two plausible and different set of callers that would be
> helped by this function, the behaviour Peff gives it would match
> what the callers want better than your version.

But I agree the docstring is extremely misleading. I've changed it to:

diff --git a/strbuf.h b/strbuf.h
index aef2794..43f27c3 100644
--- a/strbuf.h
+++ b/strbuf.h
@@ -491,10 +491,21 @@ extern void strbuf_add_lines(struct strbuf *sb, const char *prefix, const char *
  */
 extern void strbuf_addstr_xml_quoted(struct strbuf *sb, const char *s);
 
+/**
+ * "Complete" the contents of `sb` by ensuring that either it ends with the
+ * character `term`, or it is empty.  This can be used, for example,
+ * to ensure that text ends with a newline, but without creating an empty
+ * blank line if there is no content in the first place.
+ */
+static inline void strbuf_complete(struct strbuf *sb, char term)
+{
+	if (sb->len && sb->buf[sb->len - 1] != term)
+		strbuf_addch(sb, term);
+}
+
 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');
 }
 
 extern int strbuf_branchname(struct strbuf *sb, const char *name);


It may be that we will not find other uses beyond completing slash and
newline, but at least this version should not actively mislead anybody.

-Peff