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

[Jonathan Nieder <jrnieder@gmail.com>]

Hi Ram,

Glad to see you are feeling a little better.

Ramkumar Ramachandra wrote:
> David Barr writes:

>> +	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.

>> +	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).

>> +	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?

>> +		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. :)

>> +			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?

[...]
>>  	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.