login
Header Space

 
 

[RFC] strbuf's in builtin-apply

Score:
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]
To: <git@...>, Junio C Hamano <gitster@...>
Date: Saturday, September 15, 2007 - 10:12 am

I promised it, here it comes. It comes though with some possible
problems, so I'd like to see them discussed first.

  builtin-apply.c was messing with its custom buffers a lot,
reallocating the buffers and so on, hence I had to create strbuf_embed
to have a strbuf-safe API doing exactly the same. Though, because of the
"I want an extra NUL in the end"-invariant, this can come with a quite
high price. in the second patch, there is a hunk:


-		nsize =3D got;
-		nbuf =3D convert_to_git(path, buf, &nsize);
-		if (nbuf) {
-			free(buf);
-			*buf_p =3D nbuf;
-			*alloc_p =3D nsize;
-			*size_p =3D nsize;
-		}
-		return got !=3D size;
+
+		nsize =3D buf->len;
+		nbuf =3D convert_to_git(path, buf->buf, &nsize);
+		if (nbuf)
+			strbuf_embed(buf, nbuf, nsize, nsize);
+		return 0;

  Here, I've not been able to check if convert_to_git was in fact always
dealing with a NUL-terminated buffer (That would be in fact nsize+1) or
not, hence here this strbuf_embed will likely perform a realloc. I don't
know git enough to know if this can become an horrible burden though.

  Another suspicious hunk is:

-	data =3D (void*) fragment->patch;
[...]
 	case BINARY_LITERAL_DEFLATED:
-		free(desc->buffer);
-		desc->buffer =3D data;
-		dst_size =3D fragment->size;
-		break;
+		strbuf_embed(buf, fragment->patch, fragment->size, fragment->size);
+		return 0;

  TTBOMK the ->patch pointer is a pointer inside a buffer, not a buffer
that has been malloc'ed (and there are code paths before my patch that
would still realloc the buffer so I don't think I introduce an issue).
It passes the test-suite without crashing, but well, maybe this should
be a copy instead.

  The rest is pretty straightforward.

--=20
=C2=B7O=C2=B7  Pierre Habouzit
=C2=B7=C2=B7O                                                madcoder@debia=
n.org
OOO                                                http://www.madism.org
Previous message: [thread] [date] [author]
Next message: [thread] [date] [author]

Messages in current thread:
[RFC] strbuf's in builtin-apply, Pierre Habouzit, (Sat Sep 15, 10:12 am)
Re: [RFC] strbuf's in builtin-apply, Pierre Habouzit, (Sun Sep 16, 1:21 pm)
Re: [RFC] strbuf's in builtin-apply, Pierre Habouzit, (Sun Sep 16, 1:28 pm)
Re: [RFC] strbuf's in builtin-apply, Junio C Hamano, (Sun Sep 16, 6:54 pm)
[PATCH] builtin-apply: use strbuf's instead of buffer_desc's., Pierre Habouzit, (Sun Sep 16, 12:54 pm)
[PATCH] Remove preemptive allocations., Pierre Habouzit, (Sun Sep 16, 4:19 am)
[PATCH] Refactor replace_encoding_header., Pierre Habouzit, (Sat Sep 15, 5:50 pm)
[PATCH] New strbuf APIs: splice and attach., Pierre Habouzit, (Sat Sep 15, 9:56 am)
Re: [PATCH] New strbuf APIs: splice and attach., Florian Weimer, (Sun Sep 16, 4:20 pm)
Re: [PATCH] New strbuf APIs: splice and attach., Pierre Habouzit, (Sun Sep 16, 4:51 pm)
Re: [PATCH] New strbuf APIs: splice and attach., Florian Weimer, (Mon Sep 17, 1:43 am)
Re: [RFC] strbuf's in builtin-apply, Pierre Habouzit, (Sat Sep 15, 1:07 pm)
[PATCH] builtin-apply: use strbuf's instead of buffer_desc's., Pierre Habouzit, (Sat Sep 15, 10:04 am)
[PATCH] New strbuf APIs: splice and embed., Pierre Habouzit, (Sat Sep 15, 9:56 am)
speck-geostationary