threads / patch / 23764

patchWrite stubby remote helper for SVN

Subject: [WIP PATCH] Write stubby remote helper for SVN

## tl;dr

7 messages between May 10, 2010 and May 10, 2010. Diffs are folded; open one to read it.

replies: 6people: 3as markdown or json

Ramkumar Ramachandra· May 10, 2010, 11:39 UTC · lore

This is a WIP patch based on master, and is part of my GSoC project git-remote-svn. It is not intended for inclusion- the purpose of the patch is to get feedback on aspects such as style and overall design. I plan to put an SVN client, fast-import and fast-export helpers for SVN inside a directory vcs-svn. Only this file will be in the root directory.

I've also included a Makefile target git-remote-svn, which should build just fine, but do nothing except list capabilities.

Ramkumar Ramachandra (1):
  Add skeleton remote helper for SVN
 Makefile     |    6 ++-
 remote-svn.c |  202 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+), 1 deletions(-)
 create mode 100644 remote-svn.c
Ramkumar Ramachandra· May 10, 2010, 11:39 UTC · re: Ramkumar Ramachandra · lore

[WIP PATCH] Add skeleton remote helper for SVN

Create remote-svn.c, which is essentially a stripped-down version of remote-curl.c to build the SVN remote helper upon. Also add a Makefile rule to build it.

Signed-off-by: Ramkumar Ramachandra <artagnon@gmail.com>
---
 Makefile     |    6 ++-
 remote-svn.c |  202 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 207 insertions(+), 1 deletions(-)
 create mode 100644 remote-svn.c
Show changes to 2 files +207 −1

Makefile, remote-svn.c

diff --git a/Makefile b/Makefile
index 4f7224a..06b969f 100644
--- a/Makefile
+++ b/Makefile
@@ -1658,7 +1658,7 @@ git.o git.spec \
 
 TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
 GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
-	git.o http.o http-walker.o remote-curl.o
+	git.o http.o http-walker.o remote-curl.o remote-svn.o
 XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \
 	xdiff/xmerge.o xdiff/xpatience.o
 OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS)
@@ -1824,6 +1824,10 @@ $(REMOTE_CURL_PRIMARY): remote-curl.o http.o http-walker.o $(GITLIBS)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ $(ALL_LDFLAGS) $(filter %.o,$^) \
 		$(LIBS) $(CURL_LIBCURL) $(EXPAT_LIBEXPAT)
 
+git-remote-svn$X: remote-svn.o $(GITLIBS)
+	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@	\
+		$(ALL_LDFLAGS) $(filter %.o,$^) $(LIBS)
+
 $(LIB_FILE): $(LIB_OBJS)
 	$(QUIET_AR)$(RM) $@ && $(AR) rcs $@ $(LIB_OBJS)
 
diff --git a/remote-svn.c b/remote-svn.c
new file mode 100644
index 0000000..dafbd6a
--- /dev/null
+++ b/remote-svn.c
@@ -0,0 +1,202 @@
+#include "cache.h"
+#include "strbuf.h"
+#include "remote.h"
+#include "strbuf.h"
+
+static struct remote *remote;
+static const char *url;
+
+struct options {
+	int verbosity;
+	unsigned long depth;
+	unsigned progress : 1,
+		dry_run : 1;
+};
+static struct options options;
+
+static int set_option(const char *name, const char *value)
+{
+	if (!strcmp(name, "verbosity")) {
+		char *end;
+		int v = strtol(value, &end, 10);
+		if (value == end || *end)
+			return -1;
+		options.verbosity = v;
+		return 0;
+	}
+	else if (!strcmp(name, "progress")) {
+		if (!strcmp(value, "true"))
+			options.progress = 1;
+		else if (!strcmp(value, "false"))
+			options.progress = 0;
+		else
+			return -1;
+		return 0;
+	}
+	else if (!strcmp(name, "dry-run")) {
+		if (!strcmp(value, "true"))
+			options.dry_run = 1;
+		else if (!strcmp(value, "false"))
+			options.dry_run = 0;
+		else
+			return -1;
+		return 0;
+	}
+	else {
+		return 1;
+	}
+}
+
+static int export_handler(int nr_spec, char **specs)
+{
+	int err = 0;
+
+	// TODO: The real exporting
+        // TODO: Write an importer for SVN
+	
+	return err;
+}
+
+static int import_handler(int nr_spec, char **specs)
+{
+	int err = 0;
+	
+	// TODO: The real importing
+	// TODO: Hook up an SVN exporter's fast-export stream
+
+	return err;
+}
+
+static void parse_import(struct strbuf *buf)
+{
+	char **specs = NULL;
+	int alloc_spec = 0, nr_spec = 0, i;
+
+	do {
+		if (!prefixcmp(buf->buf, "import ")) {
+			ALLOC_GROW(specs, nr_spec + 1, alloc_spec);
+			specs[nr_spec++] = xstrdup(buf->buf + 5);
+		}
+		else
+			die("remote helper does not support %s", buf->buf);
+
+		strbuf_reset(buf);
+		if (strbuf_getline(buf, stdin, '\n') == EOF)
+			return;
+		if (!*buf->buf)
+			break;
+	} while (1);
+
+	if (import_handler(nr_spec, specs))
+		exit(128); /* error already reported */
+	for (i = 0; i < nr_spec; i++)
+		free(specs[i]);
+	free(specs);
+
+	printf("\n");
+	fflush(stdout);
+}
+
+static void parse_export(struct strbuf *buf)
+{
+	char **specs = NULL;
+	int alloc_spec = 0, nr_spec = 0, i;
+
+	do {
+		if (!prefixcmp(buf->buf, "export ")) {
+			ALLOC_GROW(specs, nr_spec + 1, alloc_spec);
+			specs[nr_spec++] = xstrdup(buf->buf + 5);
+		}
+		else
+			die("remote helper does not support %s", buf->buf);
+
+		strbuf_reset(buf);
+		if (strbuf_getline(buf, stdin, '\n') == EOF)
+			return;
+		if (!*buf->buf)
+			break;
+	} while (1);
+
+	if (export_handler(nr_spec, specs))
+		exit(128); /* error already reported */
+	for (i = 0; i < nr_spec; i++)
+		free(specs[i]);
+	free(specs);
+
+	printf("\n");
+	fflush(stdout);
+}
+
+int main(int argc, const char **argv)
+{
+	struct strbuf buf = STRBUF_INIT;
+	int nongit;
+
+	// git_extract_argv0_path(argv[0]);
+	setup_git_directory_gently(&nongit);
+	if (argc < 2) {
+		fprintf(stderr, "Remote needed\n");
+		return 1;
+	}
+
+	options.verbosity = 1;
+	options.progress = !!isatty(2);
+
+	remote = remote_get(argv[1]);
+
+	if (argc > 2) {
+		url = argv[2];
+	} else {
+		url = remote->url[0];
+	}
+
+	// open_svn_connection(remote);
+	do {
+		if (strbuf_getline(&buf, stdin, '\n') == EOF)
+			break;
+
+		else if (!strcmp(buf.buf, "list")) {
+			// TODO: Code to list refs
+			break;
+
+		} else if (!prefixcmp(buf.buf, "import ")) {
+			if (nongit)
+				die("Import attempted without a local repo");
+
+			parse_import(&buf);
+
+		} else if (!prefixcmp(buf.buf, "export ")) {
+			parse_export(&buf);
+
+		} else if (!prefixcmp(buf.buf, "option ")) {
+			char *name = buf.buf + strlen("option ");
+			char *value = strchr(name, ' ');
+			int result;
+			if (value)
+				*value++ = '\0';
+			else
+				value = "true";
+			result = set_option(name, value);
+			if (!result)
+				printf("ok\n");
+			else if (result < 0)
+				printf("error invalid value\n");
+			else
+				printf("unsupported\n");
+			fflush(stdout);
+
+		} else if (!strcmp(buf.buf, "capabilities")) {
+			printf("option\n");
+			printf("import\n");
+			printf("export\n");
+			printf("\n");
+			fflush(stdout);
+		} else
+			return 1;
+
+		strbuf_reset(&buf);
+	} while (1);
+
+	// close_svn_connection(remote);
+	return 0;
+}
-- 
1.7.1
Tay Ray Chuan· May 10, 2010, 12:00 UTC · re: Ramkumar Ramachandra · lore

Re: [WIP PATCH] Add skeleton remote helper for SVN

Hi,

On Mon, May 10, 2010 at 7:39 PM, Ramkumar Ramachandra <artagnon@gmail.com> wrote:

Show 13 quoted lines
> diff --git a/Makefile b/Makefile
> index 4f7224a..06b969f 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -1658,7 +1658,7 @@ git.o git.spec \
>
>  TEST_OBJS := $(patsubst test-%$X,test-%.o,$(TEST_PROGRAMS))
>  GIT_OBJS := $(LIB_OBJS) $(BUILTIN_OBJS) $(PROGRAM_OBJS) $(TEST_OBJS) \
> -       git.o http.o http-walker.o remote-curl.o
> +       git.o http.o http-walker.o remote-curl.o remote-svn.o
>  XDIFF_OBJS = xdiff/xdiffi.o xdiff/xprepare.o xdiff/xutils.o xdiff/xemit.o \
>        xdiff/xmerge.o xdiff/xpatience.o
>  OBJECTS := $(GIT_OBJS) $(XDIFF_OBJS)

Johnathan (Cc'ed) had a recent patch (<20100509035728.GA8198@progeny.tock>) that changed this area, and I think it would be good to build on top of that.

Show 19 quoted lines
> [snip]
> diff --git a/remote-svn.c b/remote-svn.c
> new file mode 100644
> index 0000000..dafbd6a
> --- /dev/null
> +++ b/remote-svn.c
> @@ -0,0 +1,202 @@
> [snip]
> +static int set_option(const char *name, const char *value)
> +{
> +       if (!strcmp(name, "verbosity")) {
> +               char *end;
> +               int v = strtol(value, &end, 10);
> +               if (value == end || *end)
> +                       return -1;
> +               options.verbosity = v;
> +               return 0;
> +       }
> +       else if (!strcmp(name, "progress")) {
We usually put the brace on the same line as the "else":
    if (...) {
      /* stuff */
    } else if (...) {
      /* stuff */
    }
(of course, tabs should be used - gmail doesn't allow me to do it here.)
Show 7 quoted lines
> [snip]
> +static int export_handler(int nr_spec, char **specs)
> +{
> +       int err = 0;
> +
> +       // TODO: The real exporting
> +        // TODO: Write an importer for SVN
Here and elsewhere - no C++-style comments please.
Show 13 quoted lines
> [snip]
> +static void parse_import(struct strbuf *buf)
> +{
> +       char **specs = NULL;
> +       int alloc_spec = 0, nr_spec = 0, i;
> +
> +       do {
> +               if (!prefixcmp(buf->buf, "import ")) {
> +                       ALLOC_GROW(specs, nr_spec + 1, alloc_spec);
> +                       specs[nr_spec++] = xstrdup(buf->buf + 5);
> +               }
> +               else
> +                       die("remote helper does not support %s", buf->buf);
See note on else-brace style.
-- 
Cheers,
Ray Chuan
Ramkumar Ramachandra· May 10, 2010, 12:31 UTC · re: Tay Ray Chuan · lore

Re: [WIP PATCH] Add skeleton remote helper for SVN

Hi Tay,
Thanks for the style nitpicks.
On Mon, May 10, 2010 at 2:00 PM, Tay Ray Chuan <rctay89@gmail.com> wrote:
> Johnathan (Cc'ed) had a recent patch
> (<20100509035728.GA8198@progeny.tock>) that changed this area, and I
> think it would be good to build on top of that.

This'll probably be ready for inclusion atleast into `pu` in a few weeks- I'll rebase it on top of the `master` then before sending it out anyway. It'll be good if Jonathan's patch makes it to `master` by then, but I don't think I should prematurely rebase on patches that aren't in `master` yet.

-- Ram
Jonathan Nieder· May 10, 2010, 12:59 UTC · re: Ramkumar Ramachandra · lore

Re: [WIP PATCH] Add skeleton remote helper for SVN

Hi Ram,
Ramkumar Ramachandra wrote:
> It'll be good if Jonathan's patch makes it to `master` by
> then, but I don't think I should prematurely rebase on patches that
> aren't in `master` yet.

Since the conflict resolution is trivial, I agree. When there is an actual dependency between patches, I think there is nothing wrong with basing one patch on another; as long as you make it clear what your patch applies to, humans and Junio’s scripts can cope with the task of applying one series on top of the topic branch for another.

Re your skeleton code, it looks reasonable to me. I could nitpick the names of the import_handler() and export_handler() functions and the needless use of do ... while, but I will refrain. :)

Much more importantly, there is something to play with now. On that note, is David’s code out there somewhere for people to fool around with? At this point, I am not interested in a stable interface or permanent history, just a way to try out the code and start to get used to it.

Thanks, Jonathan

[1] git://repo.or.cz/svn-all-fast-export.git
Ramkumar Ramachandra· May 10, 2010, 13:08 UTC · re: Jonathan Nieder · lore

Re: [WIP PATCH] Add skeleton remote helper for SVN

Hi Jonathan,
On Mon, May 10, 2010 at 2:59 PM, Jonathan Nieder <jrnieder@gmail.com> wrote:
Show 6 quoted lines
> Since the conflict resolution is trivial, I agree.  When there is an
> actual dependency between patches, I think there is nothing wrong
> with basing one patch on another; as long as you make it clear
> what your patch applies to, humans and Junio’s scripts can cope with
> the task of applying one series on top of the topic branch for
> another.
Okay, got it.
> Re your skeleton code, it looks reasonable to me.  I could nitpick the
> names of the import_handler() and export_handler() functions and the
> needless use of do ... while, but I will refrain. :)

Yeah, I suppose that can wait for a couple of weeks- atleast until I have something minimally working :)

Show 5 quoted lines
> Much more importantly, there is something to play with now.  On that
> note, is David’s code out there somewhere for people to fool around
> with?  At this point, I am not interested in a stable interface or
> permanent history, just a way to try out the code and start to get
> used to it.

Yes. You can see it on David's GitHub [1] and my fork [2]. I send patches to David by email, so both should be in sync.

-- Ram

[1] http://github.com/barrbrain/svn-dump-fast-export.git [2] http://github.com/artagnon/svn-dump-fast-export.git

Jonathan Nieder· May 10, 2010, 13:21 UTC · re: Ramkumar Ramachandra · lore

Re: [WIP PATCH] Add skeleton remote helper for SVN

Ramkumar Ramachandra wrote:
> [1] http://github.com/barrbrain/svn-dump-fast-export.git
> [2] http://github.com/artagnon/svn-dump-fast-export.git
>From README.textile:
| h1. A tool to convert a subversion dump for input to git-fast-import
|
| This project is now largely functionally complete. There is still
| quite a bit of testing and code cleanup to be done before the first
| major release.
Awesome. :) Thank you.

Cheers, Jonathan

← back to recent threads