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