Re: [PATCH 3/6] strbuf_getwholeline: use getc_unlocked

From: Rasmus Villemoes <rv@rasmusvillemoes.dk>
Date: Apr 7, 2015, 13:48 UTC
Message-ID: <87zj6kjbgu.fsf@rasmusvillemoes.dk>
In-Reply-To: <20150405045614.GA12053@peff.net>

On Sun, Apr 05 2015, Jeff King <peff@peff.net> wrote:

Show 22 quoted lines

> which is back to the v2.0.0 number. Even with the extra strlen, it seems
> that what fgets does internally beats repeated getc calls. Which I guess
> is not too surprising, as each getc() will have to check for underflow
> in the buffer. Perhaps there is more room to micro-optimize
> strbuf_getwholeline, but I kind of doubt it.
>
> The big downside is that our input strings are no longer NUL-clean
> (reading "foo\0bar\n" would yield just "foo". I doubt that matters in
> the real world, but it does fail a few of the tests (e.g., t7008 tries
> to read a list of patterns which includes NUL, and we silently truncate
> the pattern rather than read in the NUL and barf).
>
> So we'd have to either:
>
>   1. Decide that doesn't matter.
>
>   2. Have callers specify a "damn the NULs, I want it fast" flag.
>
>   3. Find some alternative that is more robust than fgets, and faster
>      than getc. I don't think there is anything in stdio, but I am not
>      above dropping in a faster non-portable call if it is available,
>      and then falling back to the current code otherwise.

getdelim is in POSIX 2008 (http://pubs.opengroup.org/stage7tc1/functions/getdelim.html), so should be available on any half-{d,r}ecent platform. It obviously has the advantage of having access to the internal stdio buffer, and by definition handles embedded NULs. No idea if using such modern interfaces in git is ok, though.

Implementation-wise, I think strbuf_getwholeline could be implemented mostly as a simple wrapper for getdelim. If I'm reading the current code and the posix spec for getdelim correctly, something like this should do it (though obviously not meant to be included as-is):

diff --git a/strbuf.c b/strbuf.c
index 0346e74..d3338b9 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -434,6 +434,32 @@ int strbuf_getcwd(struct strbuf *sb)
 	return -1;
 }
 
+#if USE_GETDELIM
+/* Hacky declaration to avoid messing with feature test macros. */
+ssize_t getdelim(char **, size_t *, int, FILE *);
+int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
+{
+	ssize_t r;
+
+	if (feof(fp))
+		return EOF;
+
+	strbuf_reset(sb);
+	if (!sb->alloc)
+		sb->buf = NULL;
+
+	r = getdelim(&sb->buf, &sb->alloc, term, fp);
+
+	if (r > 0) {
+		sb->len = r;
+		return 0;
+	}
+	assert(r == -1);
+	if (!sb->buf)
+		strbuf_init(sb);
+	return EOF;
+}
+#else
 int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 {
 	int ch;
@@ -454,6 +480,7 @@ int strbuf_getwholeline(struct strbuf *sb, FILE *fp, int term)
 	sb->buf[sb->len] = '\0';
 	return 0;
 }
+#endif
 
 int strbuf_getline(struct strbuf *sb, FILE *fp, int term)
 {


Rasmus