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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html