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

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 13, 2010, 19:47 UTC
Message-ID: <7vwrzln5x5.fsf@alter.siamese.dyndns.org>
In-Reply-To: <20100113170636.GA21318@coredump.intra.peff.net>

Jeff King <peff@peff.net> writes:

Show 6 quoted lines

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

As I said, I don't think appending p-quoted version of itself to a string makes much sense, but I don't think in-place is too difficult.

	strbuf_addbuf_pquote(*dst, *src)
        {
		int len = src->len, i;
		for (i = 0; i < len; i++) {
			if (src->buf[i] == '%')
                        	strbuf_addch(dst, '%');
			strbuf_addch(dst, src->buf[i]);
		}
        }

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

Ok, that is a good catch. And two strbufs that share the same allocated string is a user error

Show 5 quoted lines

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

I didn't want to worry about a semi-clever (read: broken) compilers doing semi-clever things assuming sb and sb2 do not alias, but I agree that your approach is much simpler.

Thanks.