Re: [PATCH 5/5] svn-fe: Use the cat-blob command to apply deltas

Hi Ram,

Glad to see you are feeling a little better.

Ramkumar Ramachandra wrote:

David Barr writes:

quoted

+	if (!backchannel.infile)
+		backchannel.infile = fdopen(REPORT_FILENO, "r");
+	if (!backchannel.infile)
+		return error("Could not open backchannel fd: %d", REPORT_FILENO);

REPORT_FILENO = 3 is hard-coded. Is this intended? Maybe a
command-line option to specify the fd?

fast-import gets the --cat-file-fd parameter to choose between stdout,
stdin-as-socket, stderr, or another fd (not necessarily 3 because it
might have to compete with other similar features some day).

For svn-fe, it is just like another stdin.  stdin is always fd 0,
so...

For callers other than svn-fe, it would be especially useful to
make it configurable, yes.

quoted

+	tail = buffer_read_line(&backchannel);
+	if (!tail)
+		return 1;

Could you clarify when exactly will this happen?

buffer_read_line() returns NULL on error and when data is exhausted
without the trailing newline appearing.  The input here is supposed to
be just a single newline (trimmed to an empty string).

quoted

+	long preimage_len = 0;
+
+	if (delta) {
+		if (!preimage.infile)
+			preimage.infile = tmpfile();

Didn't you later decide against this and use one tmpfile instead?

This is a single tempfile (because static).  Or am I missing
something?

quoted

+		if (!preimage.infile)
+			die("Unable to open temp file for blob retrieval");
+		if (srcMark) {
+			printf("cat-blob :%"PRIu32"\n", srcMark);
+			fflush(stdout);
+			if (srcMode == REPO_MODE_LNK)
+				fwrite("link ", 1, 5, preimage.infile);

Special handling for symbolic links. Perhaps you should mention it in
a comment here?

Or better yet, a comment in the commit message. :)

quoted

+			if (fast_export_save_blob(preimage.infile))
+				die("Failed to retrieve blob for delta application");
+		}
+		preimage_len = ftell(preimage.infile);
+		fseek(preimage.infile, 0, SEEK_SET);
+		if (!postimage.infile)
+			postimage.infile = tmpfile();

One tmpfile?

Do you mean letting the preimage and postimage share a file?

[...]

quoted

 	printf("blob\nmark :%"PRIu32"\ndata %"PRIu32"\n", mark, len);
-	buffer_copy_bytes(input, stdout, len);
+	if (!delta)
+		buffer_copy_bytes(input, stdout, len);
+	else
+		buffer_copy_bytes(&postimage, stdout, len);
 	fputc('\n', stdout);

I should have asked this a long time ago: why the extra newline?

From the fast-import manual:

	The LF after <raw> is optional (it used to be required)
	but recommended. Always including it makes debugging a
	fast-import stream easier as the next command always
	starts in column 0 of the next line, even if <raw> did
	not end with an LF.

Overall, pleasant read. Thanks for taking this forward.

Seconded.  Thanks, both.