Re: [PATCH 2/3] strbuf: add strbuf_percentquote_buf
- From
Junio C Hamano <gitster@pobox.com>
- Date
- Jan 13, 2010, 06:55 UTC
- Message-ID
- <7viqb6trwu.fsf@alter.siamese.dyndns.org>
- In-Reply-To
- <20100112154153.GB24957@coredump.intra.peff.net>
Jeff King <peff@peff.net> writes:
Show 20 quoted lines
> +`strbuf_percentquote_buf`::
> +
> + Append the contents of one strbuf to another, quoting any
> + percent signs ("%") into double-percents ("%%") in the
> + destination. This is useful for literal data to be fed to either
> + strbuf_expand or to the *printf family of functions.
> +
> `strbuf_addf`::
>
> Add a formatted string to the buffer.
> diff --git a/strbuf.c b/strbuf.c
> index 6cbc1fc..b5183c6 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -257,6 +257,16 @@ size_t strbuf_expand_dict_cb(struct strbuf *sb, const char *placeholder,
> return 0;
> }
>
> +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 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.
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;
(1) mark the src side as const; and
(2) perhaps have assert(dst != src). The loop won't terminate when
called with src == dst, I think.There seems to be only one other strbuf function that takes two strbufs in the suite (strbuf_addbuf), and I think it is unsafe in a different way, which is trivial to fix.
-- >8 --
Subject: [PATCH] strbuf_addbuf(): allow passing the same buf to dst and src
If sb and sb2 are the same (i.e. doubling the string), the underlying strbuf_add() will make sb2->buf invalid by calling strbuf_grow(sb) at the beginning and will read from the freed buffer.
Signed-off-by: Junio C Hamano <gitster@pobox.com> --- strbuf.h | 8 +++++++- 1 files changed, 7 insertions(+), 1 deletions(-)
diff --git a/strbuf.h b/strbuf.h index fa07ecf..e272359 100644 --- 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); } extern void strbuf_adddup(struct strbuf *sb, size_t pos, size_t len);
-- 1.6.6.280.ge295b7.dirty