Re: [RFC 1/4 v2] Implement a basic remote helper for svn in C.

[Jonathan Nieder <jrnieder@gmail.com>]

Florian Achleitner wrote:

> So I should kick printd out?

I think so, yes.

"git log -SGIT_TRANSPORT_HELPER_DEBUG transport-helper.c" tells me
that that option was added to make the transport-helper machinery make
noise to make it obvious at what stage a remote helper has deadlocked.

GIT_TRANSPORT_HELPER_DEBUG already takes care of that, so there would
not be need for an imitation of that in remote-svn, unless I am
missing something (and even if I am missing something, it seems
complicated enough to be worth moving to another patch where it can be
explained more easily).

[...]
>>>>> +
>>>>> +	printf("import\n");
>>>>> +	printf("\n");
>>>>> +	fflush(stdout);
>>>>> +	return SUCCESS;
>>>>> +}
[...]
>>                                Maybe the purpose of the flush would
>> be more obvious if it were moved to the caller.
>
> Acutally this goes to the git parent process (not fast-import), waiting for a 
> reply to the command. I think I have to call flush on this side of the pipe. 
> Can you flush it from the reader? This wouldn't have the desired effect, it 
> drops buffered data.

*slaps head*  This is the "capabilities" command, and it needs to
flush because the reader needs to know what commands it's allowed to
use next before it starts using them.  My brain turned off and I
thought you were emitting an "import" command rather than advertising
that you support it for some reason.

And 'printf("\n")' was a separate printf because that way, patches
like

	 	printf("import\n");
	+	printf("bidi-import\n");
	 	printf("\n");
	 	fflush(stdout);

become simpler.

I'm tempted to suggest a structure like

		const char * const capabilities[] = {"import"};
		int i;

		for (i = 0; i < ARRAY_SIZE(capabilities); i++)
			puts(capabilities[i]);
		puts("");	/* blank line */

		fflush(stdout);

but your original code was fine, too.

[...]
>>>>> +	/* opening a fifo for usually reading blocks until a writer has opened
>>>>> it too. +	 * Therefore, we open with RDWR.
>>>>> +	 */
>>>>> +	report_fd = open(back_pipe_env, O_RDWR);
>>>>> +	if(report_fd < 0) {
>>>>> +		die("Unable to open fast-import back-pipe! %s", strerror(errno));
[...]
> I believe it can be solved using RDONLY and WRONLY too. Probably we solve it 
> by not using the fifo at all.
> Currently the blocking comes from the fact, that fast-import doesn't parse 
> it's command line at startup. It rather reads an input line first and decides 
> whether to parse the argv after reading the first input line or at the end of 
> the input. (don't know why)
> remote-svn opens the pipe before sending the first command to fast-import and 
> blocks on the open, while fast-import waits for input --> deadlock.

Thanks for explaining.  Now we've discussed a few different approproaches,
none of which is perfect.

a. use --cat-blob-fd, no FIFO

   Doing this unconditionally would break platforms that don't support
   --cat-blob-fd=(descriptor >2), like Windows, so we'd have to:

   * Make it conditional --- only do it (1) we are not on Windows and
     (2) the remote helper requests backflow by advertising the
     import-bidi capability.

   * Let the remote helper know what's going on by using
     "import-bidi" instead of "import" in the command stream to
     initiate the import.

b. use envvars to pass around FIFO path

   This complicates the fast-import interface and makes debugging hard.
   It would be nice to avoid this if we can, but in case we can't, it's
   nice to have the option available.

c. transport-helper.c uses FIFO behind the scenes.

   Like (a), except it would require a fast-import tweak (boo) and
   would work on Windows (yea)

d. use --cat-blob-fd with FIFO

   Early scripted remote-svn prototypes did this to fulfill "fetch"
   requests.

   It has no advantage over "use --cat-blob-fd, no FIFO" except being
   easier to implement as a shell script.  I'm listing this just for
   comparison; since (a) looks better in every way, I don't see any
   reason to pursue this one.

Since avoiding deadlocks with bidirectional communication is always a
little subtle, it would be nice for this to be implemented once in
transport-helper.c rather than each remote helper author having to
reimplement it again.  As a result, my knee-jerk ranking is a > c >
b > d.

Sane?
Jonathan