git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
Date
Jul 26, 2012, 16:16 UTC
Message-ID
<1609414.ugUML9Yn73@flomedio>
In-Reply-To
<20120726090842.GA4999@burratino>
On Thursday 26 July 2012 04:08:42 Jonathan Nieder wrote:
> Florian Achleitner wrote:
Show 28 quoted lines
> > On Monday 02 July 2012 06:07:41 Jonathan Nieder wrote:
> [...]
> 
> >>> +
> >>> +static inline void printd(const char* fmt, ...)
> 
> [...]
> 
> >> Why not use trace_printf and avoid the complication?
> > 
> > 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. (?)
> 
> 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.

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?

Show 15 quoted lines
> >>> +
> >>> +	printf("import\n");
> >>> +	printf("\n");
> >>> +	fflush(stdout);
> >>> +	return SUCCESS;
> >>> +}
> >> 
> >> Why the multiple printf?  Is the flush needed?
> > 
> > Excess printf gone.
> > Flush is needed. Otherwise it doesn't flush and the other end waits
> > forever.
> 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.

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.

Show 17 quoted lines
> [...]
> 
> >>> +	/* 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));
> >>> +	}
> >> 
> >> Is this necessary?  Why shouldn't we fork the writer first and wait
> >> for it here?
> > 
> > Yes, necessary.
> 
> Oh, dear.  I hope not.  E.g., Cygwin doesn't support opening fifos
> RDWR (out of scope for the gsoc project, but still).

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.

Show 30 quoted lines
> 
> [...]
> 
> > E.g. If there's have nothing to import, the helper sends only 'done' to
> > fast- import and quits.
> 
> Won't the writer open the pipe and wait for us to open our end before
> doing that?
> 
> [...]
> 
> >>> +
> >>> +	code = start_command(&svndump_proc);
> >>> +	if(code)
> >>> +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> >> 
> >> start_command() is supposed to have printed a message already when it
> >> fails, unless errno == ENOENT and silent_exec_failure was set.
> > 
> > Yes, but it doesn't die, right?
> 
> You can exit without writing a message with exit(), e.g. like so:
> 
> 	if (code)
> 		exit(code);
> 
> or like so:
> 
> 	if (code)
> 		exit(128);
ok, why not..
Show 13 quoted lines
> 
> [...]
> 
> >>> +
> >>> +	close(svndump_proc.out);
> >> 
> >> Important?  Wouldn't finish_command do this?
> > 
> > As far as I understood it, it doesn't close extra created pipes. Probably
> > I
> > just didn't find it in the code ..
> 
> So this is to work around a bug in the run-command interface?
Good question.
Show 14 quoted lines
> 
> [...]
> 
> >>> +	close(report_fd);
> >> 
> >> What is the purpose of this step?
> > 
> > Close the back-report pipe end of the remote-helper.
> 
> 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.

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

Show 45 quoted lines
> 
> [...]
> 
> >>> +
> >>> +	code = finish_command(&svndump_proc);
> >>> +	if(code)
> >>> +		warning("Something went wrong with termination of %s, code %d",
> >>> svndump_proc.argv[0], code);
> >> 
> >> finish_command() is supposed to print a message when it fails.
> > 
> > I changed the message text. It should tell us if svnrdump exited with non-
> > zero.
> 
> I'd suggest looking at other finish_command() callers for examples.
> 
> [...]
> 
> >>> +enum cmd_result do_command(struct strbuf* line)
> >>> +{
> >>> +	const command* p = command_list;
> >>> +	enum cmd_result ret;
> >>> +	printd("command line '%s'", line->buf);
> >>> +	while(*p) {
> >>> +		ret = (*p)(line);
> >>> +		if(ret != NOT_HANDLED)
> >>> +			return ret;
> >>> +		p++;
> >>> +	}
> >> 
> >> If possible, matching commands by name (like git.c does) would make
> >> the behavior easier to predict.
> > 
> > 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.
> 
> [continued from (*) above]
> ... it isn't needed at the moment.
> 
> See http://c2.com/xp/YouArentGonnaNeedIt.html

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

Show 12 quoted lines
> 
> [...]
> 
> >>> +	free((void*)url);
> >>> +	free((void*)private_refs);
> >> 
> >> Won't this crash?
> > 
> > Crash? It frees detached strbuf buffers.
> 
> I see "url = argv[2];" a few lines above, but it looks like the variable
> was reused for two different purposes. :(

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'm not sure why you detach url, by the way.  If the goal is to free it,
> why not use strbuf_release()?

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 )

> 
> Thanks,
> Jonathan
Florian
Previous: Jonathan NiederNext: Jonathan Nieder
Message 25 of 57 in “[RFC 0/4]”
  1. Florian AchleitnerJun 4, 2012
  2. 1/4 Implement a basic remote helper vor svn in C.Florian Achleitner, Jun 4, 2012
  3. 2/4 Integrate remote-svn into svn-fe/Makefile.Florian Achleitner, Jun 4, 2012
  4. 3/4 Add svndump_init_fd to allow reading dumps from arbitrary FDs.Florian Achleitner, Jun 4, 2012
  5. 4/4 Add cat-blob report pipe from fast-import to remote-helper.Florian Achleitner, Jun 4, 2012
  6. David Michael BarrJun 5, 2012
  7. Jeff KingJun 5, 2012
  8. David Michael BarrJun 5, 2012
  9. Jeff KingJun 5, 2012
  10. Florian AchleitnerJun 5, 2012
  11. Jeff KingJun 6, 2012
  12. Florian AchleitnerJun 6, 2012
  13. Johannes SixtJun 5, 2012
  14. Jeff KingJun 5, 2012
  15. Florian AchleitnerJun 5, 2012
  16. Johannes SixtJun 5, 2012
  17. David Michael BarrJun 5, 2012
  18. 0/4 Florian Achleitner, Jun 29, 2012
  19. 1/4 Implement a basic remote helper for svn in C.Florian Achleitner, Jun 29, 2012
  20. Jonathan NiederJul 2, 2012
  21. Jonathan NiederJul 6, 2012
  22. Florian AchleitnerJul 6, 2012
  23. Florian AchleitnerJul 26, 2012
  24. Jonathan NiederJul 26, 2012
  25. Florian AchleitnerJul 26, 2012
  26. Jonathan NiederJul 28, 2012
  27. Florian AchleitnerJul 30, 2012
  28. Jonathan NiederJul 30, 2012
  29. Florian AchleitnerJul 30, 2012
  30. Jonathan NiederJul 30, 2012
  31. Florian AchleitnerJul 31, 2012
  32. Jonathan NiederJul 31, 2012
  33. Florian AchleitnerAug 1, 2012
  34. Jonathan NiederAug 1, 2012
  35. Florian AchleitnerAug 12, 2012
  36. Jonathan NiederAug 12, 2012
  37. Florian AchleitnerAug 12, 2012
  38. Jonathan NiederAug 12, 2012
  39. Jonathan NiederAug 12, 2012
  40. Junio C HamanoJul 26, 2012
  41. Florian AchleitnerJul 30, 2012
  42. Steven MichalskeJul 26, 2012
  43. 2/4 Integrate remote-svn into svn-fe/Makefile.Florian Achleitner, Jun 29, 2012
  44. 3/4 Add svndump_init_fd to allow reading dumps from arbitrary FDs.Florian Achleitner, Jun 29, 2012
  45. 4/4 Add cat-blob report fifo from fast-import to remote-helper.Florian Achleitner, Jun 29, 2012
  46. Florian AchleitnerJul 21, 2012
  47. Jonathan NiederJul 21, 2012
  48. Florian AchleitnerJul 21, 2012
  49. Jonathan NiederJul 21, 2012
  50. Florian AchleitnerJul 22, 2012
  51. Jonathan NiederJul 22, 2012
  52. Jonathan NiederJul 21, 2012
  53. Jonathan NiederJul 26, 2012
  54. Florian AchleitnerJul 26, 2012
  55. Jonathan NiederJul 26, 2012
  56. Florian AchleitnerJul 27, 2012
  57. Jonathan NiederJul 28, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.