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

From: Eric Sunshine <sunshine@sunshineco.com>
Date: Sep 16, 2015, 00:45 UTC
Message-ID: <CAPig+cT9piy2dGx6jbcQNyzY5kQ1XgaEB_mYNUOYBUCJ5wAc_w@mail.gmail.com>
In-Reply-To: <20150915152528.GG29753@sigill.intra.peff.net>

On Tue, Sep 15, 2015 at 11:25 AM, Jeff King <peff@peff.net> wrote:

> The strbuf_complete_line function make sure that a buffer

s/make/makes/

Show 16 quoted lines

> 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 <peff@peff.net>
> ---
> +/**
> + * 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.

Show 10 quoted lines

> +
> +/**
> + * 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');
>  }