Eric Sunshine <sunsh...@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.
>
> 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.
I would expect two different scenarios for which this function would
be useful.
One is when dealing with a text file and want to avoid incomplete
lines at the end. In this scenario, an empty file with zero lines
should be left as-is, instead of getting turned into a file with one
empty line. "Leave the empty input as-is" is the behaviour the
callers want.
The other is when you are given a directory name in the strbuf, you
have a name of a file you want to be in that directory, and want to
have the full path to the file in the strbuf. In this scenario,
what does it mean for the caller to give you an empty "directory
name"? I think at least in our codebase, that almost always would
mean that "the path is relative to $CWD", i.e. you would want to see
the "complete" to leave the input intact and then append the
filename there.
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.
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majord...@vger.kernel.org
More majordomo info at http://vger.kernel.org/majordomo-info.html
