Re: [PATCH] config: support values longer than 1024 bytes

On Wed, Apr 6, 2011 at 5:35 PM, Jeff King [off-list ref] wrote:

On Wed, Apr 06, 2011 at 11:10:42AM +0200, Erik Faye-Lund wrote:

quoted

quoted

But for the other, one of the invariants of strbuf is that the string is
always NUL-terminated. So I would expect strbuf_init to properly
NUL-terminate after growing based on the hint.

I agree. An unterminated yet non-NULL return from strbuf_detach is
just dangerous behavior. Something like this should probably be
applied:

---8<---

diff --git a/strbuf.c b/strbuf.c
index 77444a9..538035a 100644
--- a/strbuf.c
+++ b/strbuf.c

@@ -24,14 +24,16 @@ int suffixcmp(const char *str, const char *suffix)

  * buf is non NULL and ->buf is NUL terminated even for a freshly
  * initialized strbuf.
  */
-char strbuf_slopbuf[1];
+char strbuf_slopbuf[1] = { '\0' };

This hunk is redundant. slopbuf will already be initialized to 0.

Right, silly me. I somehow thought that implicit zero-initialization
only applied to static variables, but K&R tells me I was wrong. Thanks
for pointing it out :)

quoted

 void strbuf_init(struct strbuf *sb, size_t hint)
 {
      sb->alloc = sb->len = 0;
      sb->buf = strbuf_slopbuf;
-     if (hint)
+     if (hint) {
              strbuf_grow(sb, hint);
+             sb->buf[0] = '\0';
+     }
 }

But this one is the right fix.

OK, I'll turn this into a two-patch series, then.

quoted

that. But this brings a new issue: leaving potentially huge blocks of
memory (especially since this patch is about long lines) allocated
inside a function can be a bit nasty. But it's probably not a big deal

Yeah. It's just one block, though, and in the normal case it is probably
only about 80 characters. So it is more efficient than what's there now. :)

Somebody could have some gigantic value, though, and yes, we'll grow to
the biggest one and never free that memory. You could also have
parse_value take a strbuf parameter to output into, and then free it
after config reading is done.

quoted

In other words: I think you're right, it's a much better approach.
Less allocations, less penalty on the start-up time for every little
git-command.

I doubt the efficiency increase is measurable. We end up xstrdup'ing
quite a few of the values in the config callbacks anyway. I would do
whatever seems most natural for reading/writing the code.

I think I'll just leave the single leak in. Since it would allocate
the buffer lazily, it would really only "waste" more memory than the
existing implementation when the old implementation would fail. So my
conscience is clear ;)

quoted

quoted

I do wonder, though, if we could be reusing the unquote_c_style()
function in quote.c. They are obviously similar, but I haven't checked
if there is more going on in the config code.

Hmm, this is an interesting suggestion. It would be a part of a bigger
change though: unquote_c_style requires it's input to be in memory,
while parse_value uses a function called get_next_char to feed the
parser. So we'd either have to read the entire file into memory, or
find some way to read the file line-by-line while handling \n-escaping
correctly.

It also seems like there's differences in what kind of escaping and
normalization the two functions handle; unquote_c_style handles more
escaped character sequences, while parse_value normalize all
non-escaped space-characters ('\t' et. al) into SP. This might not be
such a big problem in reality.

This was just a random thought that I had, and I didn't investigate it
how hard it would be.  If it turns out to be too much trouble, just
forget it.

Yeah. I think I'll skip it, since it isn't dead obvious.

Thanks for the input, I'll cook up a new version soon.