Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf

From: Jeff King <peff@peff.net>
Date: Jan 13, 2010, 17:06 UTC
Message-ID: <20100113170636.GA21318@coredump.intra.peff.net>
In-Reply-To: <7viqb6trwu.fsf@alter.siamese.dyndns.org>

On Tue, Jan 12, 2010 at 10:55:45PM -0800, Junio C Hamano wrote:

Show 5 quoted lines

> > +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.

Show 5 quoted lines

> 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.

Show 15 quoted lines

> --- 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