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

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

From
Junio C Hamano <gitster@pobox.com>
Date
Aug 14, 2012, 20:07 UTC
Message-ID
<7vhas59r0b.fsf@alter.siamese.dyndns.org>
In-Reply-To
<1344971598-8213-2-git-send-email-florian.achleitner.2.6.31@gmail.com>
Florian Achleitner <florian.achleitner.2.6.31@gmail.com> writes:
Show 5 quoted lines
> 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 '>'?
Show 26 quoted lines
> 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.
Show 13 quoted lines
> +#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?

Show 13 quoted lines
> +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?
Show 17 quoted lines
> +		{ "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.
Show 12 quoted lines
> +		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 '('.
Show 6 quoted lines
> +	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.

Show 10 quoted lines
> +
> +	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?

Show 6 quoted lines
> +	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().

Show 6 quoted lines
> +	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.
Show 25 quoted lines
> +}
> +
> +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.

Show 14 quoted lines
> +			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.
Show 7 quoted lines
> +			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?
Show 15 quoted lines
> +}
> +
> +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];
Show 23 quoted lines
> +	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;
> +}
Previous: Florian AchleitnerNext: Florian Achleitner
Message 34 of 36 in “GSOC remote-svn”
  1. 00/16 GSOC remote-svnFlorian Achleitner, Aug 14, 2012
  2. 01/16 Implement a remote helper for svn in C.Florian Achleitner, Aug 14, 2012
  3. 02/16 Integrate remote-svn into svn-fe/Makefile.Florian Achleitner, Aug 14, 2012
  4. 03/16 Add svndump_init_fd to allow reading dumps from arbitrary FDs.Florian Achleitner, Aug 14, 2012
  5. 04/16 Connect fast-import to the remote-helper via pipe, adding 'bidi-import' capability.Florian Achleitner, Aug 14, 2012
  6. 05/16 Add documentation for the 'bidi-import' capability of remote-helpers.Florian Achleitner, Aug 14, 2012
  7. 06/16 remote-svn, vcs-svn: Enable fetching to private refs.Florian Achleitner, Aug 14, 2012
  8. 07/16 Add a symlink 'git-remote-svn' in base dir.Florian Achleitner, Aug 14, 2012
  9. 08/16 Allow reading svn dumps from files via file:// urls.Florian Achleitner, Aug 14, 2012
  10. 09/16 vcs-svn: add fast_export_note to create notesFlorian Achleitner, Aug 14, 2012
  11. 10/16 Create a note for every imported commit containing svn metadata.Florian Achleitner, Aug 14, 2012
  12. 11/16 When debug==1, start fast-import with "--stats" instead of "--quiet".Florian Achleitner, Aug 14, 2012
  13. 12/16 remote-svn: add incremental import.Florian Achleitner, Aug 14, 2012
  14. 13/16 Add a svnrdump-simulator replaying a dump file for testing.Florian Achleitner, Aug 14, 2012
  15. 14/16 transport-helper: add import|export-marks to fast-import command line.Florian Achleitner, Aug 14, 2012
  16. 15/16 remote-svn: add marks-file regeneration.Florian Achleitner, Aug 14, 2012
  17. 16/16 Add a test script for remote-svn.Florian Achleitner, Aug 14, 2012
  18. Florian AchleitnerAug 15, 2012
  19. Junio C HamanoAug 15, 2012
  20. Florian AchleitnerAug 15, 2012
  21. Florian AchleitnerAug 15, 2012
  22. Junio C HamanoAug 15, 2012
  23. Junio C HamanoAug 15, 2012
  24. Florian AchleitnerAug 15, 2012
  25. Junio C HamanoAug 14, 2012
  26. Junio C HamanoAug 14, 2012
  27. Florian AchleitnerAug 15, 2012
  28. Junio C HamanoAug 14, 2012
  29. Florian AchleitnerAug 15, 2012
  30. Junio C HamanoAug 15, 2012
  31. Junio C HamanoAug 14, 2012
  32. Junio C HamanoAug 14, 2012
  33. Florian AchleitnerAug 15, 2012
  34. Junio C HamanoAug 14, 2012
  35. Florian AchleitnerAug 15, 2012
  36. David Michael BarrAug 14, 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.