Skip to contentSkip to conversation

teach vcs-svn/line_buffer to handle multiple input files

PATCH In review Jonathan Nieder 4 participants 25 messages Jun 15, 2016

lore
Share & tools
lore

Jonathan Nieder

author
#0 lore raw

cover teach vcs-svn/line_buffer to handle multiple input files

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

author
#1 lore raw

PATCH 1/4 vcs-svn: eliminate global byte_buffer

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 <[email protected]>

---
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(-)
vcs-svn/line_buffer.c +2 -1
index 1543567..f22c94f 100644
@@ -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;
Signature · 1 line
1.7.2.3.554.gc9b5c.dirty
author
#2 lore raw

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

In reply to 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 <[email protected]>

---
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(-)
vcs-svn/line_buffer.c +6 -10
index f22c94f..6f32f28 100644
@@ -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);
 }
Signature · 1 line
1.7.2.3.554.gc9b5c.dirty
author
#3 lore raw

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

In reply to 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 <[email protected]>

---
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(-)
vcs-svn/line_buffer.c +22 -23
index 6f32f28..e7bc230 100644
@@ -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);
 }
vcs-svn/line_buffer.h +11 -0
index 9c78ae1..4ae1133 100644
@@ -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);
Signature · 1 line
1.7.2.3.554.gc9b5c.dirty
author
#4 lore raw

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

In reply to 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 <[email protected]>

---
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(-)
test-line-buffer.c +9 -8
index c11bf7f..f9af892 100644
@@ -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;
 }
vcs-svn/fast_export.c +3 -3
index 6cfa256..260cf50 100644
@@ -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);
 }
vcs-svn/fast_export.h +4 -1
index 2aaaea5..054e7d5 100644
@@ -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
vcs-svn/line_buffer.c +8 -12
index e7bc230..806932b 100644
@@ -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);
 }
vcs-svn/line_buffer.h +7 -7
index 4ae1133..fb37390 100644
@@ -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
vcs-svn/line_buffer.txt +3 -2
index 8906fb1..f8eaa4d 100644
@@ -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
 ---------
vcs-svn/svndump.c +16 -13
index 53d0215..3bba0fe 100644
@@ -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);
Signature · 1 line
1.7.2.3.554.gc9b5c.dirty
author
#5 lore raw
In reply to Jonathan Nieder
Quoted 5 lines · Jonathan Nieder

[*]
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
author
#6 lore raw

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

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 <[email protected]>
---
 t/t0080-vcs-svn.sh |   18 ++++++++--------
 test-line-buffer.c |   56 +++++++++++++++++++++++++++++++++------------------
 2 files changed, 45 insertions(+), 29 deletions(-)
t/t0080-vcs-svn.sh +9 -9
index d3225ad..8be9700 100755
@@ -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
test-line-buffer.c +36 -20

75 lines not on this page · raw message

Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#7 lore raw

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

In reply to 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 <[email protected]>
---
 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
t/t0080-vcs-svn.sh +0 -54
index 8be9700..99a314b 100755
@@ -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 &&
t/t0081-line-buffer.sh new file +67 -0

67 lines not on this page · raw message

Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#8 lore raw

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

In reply to 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 <[email protected]>
---
 t/t0081-line-buffer.sh |   27 +++++++++++++--------------
 test-line-buffer.c     |   10 +++++++---
 2 files changed, 20 insertions(+), 17 deletions(-)
t/t0081-line-buffer.sh +13 -14
index 13ac735..68d6163 100755
@@ -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
test-line-buffer.c +7 -3
index 383f35b..da0bc65 100644
@@ -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:
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#9 lore raw

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

In reply to 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 <[email protected]>
---
 t/t0081-line-buffer.sh |  110 +++++++++++++++++++++++++++++++++++++++++++++++-
 test-line-buffer.c     |   22 ++++++++-
 2 files changed, 128 insertions(+), 4 deletions(-)
t/t0081-line-buffer.sh +109 -1
index 68d6163..33a728e 100755
@@ -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 &&
test-line-buffer.c +19 -3
index da0bc65..ec19b13 100644
@@ -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))
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#10 lore raw

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

Quoted 4 lines · Jonathan Nieder

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.

author
#11 lore raw

PATCH 1/1 line_buffer: more wrappers around stdio functions

In reply to 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.

author
#12 lore raw

PATCH 9/12 vcs-svn: add binary-safe read function

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 <[email protected]>
---
 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(-)
t/t0081-line-buffer.sh +18 -0
index 33a728e..a8eeb20 100755
@@ -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
test-line-buffer.c +10 -0
index ec19b13..19bf2d4 100644
@@ -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));
vcs-svn/line_buffer.c +6 -0
index 806932b..661b007 100644
@@ -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];
vcs-svn/line_buffer.h +1 -0
index fb37390..0c2d3d9 100644
@@ -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);
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#13 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>
---
 vcs-svn/line_buffer.c |    5 +++++
 vcs-svn/line_buffer.h |    1 +
 2 files changed, 6 insertions(+), 0 deletions(-)
vcs-svn/line_buffer.c +5 -0
index 661b007..37ec56e 100644
@@ -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)
 {
vcs-svn/line_buffer.h +1 -0
index 0c2d3d9..0a59c73 100644
@@ -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);
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#14 lore raw

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

In reply to Jonathan Nieder
Based-on-patch-by David Barr <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---
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(-)
t/t0081-line-buffer.sh +9 -0
index a8eeb20..550fad0 100755
@@ -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 &&
test-line-buffer.c +8 -3
index 19bf2d4..25b20b9 100644
@@ -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;
 	}
vcs-svn/line_buffer.c +8 -0
index 37ec56e..e29a81a 100644
@@ -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;
vcs-svn/line_buffer.h +1 -0
index 0a59c73..630d83c 100644
@@ -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);
vcs-svn/line_buffer.txt +5 -4
index f8eaa4d..4e8fb71 100644
@@ -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
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#15 lore raw

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

In reply to 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 <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---
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(-)
vcs-svn/line_buffer.c +24 -0
index e29a81a..aedf105 100644
@@ -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);
vcs-svn/line_buffer.h +6 -1
index 630d83c..96ce966 100644
@@ -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
vcs-svn/line_buffer.txt +22 -0
index 4e8fb71..e89cc41 100644
@@ -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
 ---------
Signature · 1 line
1.7.4.rc0.580.g89dc.dirty
author
#16 lore raw

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

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 <[email protected]>

---
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(-)
vcs-svn/line_buffer.c +30 -5
index aedf105..4131e08 100644
@@ -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);
 }
vcs-svn/line_buffer.h +4 -2
index 96ce966..bd1a621 100644
@@ -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);
Signature · 1 line
1.7.4.rc2
author
#17 lore raw

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

In reply to 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(-)
#18 lore raw

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

Hi,

Quoted 4 lines

Please pull

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

Quoted 8 lines

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

#19 lore raw

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

In reply to Jonathan Nieder
Quoted 6 lines · Jonathan Nieder

Hi Junio,

Please pull

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

Quoted 8 lines

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(-)
fast-import.c +2 -2
index 6c37b84..cfddb7a 100644
@@ -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);
 	}
author
#20 lore raw

PATCH 1/1 fast-import: make code "-Wpointer-arith" clean

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 <[email protected]>
Signed-off-by Jonathan Nieder <[email protected]>

---

Quoted 5 lines · Junio C Hamano

... 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(-)
fast-import.c +1 -1
index 6c37b84..e1268b8 100644
@@ -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)
Signature · 1 line
1.7.4.1
author
#21 lore raw

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

Quoted 1 line · Jonathan Nieder

This should fix it, I suppose?

Pushed out. Thanks again.

#22 lore raw

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

Quoted 5 lines · Jonathan Nieder
Quoted 1 line · Jonathan Nieder

This should fix it, I suppose?

Pushed out. Thanks again.

Thanks, pulled.

#23 lore raw

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

In reply to Jonathan Nieder

Heya,

Quoted 13 lines · Jonathan Nieder

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

Quoted 2 lines

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?

Signature · 3 lines
Cheers,

Sverre Rabbelier
#24 lore raw

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

Quoted 2 lines · Sverre Rabbelier

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.