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

[Ramkumar Ramachandra <artagnon@gmail.com>]

Hi Jonathan,

Jonathan Nieder writes:
> 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.

Right, got it.

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

Thanks for the clarification.

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

Er, sorry about that. When I saw this code, it immediately reminded me
of one of David's commits that used several temporary files- a later
one made it a global variable. I didn't notice the static here.

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

*nod*

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

No :)

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

Thanks for the explanation. I really should have looked this up
earlier, but I suppose it's not a biggie.

-- Ram