Eric Sunshine [off-list ref] writes:
quoted
+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.
On Tue, Sep 15, 2015 at 06:27:49PM -0700, Junio C Hamano wrote:
Eric Sunshine [off-list ref] writes:
quoted
quoted
+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.
Right. I think what the function is doing is the right thing (and
certainly it matches what the callers I'm changing are doing already
:) ).
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
On Wed, Sep 16, 2015 at 05:57:41AM -0400, Jeff King wrote:
On Tue, Sep 15, 2015 at 06:27:49PM -0700, Junio C Hamano wrote:
quoted
Eric Sunshine [off-list ref] writes:
quoted
quoted
+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.
Right. I think what the function is doing is the right thing (and
certainly it matches what the callers I'm changing are doing already
:) ).
But I agree the docstring is extremely misleading. I've changed it to:
+/**
+ * "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.
+ */
Sounds better, thanks.