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

[Florian Achleitner <florian.achleitner.2.6.31@gmail.com>]

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:
> Hi,
> 
> Florian Achleitner wrote:

> 
> > --- /dev/null
> > +++ b/contrib/svn-fe/remote-svn.c
> > @@ -0,0 +1,207 @@
> > +
> > +#include <stdlib.h>
> > +#include <string.h>
> > +#include <stdio.h>
> 
> 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.

check.

> 
> > +#include "cache.h"
> > +#include "remote.h"
> > +#include "strbuf.h"
> > +#include "url.h"
> > +#include "exec_cmd.h"
> > +#include "run-command.h"
> > +#include "svndump.h"
> > +
> > +static int debug = 0;
> 
> Small nit: please drop the redundant "= 0" here.  Or:

check.

> > +
> > +static inline void printd(const char* fmt, ...)
> > +{
> > +	if(debug) {
> > +		va_list vargs;
> > +		va_start(vargs, fmt);
> > +		fprintf(stderr, "rhsvn debug: ");
> > +		vfprintf(stderr, fmt, vargs);
> > +		fprintf(stderr, "\n");
> > +		va_end(vargs);
> > +	}
> > +}
> 
> 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. (?)


> > +
> > +enum cmd_result cmd_capabilities(struct strbuf* line);
> > +enum cmd_result cmd_import(struct strbuf* line);
> > +enum cmd_result cmd_list(struct strbuf* line);
> 
> What's a cmd_result?  '*' sticks to variable name.
> 
> > +
> > +enum cmd_result { SUCCESS, NOT_HANDLED, ERROR };
> 
> 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?

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

> 
> > +typedef enum cmd_result (*command)(struct strbuf*);
> 
> 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?

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

> 
> [...]
> 
> > +
> > +const command command_list[] = {
> > +		cmd_capabilities, cmd_import, cmd_list, NULL
> > +};
> 
> 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.

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.

> 
> [...]
> 
> > +enum cmd_result cmd_capabilities(struct strbuf* line)
> > +{
> > +	if(strcmp(line->buf, "capabilities"))
> > +		return NOT_HANDLED;
> 
> Style: missing SP after keyword.
> 
> > +
> > +	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.
Don't know exactly why. Some pipe-buffer ..

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


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

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

> > +	close(report_fd);
> 
> What is the purpose of this step?

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

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

> 
> > +	free(svndump_proc.argv);
> > +
> > +	printf("done\n");
> > +	return SUCCESS;
> 
> Success even if it failed?

On fatal errors it dies.

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

> [...]
> 
> > +	if (argc < 2) {
> > +		fprintf(stderr, "Remote needed\n");
> > +		return 1;
> > +	}
> 
> usage() can be used to write a clearer error message.

> [...]
> 
> > +	free((void*)url);
> > +	free((void*)private_refs);
> 
> Won't this crash?

Crash? It frees detached strbuf buffers.

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

check.

> 
> Hope that helps,
> Jonathan

It helped ;)

thx, Florian