[RFC 1/4] Implement a basic remote helper vor svn in C.

Series of patches creating a very basic remote helper in C.

[RFC 1/4] Implement a basic remote helper vor svn in C. [RFC 2/4] Integrate remote-svn into svn-fe/Makefile. [RFC 3/4] Add svndump_init_fd to allow reading dumps from arbitrary [RFC 4/4] Add cat-blob report pipe from fast-import to

Experimental implementation. Inspired by the existing shell script at divanorama/remote-svn-alpha. It doesn't use marks or notes yet, always imports the full history.

svnrdump is started as a subprocess while svn-fe's functions are called directly.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/remote-svn.c |  188 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 188 insertions(+)
 create mode 100644 contrib/svn-fe/remote-svn.c

Requires some sha.h to be used and the libraries to be linked, this is currently hardcoded.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 contrib/svn-fe/Makefile |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

The existing function only allowed reading from a filename or from stdin.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/svndump.c |   20 +++++++++++++++++---
 vcs-svn/svndump.h |    1 +
 2 files changed, 18 insertions(+), 3 deletions(-)

On invocation of a helper a new pipe is opened. To close the other end after fork, the prexec_cb feature of the run_command api is used. If the helper is not used with fast-import later the pipe is unused. The FD is passed to the remote-helper via it's environment, helpers that don't use fast-import can simply ignore it. fast-import has an argv for that.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 transport-helper.c |   48 ++++++++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/svndump.c  |    9 ++++++++-
 2 files changed, 56 insertions(+), 1 deletion(-)

On Tue, Jun 5, 2012 at 3:20 AM, Florian Achleitner <florian.achleitner.2.6.31@gmail.com> wrote:

+cc Sverre Rabbelier and Jeff King, as they have been active in transport-helper.c.

-- David Barr

On Mon, Jun 04, 2012 at 07:20:55PM +0200, Florian Achleitner wrote:

I don't keep up on fast-import development, so I have no clue how useful this extra pipe is, or whether this patch is a good idea overall. But a few comments on the transport.c half of things:

Note that preexec_cb does not work at all on Windows, as it assumes a forking model (rather than a spawn, which leaves no room to execute arbitrary code in the child). If all you want to do is open an extra pipe, then probably run-command should be extended to handle this (though I have no idea how complex that would be for the Windows side of things, it is at least _possible_, as opposed to preexec_cb, which will never be possible).

Consider converting this to use argv_array. You can drop the magic numbers, and "argv_array_pushf" handles the strbuf bits for you automatically.

And this grossness can go away:

(you'd instead want to free everything; it would probably make sense to add an argv_array_free_detached() function to do so).

I'm confused. We close both ends? Who is actually reading and writing to this pipe, then?

-Peff

On Tue, Jun 5, 2012 at 4:56 PM, Jeff King <peff@peff.net> wrote:

One child, git-fast-import writes to one end. The other child, git-remote-* reads from the other end.

-- David Barr

On Tue, Jun 05, 2012 at 05:07:53PM +1000, David Michael Barr wrote:

Ah, thanks. I missed where the write end was going, but now I see it. Overall, the point of the patch makes sense to me (it would have been nice if the commit message described the rationale a bit more completely).

Is there a reason that the patch unconditionally creates the pipe in get_helper? I.e., isn't it specific to the get_importer code path? It feels a little hacky to have it infect the other code paths.

-Peff

On Tuesday 05 June 2012 04:14:02 Jeff King wrote:

I agree, it's a bit hacky. For me as a newbee, it was just a way to make fast- import have the pipe it needs. I didn't know about the history of the preexec_cb as a fix for a bug in less.

The pipe is created unconditionally, because at the fork-time of the remote- helper it is not known whether the import command will be used later together with fast-import, or not. (and later, there's no way, I think). Helpers that don't use the pipe could simply ignore it.

-- Florian Achleitner

On Wed, Jun 06, 2012 at 12:16:25AM +0200, Florian Achleitner wrote:

Good point. I think we really are stuck with doing it in every case, unless we want to turn to something that can be opened after the fact (like a fifo).

-Peff

On Wednesday 06 June 2012 09:43:21 you wrote:

A fifo could be clever. There's also something similar on windows, maybe we can use named pipes there too.

-- Flo

Am 6/5/2012 8:56, schrieb Jeff King:

The lack of support for preexec_cb on Windows is actually not the problem in this case. Our emulation of pipe() actually creates file handles that are not inherited by child processes. (For the standard channels 0,1,2 we rely on that dup() creates duplicates that *can* be inherited; so they still work.)

The first problem with the new infrastructure in this patch is that dup() is not called anywhere after pipe(). To solve this, we would have to extend run-command in some way to allow passing along arbitrary pipes and handles.

The second problem is more severe and is at the lowest level of our infrastructure: We set up our child processes so that they know only about file descriptors other than 0,1,2 to the child process. Even if the first problem were solved, the child process does not receive sufficient information to know that there are open file descriptors other than 0,1,2. There is a facility to pass along this information from the parent to the child, but we simply do not implement it.

IOW: Everything that uses --cat-blob-fd or a similar facility cannot work
on Windows without considerable additional effort.

-- Hannes

On Tue, Jun 05, 2012 at 10:51:32AM +0200, Johannes Sixt wrote:

Thanks, Johannes, that makes sense to me.

Florian, does that mean that making the svn helper start to use --cat-blob-fd at all is a potential regression for Windows? The fast-import documentation says that the cat-blob output will go to stdout now. Does it even work at all now? I don't really know or understand all of the reasons for cat-blob-fd to exist in the first place.

I expect one answer might be "well, the svn remote helper does not work at all on Windows already, so there's no regression". But this affects _all_ fast-import calls that git's transport-helper makes. Are there other ones that use import, and would they be affected by this?

For that matter, isn't this a backwards-incompatible change for other third-party helpers? Won't they need to respect the new GIT_REPORT_FILENO environment variable? Do we need the helper to specify "yes, I am ready to handle cat-blob-fd" in its capabilities list?

-Peff

On Tuesday 05 June 2012 05:07:02 Jeff King wrote:

Actually I don't know. I'm don't know a lot about Windows support in git at all. I created the pipe, because the current vcs-svn/* code requires it for importing svn dumps. I can't explain yet why it requires it.

stdout is connected to the parent, while the new pipe connects it's two childs.. It works.

Good point.

I think whenever the remote helper uses some specific commands of fast-import it needs the cat-blob-fd to read feedback, but I haven't digged into that yet..

-- Florian Achleitner

Am 6/5/2012 10:51, schrieb Johannes Sixt:

That should read:

We set up our child processes so that they know only about file descriptors 0,1,2.

On Tue, Jun 5, 2012 at 3:20 AM, Florian Achleitner <florian.achleitner.2.6.31@gmail.com> wrote:

Note: filename is allowed to be NULL here.
This is a bug in the existing code that you just moved.

I suggest moving error printing into buffer_init(). This way the basis for the message is clearer.

For bonus points, we should split buffer_init(). Plain buffer_init() should use stdin. The new buff_init_path() should take a filename.

Otherwise, I like the direction of this patch.

-- David Barr

This new version uses a fifo instead of a pipe and addresses other issues discussed in this thread. To pass the name of the fifo to fast-import, it gets a new cmd-line-arg. It no longer requires the prexec_cb and should be more portable, as there exist pipes on windows too.

On Monday 04 June 2012 19:20:51 Florian Achleitner wrote:

Experimental implementation. Inspired by the existing shell script at divanorama/remote-svn-alpha. It doesn't use marks or notes yet, always imports the full history.

svnrdump is started as a subprocess while svn-fe's functions are called directly.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
diff: Use fifo instead of pipe: Retrieve the name of the pipe from env and open it
for svndump.

 contrib/svn-fe/remote-svn.c |  207 +++++++++++++++++++++++++++++++++++++++++++
 1 file changed, 207 insertions(+)
 create mode 100644 contrib/svn-fe/remote-svn.c

Hi,

Florian Achleitner wrote:

Ok, so this adds a new program named "remote-svn". How do I build it? What does it do? Will it make my life better?

[...]

I'd prefer to avoid this if possible, since it means having to decide where the pipe goes on the filesystem. Can you summarize the discussion in the commit message so future readers understand why we're doing it?

[...]

git-compat-util.h (or some header that includes it) must be the first header included so the appropriate feature test macros can be defined. See Documentation/CodingGuidelines for more on that.

Small nit: please drop the redundant "= 0" here.  Or:

Why not use trace_printf and avoid the complication?

[...]

Style: '*' attaches to the variable name, to avoid making declarations
like

	char *p, c;

confusing.

What's a cmd_result?  '*' sticks to variable name.

Oh, that's what a cmd_result is. :) Why not define the type before using it to avoid keeping the reader in suspense?

What does each result represent?  If this is a convention like

 1: handled
 0: not handled
 -1: error, callee takes care of printing the error message

then please document it in a comment near the caller so the reader can understand what is happening without too much confusion. Given such a comment, does the enum add clarity?

When I first read this, I wonder what is being commanded. Are these commands passed on the remote helper's standard input, commands passed on its output, or commands run at some point in the process? What is the effect and return value of associated function? Does the function always return some success/failure value, or does it sometimes exit?

Maybe a more specific type name would be clearer?

[...]

First association is to functions like cmd_fetch() which implement git subcommands. So I thought these were going to implement subcommands like "git remote-svn capabilities", "git remote-svn import" and would use the same cmd_foo(argc, argv, prefix) calling convention that git subcommands do. Maybe a different naming convention could avoid confusion.

[...]

Style: missing SP after keyword.

Why the multiple printf?  Is the flush needed?

[...]

Style: * goes with ... (I won't point out the rest of these.

I don't understand this comment.

Style: comments in git are spelled like this:

			/*
			 * Here I put a sentence or two explaining some
			 * relevant design decision or fact about the world
			 * that will provide useful context for
			 * understanding the following code.
			 */

I couldn't parse the above comment, so I'm skipping it for now.

[...]

Style: C99-style initializers are (unfortunately) not supported in
some compilers we want to support.

No need to initialize all fields --- any trailing unlisted fields are automatically initialized to zero.

Style: missing SP after keyword (I won't point out the rest of these).

Does this mean that expected usage is something like

	GIT_REPORT_FIFO=/tmp/foo/bar git clone svn::foo/bar/baz

?  And if I don't do that, I get

	fatal: Cannot get cat-blob-pipe from environment!

and am somehow supposed to understand what to do?

Is this necessary? Why shouldn't we fork the writer first and wait for it here?

Style: could simplify by using struct argv_array.

start_command() is supposed to have printed a message already when it fails, unless errno == ENOENT and silent_exec_failure was set.

Style: looks like some stray carriage returns snuck in.

Not your fault: this API looks a little overcomplicated.

Important?  Wouldn't finish_command do this?

What is the purpose of this step?

finish_command() is supposed to print a message when it fails.

Success even if it failed?

Blank lines seem to have snuck in.

Why is this variable?

Why the flush?

If possible, matching commands by name (like git.c does) would make the behavior easier to predict.

[...]

usage() can be used to write a clearer error message.

[...]

Style: no need for these braces.  usage() could be used to make it
clearer to the user what she can do next.

[...]

What is a private ref namespace path? An example would make the comment clearer.

errno will be meaningful; the message can be made clearer by using it.

Maybe this could use error() or die().

[...]

Won't this crash?

It would also be nice to add a test case to the t/ directory to make others changing this code do not accidentally break your new functionality.

Hope that helps, Jonathan

Jonathan Nieder wrote:

[...]

I forgot to say: thanks for working on this! I hope my comments are not demoralizing. They are meant in the opposite vein --- if I expected you to be a one-time contributor then I would just take what is useful from the patch and let it be, but I would be happy to see more changes from you in the future so I gave some hints to explain how.

The next step is to work with the list to figure out what a second version of the patch should do, and then to send that with something like "RFC/PATCH v2" in the subject line to clarify that it supersedes this one.

If you have any questions about the review or the codebase in general, please don't hesitate to ask. Especially, if you get stuck on something and documentation is unhelpful, please do complain. :)

Hope that helps, Jonathan

Hi Jonathan!

Thanks for your review! I will come up with a new version later after finishing some other feature.. Now I've done all exams (last 2 yesterday :) ) and submissions and commit myself to git and gsoc much more.

On Thursday 05 July 2012 19:30:24 Jonathan Nieder wrote:

I need to emphasize that it is in no way something complete and truely useful. It's a starting point for the rest of my project. It will change. Your review will help to make it a good basis!

I will come back to that soon..

Florian

Hi!

Most of this review went into the new version.. For the remaining points, some comments follow.

On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:

check.

check.

Hm.. I tried. It wasn't exactly what I wanted. When I use trace_printf, it's activated together with all other traces. I can use trace_vprintf and specify a key, but I would always have to print the header "rhsvn debug: " and the key by hand. So I could replace vfprintf in this function by trace_vprintf to do that. But then there's not much simplification. (?)

Hm.. the enum now has SUCCESS, NOT_HANDLED, TERMINATE. It gives the numbers a name, thats it.

I renamed it to input_command_handler. Unfortunately the remote-helper spec calls what is sent to the helper a 'command'.

Ok.. same as above, they are kind of commands. Of course I can change the names. For me it's not too confusing, because I don't know the git subcommands convention very well. You can choose a name.

Excess printf gone. Flush is needed. Otherwise it doesn't flush and the other end waits forever. Don't know exactly why. Some pipe-buffer ..

Yes, necessary. Blocking on this open call prevents fast-import as well as the remote helper from reading and writing on their normal command streams. This leads to deadlocks.

E.g. If there's have nothing to import, the helper sends only 'done' to fast- import and quits. That might happen before fast-import opened this pipe. Then it waits forever because the reader has already closed it.

Yes, but it doesn't die, right?

As far as I understood it, it doesn't close extra created pipes. Probably I just didn't find it in the code ..

Close the back-report pipe end of the remote-helper.

I changed the message text. It should tell us if svnrdump exited with non- zero.

On fatal errors it dies.

There is some usecase for this. The intention was, that command handlers should be able to process more than one 'name'. E.g. an import batch is terminated by a newline. This newline is handled by the import handler if it is a batch. (This feature wasn't implemented in the version reviewed here.)

So I decided to let the handler functions tell if they handle this line.

Crash? It frees detached strbuf buffers.

check.

It helped ;)

thx, Florian

Florian Achleitner wrote:

Thanks for this.

[...]

[...]

Hmm. There's no trace_printf_with_key() but that's presumably because no one has needed it. If it existed, you could use

	#define printd(msg) trace_printf_with_key("GIT_TRACE_REMOTE_SVN", "%s", msg)

But now that I check, I don't see how the current printd() calls would be useful to other people. Why announce these moments and not others? They're just temporary debugging cruft, right?

For that, plain trace_printf() works great.

[...]

[...]

Much nicer.

I think this tristate return value could be avoided entirely because... [continued at (*) below]

[...]

Ah, fast-import is ready, remote helper is ready, no one initiates pumping of data between them. Maybe the purpose of the flush would be more obvious if it were moved to the caller.

[...]

Oh, dear. I hope not. E.g., Cygwin doesn't support opening fifos RDWR (out of scope for the gsoc project, but still).

[...]

Won't the writer open the pipe and wait for us to open our end before doing that?

[...]

You can exit without writing a message with exit(), e.g. like so:

	if (code)
		exit(code);

or like so:

	if (code)
		exit(128);

[...]

So this is to work around a bug in the run-command interface?

[...]

That's just repeating the question. :) Perhaps it's supposed to trigger some action on the other end of the pipe? It would just be useful to add a comment documenting why one shouldn't remove this close() call, or else will probably end up removing it and needlessly suffering.

[...]

I'd suggest looking at other finish_command() callers for examples.

[...]

[continued from (*) above] ... it isn't needed at the moment.

See http://c2.com/xp/YouArentGonnaNeedIt.html

[...]

I see "url = argv[2];" a few lines above, but it looks like the variable was reused for two different purposes. :(

I'm not sure why you detach url, by the way. If the goal is to free it, why not use strbuf_release()?

Thanks, Jonathan

On Thursday 26 July 2012 04:08:42 Jonathan Nieder wrote:

Yes, it's for debugging only, I could just delete it all. It's inspired by transport-helper.c. The env var GIT_TRANSPORT_HELPER_DEBUG enables it. While transport-helper has a lot of if (debug) fprintf(..), I encapsulated it in printd. So I should kick printd out?

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.

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. with remote-svn: RDWR, fast-import: WRONLY, this works.

Other scenario: Nothing to import, remote-svn only sends 'done' and closes the pipe again. After fast-import reads the first line it parses it's command line and tries to open the fifo which is already closed on the other side --> blocks. This is solved by using RDWR on both sides.

If we change the points where the pipes are openend and closed, this could be circumvented.

ok, why not..

Good question. 

It's just closing files I've openend before. I usually do that, when i no longer need them. Isn't that common? I believe it makes no difference if you only call import once. On subsequent imports the pipe is still open when it tries to open it again, that could be a problem. It would have to statically store report_fd, but what for??

Hm.. all three values are used, they're not for the future but for now. Of course it could be done somehow else.

Ooops. That was introduced by splitting out "[RFC 07/16] Allow reading svn dumps from files via file:// urls." It makes url malloced unconditionally. Will fix.

I used the strbuf because of it's nice formatting functions. But on the other hand, e.g. url_decode returns a char *, so I stored it in this type. It doesn't make a big difference if I use three strbufs or one and detach and free the char * at the end. In fact, char * will consume some bytes less than a strbuf ;) Another question is, if calling free just before exit makes sense anyways. (But you learn it in programming courses, and it pleases valgrind :D )

Florian

Florian Achleitner wrote:

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

[...]

[...]

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

[...]

[...]

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

On Saturday 28 July 2012 02:00:31 Jonathan Nieder wrote:

Generally I like your prefered solution. I think there's one problem: The pipe needs to be created before the fork, so that the fd can be inherited. There is no way of creating it if the remote-helper advertises a capability, because it is already forked then. This would work with fifos, though.

We could:
- add a capability: bidi-import. 
- make transport-helper create a fifo if the helper advertises it.
- add a command for remote-helpers, like 'bidi-import <pipename>' that makes 
the remote helper open the fifo at <pipename> and use it.
- fast-import is forked after the helper, so we do already know if there will 
be a back-pipe. If yes, open it in transport-helper and pass the fd as command 
line argument cat-blob-fd. 

--> fast-import wouldn't need to be changed, but we'd use a fifo, and we get rid of the env-vars. (I guess it could work on windows too).

What do you think?

Florian Achleitner wrote:

[...]

The relevant pipe already exists at that point: the remote helper's stdin.

In other words, it could work like this (just like the existing demo code, except adding a conditional based on the "capabilities" response):

	0. transport-helper.c invokes the remote helper.  This requires
	   a pipe used to send commands to the remote helper
	   (helper->in) and a pipe used to receive responses from the
	   remote helper (helper->out)

	1. transport-helper.c sends the "capabilities" command to decide
	   what to do.  The remote helper replies that it would like
	   some feedback from fast-import.

	2. transport-helper.c forks and execs git fast-import with input
	   redirected from helper->out and the cat-blob fd redirected
	   to helper->in

	3. transport-helper.c tells the remote helper to start the
	   import

	4. wait for fast-import to exit

On Monday 30 July 2012 03:29:52 Jonathan Nieder wrote:

fast-import writes to the helpers stdin..

transport-helper writes commands to the helper's stdin.

Hm .. that would mean, that both fast-import and git (transport-helper) would write to the remote-helper's stdin, right?

Florian Achleitner wrote:

Yes, first git writes the list of refs to import, and then fast-import writes feedback during the import. Is that a problem?

On Monday 30 July 2012 11:55:02 Jonathan Nieder wrote:

I haven't tried that yet, nor do I remember anything where I've already seen two processes writing to the same pipe. At least it sounds cumbersome to me. Processes' lifetimes overlap, so buffering and flushing could mix data. We have to use it for both purposes interchangably because there can be more than one import command to the remote-helper, of course.

Will try that in test-program..

Florian Achleitner wrote:

It's a perfectly normal and well supported thing to do.

[...]

Thanks.

Good luck, Jonathan

On Tuesday 31 July 2012 15:43:57 Jonathan Nieder wrote:

I played around with a little testprogram. It generally works. I'm still not convinced that this doesn't cause more problems than it can solve. The standard defines that write calls to pipe fds are atomic, i.e. data is not interleaved with data from other processes, if the data is less than PIPE_BUF [1]. We would need some kind of locking/synchronization to make it work for sure, while I believe it will work most of the time.

Currently it runs like this: transport-helper.c writes one or more 'import <ref>' lines, we don't know in advance how many and how long they are. Then it waits for fast-import to finish.

When the first line arrives at the remote-helper, it starts importing one line at a time, leaving the remaining lines in the pipe. For importing it requires the data from fast-import, which would be mixed with import lines or queued at the end of them.

[1] http://pubs.opengroup.org/onlinepubs/009695399/functions/write.html

Hi again,

Florian Achleitner wrote:

Oh, good catch.

The way it's supposed to work is that in a bidi-import, the remote helper reads in the entire list of refs to be imported and only once the newline indicating that that list is over arrives starts writing its fast-import stream. We could make this more obvious by not spawning fast-import until immediately before writing that newline.

This needs to be clearly documented in the git-remote-helpers(1) page if the bidi-import command is introduced.

If a remote helper writes commands for fast-import before that newline comes, that is a bug in the remote helper, plain and simple. It might be fun to diagnose this problem:

	static void pipe_drained_or_die(int fd, const char *msg)
	{
		char buf[1];
		int flags = fcntl(fd, F_GETFL);
		if (flags < 0)
			die_errno("cannot get pipe flags");
		if (fcntl(fd, F_SETFL, flags | O_NONBLOCK))
			die_errno("cannot set up non-blocking pipe read");
		if (read(fd, buf, 1) > 0)
			die("%s", msg);
		if (fcntl(fd, F_SETFL, flags))
			die_errno("cannot restore pipe flags");
	}
	...

	for (i = 0; i < nr_heads; i++) {
		write "import %s\n", to_fetch[i]->name;
	}

	if (getenv("GIT_REMOTE_HELPERS_SLOW_SANITY_CHECK"))
		sleep(1);

	pipe_drained_or_die("unexpected output from remote helper before fast-import launch");

	if (get_importer(transport, &fastimport))
		die("couldn't run fast-import");
	write_constant(data->helper->in, "\n");

Hi,

back to the pipe-topic.

On Wednesday 01 August 2012 12:42:48 Jonathan Nieder wrote:

This would require all existing remote helpers that use 'import' to be ported to the new concept, right? Probably there is no other..

I still don't believe that sharing the input pipe of the remote helper is worth the hazzle. It still requires an additional pipe to be setup, the one from fast-import to the remote-helper, sharing one FD at the remote helper. It still requires more than just stdin, stdout, stderr.

I would suggest to use a fifo. It can be openend independently, after forking 
and on windows they have named pipes with similar semantics, so I think this 
could be easily ported. 
I would suggest the following changes:
- add a capability to the remote helper 'bidi-import', or 'bidi-pipe'. This 
signals that the remote helper requires data from fast-import.

- add a command 'bidi-import', or 'bidi-pipe' that is tells the remote helper 
which filename the fifo is at, so that it can open it and read it when it 
handles 'import' commands.

- transport-helper.c creates the fifo on demand, i.e. on seeing the capability, 
in the gitdir or in /tmp.

- fast-import gets the name of the fifo as a command-line argument. The 
alternative would be to add a command, but that's not allowed, because it 
changes the stream semantics.
Another alternative would be to use the existing --cat-pipe-fd argument. But 
that requires to open the fifo before execing fast-import and makes us 
dependent on the posix model of forking and inheriting file descriptors, while 
opening a fifo in fast-import would not.

Hi again,

Florian Achleitner wrote:

Ok, thanks.

[...]

[...]

You mean all existing remote helpers that use 'bidi-import', right? There are none.

[...]

If I understand correctly, you misunderstood how sharing the input pipe works. Have you tried it?

It does not involve setting up an additional pipe. Standard input for the remote helper is already a pipe. That pipe is what allows transport-helper.c to communicate with the remote helper. Letting fast-import share that pipe involves passing that file descriptor to git fast-import. No additional pipe() calls.

Do you mean that it would be too much work to implement? This explanation just doesn't make sense to me, given that the version using pipe() *already* *exists* and is *tested*.

I get the feeling I am missing something very basic. I would welcome input from others that shows what I am missing.

[...]

I'm getting kind of frustrated with this conversation going nowhere. Here is a compromise to explain why. Suppose:

- fast-import learns a --cat-blob-file parameter, which tells it to open a
  file to write responses to "cat-blob" and "ls" commands to instead of
  using an inherited file descriptor

- transport-helper.c sets up a named pipe and passes it using --cat-blob-file.

- transport-helper.c reads from that named pipe and copies everything it sees
  to the remote helper's standard input, until fast-import exits.

This would:

 - allow us to continue to use a very simple protocol for communicating
   with the remote helper, where commands and fast-import backflow both
   come on standard input

 - work fine on both Windows and Unix

Meanwhile it would:

 - be 100% functionally equivalent to the solution where fast-import
   writes directly to the remote helper's standard input.  Two programs
   can have the same pipe open for writing at the same time for a few
   seconds and that is *perfectly fine*.  On Unix and on Windows.

   On Windows the only complication with the pipe()-based  is that we haven't
   wired up the low-level logic to pass file descriptors other than
   stdin, stdout, stderr to child processes; and if I have understood
   earlier messages correctly, the operating system *does* have a
   concept of that and this is just a todo item in msys
   implementation.

 - be more complicated than the code that already exists for this
   stuff.

So while I presented this as a compromise, I don't see the point.

Is your goal portability, a dislike of the interface, some implementation detail I have missed, or something else? Could you explain the problem as concisely but clearly as possible (perhaps using an example) so that others like Sverre, Peff, or David can help think through it and to explain it in a way that dim people like me understand what's going on?

Puzzled, Jonathan

On Sunday 12 August 2012 09:12:58 Jonathan Nieder wrote:

Ok, it would not affect the existing import command.

Yes wrote a test program, sharing works, that's not the problem.

Yes, that was the first version I wrote, and remote-svn-alpha uses.

This is how I see it, probably it's all wrong: I thought the main problem is, that we don't want processes to have *more than three pipes attached*, i.e. stdout, stdin, stderr, because existing APIs don't allow it. When we share stdin of the remote helper, we achieve this goal for this one process, but fast-import still has an additional pipe: stdout --> shell; stderr --> shell; stdin <-- remote-helper; additional_pipe --> remote-helper.

That's what I wanted to say: We still have more than three pipes on fast- import. And we need to transfer that fourth file descriptor by inheritance and it's number as a command line argument. So if we make the remote-helper have only three pipes by double-using stdin, but fast-import still has four pipes, what problem does it solve?

Using fifos would remove the requirement to inherit more than three pipes. That's my point.

[..]

I digged into MSDN and it seems it's not a problem at all on the windows api layer. Pipe handles can be inherited. [1] If the low-level logic once supports passing more than 3 fds, it will work on fast-import as well as remote-helper.

It all started as portability-only discussion. On Linux, my first version would have worked. It created an additional pipe before forking using pipe(). Runs great, it did it like remote-svn-alpha.sh.

I wouldn't have started to produce something else or start a discussion on my 
own. But I was told, it's not good because of portability. This is the root of 
this endless story. (you already know the thread, I think). Since weeks nobody 
of them is interested in that except you and me.
 
So if we accept having more than three pipes on a process, we have no more 
problem.
We can dig out that first version, as well as write the one proposed by you.
While your version saves some trouble by not requiring an additional pipe() 
call and not requiring the prexec_cb, but adding a little complexity with the 
re-using of stdin.

Currently I have the implemented the original pipe version, the original fifo version, the fifo version described a mail ago. I'm going to implement the stdin-sharing version now..

[1] http://msdn.microsoft.com/en- us/library/windows/desktop/aa365782(v=vs.85).aspx

Piped, Florian ;)

Florian Achleitner wrote:

Oh, that makes sense. Thanks for explaining, and sorry to have been so dense.

At the Windows API level, Set/GetStdHandle() is only advertised to handle stdin, stdout, and stderr, so on Windows there would indeed need to be some magic to communicate the inherited HANDLE value to fast-import.

But I am confident in the Windows porters, and if a fast-import interface change ends up being needed, I think we can deal with it when the moment comes and it wouldn't necessitate changing the remote helper interface.

You also mentioned before that passing fast-import responses to the remote helper stdin means that the remote helper has to slurp up the whole list of refs to import before starting to talk to fast-import. That was good practice already anyway, but it is a real pitfall and a real downside to the single-input approach. Thanks for bringing it up.

Jonathan

Hi again,

Florian Achleitner wrote:

Let me elaborate on this, which I think is the real point (though I could easily be wrong).

Probably instead of just sending feedback I should have been sending suggested patches to compare. You do not work for me and your time is not my time, and I would not want to have the responsibility of dictating how your code works, anyway. Generally speaking, as long as code is useful and not hurting maintainability, the best thing I can do is to make it easy to incorporate. That has side benefits, too, like giving an example of what it is like to have your code incorporated and creating momentum. Small mistakes can be fixed later.

Unfortunately here we are talking about two interfaces that need to be stable. Mistakes stay --- once there is a UI wart in interfaces people are using, we are stuck continuing to support it.

To make life even more complicated, there are two interfaces involved here:

 - the remote-helper protocol, which I think it is very important
   to keep sane and stable.  Despite the remote-helper protocol
   existing for a long time, it hasn't seen a lot of adoption
   outside git yet, and complicating the protocol will not help
   that.

   I am worried about what it would mean to add an additional
   stream on top of the standard input and output used in the current
   remote-helper protocol.  Receiving responses to fast-import
   commands on stdin seems very simple.  Maybe I am wrong?

 - the fast-import interface, which is also important but I'm not
   as worried about.  Adding a new command-line parameter means
   importers that call fast-import can unnecessarily depend on
   newer versions of git, so it's still something to be avoided.

Given a particular interface, there may be multiple choices of how to implement it. For example, on Unix the remote-helper protocol could be fulfilled by letting fast-import inherit the file descriptor helper->in, while on Windows it could for example be fulfilled by using DuplicateHandle() to transmit the pipe handle before or after fast-import has already started running.

The implementation strategy can easily be changed later. What is important is that the interface be pleasant and *possible* to implement sanely.

Hoping that clarifies a little, Jonathan

Jonathan Nieder <jrnieder@gmail.com> writes:

I am not sure what you mean by that.

The command dispatcher loop in [Patch v2 1/16] seems to call every possible input handler with the first line of the input and expect them to answer "This is not for me", so NOT_HANDLED is needed.

An alternative dispatcher could be written in such a way that the dispatcher inspects the first line and decide what to call, and in such a scheme, you do not need NOT_HANDLED. My intuition tells me that such an arrangement is in general a better organization.

Looking at what cmd_import() does, however, I think the approach the patch takes might make sense for this application. Unlike other handlers like "capabilities" that do not want to handle anything other than "capabilities", it wants to handle two:

 - "import" that starts an import batch;
 - "" (an empty line), but only when an import batch is in effect.

A centralized dispatcher that does not use NOT_HANDLED could be written for such an input stream, but then the state information (i.e. "are we in an import batch?") needs to be global, which may or may not be desirable (I haven't thought things through on this).

In any case, if you are going to use dispatching based on NOT_HANDLED, the result may have to be (at least) quadri-state. In addition to "I am done successfully, please go back and dispatch another command" (SUCCESS), "This is not for me" (NOT_HANDLED), and "I am done successfully, and there is no need to dispatch and process another command further" (TERMINATE), you may want to be able to say "This was for me, but I found an error" (ERROR).

Of course, if the dispatch loop has to be rewritten so that a central dispatcher decides what to call, individual input handlers do not need to say NOT_HANDLED nor TERMINATE, as the central dispatcher should keep track of the overall state of the system, and the usual "0 on success, negative on error" may be sufficient.

One thing I wondered was how an input "capability" (or "list") should be handled after "import" was issued (hence batch_active becomes true). The dispatcher loop in the patch based on NOT_HANDLED convention will happily call cmd_capabilities(), which does not have any notion of the batch_active state (because it is a function scope static inside cmd_import()), and will say "Ah, that is mine, and let me do my thing." If we want to diagnose such an input stream as an error, the dispatch loop needs to become aware of the overall state of the system _anyway_, so that may be an argument against the NOT_HANDLED based dispatch system the patch series uses.

On Thursday 26 July 2012 10:29:51 Junio C Hamano wrote:

That's a good point. The current implementation allows other commands to appear during import batches. This shouldn't be possible according to the protocol, I think. But it doesn't do harm. Solving it will require a global state and go towards a global displatcher.

On Jul 2, 2012, at 4:07 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

Crazy thought here but would a socket not be a bad choice here?

Imagine being able to ssh tunnel into the SVN server and run the helper with filesystem access to the SVN repo.

Akin to the pushy project use case. http://packages.python.org/pushy/

SSH into the machine, copy the required components to the machine, and use the RPC. Nothing needed but SSH and python. In this case SSH, SVN, and the helper would be needed.

This also would work just fine with the local host too.

Steve

Note: Resent, Sorry it was signed, and rejected before.

Requires some sha.h to be used and the libraries to be linked, this is currently hardcoded.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
diff: add  -Wdeclaration-after-statement to CFLAGS

 contrib/svn-fe/Makefile |   14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

The existing function only allowed reading from a filename or from stdin. Allow passing of a FD and an additional FD for the back report pipe. This allows us to retrieve the name of the pipe in the caller.

Fixes the filename could be NULL bug.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 vcs-svn/svndump.c |   22 ++++++++++++++++++----
 vcs-svn/svndump.h |    1 +
 2 files changed, 19 insertions(+), 4 deletions(-)

For some fast-import commands (e.g. cat-blob) an answer-channel is required. For this purpose a fifo (aka named pipe) (mkfifo) is created (.git/fast-import-report-fifo) by the transport-helper when fetch via import is requested. The remote-helper and fast-import open the ends of the pipe.

The filename of the fifo is passed to the remote-helper via it's environment, helpers that don't use fast-import can simply ignore it. Add a new command line option --cat-blob-pipe to fast-import, for this purpose.

Use argv_arrays in get_helper and get_importer.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
 fast-import.c      |   15 ++++++++++++
 transport-helper.c |   64 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 64 insertions(+), 15 deletions(-)

For some fast-import commands (e.g. cat-blob) an answer-channel is required. For this purpose a fifo (aka named pipe) (mkfifo) is created (.git/fast-import-report-fifo) by the transport-helper when fetch via import is requested. The remote-helper and fast-import open the ends of the pipe.

The filename of the fifo is passed to the remote-helper via it's environment, helpers that don't use fast-import can simply ignore it. Add a new command line option --cat-blob-pipe to fast-import, for this purpose.

Use argv_arrays in get_helper and get_importer.

Opening the pipe with O_RDWR prevents blocking open calls on both ends.

Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
---
diff: Opening the pipe with O_RDWR prevents blocking open calls on both ends.
 fast-import.c      |   15 ++++++++++++
 transport-helper.c |   64 ++++++++++++++++++++++++++++++++++++++++------------
 2 files changed, 64 insertions(+), 15 deletions(-)

Hi,

Florian Achleitner wrote:

Is this on top of patches 1, 2, and 3 from v2 of the series?

*checks* Looks like it doesn't overlap with any of the files from those patches, so I don't have to understand them first. Phew. My suggestion for next time would be to submit patches that can be understood on their own independently instead of as part of a series.

Motivation described! But it's odd --- it seems like this is doing at least two things:

 1) adding to the fast-import interface
 2) using the new fast-import feature in some in-tree callers

Those really want to be separate patches. That way, the fast-import change can be studied by other implementers of the fast-import interface (hg-fast-import, bzr-fast-import). As a side-benefit, it gives an easy check that any changes to fast-import were at least roughly backward-compatible ("did all the in-tree users still work?").

I'll focus on the new fast-import change below, since it's the most important part.

My first impression is that I'd rather there be a command to request the filename instead of using the environment for the first time, since when debugging people would already be monitoring the command stream and responses.

This is completely redundant next to --cat-blob-fd, right? That's really problematic --- adding new interfaces means new code and gratuitous incompatibility with all existing fast-import backends, with no benefit in return.

I imagine that there was some portability reason you were thinking about, but the above doesn't mention it at all. Future readers scratching their heads at the changelog can't read your mind! Please please please explain what you're trying to do.

Since if we're lucky fixing that could mean not having to change fast-import at all, I'm stopping here.

Another quick thought: any finished patch adding a new fast-import feature should also include

 - documentation in the manpage (Documentation/fast-import.txt)
 - testcases to make sure your careful work does not get broken
   by later changes (somewhere in t/*fast-import*.sh)

But don't worry too much about that now --- sending incomplete patches for review before then to make sure the direction is sane is a very good idea, as long as they are marked as such (as you've already done by marking this as RFC).

To sum up: I think we should just stick to pipes --- why all this fifo complication?

Hope that helps, Jonathan

On Saturday 21 July 2012 09:48:34 Jonathan Nieder wrote:

People didn't like pipe variant (prexec_cb not being compatible to windows' process creation model), so I learned about fifos and implemented a (basic) fifo variant. *shrug*

Florian Achleitner wrote:

Ok, can you elaborate on that? What does it mean that preexec_cb is not compatible to windows' process creation model? Don't the people of the future working on this code deserve to know about that, too, so they don't break it?

Come on --- I'm not asking these questions just to make your life difficult. Please make it easy to understand your code changes and to keep them maintained.

On Saturday 21 July 2012 10:44:37 Jonathan Nieder wrote:

Let's discuss how to describe the solution after we decide which of the two variants we choose.

Summarizing the earlier discussion (in this thread as replies to version 1 of the patch) more verbosely: I used the prexec_cb feature of start_command which allows to install a callback function that is called just before exec. It's purpose is to close the other pipe end in each of the processes that inherited both ends after fork. The pipe is created in transport-helper.c, because it forks fast-import as well as the remote-helper, which inherit the pipe's file descriptors.

On Windows, processses are not forked but spawned from new and therefore can't inherit pipe file descriptors. So we had the idea to use a fifo, which can be opened after the creation of the process. Also there are named pipes on Windows which are similar to fifos (though I've never used them).

Mostly out of curiosity I played around with fifos and implemented this additional cat-blob-pipe feature, like a proof-of-concept.

The first version used the pipes because this feature already exists.

The reason why I resent the patch in version 3 these days is that I the problem of blocking open calls leading to potential deadlocks. Of course I should tell future maintainers better what its all about, if we really want that feature.

Hope that helps, Florian

Hi,

Florian Achleitner wrote:

Ok, this is what I didn't understand (and still don't understand).

Yes, on Windows processes are spawned instead of forked. But does that mean they can't inherit pipes? How do pipelines in the shell work at all, then?

I thought what Hannes said was rather that the current C runtime used by Git for Windows doesn't take care of inherited file descriptors, period, other than 0, 1, and 2. This has nothing to do with the fork/spawn distinction. So while --cat-blob-fd or some other mechanism could be made to work on Windows in the future, *relying* on --cat-blob-fd on Windows today is a no-go, since it rely on file descriptors >2 and break the remote helper infrastructure completely there.

That does not force us into a corner at all. We have multiple options. Here is what I propose:

Remote helpers declare using a capability (e.g., "bidi-import") that they would like a bidirectional communication channel with fast-import. Git tells the remote helper that its request has been granted by using a different command (e.g., "bidi-import") instead of "import" to start the import process.

The responses from fast-import come on stdin (file descriptor 0), so in principle it should be possible to implement this interface on Windows. The interface is portable, even if the initial implementation isn't.

Existing remote helpers using the "import" capability would be unaffected and would work as before.

On Windows, Git would not take advantage of the bidi-import capability for now. Windows support is an added complication and we have enough to do this summer.

Then interested people using Windows would be able to experiment using whatever mechanism they please (CRT support for inherited file descriptors >2, fifos, sockets, some other Windows-specific thing) to implement the bidi-import capability. Once that is implemented, they automatically get support for remote helpers that rely on the bidirectional communication functionality.

What do you think?

Thanks for explaining, Jonathan

(adding msysgit list to cc for a Windows question) Hi,

(regarding bidirectional communication between git and fast-import for remote helpers) Florian Achleitner wrote:

Is this meant as a summary of [1]?  Hannes wrote:

| The second problem is more severe and is at the lowest level of our
| infrastructure: We set up our child processes so that they know only about
| file descriptors other than 0,1,2 to the child process. Even if the first
| problem were solved, the child process does not receive sufficient
| information to know that there are open file descriptors other than 0,1,2.
| There is a facility to pass along this information from the parent to the
| child, but we simply do not implement it.

It sounds to me like the pipe model would work fine on Windows, and it would just require some porting work (out of scope for this summer of code project). Am I misunderstanding?

Thanks, Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/199216

Steven Michalske wrote:

Not crazy --- it was already mentioned. It could probably allow using --cat-blob-fd even on the platforms that don't inherit file descriptors >2, though it wuld take some tweaking. Though I still think the way forward is to keep using plain pipes internally for now and to make the bidirectional communication optional, since it wouldn't close any doors to whatever is most convenient on each platform. Hopefully I'll hear more from Florian about this in time.

We're talking about what communicates between the SVN dump parser the version control system-specific backend (git fast-import) that reads the converted result, so that particular socket wouldn't help much.

Jonathan

On Thursday 26 July 2012 06:40:39 Jonathan Nieder wrote:

Would you like to see a new pipe patch?

Yes .. the network part is already handled quite well by svnrdump.
 

(cc-ing Ram since he's also knowledgeable about remote-helper protocol) Florian Achleitner wrote:

Since the svn remote helper relies on this, it seems worth working on, yeah. As for how to spend your time (and whether to beg someone else to work on it instead :)): I'm not sure what's on your plate or where you are with respect to the original plan for the summer at the moment, so it would be hard for me to give useful advice about how to balance things.

What did you think of the suggestion of adding a new bidi-import capability and command to the remote helper protocol? I think this would be clean and avoid causing a regression on Windows, but it's easily possible I am missing something fundamental.

Thanks, Jonathan

On Thursday 26 July 2012 09:54:26 Jonathan Nieder wrote:

Btw, the pipe version did already exist before I started, it was added with the cat-blob command and already used by Dmitry's remote-svn-alpha. I didn't search for design discussions in the past ..

I don't have much overview over this topic besides the part I'm working on, like other users of fast-import. The bidi-import capability/command would have the advantage, that we don't have to bother with the pipe/fifo at all, if the remote-helper doesn't use it.

When I implemented the two variants I had the idea to pass it to the 'option' command, that fast-import already has. Anyways, specifying cat-blob-fd is not allowed via the 'option' command (see Documentation and 85c62395). It wouldn't make too much sense, because the file descriptor must be set up by the parent.

But for the fifo, it would, probably. The backward channel is only used by the commands 'cat-blob' and 'ls' of fast-import. If a remote helper wants to use them, it would could make fast-import open the pipe by sending an 'option' command with the fifo filename, otherwise it defaults to stdout (like now) and is rather useless. This would take the fifo setup out of transport-helper. The remote-helper would have to create it, if it needs it.

Apropos stdout. That leads to another idea. You already suggested that it would be easiest to only use FDs 0..2. Currently stdout and stderr of fast- import go to the shell. We could connect stdout to the remote-helper and don't need the additional channel at all. (Probably there's a good reason why they haven't done that ..) Maybe this requires many changes to fast-import and breaks existing frontends.

-- Florian

Hi,

Some quick details.

Florian Achleitner wrote:

More precisely, requiring that the cat-blob fd go on the command line instead of in the stream matches a model where fast-import's three standard streams are abstract:

 - its input, using the INPUT FORMAT described in git-fast-import(1)
 - its standard output, which echoes "progress" commands
 - its backflow stream, where responses to "cat-blob" and "ls" commands go

The stream format is not necessarily pinned to a Unix model where input and output are based on filesystems and file descriptors. We can imagine a fast-import backend that runs on a remote server and the transport used for these streams is sockets; for fast-import frontends to be usable in such a context, the streams they produce must not rely on particular fd numbers, nor on pathnames (except for mark state saved to relative paths with --relative-marks) representing anything in particular.

This goes just as much for a fifo set up on the filesystem where the fast-import backend runs as for an inherited file descriptor. In the current model, such backend-specific details of setup go on the command line.

I'm getting confused by terminology again. Let's see if I have the cast of characters straight:

 - the fast-import backend (e.g., "git fast-import" or "hg-fastimport")
 - the fast-import frontend (e.g., "git fast-export" or svn-fe)
 - git's generic foreign VCS support plumbing, also known as
   transport-helper.c
 - the remote helper (e.g., "git remote-svn" or "git remote-testgit")

Why would the fast-import backend ever need to open a pipe? If I want it to write backflow to a fifo, I can use

	mkfifo my-favorite-fifo
	git fast-import --cat-blob-fd=3 3>my-favorite-fifo

If I want it to write backflow to a pipe, I can use (using ksh syntax)

	cat |&
	git fast-import --cat-blob-fd=3 3>&p

We can imagine transport-helper.c learning the name of a fifo set up by the remote helper by sending it the "capabilities" command:

	git> capabilities
	helper> option
	helper> import
	helper> cat-blob-file my-favorite-fifo
	helper> refspec refs/heads/*:refs/helper/remotename/*
	helper>

transport-helper.c could then use that information to invoke fast-import appropriately:

	git fast-import --cat-blob-fd=3 3>my-favorite-fifo

But this seems like pushing complication onto the remote helper; since there is expected to be one remote helper per foreign VCS, implementing the appropriate logic correctly once and for all in transport-helper.c for all interested remote helpers to take advantage of seems to me like a better policy.

The complication that makes this strategy not so easy is "progress" commands in the fast-import input stream. (Incidentally, it would be nice to teach transport-helper.c to display specially formatted "progress" commands using a progress bar some day.)

Hoping that clarifies, Jonathan