[PATCH 0/4] teach vcs-svn/line_buffer to handle multiple input files

[PATCH 0/4] teach vcs-svn/line_buffer to handle multiple input files

[Jonathan Nieder]

Hi David et al,

This collection of patches comes from the svndiff0 series[1].  They
are not urgent --- the motivation is for svn-fe to be able to keep
separate track of input from stdin (svnrdump) and the report-fd (blobs
from fast-import) for the coming Text-delta support --- but ideally I
would like to see them applied at the start of the next merge window,
since they change API that other patches use.  See [*] below for the
open question.

The idea: instead of keeping the input file handle and input buffer as
global variables, pack them in a struct and let the calling program
keep track of them.

Patch 1 makes a previously global buffer local to the two functions
that use it.  Performance impact should be negligibile.  Ideally the
buffer would not be needed at all --- there is enough buffering at
lower layers already --- but stdio does not provide the calls that
would be needed to eliminate it (in particular a wrapper for
sendfile(2)).

Patch 2 replaces a use of the obj_pool library with a strbuf.  The
main immediate effect is to improve error handling behavior (more
importantly, this is needed for patches 3 and 4 since obj_pool is
defined to be global).

Patches 3 and 4 are the main patches, collecting input state in a
struct and moving resposibility for that struct to the calling
program, respectively.

The patches have already received a lot of testing.

[*]
I am not sure whether this is the right approach for reading from the
report-fd.  To avoid deadlock, we cannot issue a blocking read(2)
after the trailing newline has been read from an expected line or the
nth byte has been read in fixed-length input.  This would rule out
fread/fgets if implemented as follows with too large an internal
buffer:

 1. fill internal buffer completely (or stop when an error or
    end of file is encountered)
 2. fill caller's buffer from internal buffer

glibc fread/fgets do not work that way (and in fact will never
deadlock for us).  What about other platforms?  The standards (C,
POSIX) do not make it obvious.

 - maybe setvbuf(f, NULL, _IOLBF, 0) would make fgets safe to use
 - maybe setvbuf(f, NULL, _IONBF, 0) would make fread safe to use.
   On the other hand, it is not clear to me what unbuffered input
   means in this context.  That flag does not do anything meaningful
   on glibc, for example, for input streams.

If all else fails, setting the O_NONBLOCK flag with fcntl (this
could presumably be implemented as SetNamedPipeHandleState(...,
PIPE_NOWAIT) on Windows) would avoid trouble.  After any failing
operation we would have to check that the error is EAGAIN and
clear the error indicator.  Simple.  But it would be even better
to learn that that is not needed.

So far I have been playing it safe with the read(fd, buf, 1) trick
but that does not have great performance, as David noticed[2].

Thoughts?

Jonathan Nieder (4):
  vcs-svn: Eliminate global byte_buffer[] array
  vcs-svn: Replace buffer_read_string memory pool with a strbuf
  vcs-svn: Collect line_buffer data in a struct
  vcs-svn: Teach line_buffer to handle multiple input files

 test-line-buffer.c      |   17 ++++++-----
 vcs-svn/fast_export.c   |    6 ++--
 vcs-svn/fast_export.h   |    5 +++-
 vcs-svn/line_buffer.c   |   66 ++++++++++++++++++++--------------------------
 vcs-svn/line_buffer.h   |   25 +++++++++++++-----
 vcs-svn/line_buffer.txt |    5 ++-
 vcs-svn/svndump.c       |   29 +++++++++++---------
 7 files changed, 82 insertions(+), 71 deletions(-)

[1] http://thread.gmane.org/gmane.comp.version-control.git/158731
[2] http://colabti.org/irclogger/irclogger_log/git-devel?date=2010-12-18

[PATCH 1/4] vcs-svn: eliminate global byte_buffer

[Jonathan Nieder]

Date: Sun, 10 Oct 2010 21:37:10 -0500

The data stored in byte_buffer[] is always either discarded or
written to stdout immediately.  No need for it to persist between
function calls.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
For the line_buffer library.  Decreases BSS size.  Increase stack
overhead of two I/O functions by 4096 bytes.  Performance effect
hasn't been measured.  Alas there is no stdio wrapper for sendfile; if
there were, we could eliminate the buffer altogether.

Of course the goal is to make line_buffer more easily reusable, by
eliminating _all_ global state.  I assume David would like this but I
don't remember if he said so.

 vcs-svn/line_buffer.c |    3 ++-
 1 files changed, 2 insertions(+), 1 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 1543567..f22c94f 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -14,7 +14,6 @@
 obj_pool_gen(blob, char, 4096)
 
 static char line_buffer[LINE_BUFFER_LEN];
-static char byte_buffer[COPY_BUFFER_LEN];
 static FILE *infile;
 
 int buffer_init(const char *filename)
@@ -68,6 +67,7 @@ char *buffer_read_string(uint32_t len)
 
 void buffer_copy_bytes(uint32_t len)
 {
+	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
 	while (len > 0 && !feof(infile) && !ferror(infile)) {
 		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
@@ -83,6 +83,7 @@ void buffer_copy_bytes(uint32_t len)
 
 void buffer_skip_bytes(uint32_t len)
 {
+	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
 	while (len > 0 && !feof(infile) && !ferror(infile)) {
 		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 2/4] vcs-svn: replace buffer_read_string memory pool with a strbuf

[Jonathan Nieder]

Date: Sat, 6 Nov 2010 12:01:28 -0500

obj_pool is inherently global and does not use the standard growing
factor alloc_nr, which makes it feel out of place in the git codebase.
Plus it is overkill for this application: all that is needed is a
buffer that can grow between requests to accomodate larger strings.
Use a strbuf instead.

As a side effect, this improves the error handling: allocation
failures will result in a clean exit instead of segfaults.  It would
be nice to add a test case (using ulimit or failmalloc) but that can
wait for another day.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Requires jn/thinner-wrapper (from master) if contrib/svn-fe/svn-fe is
to build without linking to libz et al.

The initial size of the per-line buffer shrinks from 4096 to 0 (well,
maybe 16 or so).  strbuf_fread is not inline.  I haven't looked into
the effect on performance from these changes.

I find obj_pool tricky to use correctly (see 3c93983, vcs-svn: fix
intermittent repo_tree corruption, 2010-12-05 for example) so I look
forward to eliminating obj_pool from the vcs-svn/ dir altogether.
Excitingly enough, David has already done that, it seems[1].

[1] git://github.com/barrbrain/git.git vcs-svn-incremental

 vcs-svn/line_buffer.c |   16 ++++++----------
 1 files changed, 6 insertions(+), 10 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index f22c94f..6f32f28 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -5,15 +5,13 @@
 
 #include "git-compat-util.h"
 #include "line_buffer.h"
-#include "obj_pool.h"
+#include "strbuf.h"
 
 #define LINE_BUFFER_LEN 10000
 #define COPY_BUFFER_LEN 4096
 
-/* Create memory pool for char sequence of known length */
-obj_pool_gen(blob, char, 4096)
-
 static char line_buffer[LINE_BUFFER_LEN];
+static struct strbuf blob_buffer = STRBUF_INIT;
 static FILE *infile;
 
 int buffer_init(const char *filename)
@@ -58,11 +56,9 @@ char *buffer_read_line(void)
 
 char *buffer_read_string(uint32_t len)
 {
-	char *s;
-	blob_free(blob_pool.size);
-	s = blob_pointer(blob_alloc(len + 1));
-	s[fread(s, 1, len, infile)] = '\0';
-	return ferror(infile) ? NULL : s;
+	strbuf_reset(&blob_buffer);
+	strbuf_fread(&blob_buffer, len, infile);
+	return ferror(infile) ? NULL : blob_buffer.buf;
 }
 
 void buffer_copy_bytes(uint32_t len)
@@ -94,5 +90,5 @@ void buffer_skip_bytes(uint32_t len)
 
 void buffer_reset(void)
 {
-	blob_reset();
+	strbuf_release(&blob_buffer);
 }
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 3/4] vcs-svn: collect line_buffer data in a struct

[Jonathan Nieder]

Date: Sun, 10 Oct 2010 21:39:21 -0500

Prepare for the line_buffer lib to support input from multiple files,
by collecting global state in a struct that can be easily passed
around.

No API change yet.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
A simple search-and-replace kind of job.  If gcc is smart, there
should be no change in code size or performance, but I haven't
checked.  

 vcs-svn/line_buffer.c |   45 ++++++++++++++++++++++-----------------------
 vcs-svn/line_buffer.h |   11 +++++++++++
 2 files changed, 33 insertions(+), 23 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 6f32f28..e7bc230 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -7,17 +7,16 @@
 #include "line_buffer.h"
 #include "strbuf.h"
 
-#define LINE_BUFFER_LEN 10000
 #define COPY_BUFFER_LEN 4096
-
-static char line_buffer[LINE_BUFFER_LEN];
-static struct strbuf blob_buffer = STRBUF_INIT;
-static FILE *infile;
+static struct line_buffer buf_ = LINE_BUFFER_INIT;
+static struct line_buffer *buf;
 
 int buffer_init(const char *filename)
 {
-	infile = filename ? fopen(filename, "r") : stdin;
-	if (!infile)
+	buf = &buf_;
+
+	buf->infile = filename ? fopen(filename, "r") : stdin;
+	if (!buf->infile)
 		return -1;
 	return 0;
 }
@@ -25,10 +24,10 @@ int buffer_init(const char *filename)
 int buffer_deinit(void)
 {
 	int err;
-	if (infile == stdin)
-		return ferror(infile);
-	err = ferror(infile);
-	err |= fclose(infile);
+	if (buf->infile == stdin)
+		return ferror(buf->infile);
+	err = ferror(buf->infile);
+	err |= fclose(buf->infile);
 	return err;
 }
 
@@ -36,13 +35,13 @@ int buffer_deinit(void)
 char *buffer_read_line(void)
 {
 	char *end;
-	if (!fgets(line_buffer, sizeof(line_buffer), infile))
+	if (!fgets(buf->line_buffer, sizeof(buf->line_buffer), buf->infile))
 		/* Error or data exhausted. */
 		return NULL;
-	end = line_buffer + strlen(line_buffer);
+	end = buf->line_buffer + strlen(buf->line_buffer);
 	if (end[-1] == '\n')
 		end[-1] = '\0';
-	else if (feof(infile))
+	else if (feof(buf->infile))
 		; /* No newline at end of file.  That's fine. */
 	else
 		/*
@@ -51,23 +50,23 @@ char *buffer_read_line(void)
 		 * but for now let's return an error.
 		 */
 		return NULL;
-	return line_buffer;
+	return buf->line_buffer;
 }
 
 char *buffer_read_string(uint32_t len)
 {
-	strbuf_reset(&blob_buffer);
-	strbuf_fread(&blob_buffer, len, infile);
-	return ferror(infile) ? NULL : blob_buffer.buf;
+	strbuf_reset(&buf->blob_buffer);
+	strbuf_fread(&buf->blob_buffer, len, buf->infile);
+	return ferror(buf->infile) ? NULL : buf->blob_buffer.buf;
 }
 
 void buffer_copy_bytes(uint32_t len)
 {
 	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
-	while (len > 0 && !feof(infile) && !ferror(infile)) {
+	while (len > 0 && !feof(buf->infile) && !ferror(buf->infile)) {
 		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
-		in = fread(byte_buffer, 1, in, infile);
+		in = fread(byte_buffer, 1, in, buf->infile);
 		len -= in;
 		fwrite(byte_buffer, 1, in, stdout);
 		if (ferror(stdout)) {
@@ -81,14 +80,14 @@ void buffer_skip_bytes(uint32_t len)
 {
 	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
-	while (len > 0 && !feof(infile) && !ferror(infile)) {
+	while (len > 0 && !feof(buf->infile) && !ferror(buf->infile)) {
 		in = len < COPY_BUFFER_LEN ? len : COPY_BUFFER_LEN;
-		in = fread(byte_buffer, 1, in, infile);
+		in = fread(byte_buffer, 1, in, buf->infile);
 		len -= in;
 	}
 }
 
 void buffer_reset(void)
 {
-	strbuf_release(&blob_buffer);
+	strbuf_release(&buf->blob_buffer);
 }
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 9c78ae1..4ae1133 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -1,6 +1,17 @@
 #ifndef LINE_BUFFER_H_
 #define LINE_BUFFER_H_
 
+#include "strbuf.h"
+
+#define LINE_BUFFER_LEN 10000
+
+struct line_buffer {
+	char line_buffer[LINE_BUFFER_LEN];
+	struct strbuf blob_buffer;
+	FILE *infile;
+};
+#define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL}
+
 int buffer_init(const char *filename);
 int buffer_deinit(void);
 char *buffer_read_line(void);
-- 
1.7.2.3.554.gc9b5c.dirty

[PATCH 4/4] vcs-svn: teach line_buffer to handle multiple input files

[Jonathan Nieder]

Date: Sun, 10 Oct 2010 21:41:06 -0500

Collect the line_buffer state in a newly public line_buffer struct.
Callers can use multiple line_buffers to manage input from multiple
files at a time.

svn-fe's delta applier will use this to stream a delta from svnrdump
and the preimage it applies to from fast-import at the same time.

The tests don't take advantage of the new features, but I think that's
okay.  It is easier to find lingering examples of nonreentrant code by
searching for "static" in line_buffer.c.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
That's it.  The API change feels inevitable to me, perhaps because it
has been in use for so long.  Hopefully it can be a good basis for
future changes:

 - buffer_tmpfile_{init,rewind,prepare_to_read}, for when files are
   just a place to stash data for later
 - buffer_fdopen to read from a file descriptor
 - buffer_interactive for deadlock avoidance (if that turns out to
   be needed)

I would be happy to hear any thoughts you have, even as basic as
"looks ok" or "confusing".

thanks,
Jonathan

 test-line-buffer.c      |   17 +++++++++--------
 vcs-svn/fast_export.c   |    6 +++---
 vcs-svn/fast_export.h   |    5 ++++-
 vcs-svn/line_buffer.c   |   20 ++++++++------------
 vcs-svn/line_buffer.h   |   14 +++++++-------
 vcs-svn/line_buffer.txt |    5 +++--
 vcs-svn/svndump.c       |   29 ++++++++++++++++-------------
 7 files changed, 50 insertions(+), 46 deletions(-)

diff --git a/test-line-buffer.c b/test-line-buffer.c
index c11bf7f..f9af892 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -22,25 +22,26 @@ static uint32_t strtouint32(const char *s)
 
 int main(int argc, char *argv[])
 {
+	struct line_buffer buf = LINE_BUFFER_INIT;
 	char *s;
 
 	if (argc != 1)
 		usage("test-line-buffer < input.txt");
-	if (buffer_init(NULL))
+	if (buffer_init(&buf, NULL))
 		die_errno("open error");
-	while ((s = buffer_read_line())) {
-		s = buffer_read_string(strtouint32(s));
+	while ((s = buffer_read_line(&buf))) {
+		s = buffer_read_string(&buf, strtouint32(s));
 		fputs(s, stdout);
 		fputc('\n', stdout);
-		buffer_skip_bytes(1);
-		if (!(s = buffer_read_line()))
+		buffer_skip_bytes(&buf, 1);
+		if (!(s = buffer_read_line(&buf)))
 			break;
-		buffer_copy_bytes(strtouint32(s) + 1);
+		buffer_copy_bytes(&buf, strtouint32(s) + 1);
 	}
-	if (buffer_deinit())
+	if (buffer_deinit(&buf))
 		die("input error");
 	if (ferror(stdout))
 		die("output error");
-	buffer_reset();
+	buffer_reset(&buf);
 	return 0;
 }
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 6cfa256..260cf50 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -63,14 +63,14 @@ void fast_export_commit(uint32_t revision, uint32_t author, char *log,
 	printf("progress Imported commit %"PRIu32".\n\n", revision);
 }
 
-void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len)
+void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len, struct line_buffer *input)
 {
 	if (mode == REPO_MODE_LNK) {
 		/* svn symlink blobs start with "link " */
-		buffer_skip_bytes(5);
+		buffer_skip_bytes(input, 5);
 		len -= 5;
 	}
 	printf("blob\nmark :%"PRIu32"\ndata %"PRIu32"\n", mark, len);
-	buffer_copy_bytes(len);
+	buffer_copy_bytes(input, len);
 	fputc('\n', stdout);
 }
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 2aaaea5..054e7d5 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -1,11 +1,14 @@
 #ifndef FAST_EXPORT_H_
 #define FAST_EXPORT_H_
 
+#include "line_buffer.h"
+
 void fast_export_delete(uint32_t depth, uint32_t *path);
 void fast_export_modify(uint32_t depth, uint32_t *path, uint32_t mode,
 			uint32_t mark);
 void fast_export_commit(uint32_t revision, uint32_t author, char *log,
 			uint32_t uuid, uint32_t url, unsigned long timestamp);
-void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len);
+void fast_export_blob(uint32_t mode, uint32_t mark, uint32_t len,
+		      struct line_buffer *input);
 
 #endif
diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index e7bc230..806932b 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -8,20 +8,16 @@
 #include "strbuf.h"
 
 #define COPY_BUFFER_LEN 4096
-static struct line_buffer buf_ = LINE_BUFFER_INIT;
-static struct line_buffer *buf;
 
-int buffer_init(const char *filename)
+int buffer_init(struct line_buffer *buf, const char *filename)
 {
-	buf = &buf_;
-
 	buf->infile = filename ? fopen(filename, "r") : stdin;
 	if (!buf->infile)
 		return -1;
 	return 0;
 }
 
-int buffer_deinit(void)
+int buffer_deinit(struct line_buffer *buf)
 {
 	int err;
 	if (buf->infile == stdin)
@@ -32,7 +28,7 @@ int buffer_deinit(void)
 }
 
 /* Read a line without trailing newline. */
-char *buffer_read_line(void)
+char *buffer_read_line(struct line_buffer *buf)
 {
 	char *end;
 	if (!fgets(buf->line_buffer, sizeof(buf->line_buffer), buf->infile))
@@ -53,14 +49,14 @@ char *buffer_read_line(void)
 	return buf->line_buffer;
 }
 
-char *buffer_read_string(uint32_t len)
+char *buffer_read_string(struct line_buffer *buf, uint32_t len)
 {
 	strbuf_reset(&buf->blob_buffer);
 	strbuf_fread(&buf->blob_buffer, len, buf->infile);
 	return ferror(buf->infile) ? NULL : buf->blob_buffer.buf;
 }
 
-void buffer_copy_bytes(uint32_t len)
+void buffer_copy_bytes(struct line_buffer *buf, uint32_t len)
 {
 	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
@@ -70,13 +66,13 @@ void buffer_copy_bytes(uint32_t len)
 		len -= in;
 		fwrite(byte_buffer, 1, in, stdout);
 		if (ferror(stdout)) {
-			buffer_skip_bytes(len);
+			buffer_skip_bytes(buf, len);
 			return;
 		}
 	}
 }
 
-void buffer_skip_bytes(uint32_t len)
+void buffer_skip_bytes(struct line_buffer *buf, uint32_t len)
 {
 	char byte_buffer[COPY_BUFFER_LEN];
 	uint32_t in;
@@ -87,7 +83,7 @@ void buffer_skip_bytes(uint32_t len)
 	}
 }
 
-void buffer_reset(void)
+void buffer_reset(struct line_buffer *buf)
 {
 	strbuf_release(&buf->blob_buffer);
 }
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 4ae1133..fb37390 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -12,12 +12,12 @@ struct line_buffer {
 };
 #define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL}
 
-int buffer_init(const char *filename);
-int buffer_deinit(void);
-char *buffer_read_line(void);
-char *buffer_read_string(uint32_t len);
-void buffer_copy_bytes(uint32_t len);
-void buffer_skip_bytes(uint32_t len);
-void buffer_reset(void);
+int buffer_init(struct line_buffer *buf, const char *filename);
+int buffer_deinit(struct line_buffer *buf);
+char *buffer_read_line(struct line_buffer *buf);
+char *buffer_read_string(struct line_buffer *buf, uint32_t len);
+void buffer_copy_bytes(struct line_buffer *buf, uint32_t len);
+void buffer_skip_bytes(struct line_buffer *buf, uint32_t len);
+void buffer_reset(struct line_buffer *buf);
 
 #endif
diff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt
index 8906fb1..f8eaa4d 100644
--- a/vcs-svn/line_buffer.txt
+++ b/vcs-svn/line_buffer.txt
@@ -14,14 +14,15 @@ Calling sequence
 
 The calling program:
 
+ - initializes a `struct line_buffer` to LINE_BUFFER_INIT
  - specifies a file to read with `buffer_init`
  - processes input with `buffer_read_line`, `buffer_read_string`,
    `buffer_skip_bytes`, and `buffer_copy_bytes`
  - closes the file with `buffer_deinit`, perhaps to start over and
    read another file.
 
-Before exiting, the caller can use `buffer_reset` to deallocate
-resources for the benefit of profiling tools.
+When finished, the caller can use `buffer_reset` to deallocate
+resources.
 
 Functions
 ---------
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 53d0215..3bba0fe 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -30,6 +30,8 @@
 /* Create memory pool for log messages */
 obj_pool_gen(log, char, 4096)
 
+static struct line_buffer input = LINE_BUFFER_INIT;
+
 static char* log_copy(uint32_t length, char *log)
 {
 	char *buffer;
@@ -113,14 +115,14 @@ static void read_props(void)
 	uint32_t key = ~0;
 	char *val = NULL;
 	char *t;
-	while ((t = buffer_read_line()) && strcmp(t, "PROPS-END")) {
+	while ((t = buffer_read_line(&input)) && strcmp(t, "PROPS-END")) {
 		if (!strncmp(t, "K ", 2)) {
 			len = atoi(&t[2]);
-			key = pool_intern(buffer_read_string(len));
-			buffer_read_line();
+			key = pool_intern(buffer_read_string(&input, len));
+			buffer_read_line(&input);
 		} else if (!strncmp(t, "V ", 2)) {
 			len = atoi(&t[2]);
-			val = buffer_read_string(len);
+			val = buffer_read_string(&input, len);
 			if (key == keys.svn_log) {
 				/* Value length excludes terminating nul. */
 				rev_ctx.log = log_copy(len + 1, val);
@@ -135,7 +137,7 @@ static void read_props(void)
 				node_ctx.type = REPO_MODE_LNK;
 			}
 			key = ~0;
-			buffer_read_line();
+			buffer_read_line(&input);
 		}
 	}
 }
@@ -177,9 +179,10 @@ static void handle_node(void)
 		node_ctx.type = node_ctx.srcMode;
 
 	if (node_ctx.mark)
-		fast_export_blob(node_ctx.type, node_ctx.mark, node_ctx.textLength);
+		fast_export_blob(node_ctx.type,
+				 node_ctx.mark, node_ctx.textLength, &input);
 	else if (node_ctx.textLength != LENGTH_UNKNOWN)
-		buffer_skip_bytes(node_ctx.textLength);
+		buffer_skip_bytes(&input, node_ctx.textLength);
 }
 
 static void handle_revision(void)
@@ -198,7 +201,7 @@ void svndump_read(const char *url)
 	uint32_t key;
 
 	reset_dump_ctx(pool_intern(url));
-	while ((t = buffer_read_line())) {
+	while ((t = buffer_read_line(&input))) {
 		val = strstr(t, ": ");
 		if (!val)
 			continue;
@@ -250,7 +253,7 @@ void svndump_read(const char *url)
 			node_ctx.propLength = atoi(val);
 		} else if (key == keys.content_length) {
 			len = atoi(val);
-			buffer_read_line();
+			buffer_read_line(&input);
 			if (active_ctx == REV_CTX) {
 				read_props();
 			} else if (active_ctx == NODE_CTX) {
@@ -258,7 +261,7 @@ void svndump_read(const char *url)
 				active_ctx = REV_CTX;
 			} else {
 				fprintf(stderr, "Unexpected content length header: %"PRIu32"\n", len);
-				buffer_skip_bytes(len);
+				buffer_skip_bytes(&input, len);
 			}
 		}
 	}
@@ -270,7 +273,7 @@ void svndump_read(const char *url)
 
 void svndump_init(const char *filename)
 {
-	buffer_init(filename);
+	buffer_init(&input, filename);
 	repo_init();
 	reset_dump_ctx(~0);
 	reset_rev_ctx(0);
@@ -285,7 +288,7 @@ void svndump_deinit(void)
 	reset_dump_ctx(~0);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
-	if (buffer_deinit())
+	if (buffer_deinit(&input))
 		fprintf(stderr, "Input error\n");
 	if (ferror(stdout))
 		fprintf(stderr, "Output error\n");
@@ -294,7 +297,7 @@ void svndump_deinit(void)
 void svndump_reset(void)
 {
 	log_reset();
-	buffer_reset();
+	buffer_reset(&input);
 	repo_reset();
 	reset_dump_ctx(~0);
 	reset_rev_ctx(0);
-- 
1.7.2.3.554.gc9b5c.dirty

Re: [PATCH 0/4] teach vcs-svn/line_buffer to handle multiple input files

[Jonathan Nieder]

Jonathan Nieder wrote:

> [*]
> I am not sure whether this is the right approach for reading from the
> report-fd.  To avoid deadlock, we cannot issue a blocking read(2)
> after the trailing newline has been read from an expected line or the
> nth byte has been read in fixed-length input.

I think this isn't a problem (even though I haven't found any
comforting text in the standards themselves).

To convince myself so, I wrote a couple of new tests.  The change
descriptions explain the reasoning.

These four patches apply on top of

  [PATCH 4/4] vcs-svn: teach line_buffer to handle multiple input files

and are numbered accordingly.  Thoughts welcome, as always.

Jonathan Nieder (4):
  vcs-svn: make test-line-buffer input format more flexible
  tests: give vcs-svn/line_buffer its own test script
  vcs-svn: tweak test-line-buffer to not assume line-oriented input
  t0081 (line-buffer): add buffering test

 t/t0080-vcs-svn.sh     |   54 ---------------
 t/t0081-line-buffer.sh |  174 ++++++++++++++++++++++++++++++++++++++++++++++++
 test-line-buffer.c     |   76 +++++++++++++++------
 3 files changed, 230 insertions(+), 74 deletions(-)
 create mode 100755 t/t0081-line-buffer.sh

[PATCH 5/8] vcs-svn: make test-line-buffer input format more flexible

[Jonathan Nieder]

Imitate the input format of test-obj-pool to support arbitrary
sequences of commands rather than alternating read/copy.  This should
make it easier to add tests that exercise other line_buffer functions.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0080-vcs-svn.sh |   18 ++++++++--------
 test-line-buffer.c |   56 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 45 insertions(+), 29 deletions(-)

diff --git a/t/t0080-vcs-svn.sh b/t/t0080-vcs-svn.sh
index d3225ad..8be9700 100755
--- a/t/t0080-vcs-svn.sh
+++ b/t/t0080-vcs-svn.sh
@@ -85,40 +85,40 @@ test_expect_success 'line buffer' '
 	printf "%s\n" "" foo >expected6 &&
 
 	test-line-buffer <<-\EOF >actual1 &&
-	5
+	read 5
 	HELLO
 	EOF
 
 	test-line-buffer <<-\EOF >actual2 &&
-	0
+	read 0
 
-	5
+	copy 5
 	HELLO
 	EOF
 
 	q_to_nul <<-\EOF |
-	1
+	read 1
 	Q
 	EOF
 	test-line-buffer >actual3 &&
 
 	q_to_nul <<-\EOF |
-	0
+	read 0
 
-	1
+	copy 1
 	Q
 	EOF
 	test-line-buffer >actual4 &&
 
 	test-line-buffer <<-\EOF >actual5 &&
-	5
+	read 5
 	foo
 	EOF
 
 	test-line-buffer <<-\EOF >actual6 &&
-	0
+	read 0
 
-	5
+	copy 5
 	foo
 	EOF
 
diff --git a/test-line-buffer.c b/test-line-buffer.c
index f9af892..383f35b 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -1,11 +1,5 @@
 /*
  * test-line-buffer.c: code to exercise the svn importer's input helper
- *
- * Input format:
- *	number NL
- *	(number bytes) NL
- *	number NL
- *	...
  */
 
 #include "git-compat-util.h"
@@ -20,28 +14,50 @@ static uint32_t strtouint32(const char *s)
 	return (uint32_t) n;
 }
 
+static void handle_command(const char *command, const char *arg, struct line_buffer *buf)
+{
+	switch (*command) {
+	case 'c':
+		if (!prefixcmp(command, "copy ")) {
+			buffer_copy_bytes(buf, strtouint32(arg) + 1);
+			return;
+		}
+	case 'r':
+		if (!prefixcmp(command, "read ")) {
+			const char *s = buffer_read_string(buf, strtouint32(arg));
+			printf("%s\n", s);
+			buffer_skip_bytes(buf, 1);	/* consume newline */
+			return;
+		}
+	default:
+		die("unrecognized command: %s", command);
+	}
+}
+
+static void handle_line(const char *line, struct line_buffer *stdin_buf)
+{
+	const char *arg = strchr(line, ' ');
+	if (!arg)
+		die("no argument in line: %s", line);
+	handle_command(line, arg + 1, stdin_buf);
+}
+
 int main(int argc, char *argv[])
 {
-	struct line_buffer buf = LINE_BUFFER_INIT;
+	struct line_buffer stdin_buf = LINE_BUFFER_INIT;
 	char *s;
 
 	if (argc != 1)
-		usage("test-line-buffer < input.txt");
-	if (buffer_init(&buf, NULL))
+		usage("test-line-buffer < script");
+
+	if (buffer_init(&stdin_buf, NULL))
 		die_errno("open error");
-	while ((s = buffer_read_line(&buf))) {
-		s = buffer_read_string(&buf, strtouint32(s));
-		fputs(s, stdout);
-		fputc('\n', stdout);
-		buffer_skip_bytes(&buf, 1);
-		if (!(s = buffer_read_line(&buf)))
-			break;
-		buffer_copy_bytes(&buf, strtouint32(s) + 1);
-	}
-	if (buffer_deinit(&buf))
+	while ((s = buffer_read_line(&stdin_buf)))
+		handle_line(s, &stdin_buf);
+	if (buffer_deinit(&stdin_buf))
 		die("input error");
 	if (ferror(stdout))
 		die("output error");
-	buffer_reset(&buf);
+	buffer_reset(&stdin_buf);
 	return 0;
 }
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 6/8] tests: give vcs-svn/line_buffer its own test script

[Jonathan Nieder]

Split the line_buffer test into small pieces and move it to its
own file as preparation for adding more tests.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0080-vcs-svn.sh     |   54 --------------------------------------
 t/t0081-line-buffer.sh |   67 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 67 insertions(+), 54 deletions(-)
 create mode 100755 t/t0081-line-buffer.sh

diff --git a/t/t0080-vcs-svn.sh b/t/t0080-vcs-svn.sh
index 8be9700..99a314b 100755
--- a/t/t0080-vcs-svn.sh
+++ b/t/t0080-vcs-svn.sh
@@ -76,60 +76,6 @@ test_expect_success 'obj pool: high-water mark' '
 	test_cmp expected actual
 '
 
-test_expect_success 'line buffer' '
-	echo HELLO >expected1 &&
-	printf "%s\n" "" HELLO >expected2 &&
-	echo >expected3 &&
-	printf "%s\n" "" Q | q_to_nul >expected4 &&
-	printf "%s\n" foo "" >expected5 &&
-	printf "%s\n" "" foo >expected6 &&
-
-	test-line-buffer <<-\EOF >actual1 &&
-	read 5
-	HELLO
-	EOF
-
-	test-line-buffer <<-\EOF >actual2 &&
-	read 0
-
-	copy 5
-	HELLO
-	EOF
-
-	q_to_nul <<-\EOF |
-	read 1
-	Q
-	EOF
-	test-line-buffer >actual3 &&
-
-	q_to_nul <<-\EOF |
-	read 0
-
-	copy 1
-	Q
-	EOF
-	test-line-buffer >actual4 &&
-
-	test-line-buffer <<-\EOF >actual5 &&
-	read 5
-	foo
-	EOF
-
-	test-line-buffer <<-\EOF >actual6 &&
-	read 0
-
-	copy 5
-	foo
-	EOF
-
-	test_cmp expected1 actual1 &&
-	test_cmp expected2 actual2 &&
-	test_cmp expected3 actual3 &&
-	test_cmp expected4 actual4 &&
-	test_cmp expected5 actual5 &&
-	test_cmp expected6 actual6
-'
-
 test_expect_success 'string pool' '
 	echo a does not equal b >expected.differ &&
 	echo a equals a >expected.match &&
diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
new file mode 100755
index 0000000..13ac735
--- /dev/null
+++ b/t/t0081-line-buffer.sh
@@ -0,0 +1,67 @@
+#!/bin/sh
+
+test_description="Test the svn importer's input handling routines.
+"
+. ./test-lib.sh
+
+test_expect_success 'read greeting' '
+	echo HELLO >expect &&
+	test-line-buffer <<-\EOF >actual &&
+	read 5
+	HELLO
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success '0-length read, send along greeting' '
+	printf "%s\n" "" HELLO >expect &&
+	test-line-buffer <<-\EOF >actual &&
+	read 0
+
+	copy 5
+	HELLO
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'buffer_read_string copes with trailing null byte' '
+	echo >expect &&
+	q_to_nul <<-\EOF | test-line-buffer >actual &&
+	read 1
+	Q
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success '0-length read, copy null byte' '
+	printf "%s\n" "" Q | q_to_nul >expect &&
+	q_to_nul <<-\EOF | test-line-buffer >actual &&
+	read 0
+
+	copy 1
+	Q
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'long reads are truncated' '
+	printf "%s\n" foo "" >expect &&
+	test-line-buffer <<-\EOF >actual &&
+	read 5
+	foo
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'long copies are truncated' '
+	printf "%s\n" "" foo >expect &&
+	test-line-buffer <<-\EOF >actual &&
+	read 0
+
+	copy 5
+	foo
+	EOF
+	test_cmp expect actual
+'
+
+test_done
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 7/8] vcs-svn: tweak test-line-buffer to not assume line-oriented input

[Jonathan Nieder]

Do not expect an implicit newline after each input record.
Use a separate command to exercise buffer_skip_bytes.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0081-line-buffer.sh |   27 +++++++++++++--------------
 test-line-buffer.c     |   10 +++++++---
 2 files changed, 20 insertions(+), 17 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 13ac735..68d6163 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -7,45 +7,44 @@ test_description="Test the svn importer's input handling routines.
 test_expect_success 'read greeting' '
 	echo HELLO >expect &&
 	test-line-buffer <<-\EOF >actual &&
-	read 5
+	read 6
 	HELLO
 	EOF
 	test_cmp expect actual
 '
 
 test_expect_success '0-length read, send along greeting' '
-	printf "%s\n" "" HELLO >expect &&
+	echo HELLO >expect &&
 	test-line-buffer <<-\EOF >actual &&
 	read 0
-
-	copy 5
+	copy 6
 	HELLO
 	EOF
 	test_cmp expect actual
 '
 
-test_expect_success 'buffer_read_string copes with trailing null byte' '
-	echo >expect &&
+test_expect_success 'buffer_read_string copes with null byte' '
+	>expect &&
 	q_to_nul <<-\EOF | test-line-buffer >actual &&
-	read 1
+	read 2
 	Q
 	EOF
 	test_cmp expect actual
 '
 
-test_expect_success '0-length read, copy null byte' '
-	printf "%s\n" "" Q | q_to_nul >expect &&
+test_expect_success 'skip, copy null byte' '
+	echo Q | q_to_nul >expect &&
 	q_to_nul <<-\EOF | test-line-buffer >actual &&
-	read 0
-
-	copy 1
+	skip 2
+	Q
+	copy 2
 	Q
 	EOF
 	test_cmp expect actual
 '
 
 test_expect_success 'long reads are truncated' '
-	printf "%s\n" foo "" >expect &&
+	echo foo >expect &&
 	test-line-buffer <<-\EOF >actual &&
 	read 5
 	foo
@@ -56,7 +55,7 @@ test_expect_success 'long reads are truncated' '
 test_expect_success 'long copies are truncated' '
 	printf "%s\n" "" foo >expect &&
 	test-line-buffer <<-\EOF >actual &&
-	read 0
+	read 1
 
 	copy 5
 	foo
diff --git a/test-line-buffer.c b/test-line-buffer.c
index 383f35b..da0bc65 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -19,14 +19,18 @@ static void handle_command(const char *command, const char *arg, struct line_buf
 	switch (*command) {
 	case 'c':
 		if (!prefixcmp(command, "copy ")) {
-			buffer_copy_bytes(buf, strtouint32(arg) + 1);
+			buffer_copy_bytes(buf, strtouint32(arg));
 			return;
 		}
 	case 'r':
 		if (!prefixcmp(command, "read ")) {
 			const char *s = buffer_read_string(buf, strtouint32(arg));
-			printf("%s\n", s);
-			buffer_skip_bytes(buf, 1);	/* consume newline */
+			fputs(s, stdout);
+			return;
+		}
+	case 's':
+		if (!prefixcmp(command, "skip ")) {
+			buffer_skip_bytes(buf, strtouint32(arg));
 			return;
 		}
 	default:
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 8/8] t0081 (line-buffer): add buffering tests

[Jonathan Nieder]

POSIX makes the behavior of read(2) from a pipe fairly clear: a read
from an empty pipe will block until there is data available and any
other read will not block, prefering to return a partial result.
Likewise, fread(3) and fgets(3) are clearly specified to act as
though implemented by calling fgetc(3) in a simple loop.  But the
buffering behavior of fgetc is less clear.

Luckily, no sane platform is going to implement fgetc by calling the
equivalent of read(2) more than once.  fgetc has to be able to
return without filling its buffer to preserve errno when errors are
encountered anyway.  So let's assume the simpler behavior (trust) but
add some tests to catch insane platforms that violate that when they
come (verify).

First check that fread can handle a 0-length read from an empty fifo.
The writing end of the fifo is opened in advance in a subshell since
even 0-length reads are allowed to block when the writing end of a
pipe is not open.

Next try short inputs from a pipe that is not filled all the way.

Lastly (two tests) try very large inputs from a pipe that will not fit
in the relevant buffers.  The first of these tests reads a little
more than 8192 bytes, which is BUFSIZ (the size of stdio's buffers)
on this Linux machine.  The second reads a little over 64 KiB (the
pipe capacity on Linux) and is not run unless requested by setting
the GIT_REMOTE_SVN_TEST_BIG_FILES environment variable.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0081-line-buffer.sh |  110 +++++++++++++++++++++++++++++++++++++++++++++++-
 test-line-buffer.c     |   22 ++++++++-
 2 files changed, 128 insertions(+), 4 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 68d6163..33a728e 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -1,10 +1,76 @@
 #!/bin/sh
 
 test_description="Test the svn importer's input handling routines.
+
+These tests exercise the line_buffer library, but their real purpose
+is to check the assumptions that library makes of the platform's input
+routines.  Processes engaged in bi-directional communication would
+hang if fread or fgets is too greedy.
+
+While at it, check that input of newlines and null bytes are handled
+correctly.
 "
 . ./test-lib.sh
 
-test_expect_success 'read greeting' '
+test -n "$GIT_REMOTE_SVN_TEST_BIG_FILES" && test_set_prereq EXPENSIVE
+
+generate_tens_of_lines () {
+	tens=$1 &&
+	line=$2 &&
+
+	i=0 &&
+	while test $i -lt "$tens"
+	do
+		for j in a b c d e f g h i j
+		do
+			echo "$line"
+		done &&
+		: $((i = $i + 1)) ||
+		return
+	done
+}
+
+long_read_test () {
+	: each line is 10 bytes, including newline &&
+	line=abcdefghi &&
+	echo "$line" >expect &&
+
+	if ! test_declared_prereq PIPE
+	then
+		echo >&4 "long_read_test: need to declare PIPE prerequisite"
+		return 127
+	fi &&
+	tens_of_lines=$(($1 / 100 + 1)) &&
+	lines=$(($tens_of_lines * 10)) &&
+	readsize=$((($lines - 1) * 10 + 3)) &&
+	copysize=7 &&
+	rm -f input &&
+	mkfifo input &&
+	{
+		{
+			generate_tens_of_lines $tens_of_lines "$line" &&
+			sleep 100
+		} >input &
+	} &&
+	test-line-buffer input <<-EOF >output &&
+	read $readsize
+	copy $copysize
+	EOF
+	kill $! &&
+	test_line_count = $lines output &&
+	tail -n 1 <output >actual &&
+	test_cmp expect actual
+}
+
+test_expect_success 'setup: have pipes?' '
+      rm -f frob &&
+      if mkfifo frob
+      then
+		test_set_prereq PIPE
+      fi
+'
+
+test_expect_success 'hello world' '
 	echo HELLO >expect &&
 	test-line-buffer <<-\EOF >actual &&
 	read 6
@@ -13,6 +79,21 @@ test_expect_success 'read greeting' '
 	test_cmp expect actual
 '
 
+test_expect_success PIPE '0-length read, no input available' '
+	>expect &&
+	rm -f input &&
+	mkfifo input &&
+	{
+		sleep 100 >input &
+	} &&
+	test-line-buffer input <<-\EOF >actual &&
+	read 0
+	copy 0
+	EOF
+	kill $! &&
+	test_cmp expect actual
+'
+
 test_expect_success '0-length read, send along greeting' '
 	echo HELLO >expect &&
 	test-line-buffer <<-\EOF >actual &&
@@ -23,6 +104,33 @@ test_expect_success '0-length read, send along greeting' '
 	test_cmp expect actual
 '
 
+test_expect_success PIPE '1-byte read, no input available' '
+	printf "%s" ab >expect &&
+	rm -f input &&
+	mkfifo input &&
+	{
+		{
+			printf "%s" a &&
+			printf "%s" b &&
+			sleep 100
+		} >input &
+	} &&
+	test-line-buffer input <<-\EOF >actual &&
+	read 1
+	copy 1
+	EOF
+	kill $! &&
+	test_cmp expect actual
+'
+
+test_expect_success PIPE 'long read (around 8192 bytes)' '
+	long_read_test 8192
+'
+
+test_expect_success PIPE,EXPENSIVE 'longer read (around 65536 bytes)' '
+	long_read_test 65536
+'
+
 test_expect_success 'buffer_read_string copes with null byte' '
 	>expect &&
 	q_to_nul <<-\EOF | test-line-buffer >actual &&
diff --git a/test-line-buffer.c b/test-line-buffer.c
index da0bc65..ec19b13 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -49,15 +49,31 @@ static void handle_line(const char *line, struct line_buffer *stdin_buf)
 int main(int argc, char *argv[])
 {
 	struct line_buffer stdin_buf = LINE_BUFFER_INIT;
+	struct line_buffer file_buf = LINE_BUFFER_INIT;
+	struct line_buffer *input = &stdin_buf;
+	const char *filename;
 	char *s;
 
-	if (argc != 1)
-		usage("test-line-buffer < script");
+	if (argc == 1)
+		filename = NULL;
+	else if (argc == 2)
+		filename = argv[1];
+	else
+		usage("test-line-buffer [file] < script");
 
 	if (buffer_init(&stdin_buf, NULL))
 		die_errno("open error");
+	if (filename) {
+		if (buffer_init(&file_buf, filename))
+			die_errno("error opening %s", filename);
+		input = &file_buf;
+	}
+
 	while ((s = buffer_read_line(&stdin_buf)))
-		handle_line(s, &stdin_buf);
+		handle_line(s, input);
+
+	if (filename && buffer_deinit(&file_buf))
+		die("error reading from %s", filename);
 	if (buffer_deinit(&stdin_buf))
 		die("input error");
 	if (ferror(stdout))
-- 
1.7.4.rc0.580.g89dc.dirty

Re: [PATCH 8/8] t0081 (line-buffer): add buffering tests

[Jonathan Nieder]

Jonathan Nieder wrote:

> First check that fread can handle a 0-length read from an empty fifo.
> The writing end of the fifo is opened in advance in a subshell since
> even 0-length reads are allowed to block when the writing end of a
> pipe is not open.

Erm, what I mean here is that open(O_RDONLY) will block until the
writing end has been opened.

[PATCHES 9-12/12] line_buffer: more wrappers around stdio functions

[Jonathan Nieder]

This might be the last batch of line_buffer patches for the moment[1].
It introduces wrappers around strbuf_fread, fgetc, fdopen, and tmpfile
so the line_buffer lib can do what those functions do.

The motivation in the background is delta application.  We need:

 - binary-safe input, since svn (like most version control systems)
   gets used to store binary files from time to time.
 - character-oriented input (fgetc) as a basic convenience, needed in
   particular for reading variable-length integers in svndiff blocks.
 - input from file descriptors, to read information requested from
   fast-import (in particular, delta preimages).
 - temporary files, to store the result of delta application and
   retrieve its length so svn-fe can write

	data <length>
	... delta application result ...

   to fast-import.

The ideas behind the third and fourth patches (patches 11 and 12) are
from David Barr's earlier work in the same direction.  Patches are
based against

  [PATCH 8/8] t0081 (line-buffer): add buffering tests

so we can reuse some of the testing infrastructure.  They are numbered
accordingly for easy application.

Each patch introduces new API.  I would be happy if you can find an
infelicity or two so we can fix the functions now before people get
used to them.

Jonathan Nieder (4):
  vcs-svn: add binary-safe read function
  vcs-svn: allow character-oriented input
  vcs-svn: allow input from file descriptor
  vcs-svn: teach line_buffer about temporary files

 t/t0081-line-buffer.sh  |   27 +++++++++++++++++++++++++++
 test-line-buffer.c      |   21 ++++++++++++++++++---
 vcs-svn/line_buffer.c   |   43 +++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/line_buffer.h   |   10 +++++++++-
 vcs-svn/line_buffer.txt |   31 +++++++++++++++++++++++++++----
 5 files changed, 124 insertions(+), 8 deletions(-)

[1] There's another patch to use 64-bit offsets but the API changes
are more obvious.

[PATCH 09/12] vcs-svn: add binary-safe read function

[Jonathan Nieder]

buffer_read_string works well for non line-oriented input except for
one problem: it does not tell the caller how many bytes were actually
written.  This means that unless one is very careful about checking
for errors (and eof) the calling program cannot tell the difference
between the string "foo" followed by an early end of file and the
string "foo\0bar\0baz".

So introduce a variant that reports the length, too, a thinner wrapper
around strbuf_fread.  Its result is written to a strbuf so the caller
does not need to keep track of the number of bytes read.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 t/t0081-line-buffer.sh |   18 ++++++++++++++++++
 test-line-buffer.c     |   10 ++++++++++
 vcs-svn/line_buffer.c  |    6 ++++++
 vcs-svn/line_buffer.h  |    1 +
 4 files changed, 35 insertions(+), 0 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index 33a728e..a8eeb20 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -151,6 +151,15 @@ test_expect_success 'skip, copy null byte' '
 	test_cmp expect actual
 '
 
+test_expect_success 'read null byte' '
+	echo ">QhelloQ" | q_to_nul >expect &&
+	q_to_nul <<-\EOF | test-line-buffer >actual &&
+	binary 8
+	QhelloQ
+	EOF
+	test_cmp expect actual
+'
+
 test_expect_success 'long reads are truncated' '
 	echo foo >expect &&
 	test-line-buffer <<-\EOF >actual &&
@@ -171,4 +180,13 @@ test_expect_success 'long copies are truncated' '
 	test_cmp expect actual
 '
 
+test_expect_success 'long binary reads are truncated' '
+	echo ">foo" >expect &&
+	test-line-buffer <<-\EOF >actual &&
+	binary 5
+	foo
+	EOF
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-line-buffer.c b/test-line-buffer.c
index ec19b13..19bf2d4 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -3,6 +3,7 @@
  */
 
 #include "git-compat-util.h"
+#include "strbuf.h"
 #include "vcs-svn/line_buffer.h"
 
 static uint32_t strtouint32(const char *s)
@@ -17,6 +18,15 @@ static uint32_t strtouint32(const char *s)
 static void handle_command(const char *command, const char *arg, struct line_buffer *buf)
 {
 	switch (*command) {
+	case 'b':
+		if (!prefixcmp(command, "binary ")) {
+			struct strbuf sb = STRBUF_INIT;
+			strbuf_addch(&sb, '>');
+			buffer_read_binary(buf, &sb, strtouint32(arg));
+			fwrite(sb.buf, 1, sb.len, stdout);
+			strbuf_release(&sb);
+			return;
+		}
 	case 'c':
 		if (!prefixcmp(command, "copy ")) {
 			buffer_copy_bytes(buf, strtouint32(arg));
diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 806932b..661b007 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -56,6 +56,12 @@ char *buffer_read_string(struct line_buffer *buf, uint32_t len)
 	return ferror(buf->infile) ? NULL : buf->blob_buffer.buf;
 }
 
+void buffer_read_binary(struct line_buffer *buf,
+				struct strbuf *sb, uint32_t size)
+{
+	strbuf_fread(sb, size, buf->infile);
+}
+
 void buffer_copy_bytes(struct line_buffer *buf, uint32_t len)
 {
 	char byte_buffer[COPY_BUFFER_LEN];
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index fb37390..0c2d3d9 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -16,6 +16,7 @@ int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_deinit(struct line_buffer *buf);
 char *buffer_read_line(struct line_buffer *buf);
 char *buffer_read_string(struct line_buffer *buf, uint32_t len);
+void buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, uint32_t len);
 void buffer_copy_bytes(struct line_buffer *buf, uint32_t len);
 void buffer_skip_bytes(struct line_buffer *buf, uint32_t len);
 void buffer_reset(struct line_buffer *buf);
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 10/12] vcs-svn: allow character-oriented input

[Jonathan Nieder]

buffer_read_char can be used in place of buffer_read_string(1) to
avoid consuming valuable static buffer space.  The delta applier will
use this to read variable-length integers one byte at a time.

Underneath, it is fgetc, wrapped so the line_buffer library can
maintain its role as gatekeeper of input.

Later it might be worth checking if fgetc_unlocked is faster ---
most line_buffer functions are not thread-safe anyway.

Helpd-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 vcs-svn/line_buffer.c |    5 +++++
 vcs-svn/line_buffer.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 661b007..37ec56e 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -27,6 +27,11 @@ int buffer_deinit(struct line_buffer *buf)
 	return err;
 }
 
+int buffer_read_char(struct line_buffer *buf)
+{
+	return fgetc(buf->infile);
+}
+
 /* Read a line without trailing newline. */
 char *buffer_read_line(struct line_buffer *buf)
 {
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 0c2d3d9..0a59c73 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -16,6 +16,7 @@ int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_deinit(struct line_buffer *buf);
 char *buffer_read_line(struct line_buffer *buf);
 char *buffer_read_string(struct line_buffer *buf, uint32_t len);
+int buffer_read_char(struct line_buffer *buf);
 void buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, uint32_t len);
 void buffer_copy_bytes(struct line_buffer *buf, uint32_t len);
 void buffer_skip_bytes(struct line_buffer *buf, uint32_t len);
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 11/12] vcs-svn: allow input from file descriptor

[Jonathan Nieder]

Based-on-patch-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
No fd_buffer.  This is essentially the way David did it in the first
place; sorry to have doubted.

 t/t0081-line-buffer.sh  |    9 +++++++++
 test-line-buffer.c      |   11 ++++++++---
 vcs-svn/line_buffer.c   |    8 ++++++++
 vcs-svn/line_buffer.h   |    1 +
 vcs-svn/line_buffer.txt |    9 +++++----
 5 files changed, 31 insertions(+), 7 deletions(-)

diff --git a/t/t0081-line-buffer.sh b/t/t0081-line-buffer.sh
index a8eeb20..550fad0 100755
--- a/t/t0081-line-buffer.sh
+++ b/t/t0081-line-buffer.sh
@@ -131,6 +131,15 @@ test_expect_success PIPE,EXPENSIVE 'longer read (around 65536 bytes)' '
 	long_read_test 65536
 '
 
+test_expect_success 'read from file descriptor' '
+	rm -f input &&
+	echo hello >expect &&
+	echo hello >input &&
+	echo copy 6 |
+	test-line-buffer "&4" 4<input >actual &&
+	test_cmp expect actual
+'
+
 test_expect_success 'buffer_read_string copes with null byte' '
 	>expect &&
 	q_to_nul <<-\EOF | test-line-buffer >actual &&
diff --git a/test-line-buffer.c b/test-line-buffer.c
index 19bf2d4..25b20b9 100644
--- a/test-line-buffer.c
+++ b/test-line-buffer.c
@@ -69,13 +69,18 @@ int main(int argc, char *argv[])
 	else if (argc == 2)
 		filename = argv[1];
 	else
-		usage("test-line-buffer [file] < script");
+		usage("test-line-buffer [file | &fd] < script");
 
 	if (buffer_init(&stdin_buf, NULL))
 		die_errno("open error");
 	if (filename) {
-		if (buffer_init(&file_buf, filename))
-			die_errno("error opening %s", filename);
+		if (*filename == '&') {
+			if (buffer_fdinit(&file_buf, strtouint32(filename + 1)))
+				die_errno("error opening fd %s", filename + 1);
+		} else {
+			if (buffer_init(&file_buf, filename))
+				die_errno("error opening %s", filename);
+		}
 		input = &file_buf;
 	}
 
diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index 37ec56e..e29a81a 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -17,6 +17,14 @@ int buffer_init(struct line_buffer *buf, const char *filename)
 	return 0;
 }
 
+int buffer_fdinit(struct line_buffer *buf, int fd)
+{
+	buf->infile = fdopen(fd, "r");
+	if (!buf->infile)
+		return -1;
+	return 0;
+}
+
 int buffer_deinit(struct line_buffer *buf)
 {
 	int err;
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 0a59c73..630d83c 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -13,6 +13,7 @@ struct line_buffer {
 #define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL}
 
 int buffer_init(struct line_buffer *buf, const char *filename);
+int buffer_fdinit(struct line_buffer *buf, int fd);
 int buffer_deinit(struct line_buffer *buf);
 char *buffer_read_line(struct line_buffer *buf);
 char *buffer_read_string(struct line_buffer *buf, uint32_t len);
diff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt
index f8eaa4d..4e8fb71 100644
--- a/vcs-svn/line_buffer.txt
+++ b/vcs-svn/line_buffer.txt
@@ -27,10 +27,11 @@ resources.
 Functions
 ---------
 
-`buffer_init`::
-	Open the named file for input.  If filename is NULL,
-	start reading from stdin.  On failure, returns -1 (with
-	errno indicating the nature of the failure).
+`buffer_init`, `buffer_fdinit`::
+	Open the named file or file descriptor for input.
+	buffer_init(buf, NULL) prepares to read from stdin.
+	On failure, returns -1 (with errno indicating the nature
+	of the failure).
 
 `buffer_deinit`::
 	Stop reading from the current file (closing it unless
-- 
1.7.4.rc0.580.g89dc.dirty

[PATCH 12/12] vcs-svn: teach line_buffer about temporary files

[Jonathan Nieder]

It can sometimes be useful to write information temporarily to file,
to read back later.  These functions allow a program to use the
line_buffer facilities when doing so.

It works like this:

 1. find a unique filename with buffer_tmpfile_init.
 2. rewind with buffer_tmpfile_rewind.  This returns a stdio
    handle for writing.
 3. when finished writing, declare so with
    buffer_tmpfile_prepare_to_read.  The return value indicates
    how many bytes were written.
 4. read whatever portion of the file is needed.
 5. if finished, remove the temporary file with buffer_deinit.
    otherwise, go back to step 2,

The svn support would use this to buffer the postimage from delta
application until the length is known and fast-import can receive
the resulting blob.

Based-on-patch-by: David Barr <david.barr@cordelta.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
David, this is another piece of infrastructure from early svn-fe3
history.  I've cleaned up the API a little bit but the idea is the
same.  Thank you.

 vcs-svn/line_buffer.c   |   24 ++++++++++++++++++++++++
 vcs-svn/line_buffer.h   |    7 ++++++-
 vcs-svn/line_buffer.txt |   22 ++++++++++++++++++++++
 3 files changed, 52 insertions(+), 1 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index e29a81a..aedf105 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -25,6 +25,14 @@ int buffer_fdinit(struct line_buffer *buf, int fd)
 	return 0;
 }
 
+int buffer_tmpfile_init(struct line_buffer *buf)
+{
+	buf->infile = tmpfile();
+	if (!buf->infile)
+		return -1;
+	return 0;
+}
+
 int buffer_deinit(struct line_buffer *buf)
 {
 	int err;
@@ -35,6 +43,22 @@ int buffer_deinit(struct line_buffer *buf)
 	return err;
 }
 
+FILE *buffer_tmpfile_rewind(struct line_buffer *buf)
+{
+	rewind(buf->infile);
+	return buf->infile;
+}
+
+long buffer_tmpfile_prepare_to_read(struct line_buffer *buf)
+{
+	long pos = ftell(buf->infile);
+	if (pos < 0)
+		return error("ftell error: %s", strerror(errno));
+	if (fseek(buf->infile, 0, SEEK_SET))
+		return error("seek error: %s", strerror(errno));
+	return pos;
+}
+
 int buffer_read_char(struct line_buffer *buf)
 {
 	return fgetc(buf->infile);
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 630d83c..96ce966 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -15,12 +15,17 @@ struct line_buffer {
 int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_fdinit(struct line_buffer *buf, int fd);
 int buffer_deinit(struct line_buffer *buf);
+void buffer_reset(struct line_buffer *buf);
+
+int buffer_tmpfile_init(struct line_buffer *buf);
+FILE *buffer_tmpfile_rewind(struct line_buffer *buf);	/* prepare to write. */
+long buffer_tmpfile_prepare_to_read(struct line_buffer *buf);
+
 char *buffer_read_line(struct line_buffer *buf);
 char *buffer_read_string(struct line_buffer *buf, uint32_t len);
 int buffer_read_char(struct line_buffer *buf);
 void buffer_read_binary(struct line_buffer *buf, struct strbuf *sb, uint32_t len);
 void buffer_copy_bytes(struct line_buffer *buf, uint32_t len);
 void buffer_skip_bytes(struct line_buffer *buf, uint32_t len);
-void buffer_reset(struct line_buffer *buf);
 
 #endif
diff --git a/vcs-svn/line_buffer.txt b/vcs-svn/line_buffer.txt
index 4e8fb71..e89cc41 100644
--- a/vcs-svn/line_buffer.txt
+++ b/vcs-svn/line_buffer.txt
@@ -24,6 +24,28 @@ The calling program:
 When finished, the caller can use `buffer_reset` to deallocate
 resources.
 
+Using temporary files
+---------------------
+
+Temporary files provide a place to store data that should not outlive
+the calling program.  A program
+
+ - initializes a `struct line_buffer` to LINE_BUFFER_INIT
+ - requests a temporary file with `buffer_tmpfile_init`
+ - acquires an output handle by calling `buffer_tmpfile_rewind`
+ - uses standard I/O functions like `fprintf` and `fwrite` to fill
+   the temporary file
+ - declares writing is over with `buffer_tmpfile_prepare_to_read`
+ - can re-read what was written with `buffer_read_line`,
+   `buffer_read_string`, and so on
+ - can reuse the temporary file by calling `buffer_tmpfile_rewind`
+   again
+ - removes the temporary file with `buffer_deinit`, perhaps to
+   reuse the line_buffer for some other file.
+
+When finished, the calling program can use `buffer_reset` to deallocate
+resources.
+
 Functions
 ---------
 
-- 
1.7.4.rc0.580.g89dc.dirty

[FYI/PATCH] vcs-svn: give control over temporary file names

[Jonathan Nieder]

Allow users of the line_buffer library to specify what directory and
filename to use for temporary files.  For example:

	struct line_buffer tmp = LINE_BUFFER_INIT;
	if (buffer_tmpfile_init(&tmp, ".git", "svnfe_blob.XXXXXX"))
		die_errno("opening temporary file");
	...
	if (buffer_deinit(&tmp))
		die_errno("removing temporary file");

On Windows, something like this is needed if users without write
permission to the root directory are to be able to use temporary
files.

Unlike the implementation using tmpfile, this would not take
care of automatically removing the temporary file on exit.  The
user is responsible for now for installing appropriate signal and
atexit handlers to take care of that.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
I have an odd feeling this will be necessary at some point but I'm not
sure if it's a good idea or not.  Although tmpfile(3) is a limited
interface, it's pretty much exactly what we want.

See [1] for tmpfile on Windows.

This is just to get the idea out there.  I don't think this patch's
moment has come (though I'd be interested in your thoughts either
way).

[1] http://msdn.microsoft.com/en-us/library/x8x7sakw.aspx

 vcs-svn/line_buffer.c |   35 ++++++++++++++++++++++++++++++-----
 vcs-svn/line_buffer.h |    6 ++++--
 2 files changed, 34 insertions(+), 7 deletions(-)

diff --git a/vcs-svn/line_buffer.c b/vcs-svn/line_buffer.c
index aedf105..4131e08 100644
--- a/vcs-svn/line_buffer.c
+++ b/vcs-svn/line_buffer.c
@@ -3,7 +3,7 @@
  * See LICENSE for details.
  */
 
-#include "git-compat-util.h"
+#include "cache.h"
 #include "line_buffer.h"
 #include "strbuf.h"
 
@@ -25,12 +25,33 @@ int buffer_fdinit(struct line_buffer *buf, int fd)
 	return 0;
 }
 
-int buffer_tmpfile_init(struct line_buffer *buf)
+int buffer_tmpfile_init(struct line_buffer *buf,
+			const char *dirname, const char *pattern)
 {
-	buf->infile = tmpfile();
-	if (!buf->infile)
-		return -1;
+	int fd, saved_errno;
+	int mode = 0444;	/* just remove write permission. */
+	strbuf_addstr(&buf->temp_filename, dirname);
+	strbuf_addch(&buf->temp_filename, '/');
+	strbuf_addstr(&buf->temp_filename, pattern);
+
+	fd = git_mkstemp_mode(buf->temp_filename.buf, mode);
+	if (fd < 0) {
+		saved_errno = errno;
+		goto fail_mktemp;
+	}
+	buf->infile = fdopen(fd, "r+");
+	if (!buf->infile) {
+		saved_errno = errno;
+		goto fail_fdopen;
+	}
 	return 0;
+
+fail_fdopen:
+	close(fd);
+fail_mktemp:
+	strbuf_reset(&buf->temp_filename);
+	errno = saved_errno;
+	return -1;
 }
 
 int buffer_deinit(struct line_buffer *buf)
@@ -40,6 +61,9 @@ int buffer_deinit(struct line_buffer *buf)
 		return ferror(buf->infile);
 	err = ferror(buf->infile);
 	err |= fclose(buf->infile);
+	if (buf->temp_filename.len)
+		err |= unlink_or_warn(buf->temp_filename.buf);
+	strbuf_reset(&buf->temp_filename);
 	return err;
 }
 
@@ -129,4 +153,5 @@ void buffer_skip_bytes(struct line_buffer *buf, uint32_t len)
 void buffer_reset(struct line_buffer *buf)
 {
 	strbuf_release(&buf->blob_buffer);
+	strbuf_release(&buf->temp_filename);
 }
diff --git a/vcs-svn/line_buffer.h b/vcs-svn/line_buffer.h
index 96ce966..bd1a621 100644
--- a/vcs-svn/line_buffer.h
+++ b/vcs-svn/line_buffer.h
@@ -9,15 +9,17 @@ struct line_buffer {
 	char line_buffer[LINE_BUFFER_LEN];
 	struct strbuf blob_buffer;
 	FILE *infile;
+	struct strbuf temp_filename;
 };
-#define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL}
+#define LINE_BUFFER_INIT {"", STRBUF_INIT, NULL, STRBUF_INIT}
 
 int buffer_init(struct line_buffer *buf, const char *filename);
 int buffer_fdinit(struct line_buffer *buf, int fd);
 int buffer_deinit(struct line_buffer *buf);
 void buffer_reset(struct line_buffer *buf);
 
-int buffer_tmpfile_init(struct line_buffer *buf);
+int buffer_tmpfile_init(struct line_buffer *buf,
+		const char *directory, const char *pattern);
 FILE *buffer_tmpfile_rewind(struct line_buffer *buf);	/* prepare to write. */
 long buffer_tmpfile_prepare_to_read(struct line_buffer *buf);
 
-- 
1.7.4.rc2

[PULL svn-fe] fast-import 'ls', line-buffer changes

[Jonathan Nieder]

Hi Junio,

Please pull

  git://repo.or.cz/git/jrn.git svn-fe

to get the following changes on top of master + the old jn/svn-fe.

These are patches from the threads $gmane/164146 and $gmane/167536,
providing some plumbing to support incremental import in svn-fe.  They
are probably far from perfect but they seem to work okay so far, and
I'd be fine with putting any fixes on top of them.

Thoughts, suggestions, etc welcome as always.

David Barr (1):
      fast-import: add 'ls' command

Jonathan Nieder (13):
      vcs-svn: eliminate global byte_buffer
      vcs-svn: replace buffer_read_string memory pool with a strbuf
      vcs-svn: collect line_buffer data in a struct
      vcs-svn: teach line_buffer to handle multiple input files
      vcs-svn: make test-line-buffer input format more flexible
      tests: give vcs-svn/line_buffer its own test script
      vcs-svn: tweak test-line-buffer to not assume line-oriented input
      t0081 (line-buffer): add buffering tests
      vcs-svn: add binary-safe read function
      vcs-svn: allow character-oriented input
      vcs-svn: allow input from file descriptor
      vcs-svn: teach line_buffer about temporary files
      Merge commit 'jn/svn-fe' of git://github.com/gitster/git into svn-fe

 Documentation/git-fast-import.txt |   63 +++++++++++-
 fast-import.c                     |  162 +++++++++++++++++++++++++++++-
 t/t0080-vcs-svn.sh                |   54 ----------
 t/t0081-line-buffer.sh            |  201 +++++++++++++++++++++++++++++++++++++
 t/t9010-svn-fe.sh                 |    1 +
 t/t9300-fast-import.sh            |   92 +++++++++++++++--
 test-line-buffer.c                |   90 +++++++++++++----
 vcs-svn/fast_export.c             |    6 +-
 vcs-svn/fast_export.h             |    5 +-
 vcs-svn/line_buffer.c             |  105 +++++++++++++-------
 vcs-svn/line_buffer.h             |   33 +++++--
 vcs-svn/line_buffer.txt           |   36 ++++++-
 vcs-svn/svndump.c                 |   23 +++--
 13 files changed, 722 insertions(+), 149 deletions(-)

Re: [PULL svn-fe] fast-import 'ls', line-buffer changes

[David Michael Barr]

Hi,

> Please pull
> 
>  git://repo.or.cz/git/jrn.git svn-fe
> 
> to get the following changes on top of master + the old jn/svn-fe.
> 
> These are patches from the threads $gmane/164146 and $gmane/167536,
> providing some plumbing to support incremental import in svn-fe.  They
> are probably far from perfect but they seem to work okay so far, and
> I'd be fine with putting any fixes on top of them.
> 
> Thoughts, suggestions, etc welcome as always.

I'll try to put together a new topic based on this branch.
The target is incremental imports and removal of svn-fe's internal tree representation.
The patches that I intend to pick are as follows:

David Barr (9):
vcs-svn: drop obj_pool.h
vcs-svn: drop trp.h
vcs-svn: drop string_pool
vcs-svn: factor out usage of string_pool
vcs-svn: implement perfect hash for top-level keys
vcs-svn: implement perfect hash for node-prop keys
vcs-svn: avoid using ls command twice
vcs-svn: pass paths through to fast-import
vcs-svn: use mark from previous import for parent commit

Jonathan Nieder (7):
vcs-svn: handle filenames with dq correctly
vcs-svn: add a comment before each commit
vcs-svn: simplify repo_modify_path and repo_copy
vcs-svn: prepare to eliminate repo_tree structure
vcs-svn: save marks for imported commits
vcs-svn: use higher mark numbers for blobs
vcs-svn: introduce repo_read_path to check the content at a path

Re: [PULL svn-fe] fast-import 'ls', line-buffer changes

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Hi Junio,
>
> Please pull
>
>   git://repo.or.cz/git/jrn.git svn-fe
>
> to get the following changes on top of master + the old jn/svn-fe.
>
> These are patches from the threads $gmane/164146 and $gmane/167536,
> providing some plumbing to support incremental import in svn-fe.  They
> are probably far from perfect but they seem to work okay so far, and
> I'd be fine with putting any fixes on top of them.
>
> Thoughts, suggestions, etc welcome as always.

As I was not involved in the thread heavily, I'll just pull this into
'master', trusting that the responsible parties will be able to handle
potential fallouts to fast-import users, if any, promptly.

... Yikes.  I said the above and then my build for "master" breaks with

  fast-import.c: In function 'dereference':
  fast-import.c:2885: error: pointer of type 'void *' used in arithmetic
  fast-import.c:2890: error: pointer of type 'void *' used in arithmetic

forcing me to redo all three integration branches.  What an un-fun.

Not pulled.

 fast-import.c |    4 ++--
 1 files changed, 2 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6c37b84..cfddb7a 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2882,12 +2882,12 @@ static struct object_entry *dereference(struct object_entry *oe,
 	switch (oe->type) {
 	case OBJ_TAG:
 		if (size < 40 + strlen("object ") ||
-		    get_sha1_hex(buf + strlen("object "), sha1))
+		    get_sha1_hex((char *)buf + strlen("object "), sha1))
 			die("Invalid SHA1 in tag: %s", command_buf.buf);
 		break;
 	case OBJ_COMMIT:
 		if (size < 40 + strlen("tree ") ||
-		    get_sha1_hex(buf + strlen("tree "), sha1))
+		    get_sha1_hex((char *)buf + strlen("tree "), sha1))
 			die("Invalid SHA1 in commit: %s", command_buf.buf);
 	}
 

[PATCH svn-fe] fast-import: make code "-Wpointer-arith" clean

[Jonathan Nieder]

The dereference() function to peel a tree-ish and find the underlying
tree expects arithmetic to (void *) to work on byte addresses.  We
should be reading the text of objects through a char * anyway.

Noticed-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Junio C Hamano wrote:

> ... Yikes.  I said the above and then my build for "master" breaks with
>
>   fast-import.c: In function 'dereference':
>   fast-import.c:2885: error: pointer of type 'void *' used in arithmetic
>   fast-import.c:2890: error: pointer of type 'void *' used in arithmetic

This should fix it, I suppose?  A

	-std=c99 -O3 -Wall -W
	-Wno-sign-compare
	-Wno-unused-parameter
	-Wno-missing-field-initializers
	-Wno-empty-body
	-Wno-pointer-to-int-cast
	-Wno-type-limits
	-Wno-unused-but-set-variable
	-Wold-style-definition -Wpointer-arith -Wvla
	-Wdeclaration-after-statement -Werror

build passes, except for an "unsigned long expire" in builtin-reflog
that confuses this copy of gcc.

 fast-import.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 6c37b84..e1268b8 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2848,7 +2848,7 @@ static struct object_entry *dereference(struct object_entry *oe,
 					unsigned char sha1[20])
 {
 	unsigned long size;
-	void *buf = NULL;
+	char *buf = NULL;
 	if (!oe) {
 		enum object_type type = sha1_object_info(sha1, NULL);
 		if (type < 0)
-- 
1.7.4.1

Re: [PATCH svn-fe] fast-import: make code "-Wpointer-arith" clean

[Sverre Rabbelier]

Heya,

On Mon, Feb 28, 2011 at 22:32, Jonathan Nieder <jrnieder@gmail.com> wrote:
> This should fix it, I suppose?  A
>
>        -std=c99 -O3 -Wall -W
>        -Wno-sign-compare
>        -Wno-unused-parameter
>        -Wno-missing-field-initializers
>        -Wno-empty-body
>        -Wno-pointer-to-int-cast
>        -Wno-type-limits
>        -Wno-unused-but-set-variable
>        -Wold-style-definition -Wpointer-arith -Wvla
>        -Wdeclaration-after-statement -Werror
>
> build passes, except for an "unsigned long expire" in builtin-reflog
> that confuses this copy of gcc.

How come this slipped by unnoticed (except by Junio)? Is the default
Makefile not strict enough?

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH svn-fe] fast-import: make code "-Wpointer-arith" clean

[Junio C Hamano]

Sverre Rabbelier <srabbelier@gmail.com> writes:

> How come this slipped by unnoticed (except by Junio)? Is the default
> Makefile not strict enough?

No, it's just me being a tad overly paranoid.

Re: [PATCH svn-fe] fast-import: make code "-Wpointer-arith" clean

[Jonathan Nieder]

Jonathan Nieder wrote:

> This should fix it, I suppose?

Pushed out.  Thanks again.

Re: [PATCH svn-fe] fast-import: make code "-Wpointer-arith" clean

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> Jonathan Nieder wrote:
>
>> This should fix it, I suppose?
>
> Pushed out.  Thanks again.

Thanks, pulled.