On Tue, Jan 12, 2010 at 10:55:45PM -0800, Junio C Hamano wrote:
> > +void strbuf_percentquote_buf(struct strbuf *dest, struct strbuf *src)
> > +{
>
> Just a style thing, but please call that "dst" to be consistent. You are
> already dropping vowels from the other side to spell it "src".
I personally dislike that spelling, but it certainly is consistent with
the rest of git, so OK.
> I wondered if the function should be just 1-arg that always quotes
> in-place instead, but your [PATCH 3/3] wants to have an appending
> semantics from this function, so changing it to be a 1-arg "in-place
> quoter" will force the caller to run strbuf_addbuf() on the result, which
> is not nice.
Yep. An in-place version would be a bit more complicated to write, and
would make the caller do extra work.
> Since tucking a p-quoted version of the same string to its original
> doesn't make sense at all, perhaps this should:
>
> (0) be renamed to have "append" somewhere in its name;
Yeah, I considered this. To follow the existing naming conventions, the
name should indicate:
1. it's a strbuf function
2. it's appending (and the pattern is to use "add")
3. it's appending a strbuf (and the pattern is to call this "buf")
4. it's percent-quoting
So perhaps following the existing standards, it should be
strbuf_addbuf_percentquote? Long, but I don't think there is any
confusion about what it does (and leaves room for addstr_percentquote).
> (1) mark the src side as const; and
Oops, good catch.
> (2) perhaps have assert(dst != src). The loop won't terminate when
> called with src == dst, I think.
Oops again. I think it is sensible to protect against this. I thought
about trying to make it magically work in-place, but I don't think there
is a simple way to do so. And since I don't actually need to do that, I
think leaving an assert in-place until somebody does need it and wants
to write it is fine.
> --- a/strbuf.h
> +++ b/strbuf.h
> @@ -105,7 +105,13 @@ static inline void strbuf_addstr(struct strbuf *sb, const char *s) {
> strbuf_add(sb, s, strlen(s));
> }
> static inline void strbuf_addbuf(struct strbuf *sb, const struct strbuf *sb2) {
> - strbuf_add(sb, sb2->buf, sb2->len);
> + char *buf = sb2->buf;
> + int len = sb2->len;
> + if (sb->buf == sb2->buf) {
> + strbuf_grow(sb, len);
> + buf = sb->buf;
> + }
> + strbuf_add(sb, buf, len);
> }
Shouldn't this be "if (sb == sb2)"? Two strbufs in the initial state
will point to the same strbuf_slopbuf, but obviously growing sb will not
impact sb2. Though that would simply provoke a false positive, which I
don't think has any negative consequences.
Also, since reallocating sb will reallocate sb2, can't you just write it
safely like this:
strbuf_grow(sb, sb2->len);
strbuf_add(sb, sb2->buf, sb2->len);
The grow will not affect the length of sb2, so that doesn't need to be
saved. And there is no point in deciding whether to point the buf you
pass at sb->buf or sb2->buf. If they are the same, then the grow will
have reallocated sb2 as well as sb, and they are identical. And if they
are not, then sb2->buf is the right thing to pass.
-Peff
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html