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

> Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
> > Enable 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/.
> 
> (nit) the above is a bit too wide, isn't it?
> 
> > Imported refs are created in a private namespace at
> > refs/svn/<remote-name/master.
> (nit) missing closing '>'?
> 
> > The revision history is imported linearly (no branch detection) and
> > completely, i.e. from revision 0 to HEAD.
> > 
> > The 'bidi-import' capability is used. The remote-helper expects data from
> > fast-import on its stdin. It buffers a batch of 'import' command lines
> > in a string_list before starting to process them.
> > 
> > Signed-off-by: Florian Achleitner <florian.achleitner.2.6.31@gmail.com>
> > ---
> > diff:
> > - incorporate review
> > - remove redundant strbuf_init
> > - add 'bidi-import' to capabilities
> > - buffer all lines of a command batch in string_list
> > 
> >  contrib/svn-fe/remote-svn.c |  183
> >  +++++++++++++++++++++++++++++++++++++++++++ 1 file changed, 183
> >  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..ce59344
> > --- /dev/null
> > +++ b/contrib/svn-fe/remote-svn.c
> > @@ -0,0 +1,183 @@
> > +
> 
> Remove.
> 
> > +#include "cache.h"
> > +#include "remote.h"
> > +#include "strbuf.h"
> > +#include "url.h"
> > +#include "exec_cmd.h"
> > +#include "run-command.h"
> > +#include "svndump.h"
> > +#include "notes.h"
> > +#include "argv-array.h"
> > +
> > +static const char *url;
> > +static const char *private_ref;
> > +static const char *remote_ref = "refs/heads/master";
> 
> Just wondering; is this name "master" (or "refs/heads/" for that
> matter) significant in any way when talking to a subversion remote?

> 
> > +static int cmd_capabilities(const char *line);
> > +static int cmd_import(const char *line);
> > +static int cmd_list(const char *line);
> > +
> > +typedef int (*input_command_handler)(const char *);
> > +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 },
> 
> One level too deeply indented?
> 
> > +		{ "import", cmd_import, 1 },
> > +		{ "list", cmd_list, 0 },
> > +		{ NULL, NULL }
> > +};
> > +
> > +static int cmd_capabilities(const char *line) {
> > +	printf("import\n");
> > +	printf("bidi-import\n");
> > +	printf("refspec %s:%s\n\n", remote_ref, private_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> > +
> > +static void terminate_batch(void)
> > +{
> > +	/* terminate a current batch's fast-import stream */
> > +		printf("done\n");
> 
> Likewise.
> 
> > +		fflush(stdout);
> > +}
> > +
> > +static int cmd_import(const char *line)
> > +{
> > +	int code;
> > +	int dumpin_fd;
> > +	unsigned int startrev = 0;
> > +	struct argv_array svndump_argv = ARGV_ARRAY_INIT;
> > +	struct child_process svndump_proc;
> > +
> > +	memset(&svndump_proc, 0, sizeof (struct child_process));
> 
> Please lose SP between sizeof and '('.
> 
> > +	svndump_proc.out = -1;
> > +	argv_array_push(&svndump_argv, "svnrdump");
> > +	argv_array_push(&svndump_argv, "dump");
> > +	argv_array_push(&svndump_argv, url);
> > +	argv_array_pushf(&svndump_argv, "-r%u:HEAD", startrev);
> > +	svndump_proc.argv = svndump_argv.argv;
> 
> (just me making a mental note) We read from "svnrdump", which would
> read (if it ever does) from the same stdin as ours and spits (if it
> ever does) its errors to the same stderr as ours.

> 
> > +
> > +	code = start_command(&svndump_proc);
> > +	if (code)
> > +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> > +	dumpin_fd = svndump_proc.out;
> > +
> > +	code = start_command(&svndump_proc);
> > +	if (code)
> > +		die("Unable to start %s, code %d", svndump_proc.argv[0], code);
> > +	dumpin_fd = svndump_proc.out;
> 
> You start it twice without finishing the first invocation, or just a
> double paste?

> 
> > +	svndump_init_fd(dumpin_fd, STDIN_FILENO);
> > +	svndump_read(url, private_ref);
> > +	svndump_deinit();
> > +	svndump_reset();
> > +
> > +	close(dumpin_fd);
> 
> (my mental note) And at this point, we finished feeding whatever
> comes out of "svnrdump" to svndump_read().
> 
> > +	code = finish_command(&svndump_proc);
> > +	if (code)
> > +		warning("%s, returned %d", svndump_proc.argv[0], code);
> > +	argv_array_clear(&svndump_argv);
> > +
> > +	return 0;
> 
> Other than the "twice?" puzzle, this function looks straightforward.
> 
> > +}
> > +
> > +static int cmd_list(const char *line)
> > +{
> > +	printf("? %s\n\n", remote_ref);
> > +	fflush(stdout);
> > +	return 0;
> > +}
> > +
> > +static int do_command(struct strbuf *line)
> > +{
> > +	const struct input_command_entry *p = input_command_list;
> > +	static struct string_list batchlines = STRING_LIST_INIT_DUP;
> > +	static const struct input_command_entry *batch_cmd;
> > +	/*
> > +	 * commands can be grouped together in a batch.
> > +	 * Batches are ended by \n. If no batch is active the program ends.
> > +	 * During a batch all lines are buffered and passed to the handler
> > function +	 * when the batch is terminated.
> > +	 */
> > +	if (line->len == 0) {
> > +		if (batch_cmd) {
> > +			struct string_list_item *item;
> > +			for_each_string_list_item(item, &batchlines)
> > +				batch_cmd->fct(item->string);
> 
> (style) I think we tend to call these unnamed functions "fn" in our
> codebase.
> 
> > +			terminate_batch();
> > +			batch_cmd = NULL;
> > +			string_list_clear(&batchlines, 0);
> > +			return 0;	/* end of the batch, continue reading other commands. */
> > +		}
> > +		return 1;	/* end of command stream, quit */
> > +	}
> > +	if (batch_cmd) {
> > +		if (strcmp(batch_cmd->name, line->buf))
> > +			die("Active %s batch interrupted by %s", batch_cmd->name, line-
>buf);
> > +		/* buffer batch lines */
> > +		string_list_append(&batchlines, line->buf);
> > +		return 0;
> > +	}
> 
> A "batch-able" command, e.g. "import", will first cause the
> batch_cmd to point at the command structure in this function, and
> then the next and subsequent lines, as long as the input line is
> exactly the same as the current batch_cmd->name, e.g. "import", is
> appended into batchlines.
> 
> Would this mean that you can feed something like this:
> 
> 	import foobar
>         import
>         import
>         import
> 
>         another command
> 
> and buffer the four "import" lines in batchlines, and then on the
> empty line, have the for-each-string-list-item loop to call
> cmd_import() on "import foobar", "import", "import", then "import"
> (literally, without anything other than "import" on the line).
> 
> How is that useful?  With that "if (strcmp(batch_cmd->name, line->buf))",
> I cannot think of other valid input to make this "batch" mechanism
> to trigger and do something useful.  Am I missing something?
> 

> > +
> > +	for(p = input_command_list; p->name; p++) {
> 
> Have a SP between for and '('.
> 
> > +		if (!prefixcmp(line->buf, p->name) &&
> > +				(strlen(p->name) == line->len || line->buf[strlen(p->name)] == ' 

> > {
> 
> A line way too wide.
> 
> > +			if (p->batchable) {
> > +				batch_cmd = p;
> > +				string_list_append(&batchlines, line->buf);
> > +				return 0;
> > +			}
> > +			return p->fct(line->buf);
> > +		}
> 
> OK, so a command word on a line by itself, or a command word
> followed by a SP (probably followed by its arguments) on a line
> triggers a command lookup, and individual command implementation
> parses the line.
> 
> > +	}
> > +	warning("Unknown command '%s'\n", line->buf);
> > +	return 0;
> 
> Why isn't this an error?

> 
> > +}
> > +
> > +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;
> > +	}
> 
> If this is an importer, you would be importing _into_ a git
> repository, no?  How can you not error out when you are not in one?
> In other words, why &nongit with *_gently()?

> 
> > +	remote = remote_get(argv[1]);
> > +	url_in = remote->url[0];
> > +	if (argc == 3)
> > +		url_in = argv[2];
> 
> Shouldn't it be more like this?
> 
> 	url_in = (argc == 3) ? argv[2] : remote->url[0];

> 
> > +	end_url_with_slash(&buf, url_in);
> > +	url = strbuf_detach(&buf, NULL);
> > +
> > +	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("Error reading command stream");
> > +			else
> > +				die("Unexpected end of command stream");
> > +		}
> > +		if (do_command(&buf))
> > +			break;
> > +		strbuf_reset(&buf);
> > +	}
> > +
> > +	strbuf_release(&buf);
> > +	free((void*)url);
> > +	free((void*)private_ref);
> > +	return 0;
> > +}