Re: [RFC v2 01/16] Implement a remote helper for svn in C.

> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > Enables basic fetching from subversion repositories. When processing
> > Remote URLs starting with svn::, git invokes this remote-helper.
> > It starts svnrdump to extract revisions from the subversion repository in
> > the 'dump file format', and converts them to a git-fast-import stream
> > using the functions of vcs-svn/.
> > 
> > Imported refs are created in a private namespace at
> > refs/svn/<remote-name/master. The revision history is imported linearly
> > (no branch detection) and completely, i.e. from revision 0 to HEAD.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > 
> >  contrib/svn-fe/remote-svn.c |  190
> >  +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 190
> >  insertions(+)
> >  create mode 100644 contrib/svn-fe/remote-svn.c
> > 
> > diff --git a/contrib/svn-fe/remote-svn.c b/contrib/svn-fe/remote-svn.c
> > new file mode 100644
> > index 0000000..d5c2df8
> > --- /dev/null
> > +++ b/contrib/svn-fe/remote-svn.c
> > @@ -0,0 +1,190 @@
> > +
> > +#include "cache.h"
> > +#include "remote.h"
> > +#include "strbuf.h"
> > +#include "url.h"
> > +#include "exec_cmd.h"
> > +#include "run-command.h"
> > +#include "svndump.h"
> > +#include "argv-array.h"
> > +
> > +static const char *url;
> > +static const char *private_ref;
> > +static const char *remote_ref = "refs/heads/master";
> > +
> > +int cmd_capabilities(struct strbuf *line);
> > +int cmd_import(struct strbuf *line);
> > +int cmd_list(struct strbuf *line);
> 
> How many of these and other symbols are necessary to be visible
> outside this file?

> 
> > +typedef int (*input_command_handler)(struct strbuf *);
> > +struct input_command_entry {
> > +	const char *name;
> > +	input_command_handler fct;
> > +	unsigned char batchable;	/* whether the command starts or is part of a
> > batch */ +};
> > +
> > +static const struct input_command_entry input_command_list[] = {
> > +		{ "capabilities", cmd_capabilities, 0 },
> > +		{ "import", cmd_import, 1 },
> > +		{ "list", cmd_list, 0 },
> > +		{ NULL, NULL }
> > +};
> > +
> > +int cmd_capabilities(struct strbuf *line)
> > +{
> > +	printf("import\n");
> > +	printf("refspec %s:%s\n\n", remote_ref, private_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> > +
> > +static void terminate_batch() {
> > +	/* terminate a current batch's fast-import stream */
> 
> Style:
> 
> 	static void terminate_batch(void)
> 	{
> 		/* terminate ...
> 

> > +		printf("done\n");
> > +		fflush(stdout);
> > +}
> > +
> > +int cmd_import(struct strbuf *line)
> > +{
> > +	int code, report_fd;
> > +	char *back_pipe_env;
> > +	int dumpin_fd;
> > +	unsigned int startrev = 0;
> > +	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
> > +	struct child_process svndump_proc;
> > +
> > +	/*
> > +	 * When the remote-helper is invoked by transport-helper.c it passes 

> > +	 * filename of this pipe in the env-var.
> > +	 */
> 
> s/ it passes/, &/;
> 
> > +	back_pipe_env = getenv("GIT_REPORT_FIFO");
> 
> Can we name "back pipe", "report fifo" and "report fd" more
> consistently and descriptively?
> 
> What kind of "REPORT" are we talking about here?  Is it to carry the
> contents of

> > +				"be set by the caller.");
> > +	}
> 
> Style: unnecesary {} block around a simple statement.  It is OK to
> have such a block early in a series if you add more statements to it
> in later steps, but that does not seem to be the case for this patch
> series.

> 
> > +	/*
> > +	 * Opening a fifo for reading usually blocks until a writer has opened
> > it too. +	 * Opening a fifo for writing usually blocks until a reader has
> > opened it too. +	 * Therefore, we open with RDWR on both sides to avoid
> > deadlocks. +	 * Especially if there's nothing to do and one pipe end is
> > closed immediately. +	 */
> 
> This smells somewhat fishy "justification".  Are we reading what we
> wrote to the fifo?  Who is expected to come at the other end of the
> fifo?  Is it this process that creates that other process?  Perhaps
> you should open it _after_ spawning the process, telling it to open
> the same fifo for writing (if you are sitting on the reading end)?

> 
> > +	report_fd = open(back_pipe_env, O_RDWR);
> > +	if (report_fd < 0) {
> > +		die("Unable to open fast-import back-pipe! %s", strerror(errno));
> > +	}
> 
> Style: (ditto).
> 
> > +int cmd_list(struct strbuf *line)
> > +{
> > +	printf("? HEAD\n");
> > +	printf("? %s\n\n", remote_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> 
> It somehow feels funny that remote_ref (which seems to be hardcoded
> to "refs/heads/master") and HEAD are listed here, even though the
> other side is not even in the Git land and the name "master" does
> not have any significance.  The name "refs/heads/master" may be
> necessary to form the LHS of the refspec in cmd_capabilities(), but
> it somehow feels more natural to only advertise HEAD here and also
> futz the refspec to printf("refspec HEAD:%s", private_ref) in
> cmd_capabilities().  Perhaps you tried it already and it did not
> work for some reason; I dunno.

> 
> > +int do_command(struct strbuf *line)
> > +{
> > +	const struct input_command_entry *p = input_command_list;
> > +	static int batch_active;
> > +	static struct strbuf batch_command = STRBUF_INIT;
> > +	/*
> > +	 * import commands can be grouped together in a batch.
> > +	 * Batches are ended by \n. If no batch is active the program ends.
> > +	 */
> > +	if (line->len == 0 ) {
> 
> Style: lose the SP before closing parenthesis ")".

> 
> > +		if (batch_active) {
> > +			terminate_batch();
> > +			batch_active = 0;
> > +			return 0;
> > +		}
> 
> Is it an error to feed an empty line when batch is not active?
> How is the error diagnosed?
> Is the user told about the error, and if so how?

> 
> > +		return 1;
> > +	}
> > +	if (batch_active && strcmp(batch_command.buf, line->buf))
> > +		die("Active %s batch interrupted by %s", batch_command.buf, line-
>buf);
> 
> So after issuing "import" that causes batch_active to become true,
> another command e.g. "list" cannot be issued and will result in this
> die() unless the batch is concluded by issuing an empty line.  Can
> an "import" be issued while another "import" batch is in effect?

> > {
> > +			if (p->batchable) {
> > +				batch_active = 1;
> > +				strbuf_release(&batch_command);
> > +				strbuf_addbuf(&batch_command, line);
> 
> Wouldn't it make more sense to get rid of batch_active variable and
> use the presense of batch_command as the signal for the batch in
> effect?  Your "if (batch_active)" becomes "if (batch_command.len)",
> and "batch_active = 0" becomes "strbuf_release(&batch_command)".
> 

> > +			}
> > +			return p->fct(line);
> > +		}
> > +	}
> > +	warning("Unknown command '%s'\n", line->buf);
> > +	return 0;
> > +}
> > +
> > +int main(int argc, const char **argv)
> > +{
> > +	struct strbuf buf = STRBUF_INIT;
> > +	int nongit;
> > +	static struct remote *remote;
> > +	const char *url_in;
> > +
> > +	git_extract_argv0_path(argv[0]);
> > +	setup_git_directory_gently(&nongit);
> > +	if (argc < 2 || argc > 3) {
> > +		usage("git-remote-svn <remote-name> [<url>]");
> > +		return 1;
> > +	}
> > +
> > +	remote = remote_get(argv[1]);
> > +	url_in = remote->url[0];
> > +	if (argc == 3)
> > +		url_in = argv[2];
> > +
> > +	end_url_with_slash(&buf, url_in);
> > +	url = strbuf_detach(&buf, NULL);
> > +
> > +	strbuf_init(&buf, 0);
> > +	strbuf_addf(&buf, "refs/svn/%s/master", remote->name);
> > +	private_ref = strbuf_detach(&buf, NULL);
> > +
> > +	while(1) {
> > +		if (strbuf_getline(&buf, stdin, '\n') == EOF) {
> > +			if (ferror(stdin))
> > +				die_errno("Error reading command stream");
> > +			else
> > +				die_errno("Unexpected end of command stream");
> 
> On the else clause, ferror() did not say there was an error.  What
> errno do we see from die_errno() in that case?

> 
> > +		}
> > +		if (do_command(&buf))
> > +			break;
> > +		strbuf_reset(&buf);
> > +	}
> > +
> > +	strbuf_release(&buf);
> > +	free((void*)url);
> > +	free((void*)private_ref);
> > +	return 0;
> > +}