[PATCH v2 00/11] vcs-svn,svn-fe add a couple of options

[PATCH v2 00/11] vcs-svn,svn-fe add a couple of options

[Dmitry Ivankov]

This is a second iteration of [1].

The most significant changes since [1] are:
1) svndump_init parameters are wrapped into a struct svndump_init_args
This way it is a bit easier to play with various parameters.
Thanks to Ram for insisting on this.
2) added [6/11] "vcs-svn: move commit parameters logic to svndump.c"
Now fast_export is touched only in one commit of this series and there
are other nice implications mentioned in the commit message.
3) added [10/11] and [11/11] - two more options to be used in a
upcoming iteration of remote-svn-alpha series

The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git

[1] http://thread.gmane.org/gmane.comp.version-control.git/176578

Dmitry Ivankov (11):
  svn-fe: add man target to Makefile
  test-svn-fe: use parse-options
  svn-fe: add EXTLIBS needed for parse-options
  svn-fe: add usage and unpositional arguments versions
  vcs-svn: move url parameter from _read to _init
  vcs-svn: move commit parameters logic to svndump.c
  vcs-svn,svn-fe: allow to specify dump destination ref
  vcs-svn,svn-fe: convert REPORT_FILENO to an option
  vcs-svn,svn-fe: allow to disable 'progress' lines
  vcs-svn,svn-fe: add --incremental option
  vcs-svn,svn-fe: add an option to write svnrev notes

 contrib/svn-fe/Makefile   |   18 +++---
 contrib/svn-fe/svn-fe.c   |   46 ++++++++++++-
 contrib/svn-fe/svn-fe.txt |   37 +++++++++--
 t/t9010-svn-fe.sh         |  157 +++++++++++++++++++++++++++++++++++++++-----
 test-svn-fe.c             |   59 +++++++++++++----
 vcs-svn/fast_export.c     |   44 ++++---------
 vcs-svn/fast_export.h     |    8 ++-
 vcs-svn/svndump.c         |   82 +++++++++++++++++++----
 vcs-svn/svndump.h         |   11 +++-
 9 files changed, 361 insertions(+), 101 deletions(-)

-- 
1.7.3.4

[PATCH v2 01/11] svn-fe: add man target to Makefile

[Dmitry Ivankov]

There already is a svn-fe.1 target. But 'man' being a standard
target is easier to discover or type. It can also be reused if
more manpages arise here.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/Makefile |    4 +++-
 1 files changed, 3 insertions(+), 1 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index 360d8da..bc03a3e 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -33,6 +33,8 @@ ifndef V
 endif
 endif
 
+man: svn-fe.1
+
 svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
 	$(QUIET_LINK)$(CC) $(ALL_CFLAGS) -o $@ svn-fe.o \
 		$(ALL_LDFLAGS) $(LIBS)
@@ -60,4 +62,4 @@ svn-fe.1: svn-fe.txt
 clean:
 	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1
 
-.PHONY: all clean FORCE
+.PHONY: all man clean FORCE
-- 
1.7.3.4

Re: [PATCH v2 01/11] svn-fe: add man target to Makefile

[Jonathan Nieder]

Hi Dmitry,

Dmitry Ivankov wrote:

> There already is a svn-fe.1 target. But 'man' being a standard
> target is easier to discover or type. It can also be reused if
> more manpages arise here.

This will make life a little easier when wanting to test-build
git-remote-svn.1 along with svn-fe.1 (once git-remote-svn exists).
Thanks, queued.

[PATCH v2 02/11] test-svn-fe: use parse-options

[Dmitry Ivankov]

There was custom options parsing. As more options arise it will
be easier to add and document new options with parse-options api.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 test-svn-fe.c |   42 ++++++++++++++++++++++++++++--------------
 1 files changed, 28 insertions(+), 14 deletions(-)

diff --git a/test-svn-fe.c b/test-svn-fe.c
index 332a5f7..0aab245 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -3,28 +3,38 @@
  */
 
 #include "git-compat-util.h"
+#include "parse-options.h"
 #include "vcs-svn/svndump.h"
 #include "vcs-svn/svndiff.h"
 #include "vcs-svn/sliding_window.h"
 #include "vcs-svn/line_buffer.h"
 
-static const char test_svnfe_usage[] =
-	"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)";
+static const char * const test_svnfe_usage[] = {
+	"test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
+	NULL
+};
 
-static int apply_delta(int argc, char *argv[])
+static int d;
+
+static struct option test_svnfe_options[] = {
+	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_END()
+};
+
+static int apply_delta(int argc, const char *argv[])
 {
 	struct line_buffer preimage = LINE_BUFFER_INIT;
 	struct line_buffer delta = LINE_BUFFER_INIT;
 	struct sliding_view preimage_view = SLIDING_VIEW_INIT(&preimage, -1);
 
-	if (argc != 5)
-		usage(test_svnfe_usage);
+	if (argc != 3)
+		usage_with_options(test_svnfe_usage, test_svnfe_options);
 
-	if (buffer_init(&preimage, argv[2]))
+	if (buffer_init(&preimage, argv[0]))
 		die_errno("cannot open preimage");
-	if (buffer_init(&delta, argv[3]))
+	if (buffer_init(&delta, argv[1]))
 		die_errno("cannot open delta");
-	if (svndiff0_apply(&delta, (off_t) strtoull(argv[4], NULL, 0),
+	if (svndiff0_apply(&delta, (off_t) strtoull(argv[2], NULL, 0),
 					&preimage_view, stdout))
 		return 1;
 	if (buffer_deinit(&preimage))
@@ -37,10 +47,16 @@ static int apply_delta(int argc, char *argv[])
 	return 0;
 }
 
-int main(int argc, char *argv[])
+int main(int argc, const char *argv[])
 {
-	if (argc == 2) {
-		if (svndump_init(argv[1]))
+	argc = parse_options(argc, argv, NULL, test_svnfe_options,
+						test_svnfe_usage, 0);
+
+	if (d)
+		return apply_delta(argc, argv);
+
+	if (argc == 1) {
+		if (svndump_init(argv[0]))
 			return 1;
 		svndump_read(NULL);
 		svndump_deinit();
@@ -48,7 +64,5 @@ int main(int argc, char *argv[])
 		return 0;
 	}
 
-	if (argc >= 2 && !strcmp(argv[1], "-d"))
-		return apply_delta(argc, argv);
-	usage(test_svnfe_usage);
+	usage_with_options(test_svnfe_usage, test_svnfe_options);
 }
-- 
1.7.3.4

Re: [PATCH v2 02/11] test-svn-fe: use parse-options

[Jonathan Nieder]

Hi,

Sorry for the slow response.

Dmitry Ivankov wrote:

> There was custom options parsing. As more options arise it will
> be easier to add and document new options with parse-options api.

In particular this gives us a "test-svn-fe -h" command --- sounds
good.  Might make sense to combine this with the patch that
parsoptifies contrib/svn-fe/svn-fe.c.

> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
> @@ -3,28 +3,38 @@
>   */
>  
>  #include "git-compat-util.h"
> +#include "parse-options.h"
>  #include "vcs-svn/svndump.h"
>  #include "vcs-svn/svndiff.h"
>  #include "vcs-svn/sliding_window.h"
>  #include "vcs-svn/line_buffer.h"
>  
> -static const char test_svnfe_usage[] =
> -	"test-svn-fe (<dumpfile> | [-d] <preimage> <delta> <len>)";
> +static const char * const test_svnfe_usage[] = {
> +	"test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
> +	NULL
> +};

With this API, we're allowed to print multiple usage strings.  Might as
well take advantage of that for clarity:

 static const char * const test_svnfe_usage[] = {
	"test-svn-fe <dumpfile>",
	"test-svn-fe -d <preimage> <delta> <len>",
	NULL
 };

>  
> +static int d;
> +

The variable name is not so memorable.  Maybe something like
"apply_delta" would do.

> -static int apply_delta(int argc, char *argv[])
> +static struct option test_svnfe_options[] = {
> +	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
> +	OPT_END()
> +};

Might make sense to take the opportunity to add a mnemonic long
option name while at it:

	OPT_SET_INT('d', "apply-delta", ...

[...]
> @@ -37,10 +47,16 @@ static int apply_delta(int argc, char *argv[])
>  	return 0;
>  }
>  
> -int main(int argc, char *argv[])
> +int main(int argc, const char *argv[])
>  {
> -	if (argc == 2) {
> -		if (svndump_init(argv[1]))
> +	argc = parse_options(argc, argv, NULL, test_svnfe_options,
> +						test_svnfe_usage, 0);
> +
> +	if (d)
> +		return apply_delta(argc, argv);
> +
> +	if (argc == 1) {

Probably easier to read with the simple and exceptional case first.

	if (apply_delta_instead)
		return apply_delta(argc, argv);
	if (argc != 1)
		usage_with_options(...);

	if (svndump_init(argv[0]))
		return 1;
	...

	
> +		if (svndump_init(argv[0]))
>  			return 1;
>  		svndump_read(NULL);
>  		svndump_deinit();
> @@ -48,7 +64,5 @@ int main(int argc, char *argv[])
>  		return 0;
>  	}
>  
> -	if (argc >= 2 && !strcmp(argv[1], "-d"))
> -		return apply_delta(argc, argv);
> -	usage(test_svnfe_usage);
> +	usage_with_options(test_svnfe_usage, test_svnfe_options);

Except for the minor nits noted above (in particular, hopefully this
can be squashed with the corresponding svn-fe patch),

Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>
---
 test-svn-fe.c |   29 +++++++++++++++--------------
 1 files changed, 15 insertions(+), 14 deletions(-)

diff --git a/test-svn-fe.c b/test-svn-fe.c
index 0aab2450..db56b6ba 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -3,21 +3,23 @@
  */
 
 #include "git-compat-util.h"
-#include "parse-options.h"
 #include "vcs-svn/svndump.h"
 #include "vcs-svn/svndiff.h"
 #include "vcs-svn/sliding_window.h"
 #include "vcs-svn/line_buffer.h"
+#include "parse-options.h"
 
 static const char * const test_svnfe_usage[] = {
-	"test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
+	"test-svn-fe <dumpfile>",
+	"test-svn-fe -d <preimage> <delta> <len>",
 	NULL
 };
 
-static int d;
+static int apply_delta_instead;
 
 static struct option test_svnfe_options[] = {
-	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_SET_INT('d', "apply-delta",
+		&apply_delta_instead, "apply a subversion-format delta", 1),
 	OPT_END()
 };
 
@@ -52,17 +54,16 @@ int main(int argc, const char *argv[])
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
-	if (d)
+	if (apply_delta_instead)
 		return apply_delta(argc, argv);
 
-	if (argc == 1) {
-		if (svndump_init(argv[0]))
-			return 1;
-		svndump_read(NULL);
-		svndump_deinit();
-		svndump_reset();
-		return 0;
-	}
+	if (argc != 1)
+		usage_with_options(test_svnfe_usage, test_svnfe_options);
 
-	usage_with_options(test_svnfe_usage, test_svnfe_options);
+	if (svndump_init(argv[0]))
+		return 1;
+	svndump_read(NULL);
+	svndump_deinit();
+	svndump_reset();
+	return 0;
 }
-- 
1.7.6

[PATCH v2 03/11] svn-fe: add EXTLIBS needed for parse-options

[Dmitry Ivankov]

Currently parse-options.o pull quite a big bunch of dependencies
that are neither pulled in by svn-fe Makefile nor included in libgit.a.

Use a temporary hack: put hardcoded EXTLIBS, this may not work in all
setups because /Makefile logic is not repeated.

For example, one may need -lcrypto instead of -lssl or no crypto library
if BLK_SHA1 is set, also an additional -lz or -lpcre could be required.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/Makefile |   12 +++++-------
 1 files changed, 5 insertions(+), 7 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index bc03a3e..bf1625c 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -8,11 +8,12 @@ CFLAGS = -g -O2 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-EXTLIBS =
+EXTLIBS = -lssl -lpthread
 
 GIT_LIB = ../../libgit.a
 VCSSVN_LIB = ../../vcs-svn/lib.a
-LIBS = $(VCSSVN_LIB) $(GIT_LIB) $(EXTLIBS)
+XDIFF_LIB = ../../xdiff/lib.a
+LIBS = $(VCSSVN_LIB) $(GIT_LIB) $(XDIFF_LIB) $(EXTLIBS)
 
 QUIET_SUBDIR0 = +$(MAKE) -C # space to separate -C and subdir
 QUIET_SUBDIR1 =
@@ -53,11 +54,8 @@ svn-fe.1: svn-fe.txt
 		../contrib/svn-fe/$@
 	$(MV) ../../Documentation/svn-fe.1 .
 
-../../vcs-svn/lib.a: FORCE
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) vcs-svn/lib.a
-
-../../libgit.a: FORCE
-	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) libgit.a
+$(VCSSVN_LIB) $(GIT_LIB) $(XDIFF_LIB): ../../%.a: FORCE
+	$(QUIET_SUBDIR0)../.. $(QUIET_SUBDIR1) $*.a
 
 clean:
 	$(RM) svn-fe$X svn-fe.o svn-fe.html svn-fe.xml svn-fe.1
-- 
1.7.3.4

Re: [PATCH v2 03/11] svn-fe: add EXTLIBS needed for parse-options

[Jonathan Nieder]

Dmitry Ivankov wrote:

> Currently parse-options.o pull quite a big bunch of dependencies
> that are neither pulled in by svn-fe Makefile nor included in libgit.a.
>
> Use a temporary hack: put hardcoded EXTLIBS

In other words, this is a workaround for the lack of
http://thread.gmane.org/gmane.comp.version-control.git/176318/focus=176573
(Reduce parse-options.o dependencies).

> this may not work in all
> setups because /Makefile logic is not repeated.
>
> For example, one may need -lcrypto instead of -lssl or no crypto library
> if BLK_SHA1 is set, also an additional -lz or -lpcre could be required.

Better to pull in too many libs and let the operator remove them from
the Makefile than too few and make her guess.  Though of course
neither should be needed. :)

With the following applied on top locally, it works for me.

-- >8 --
Subject: squash! svn-fe: add EXTLIBS needed for parse-options

-lcrypto is needed for SHA-1 routines unless NO_OPENSSL or BLK_SHA1
is set, -lpcre is for grep if USE_LIBPCRE is set, and -lz is needed
throughout.

In the future, none of these should be needed, after a little
rearranging to ensure that parse-options.o has no references to
translation units that need to access the object db.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/svn-fe/Makefile |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index dc6dafef..14b07b5b 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -8,7 +8,7 @@ CFLAGS = -g -O2 -Wall
 LDFLAGS =
 ALL_CFLAGS = $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
-EXTLIBS = -lssl -lpthread
+EXTLIBS = -lssl -lcrypto -lpcre -lz -lpthread
 
 GIT_LIB = ../../libgit.a
 VCSSVN_LIB = ../../vcs-svn/lib.a
-- 
1.7.6

[PATCH v2 04/11] svn-fe: add usage and unpositional arguments versions

[Dmitry Ivankov]

There will be more command line options for svn-fe so support
unpositional version for flexibility. Also clarify the meaning
of url parameter.

$ svn-fe --git-svn-id-url=url
does the same thing as
$ svn-fe url
i.e., url is used to generate git-svn-id: lines, if url is set.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/Makefile   |    2 +-
 contrib/svn-fe/svn-fe.c   |   30 +++++++++++++++++++++++++++---
 contrib/svn-fe/svn-fe.txt |   17 +++++++++++++----
 3 files changed, 41 insertions(+), 8 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index bf1625c..3e18395 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -41,7 +41,7 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
 		$(ALL_LDFLAGS) $(LIBS)
 
 svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
-	$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -I../../vcs-svn -I../.. -o $*.o -c $(ALL_CFLAGS) $<
 
 svn-fe.html: svn-fe.txt
 	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \
diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 35db24f..7e829b9 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -3,14 +3,38 @@
  * You may freely use, modify, distribute, and relicense it.
  */
 
-#include <stdlib.h>
+#include "git-compat-util.h"
+#include "parse-options.h"
 #include "svndump.h"
 
-int main(int argc, char **argv)
+static const char * const svn_fe_usage[] = {
+	"svn-fe [options] [git-svn-id-url] < dump | fast-import-backend",
+	NULL
+};
+
+static const char *url;
+
+static struct option svn_fe_options[] = {
+	OPT_STRING(0, "git-svn-id-url", &url, "url",
+		"append git-svn metadata line to commit messages"),
+	OPT_END()
+};
+
+int main(int argc, const char **argv)
 {
+	argc = parse_options(argc, argv, NULL, svn_fe_options,
+						svn_fe_usage, 0);
+	if (argc == 1) {
+		if (url)
+			usage_msg_opt("git-svn-id-url is set twice: as a "
+					"--parameter and as a [parameter]",
+					svn_fe_usage, svn_fe_options);
+		url = argv[0];
+	} else if (argc)
+		usage_with_options(svn_fe_usage, svn_fe_options);
 	if (svndump_init(NULL))
 		return 1;
-	svndump_read((argc > 1) ? argv[1] : NULL);
+	svndump_read(url);
 	svndump_deinit();
 	svndump_reset();
 	return 0;
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 2dd27ce..8c6d347 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -10,7 +10,7 @@ SYNOPSIS
 [verse]
 mkfifo backchannel &&
 svnadmin dump --deltas REPO |
-	svn-fe [url] 3<backchannel |
+	svn-fe [options] [git-svn-id-url] 3<backchannel |
 	git fast-import --cat-blob-fd=3 3>backchannel
 
 DESCRIPTION
@@ -25,6 +25,14 @@ command.
 Note: this tool is very young.  The details of its commandline
 interface may change in backward incompatible ways.
 
+OPTIONS
+-------
+
+--git-svn-id-url=<url>::
+	Url to be used in git-svn-id: lines in git-svn
+	metadata lines format. See NOTES for more detailed
+	description.
+
 INPUT FORMAT
 ------------
 Subversion's repository dump format is documented in full in
@@ -50,9 +58,10 @@ user <user@UUID>
 as committer, where 'user' is the value of the `svn:author` property
 and 'UUID' the repository's identifier.
 
-To support incremental imports, 'svn-fe' puts a `git-svn-id` line at
-the end of each commit log message if passed an url on the command
-line.  This line has the form `git-svn-id: URL@REVNO UUID`.
+'svn-fe' can be used in preparing a repository for 'git-svn' as follows.
+If `git-svn-id-url` is specified, 'svn-fe' will put `git-svn-id` line at
+the end of each commit log message.
+This line has the form `git-svn-id: URL@REVNO UUID`.
 
 The resulting repository will generally require further processing
 to put each project in its own repository and to separate the history
-- 
1.7.3.4

Re: [PATCH v2 04/11] svn-fe: add usage and unpositional arguments versions

[Jonathan Nieder]

Dmitry Ivankov wrote:

> $ svn-fe --git-svn-id-url=url
> does the same thing as
> $ svn-fe url
> i.e., url is used to generate git-svn-id: lines, if url is set.

Reasonable.  Let's see...

> --- a/contrib/svn-fe/Makefile
> +++ b/contrib/svn-fe/Makefile
> @@ -41,7 +41,7 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
>  		$(ALL_LDFLAGS) $(LIBS)
>  
>  svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
> -	$(QUIET_CC)$(CC) -I../../vcs-svn -o $*.o -c $(ALL_CFLAGS) $<
> +	$(QUIET_CC)$(CC) -I../../vcs-svn -I../.. -o $*.o -c $(ALL_CFLAGS) $<

I'd prefer to see the -I flags as part of ALL_CFLAGS, though that's
somewhat orthogonal to this patch.

[...]
> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -3,14 +3,38 @@
[...]
> +static struct option svn_fe_options[] = {
> +	OPT_STRING(0, "git-svn-id-url", &url, "url",
> +		"append git-svn metadata line to commit messages"),

Hmm.  How about this?

	"add git-svn-id line to log messages, imitating git-svn"

[...]
> +	argc = parse_options(argc, argv, NULL, svn_fe_options,
> +						svn_fe_usage, 0);
> +	if (argc == 1) {
> +		if (url)
> +			usage_msg_opt("git-svn-id-url is set twice: as a "
> +					"--parameter and as a [parameter]",
> +					svn_fe_usage, svn_fe_options);
> +		url = argv[0];
> +	} else if (argc)
> +		usage_with_options(svn_fe_usage, svn_fe_options);

IMHO would be more readable with the simplest and exceptional case
first:

	if (argc > 1)
		usage_with_options(...);

This way, a person reading can be comforted with the knowledge that
argc <= 1 from then on.

	if (argc == 1) {
		if (url)
			...
	}

To sum up, the patch looks good, and the only improvements I can think
of are tiny nits. :)

With whatever changes mentioned above seem suitable,
Reviewed-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks.
---
 contrib/svn-fe/Makefile |    4 ++--
 contrib/svn-fe/svn-fe.c |    8 +++++---
 2 files changed, 7 insertions(+), 5 deletions(-)

diff --git a/contrib/svn-fe/Makefile b/contrib/svn-fe/Makefile
index 14b07b5b..62a5c628 100644
--- a/contrib/svn-fe/Makefile
+++ b/contrib/svn-fe/Makefile
@@ -6,7 +6,7 @@ MV = mv
 
 CFLAGS = -g -O2 -Wall
 LDFLAGS =
-ALL_CFLAGS = $(CFLAGS)
+ALL_CFLAGS = -I../.. -I../../vcs-svn $(CFLAGS)
 ALL_LDFLAGS = $(LDFLAGS)
 EXTLIBS = -lssl -lcrypto -lpcre -lz -lpthread
 
@@ -39,7 +39,7 @@ svn-fe$X: svn-fe.o $(VCSSVN_LIB) $(GIT_LIB)
 		$(ALL_LDFLAGS) $(LIBS)
 
 svn-fe.o: svn-fe.c ../../vcs-svn/svndump.h
-	$(QUIET_CC)$(CC) -I../../vcs-svn -I../.. -o $*.o -c $(ALL_CFLAGS) $<
+	$(QUIET_CC)$(CC) -o $@ -c $(ALL_CFLAGS) $<
 
 svn-fe.html: svn-fe.txt
 	$(QUIET_SUBDIR0)../../Documentation $(QUIET_SUBDIR1) \
diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 7e829b91..a95e72f4 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -16,7 +16,7 @@ static const char *url;
 
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &url, "url",
-		"append git-svn metadata line to commit messages"),
+		"add git-svn-id line to log messages, imitating git-svn"),
 	OPT_END()
 };
 
@@ -24,14 +24,16 @@ int main(int argc, const char **argv)
 {
 	argc = parse_options(argc, argv, NULL, svn_fe_options,
 						svn_fe_usage, 0);
+	if (argc > 1)
+		usage_with_options(svn_fe_usage, svn_fe_options);
+
 	if (argc == 1) {
 		if (url)
 			usage_msg_opt("git-svn-id-url is set twice: as a "
 					"--parameter and as a [parameter]",
 					svn_fe_usage, svn_fe_options);
 		url = argv[0];
-	} else if (argc)
-		usage_with_options(svn_fe_usage, svn_fe_options);
+	}
 	if (svndump_init(NULL))
 		return 1;
 	svndump_read(url);
-- 
1.7.6

[PATCH v2 05/11] vcs-svn: move url parameter from _read to _init

[Dmitry Ivankov]

svndump_read takes a url parameter that is used in git-svn-id: lines
generation. Internally it is stored in dump_ctx which is initialized
in svndump_init with reset_dump_ctx and then is reinitialized again
in svndump_read.

Move url parameter to svndump_init so that reset_dump_ctx is done
once per dump and in the same place as other resets. Wrap all _init
parameters to a struct svndump_args. More parameters will arise and
all will go to this struct to setup the module for dumping. Having
a struct reduces a chance to confuse one parameter with another a
bit, if they are filled via named assignments or common defines.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c |   12 ++++++------
 test-svn-fe.c           |    7 +++++--
 vcs-svn/svndump.c       |   12 ++++++------
 vcs-svn/svndump.h       |    8 ++++++--
 4 files changed, 23 insertions(+), 16 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 7e829b9..59141d6 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -12,10 +12,10 @@ static const char * const svn_fe_usage[] = {
 	NULL
 };
 
-static const char *url;
+static struct svndump_args args;
 
 static struct option svn_fe_options[] = {
-	OPT_STRING(0, "git-svn-id-url", &url, "url",
+	OPT_STRING(0, "git-svn-id-url", &args.url, "url",
 		"append git-svn metadata line to commit messages"),
 	OPT_END()
 };
@@ -25,16 +25,16 @@ int main(int argc, const char **argv)
 	argc = parse_options(argc, argv, NULL, svn_fe_options,
 						svn_fe_usage, 0);
 	if (argc == 1) {
-		if (url)
+		if (args.url)
 			usage_msg_opt("git-svn-id-url is set twice: as a "
 					"--parameter and as a [parameter]",
 					svn_fe_usage, svn_fe_options);
-		url = argv[0];
+		args.url = argv[0];
 	} else if (argc)
 		usage_with_options(svn_fe_usage, svn_fe_options);
-	if (svndump_init(NULL))
+	if (svndump_init(&args))
 		return 1;
-	svndump_read(url);
+	svndump_read();
 	svndump_deinit();
 	svndump_reset();
 	return 0;
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 0aab245..9e5b1a6 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -14,6 +14,8 @@ static const char * const test_svnfe_usage[] = {
 	NULL
 };
 
+static struct svndump_args args;
+
 static int d;
 
 static struct option test_svnfe_options[] = {
@@ -56,9 +58,10 @@ int main(int argc, const char *argv[])
 		return apply_delta(argc, argv);
 
 	if (argc == 1) {
-		if (svndump_init(argv[0]))
+		args.filename = argv[0];
+		if (svndump_init(&args))
 			return 1;
-		svndump_read(NULL);
+		svndump_read();
 		svndump_deinit();
 		svndump_reset();
 		return 0;
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index b1f4161..60cccad 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -12,6 +12,7 @@
 #include "fast_export.h"
 #include "line_buffer.h"
 #include "strbuf.h"
+#include "svndump.h"
 
 /*
  * Compare start of string to literal of equal length;
@@ -313,14 +314,13 @@ static void end_revision(void)
 		fast_export_end_commit(rev_ctx.revision);
 }
 
-void svndump_read(const char *url)
+void svndump_read(void)
 {
 	char *val;
 	char *t;
 	uint32_t active_ctx = DUMP_CTX;
 	uint32_t len;
 
-	reset_dump_ctx(url);
 	while ((t = buffer_read_line(&input))) {
 		val = strchr(t, ':');
 		if (!val)
@@ -455,10 +455,10 @@ void svndump_read(const char *url)
 		end_revision();
 }
 
-int svndump_init(const char *filename)
+int svndump_init(const struct svndump_args *args)
 {
-	if (buffer_init(&input, filename))
-		return error("cannot open %s: %s", filename, strerror(errno));
+	if (buffer_init(&input, args->filename))
+		return error("cannot open %s: %s", args->filename, strerror(errno));
 	fast_export_init(REPORT_FILENO);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
@@ -466,7 +466,7 @@ int svndump_init(const char *filename)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(NULL);
+	reset_dump_ctx(args->url);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index df9ceb0..b3dbf24 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,8 +1,12 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-int svndump_init(const char *filename);
-void svndump_read(const char *url);
+struct svndump_args {
+	const char *filename, *url;
+};
+
+int svndump_init(const struct svndump_args *args);
+void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
 
-- 
1.7.3.4

Re: [PATCH v2 05/11] vcs-svn: move url parameter from _read to _init

[Jonathan Nieder]

Dmitry Ivankov wrote:

> svndump_read takes a url parameter that is used in git-svn-id: lines
[...]
> Move url parameter to svndump_init so that reset_dump_ctx is done
> once per dump and in the same place as other resets. Wrap all _init
[...]

I'm getting lost seeing the forest for the trees, so let me try to
summarize.

Before:

	if (svndump_init(url))
		die("svndump_init failed");
	svndump_read(dumpfile);

After:

	struct svndump_args opts;

	memset(&opts, 0, sizeof(opts));
	opts.url = url;
	opts.filename = dumpfile;

	if (svndump_init(&opts))
		die("svndump_init failed");
	svndump_read();

Using an options struct instead of a list of arguments means each
option is optional and has a descriptive name mentioned at the call
site, and means it is easy to add new arguments in the future.

The patch still keeps the init/read distinction even though we don't
need it anywhere (i.e., all call sites look the same) to minimize its
invasiveness.

Do I understand correctly?

If so, it sounds like a good idea, and I have only minor nitpicks:

 - It's tempting to call the struct svndump_options, by analogy
   with struct merge_options from merge-recursive.h.

 - Now that we're making the name of the "url" argument part of the
   public API, maybe we should emphasize that the url is only for
   show and git will never try to contact it.  Maybe something like
   "metadata_url"?  (Sorry, I'm not so great at coming up with names.)

 - Likewise, the "filename" argument could be made more
   self-explanatory.  Maybe "dumpfile"?

 - Now that the filename argument is passed at init time instead of
   read time, there is some uncertainty about when the file is going
   to be opened.  A comment could help, or merging the two functions
   could help. :)

Thanks, and hope that helps.
Jonathan
---
 contrib/svn-fe/svn-fe.c |    4 ++--
 test-svn-fe.c           |    4 ++--
 vcs-svn/svndump.c       |   10 ++++++----
 vcs-svn/svndump.h       |    7 ++++---
 4 files changed, 14 insertions(+), 11 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 22d4cc14..88fca6a3 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -9,10 +9,10 @@
 
 int main(int argc, char **argv)
 {
-	struct svndump_args args;
+	struct svndump_options args;
 
 	memset(&args, 0, sizeof(args));
-	args.url = argc > 1 ? argv[1] : NULL;
+	args.metadata_url = argc > 1 ? argv[1] : NULL;
 	if (svndump_init(&args))
 		return 1;
 	svndump_read();
diff --git a/test-svn-fe.c b/test-svn-fe.c
index d80734fd..12a5c3ab 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -39,11 +39,11 @@ static int apply_delta(int argc, char *argv[])
 
 int main(int argc, char *argv[])
 {
-	struct svndump_args args;
+	struct svndump_options args;
 
 	memset(&args, 0, sizeof(args));
 	if (argc == 2) {
-		args.filename = argv[1];
+		args.dumpfile = argv[1];
 		if (svndump_init(&args))
 			return 1;
 		svndump_read();
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 60cccadc..805c94b6 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -455,10 +455,12 @@ void svndump_read(void)
 		end_revision();
 }
 
-int svndump_init(const struct svndump_args *args)
+int svndump_init(const struct svndump_options *opts)
 {
-	if (buffer_init(&input, args->filename))
-		return error("cannot open %s: %s", args->filename, strerror(errno));
+	const char *filename = opts->dumpfile;
+
+	if (buffer_init(&input, filename))
+		return error("cannot open %s: %s", filename, strerror(errno));
 	fast_export_init(REPORT_FILENO);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
@@ -466,7 +468,7 @@ int svndump_init(const struct svndump_args *args)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(args->url);
+	reset_dump_ctx(opts->metadata_url);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index b3dbf24e..b8f52172 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,11 +1,12 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-struct svndump_args {
-	const char *filename, *url;
+struct svndump_options {
+	const char *dumpfile;
+	const char *metadata_url;
 };
 
-int svndump_init(const struct svndump_args *args);
+int svndump_init(const struct svndump_options *o);
 void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
-- 
1.7.6

[PATCH v2 06/11] vcs-svn: move commit parameters logic to svndump.c

[Dmitry Ivankov]

fast_export.c had logic to set up commit ref, author name, email,
parent commit, import mark and git-svn-id: line based on both it's
own state (current import batch history) and the arguments passed.

Lift the decision on these parameters to the caller. This way it is
easier to customize them. Move progress lines generation to the caller
for the same reason.

Now fast_export doesn't have any internal state except the files set
up in fast_export_init, so it doesn't rely on being passed commits
sequentially and to one and the same branch. It operates just on a
current commit. Which makes it possible to generate an incremental
stream (if stream's first commit parent is set up properly by the
caller) or maybe to generate a stream for multiple svn branches.

Also progress lines generation is lifted up to svndump.o. So further
progress indication enhancements won't need to change fast_export.o
api.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 vcs-svn/fast_export.c |   44 ++++++++++++++------------------------------
 vcs-svn/fast_export.h |    8 +++++---
 vcs-svn/svndump.c     |   30 ++++++++++++++++++++++++++----
 3 files changed, 45 insertions(+), 37 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34..04001b8 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -13,9 +13,6 @@
 #include "sliding_window.h"
 #include "line_buffer.h"
 
-#define MAX_GITSVN_LINE_LEN 4096
-
-static uint32_t first_commit_done;
 static struct line_buffer postimage = LINE_BUFFER_INIT;
 static struct line_buffer report_buffer = LINE_BUFFER_INIT;
 
@@ -31,7 +28,6 @@ static int init_postimage(void)
 
 void fast_export_init(int fd)
 {
-	first_commit_done = 0;
 	if (buffer_fdinit(&report_buffer, fd))
 		die_errno("cannot read from file descriptor %d", fd);
 }
@@ -73,42 +69,30 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
-static char gitsvnline[MAX_GITSVN_LINE_LEN];
-void fast_export_begin_commit(uint32_t revision, const char *author,
-			const struct strbuf *log,
-			const char *uuid, const char *url,
-			unsigned long timestamp)
+void fast_export_begin_commit(uint32_t set_mark, const char *committer_name,
+			const char *committer_login, const char *committer_domain,
+			const struct strbuf *log, const char *gitsvnline,
+			unsigned long timestamp, uint32_t from_mark,
+			const char *dst_ref)
 {
-	static const struct strbuf empty = STRBUF_INIT;
-	if (!log)
-		log = &empty;
-	if (*uuid && *url) {
-		snprintf(gitsvnline, MAX_GITSVN_LINE_LEN,
-				"\n\ngit-svn-id: %s@%"PRIu32" %s\n",
-				 url, revision, uuid);
-	} else {
-		*gitsvnline = '\0';
-	}
-	printf("commit refs/heads/master\n");
-	printf("mark :%"PRIu32"\n", revision);
+	if (!gitsvnline)
+		gitsvnline = "";
+	printf("commit %s\n", dst_ref);
+	if (set_mark)
+		printf("mark :%"PRIu32"\n", set_mark);
 	printf("committer %s <%s@%s> %ld +0000\n",
-		   *author ? author : "nobody",
-		   *author ? author : "nobody",
-		   *uuid ? uuid : "local", timestamp);
+		committer_name, committer_login, committer_domain,
+		timestamp);
 	printf("data %"PRIuMAX"\n",
 		(uintmax_t) (log->len + strlen(gitsvnline)));
 	fwrite(log->buf, log->len, 1, stdout);
 	printf("%s\n", gitsvnline);
-	if (!first_commit_done) {
-		if (revision > 1)
-			printf("from :%"PRIu32"\n", revision - 1);
-		first_commit_done = 1;
-	}
+	if (from_mark)
+		printf("from :%"PRIu32"\n", from_mark);
 }
 
 void fast_export_end_commit(uint32_t revision)
 {
-	printf("progress Imported commit %"PRIu32".\n\n", revision);
 }
 
 static void ls_from_rev(uint32_t rev, const char *path)
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b6..6c1c2be 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -10,9 +10,11 @@ void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
-void fast_export_begin_commit(uint32_t revision, const char *author,
-			const struct strbuf *log, const char *uuid,
-			const char *url, unsigned long timestamp);
+void fast_export_begin_commit(uint32_t set_mark, const char *committer_name,
+			const char *committer_login, const char *committer_domain,
+			const struct strbuf *log, const char *gitsvnline,
+			unsigned long timestamp, uint32_t from_mark,
+			const char *dst_ref);
 void fast_export_end_commit(uint32_t revision);
 void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);
 void fast_export_blob_delta(uint32_t mode,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 60cccad..c58262a 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -37,6 +37,8 @@
 #define LENGTH_UNKNOWN (~0)
 #define DATE_RFC2822_LEN 31
 
+#define MAX_GITSVN_LINE_LEN 4096
+
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
@@ -54,6 +56,7 @@ static struct {
 static struct {
 	uint32_t version;
 	struct strbuf uuid, url;
+	int first_commit_done;
 } dump_ctx;
 
 static void reset_node_ctx(char *fname)
@@ -86,6 +89,7 @@ static void reset_dump_ctx(const char *url)
 		strbuf_addstr(&dump_ctx.url, url);
 	dump_ctx.version = 1;
 	strbuf_reset(&dump_ctx.uuid);
+	dump_ctx.first_commit_done = 0;
 }
 
 static void handle_property(const struct strbuf *key_buf,
@@ -299,19 +303,37 @@ static void handle_node(void)
 				node_ctx.textLength, &input);
 }
 
+static char gitsvnline[MAX_GITSVN_LINE_LEN];
 static void begin_revision(void)
 {
+	int from_mark;
+	const char *author;
+	const char *domain;
 	if (!rev_ctx.revision)	/* revision 0 gets no git commit. */
 		return;
-	fast_export_begin_commit(rev_ctx.revision, rev_ctx.author.buf,
-		&rev_ctx.log, dump_ctx.uuid.buf, dump_ctx.url.buf,
-		rev_ctx.timestamp);
+	if (*dump_ctx.uuid.buf && *dump_ctx.url.buf) {
+		snprintf(gitsvnline, MAX_GITSVN_LINE_LEN,
+				"\n\ngit-svn-id: %s@%"PRIu32" %s\n",
+				 dump_ctx.url.buf, rev_ctx.revision, dump_ctx.uuid.buf);
+	} else {
+		*gitsvnline = 0;
+	}
+	from_mark = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0;
+	author = *rev_ctx.author.buf ? rev_ctx.author.buf : "nobody";
+	domain = *dump_ctx.uuid.buf ? dump_ctx.uuid.buf : "local";
+
+	fast_export_begin_commit(rev_ctx.revision, author, author, domain,
+		&rev_ctx.log, gitsvnline, rev_ctx.timestamp,
+		from_mark);
 }
 
 static void end_revision(void)
 {
-	if (rev_ctx.revision)
+	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
+		printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
+		dump_ctx.first_commit_done = 1;
+	}
 }
 
 void svndump_read(void)
-- 
1.7.3.4

Re: [PATCH v2 06/11] vcs-svn: move commit parameters logic to svndump.c

[Jonathan Nieder]

Dmitry Ivankov wrote:

> fast_export.c had logic to set up commit ref, author name, email,
> parent commit, import mark and git-svn-id: line based on both it's
> own state (current import batch history) and the arguments passed.
>
> Lift the decision on these parameters to the caller.

Again I find myself getting lost.  I think this is another internal
API change, with the intent being to make the fast_export lib more
intuitive by making it focus on communicating with fast-import and the
delta applier instead of taking care of so much svn-fe-specific logic.
In other words, the idea would be to avoid a few layering violations.
Is that right?

If so:

> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -13,9 +13,6 @@
[...]
> -static uint32_t first_commit_done;

This is a good change.

[...]
> @@ -73,42 +69,30 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
>  	putchar('\n');
>  }
>  
> -static char gitsvnline[MAX_GITSVN_LINE_LEN];
> -void fast_export_begin_commit(uint32_t revision, const char *author,
> -			const struct strbuf *log,
> -			const char *uuid, const char *url,
> -			unsigned long timestamp)
> +void fast_export_begin_commit(uint32_t set_mark, const char *committer_name,
> +			const char *committer_login, const char *committer_domain,
> +			const struct strbuf *log, const char *gitsvnline,
> +			unsigned long timestamp, uint32_t from_mark,
> +			const char *dst_ref)

The argument list is getting scary.

[...]
>  void fast_export_end_commit(uint32_t revision)
>  {
> -	printf("progress Imported commit %"PRIu32".\n\n", revision);
>  }

This change leaves fast_export_end_commit empty.  Why not remove
it?  (Later patches that want to insert code there could reintroduce
the function.)

[...]
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -37,6 +37,8 @@
>  #define LENGTH_UNKNOWN (~0)
>  #define DATE_RFC2822_LEN 31
>  
> +#define MAX_GITSVN_LINE_LEN 4096
> +
>  static struct line_buffer input = LINE_BUFFER_INIT;
>  
>  static struct {
> @@ -54,6 +56,7 @@ static struct {
>  static struct {
>  	uint32_t version;
>  	struct strbuf uuid, url;
> +	int first_commit_done;

Sneaky: changing the type from uint32_t to int.  Good. :)

[...]
> @@ -299,19 +303,37 @@ static void handle_node(void)
[...]
> +	fast_export_begin_commit(rev_ctx.revision, author, author, domain,
> +		&rev_ctx.log, gitsvnline, rev_ctx.timestamp,
> +		from_mark);

This doesn't compile for me (missing "ref" argument).

>  }
>  
>  static void end_revision(void)
>  {
> -	if (rev_ctx.revision)
> +	if (rev_ctx.revision) {
>  		fast_export_end_commit(rev_ctx.revision);
> +		printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);

Until now, svndump.c did not have to know about the fast-import
format (e.g., the existence of a "progress" command).  Is that
worth changing?

Quick sketch with suggestions.  What do you think?

-- >8 --
Subject: squash! vcs-svn: move commit parameters logic to svndump.c

The previous commit doesn't build because we forgot to pass the new
ref name argument to the fast_export API.  While fixing that, simplify
fast_export_begin_commit to be more intuitive by using a set of
parameters closer to what gets written to fast-import.

The actual impact of this patch would be to run a little slower, since
we needlessly copy the author name into a temporary buffer for an
email address.  That is a small per-commit rather than per-path cost
so the loss in speed might be worth the gain in readability.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 vcs-svn/fast_export.c |   35 ++++++++++++++++-------------------
 vcs-svn/fast_export.h |   10 ++++------
 vcs-svn/svndump.c     |   41 ++++++++++++++++++++++++-----------------
 3 files changed, 44 insertions(+), 42 deletions(-)

diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 04001b83..f61113b4 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -69,30 +69,27 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
-void fast_export_begin_commit(uint32_t set_mark, const char *committer_name,
-			const char *committer_login, const char *committer_domain,
-			const struct strbuf *log, const char *gitsvnline,
-			unsigned long timestamp, uint32_t from_mark,
-			const char *dst_ref)
+void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark,
+			const char *author_name, const char *author_email,
+			const struct strbuf *log, unsigned long timestamp)
 {
-	if (!gitsvnline)
-		gitsvnline = "";
-	printf("commit %s\n", dst_ref);
-	if (set_mark)
-		printf("mark :%"PRIu32"\n", set_mark);
-	printf("committer %s <%s@%s> %ld +0000\n",
-		committer_name, committer_login, committer_domain,
-		timestamp);
-	printf("data %"PRIuMAX"\n",
-		(uintmax_t) (log->len + strlen(gitsvnline)));
+	if (!ref)
+		ref = "refs/heads/master";
+	printf("commit %s\n", ref);
+	if (mark)
+		printf("mark :%"PRIu32"\n", mark);
+	printf("committer %s <%s> %ld +0000\n",
+		author_name, author_email, timestamp);
+	printf("data %"PRIuMAX"\n", (uintmax_t) log->len);
 	fwrite(log->buf, log->len, 1, stdout);
-	printf("%s\n", gitsvnline);
-	if (from_mark)
-		printf("from :%"PRIu32"\n", from_mark);
+	putchar('\n');
+	if (prev_mark)
+		printf("from :%"PRIu32"\n", prev_mark);
 }
 
-void fast_export_end_commit(uint32_t revision)
+void fast_export_progress(uint32_t revision)
 {
+	printf("progress Imported commit %"PRIu32".\n\n", revision);
 }
 
 static void ls_from_rev(uint32_t rev, const char *path)
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 6c1c2be9..f2baf99d 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -10,12 +10,10 @@ void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
-void fast_export_begin_commit(uint32_t set_mark, const char *committer_name,
-			const char *committer_login, const char *committer_domain,
-			const struct strbuf *log, const char *gitsvnline,
-			unsigned long timestamp, uint32_t from_mark,
-			const char *dst_ref);
-void fast_export_end_commit(uint32_t revision);
+void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark,
+			const char *author_name, const char *author_email,
+			const struct strbuf *log, unsigned long timestamp);
+void fast_export_progress(uint32_t revision);
 void fast_export_data(uint32_t mode, uint32_t len, struct line_buffer *input);
 void fast_export_blob_delta(uint32_t mode,
 			uint32_t old_mode, const char *old_data,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 97d5fdb7..c562cdaa 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -302,35 +302,42 @@ static void handle_node(void)
 				node_ctx.textLength, &input);
 }
 
-static char gitsvnline[MAX_GITSVN_LINE_LEN];
+static void add_metadata_trailer(struct strbuf *buf)
+{
+	if (*dump_ctx.uuid.buf && *dump_ctx.url.buf)
+		strbuf_addf(buf, "\n\ngit-svn-id: %s@%"PRIu32" %s\n",
+			 dump_ctx.url.buf, rev_ctx.revision, dump_ctx.uuid.buf);
+}
+
 static void begin_revision(void)
 {
-	int from_mark;
+	static struct strbuf email;
 	const char *author;
-	const char *domain;
+	uint32_t prev;
+
 	if (!rev_ctx.revision)	/* revision 0 gets no git commit. */
 		return;
-	if (*dump_ctx.uuid.buf && *dump_ctx.url.buf) {
-		snprintf(gitsvnline, MAX_GITSVN_LINE_LEN,
-				"\n\ngit-svn-id: %s@%"PRIu32" %s\n",
-				 dump_ctx.url.buf, rev_ctx.revision, dump_ctx.uuid.buf);
-	} else {
-		*gitsvnline = 0;
-	}
-	from_mark = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0;
+	prev = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0;
 	author = *rev_ctx.author.buf ? rev_ctx.author.buf : "nobody";
-	domain = *dump_ctx.uuid.buf ? dump_ctx.uuid.buf : "local";
 
-	fast_export_begin_commit(rev_ctx.revision, author, author, domain,
-		&rev_ctx.log, gitsvnline, rev_ctx.timestamp,
-		from_mark);
+	strbuf_reset(&email);
+	strbuf_addstr(&email, author);
+	strbuf_addch(&email, '@');
+	if (*dump_ctx.uuid.buf)
+		strbuf_addstr(&email, dump_ctx.uuid.buf);
+	else
+		strbuf_addstr(&email, "local");
+
+	add_metadata_trailer(&rev_ctx.log);
+
+	fast_export_begin_commit(NULL, rev_ctx.revision, prev,
+		author, email.buf, &rev_ctx.log, rev_ctx.timestamp);
 }
 
 static void end_revision(void)
 {
 	if (rev_ctx.revision) {
-		fast_export_end_commit(rev_ctx.revision);
-		printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
+		fast_export_progress(rev_ctx.revision);
 		dump_ctx.first_commit_done = 1;
 	}
 }
-- 
1.7.6

[PATCH v2 07/11] vcs-svn,svn-fe: allow to specify dump destination ref

[Dmitry Ivankov]

svn-fe produces fast-import stream for a fixed refs/heads/master ref.
It is usually desired to write to a different ref. In a remote helper
it would be a ref in private namespace. If svn-fe is used by someone
directly it'll be more safe to remind where the commits can go. And
in both cases it may be needed to import from two repos and hence to
different refs.

Add a destination ref parameter to vcs-svn/, a corresponding parameter
to svn-fe and a simple test for it.

$ svn-fe --ref=refs/heads/master ...
is an explicit way to stay with the default destination.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    3 ++
 contrib/svn-fe/svn-fe.txt |    3 ++
 t/t9010-svn-fe.sh         |   49 +++++++++++++++++++++++++++++---------------
 test-svn-fe.c             |    5 +++-
 vcs-svn/svndump.c         |   11 ++++++---
 vcs-svn/svndump.h         |    1 +
 6 files changed, 50 insertions(+), 22 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 59141d6..11739bc 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -17,11 +17,14 @@ static struct svndump_args args;
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &args.url, "url",
 		"append git-svn metadata line to commit messages"),
+	OPT_STRING(0, "ref", &args.ref, "dst_ref",
+		"write to dst_ref instead of refs/heads/master"),
 	OPT_END()
 };
 
 int main(int argc, const char **argv)
 {
+	args.ref = "refs/heads/master";
 	argc = parse_options(argc, argv, NULL, svn_fe_options,
 						svn_fe_usage, 0);
 	if (argc == 1) {
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 8c6d347..20c3315 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -32,6 +32,9 @@ OPTIONS
 	Url to be used in git-svn-id: lines in git-svn
 	metadata lines format. See NOTES for more detailed
 	description.
+--ref=<dst_ref>::
+	Ref to be written by the generated stream.
+	Default is refs/heads/master.
 
 INPUT FORMAT
 ------------
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed24..52efabe 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -16,18 +16,24 @@ reinit_git () {
 	mkfifo stream backflow
 }
 
-try_dump () {
+try_dump_ext () {
+	args=$1 &&
+	shift &&
 	input=$1 &&
 	maybe_fail_svnfe=${2:+test_$2} &&
 	maybe_fail_fi=${3:+test_$3} &&
 
 	{
-		$maybe_fail_svnfe test-svn-fe "$input" >stream 3<backflow &
+		$maybe_fail_svnfe test-svn-fe $args "$input" >stream 3<backflow &
 	} &&
 	$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&
 	wait $!
 }
 
+try_dump () {
+	try_dump_ext "" $@
+}
+
 properties () {
 	while test "$#" -ne 0
 	do
@@ -54,6 +60,22 @@ text_no_props () {
 
 >empty
 
+cat >emptyprop.dump <<-EOF &&
+SVN-fs-dump-format-version: 3
+
+Revision-number: 1
+Prop-content-length: 10
+Content-length: 10
+
+PROPS-END
+
+Revision-number: 2
+Prop-content-length: 10
+Content-length: 10
+
+PROPS-END
+EOF
+
 test_expect_success 'setup: have pipes?' '
 	rm -f frob &&
 	if mkfifo frob
@@ -97,21 +119,6 @@ test_expect_failure PIPE 'empty revision' '
 test_expect_success PIPE 'empty properties' '
 	reinit_git &&
 	printf "rev <nobody, nobody@local>: %s\n" "" "" >expect &&
-	cat >emptyprop.dump <<-\EOF &&
-	SVN-fs-dump-format-version: 3
-
-	Revision-number: 1
-	Prop-content-length: 10
-	Content-length: 10
-
-	PROPS-END
-
-	Revision-number: 2
-	Prop-content-length: 10
-	Content-length: 10
-
-	PROPS-END
-	EOF
 	try_dump emptyprop.dump &&
 	git log -p --format="rev <%an, %ae>: %s" HEAD >actual &&
 	test_cmp expect actual
@@ -1111,4 +1118,12 @@ test_expect_success SVNREPO,PIPE 't9135/svn.dump' '
 	)
 '
 
+test_expect_success PIPE 'import to notmaster ref' '
+	reinit_git &&
+	try_dump_ext "--ref=refs/heads/notmaster" emptyprop.dump &&
+
+	git rev-parse --verify notmaster &&
+	test_must_fail git rev-parse --verify master
+'
+
 test_done
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 9e5b1a6..bc437b3 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -10,7 +10,7 @@
 #include "vcs-svn/line_buffer.h"
 
 static const char * const test_svnfe_usage[] = {
-	"test-svn-fe (<dumpfile> | -d <preimage> <delta> <len>)",
+	"test-svn-fe ([options] <dumpfile> | -d <preimage> <delta> <len>)",
 	NULL
 };
 
@@ -20,6 +20,8 @@ static int d;
 
 static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_STRING(0, "ref", &args.ref, "dst_ref",
+		"write to dst_ref instead of refs/heads/master"),
 	OPT_END()
 };
 
@@ -51,6 +53,7 @@ static int apply_delta(int argc, const char *argv[])
 
 int main(int argc, const char *argv[])
 {
+	args.ref = "refs/heads/master";
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index c58262a..616f9ec 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -57,6 +57,7 @@ static struct {
 	uint32_t version;
 	struct strbuf uuid, url;
 	int first_commit_done;
+	struct strbuf ref_name;
 } dump_ctx;
 
 static void reset_node_ctx(char *fname)
@@ -82,7 +83,7 @@ static void reset_rev_ctx(uint32_t revision)
 	strbuf_reset(&rev_ctx.author);
 }
 
-static void reset_dump_ctx(const char *url)
+static void reset_dump_ctx(const char *url, const char *dst_ref)
 {
 	strbuf_reset(&dump_ctx.url);
 	if (url)
@@ -90,6 +91,8 @@ static void reset_dump_ctx(const char *url)
 	dump_ctx.version = 1;
 	strbuf_reset(&dump_ctx.uuid);
 	dump_ctx.first_commit_done = 0;
+	strbuf_reset(&dump_ctx.ref_name);
+	strbuf_addstr(&dump_ctx.ref_name, dst_ref);
 }
 
 static void handle_property(const struct strbuf *key_buf,
@@ -324,7 +327,7 @@ static void begin_revision(void)
 
 	fast_export_begin_commit(rev_ctx.revision, author, author, domain,
 		&rev_ctx.log, gitsvnline, rev_ctx.timestamp,
-		from_mark);
+		from_mark, dump_ctx.ref_name.buf);
 }
 
 static void end_revision(void)
@@ -488,7 +491,7 @@ int svndump_init(const struct svndump_args *args)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(args->url);
+	reset_dump_ctx(args->url, args->ref);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
@@ -497,7 +500,7 @@ int svndump_init(const struct svndump_args *args)
 void svndump_deinit(void)
 {
 	fast_export_deinit();
-	reset_dump_ctx(NULL);
+	reset_dump_ctx(NULL, "");
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index b3dbf24..904e628 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -3,6 +3,7 @@
 
 struct svndump_args {
 	const char *filename, *url;
+	const char *ref;
 };
 
 int svndump_init(const struct svndump_args *args);
-- 
1.7.3.4

Re: [PATCH v2 07/11] vcs-svn,svn-fe: allow to specify dump destination ref

[Jonathan Nieder]

Dmitry Ivankov wrote:

> svn-fe produces fast-import stream for a fixed refs/heads/master ref.
> It is usually desired to write to a different ref. In a remote helper
> it would be a ref in private namespace. If svn-fe is used by someone
> directly it'll be more safe to remind where the commits can go. And
> in both cases it may be needed to import from two repos and hence to
> different refs.
>
> Add a destination ref parameter to vcs-svn/, a corresponding parameter
> to svn-fe and a simple test for it.
>
> $ svn-fe --ref=refs/heads/master ...
> is an explicit way to stay with the default destination.

This improvement is very welcome!

Another reason to avoid fetching straight to "master" is that changing
HEAD from under the user's feet can be confusing.  See [*] for some
explanation.

> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -17,11 +17,14 @@ static struct svndump_args args;
>  static struct option svn_fe_options[] = {
>  	OPT_STRING(0, "git-svn-id-url", &args.url, "url",
>  		"append git-svn metadata line to commit messages"),
> +	OPT_STRING(0, "ref", &args.ref, "dst_ref",
> +		"write to dst_ref instead of refs/heads/master"),

A small nit: such an underscored identifier would be typical for a
variable name in code but less so for a user-visible syntactic
placeholder.  One possibility for avoiding that (inspired "git
update-ref") is:

	OPT_STRING(0, "ref", &args.ref, "refname",
		"write to <refname> instead of refs/heads/master"),

which shows up in "svn-fe -h" output as

    --ref <refname>       write to <refname> instead of refs/heads/master

>  	OPT_END()
>  };
>  
>  int main(int argc, const char **argv)
>  {
> +	args.ref = "refs/heads/master";
>  	argc = parse_options(argc, argv, NULL, svn_fe_options,
>  						svn_fe_usage, 0);

Would it make sense to avoid having to pre-initialize ref by
interpreting NULL as refs/heads/master?

My secret goal in asking that is to find some way to avoid the
git-specific idiom of the refs/heads/master ref in fast-import
frontends of the future, by coming up with some improvement to the
syntax some day (like "commit default").

> --- a/contrib/svn-fe/svn-fe.txt
> +++ b/contrib/svn-fe/svn-fe.txt
> @@ -32,6 +32,9 @@ OPTIONS
>  	Url to be used in git-svn-id: lines in git-svn
>  	metadata lines format. See NOTES for more detailed
>  	description.
> +--ref=<dst_ref>::
> +	Ref to be written by the generated stream.
> +	Default is refs/heads/master.

Style: usually there is a blank line between items in definition list
markup.  Options in manpages (unlike commands, files, and
configuration items) are usually described in the imperative mood, as
a command you give to the program.  Like so:

	--ref=<refname>::
		Make the ref <refname> point to the tip of the history
		imported so far, instead of writing to
		refs/heads/master.  This can be useful when importing
		into a non-bare repository and the "master" branch is
		checked out.

[...]
> --- a/t/t9010-svn-fe.sh
> +++ b/t/t9010-svn-fe.sh
> @@ -16,18 +16,24 @@ reinit_git () {
>  	mkfifo stream backflow
>  }
>  
> -try_dump () {
> +try_dump_ext () {
> +	args=$1 &&
> +	shift &&
>  	input=$1 &&
>  	maybe_fail_svnfe=${2:+test_$2} &&
>  	maybe_fail_fi=${3:+test_$3} &&
>  
>  	{
> -		$maybe_fail_svnfe test-svn-fe "$input" >stream 3<backflow &
> +		$maybe_fail_svnfe test-svn-fe $args "$input" >stream 3<backflow &

Is this new function needed?.  We could let the $args argument include
$input, like so:

	try_dump an-interesting-dump.dump
	try_dump "--in-a-special-way another-interesting-dump.dump"

Another alternative would be to treat arguments starting with a minus
sign specially, so the call sites could look natural:

	try_dump --in-a-special-way another-interesting-dump.dump

though that seems more fragile (e.g., if I want to test "svn-fe --ref
detached-argument").

> @@ -54,6 +60,22 @@ text_no_props () {
>  
>  >empty
>  
> +cat >emptyprop.dump <<-EOF &&

No need for the minus sign and &&, since this is unindented and just
supplying test data rather than part of a chain of test assertions
that can fail.  Putting a \ before the EOF can be a friendly touch to
save reviewers the trouble of looking for shell metacharacters in the
here document's body.

[...]
> @@ -97,21 +119,6 @@ test_expect_failure PIPE 'empty revision' '
>  test_expect_success PIPE 'empty properties' '
>  	reinit_git &&
>  	printf "rev <nobody, nobody@local>: %s\n" "" "" >expect &&
> -	cat >emptyprop.dump <<-\EOF &&

Nice.

> @@ -1111,4 +1118,12 @@ test_expect_success SVNREPO,PIPE 't9135/svn.dump' '
>  	)
>  '
>  
> +test_expect_success PIPE 'import to notmaster ref' '
> +	reinit_git &&
> +	try_dump_ext "--ref=refs/heads/notmaster" emptyprop.dump &&
> +
> +	git rev-parse --verify notmaster &&
> +	test_must_fail git rev-parse --verify master
> +'

Nice and simple.  Maybe it would be logical to put it earlier in the
file (for three reasons: on one hand to make the file easier to read
straight through; on the other hand to make other patches adding tests
less likely to conflict with this one; and to keep the slow test that
uses svnadmin at the end of the test script, so results from simpler
ones like this come sooner).

> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
[...]
> @@ -20,6 +20,8 @@ static int d;
>  
>  static struct option test_svnfe_options[] = {
>  	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
> +	OPT_STRING(0, "ref", &args.ref, "dst_ref",
> +		"write to dst_ref instead of refs/heads/master"),
[...]
> @@ -51,6 +53,7 @@ static int apply_delta(int argc, const char *argv[])
>  
>  int main(int argc, const char *argv[])
>  {
> +	args.ref = "refs/heads/master";

See corresponding comments for contrib/svn-fe.

[...]
> --- a/vcs-svn/svndump.c
> +++ b/vcs-svn/svndump.c
> @@ -57,6 +57,7 @@ static struct {
>  	uint32_t version;
>  	struct strbuf uuid, url;
>  	int first_commit_done;
> +	struct strbuf ref_name;

This means the svndump module keeps its own copy of the refname
string, saving the caller the trouble of keeping it alive.  Probably
sensible, especially because there could be code between
svndump_init() and svndump_read() that actually uses it (for example,
if the refname is taken from an envvar that getenv() clobbers).

> @@ -324,7 +327,7 @@ static void begin_revision(void)
>  
>  	fast_export_begin_commit(rev_ctx.revision, author, author, domain,
>  		&rev_ctx.log, gitsvnline, rev_ctx.timestamp,
> -		from_mark);
> +		from_mark, dump_ctx.ref_name.buf);
>  }

Hm.  fast_export_begin_commit acts on a ref in a sense.  That could
mean the argument should be the first or last one, depending on
whether we are imitating fprintf() or fwrite().

For reference, here's what I tested locally (as a full patch instead
of incremental because patches it's based on changed enough that the
original did not apply cleanly).

Thanks.

[*] https://git.wiki.kernel.org/index.php/GitFaq#How_would_I_use_.22git_push.22_to_sync_out_of_a_host_that_I_cannot_pull_from.3F

-- >8 --
From: Dmitry Ivankov <divanorama@gmail.com>
Date: Wed, 13 Jul 2011 18:21:09 +0600
Subject: vcs-svn,svn-fe: allow to specify dump destination ref

svn-fe produces fast-import stream for a fixed refs/heads/master ref.
It is usually desired to write to a different ref. In a remote helper
it would be a ref in private namespace. If svn-fe is used by someone
directly it'll be more safe to remind where the commits can go. And
in both cases it may be needed to import from two repos and hence to
different refs.

Add a destination ref parameter to vcs-svn/, a corresponding parameter
to svn-fe and a simple test for it.

$ svn-fe --ref=refs/heads/master ...
is an explicit way to stay with the default destination.

[jn: use NULL for default, clarify documentation, reorder
 reset_dump_ctx arguments, last fast_export module specify default,
 pass by strbuf so signature changes (API break), test simplifications]

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    2 ++
 contrib/svn-fe/svn-fe.txt |    7 +++++++
 t/t9010-svn-fe.sh         |   42 +++++++++++++++++++++++++-----------------
 test-svn-fe.c             |    6 +++---
 vcs-svn/fast_export.c     |    9 +++++----
 vcs-svn/fast_export.h     |    5 ++++-
 vcs-svn/svndump.c         |   14 ++++++++++----
 vcs-svn/svndump.h         |    1 +
 8 files changed, 57 insertions(+), 29 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 5adb2706..c3ebe272 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -17,6 +17,8 @@ static struct svndump_options args;
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &args.metadata_url, "url",
 		"add git-svn-id line to log messages, imitating git-svn"),
+	OPT_STRING(0, "ref", &args.ref, "refname",
+		"write to <refname> instead of refs/heads/master"),
 	OPT_END()
 };
 
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 8c6d3471..7d389ca1 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -33,6 +33,13 @@ OPTIONS
 	metadata lines format. See NOTES for more detailed
 	description.
 
+--ref=<refname>::
+	Make the ref <refname> point to the tip of the history
+	imported so far, instead of writing to
+	`refs/heads/master`.  This can be useful when importing
+	into a non-bare repository and the "master" branch is
+	checked out.
+
 INPUT FORMAT
 ------------
 Subversion's repository dump format is documented in full in
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed248..5df1a9bd 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -17,12 +17,12 @@ reinit_git () {
 }
 
 try_dump () {
-	input=$1 &&
+	args=$1 &&
 	maybe_fail_svnfe=${2:+test_$2} &&
 	maybe_fail_fi=${3:+test_$3} &&
 
 	{
-		$maybe_fail_svnfe test-svn-fe "$input" >stream 3<backflow &
+		$maybe_fail_svnfe test-svn-fe $args >stream 3<backflow &
 	} &&
 	$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&
 	wait $!
@@ -54,6 +54,22 @@ text_no_props () {
 
 >empty
 
+cat >emptyprop.dump <<\EOF
+SVN-fs-dump-format-version: 3
+
+Revision-number: 1
+Prop-content-length: 10
+Content-length: 10
+
+PROPS-END
+
+Revision-number: 2
+Prop-content-length: 10
+Content-length: 10
+
+PROPS-END
+EOF
+
 test_expect_success 'setup: have pipes?' '
 	rm -f frob &&
 	if mkfifo frob
@@ -97,26 +113,18 @@ test_expect_failure PIPE 'empty revision' '
 test_expect_success PIPE 'empty properties' '
 	reinit_git &&
 	printf "rev <nobody, nobody@local>: %s\n" "" "" >expect &&
-	cat >emptyprop.dump <<-\EOF &&
-	SVN-fs-dump-format-version: 3
-
-	Revision-number: 1
-	Prop-content-length: 10
-	Content-length: 10
-
-	PROPS-END
-
-	Revision-number: 2
-	Prop-content-length: 10
-	Content-length: 10
-
-	PROPS-END
-	EOF
 	try_dump emptyprop.dump &&
 	git log -p --format="rev <%an, %ae>: %s" HEAD >actual &&
 	test_cmp expect actual
 '
 
+test_expect_success PIPE 'import to notmaster ref' '
+	reinit_git &&
+	try_dump "--ref=refs/heads/notmaster emptyprop.dump" &&
+	git rev-parse --verify notmaster &&
+	test_must_fail git rev-parse --verify master
+'
+
 test_expect_success PIPE 'author name and commit message' '
 	reinit_git &&
 	echo "<author@example.com, author@example.com@local>" >expect.author &&
diff --git a/test-svn-fe.c b/test-svn-fe.c
index e114bc7c..40f9eaea 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -15,11 +15,14 @@ static const char * const test_svnfe_usage[] = {
 	NULL
 };
 
+static struct svndump_options args;
 static int apply_delta_instead;
 
 static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', "apply-delta",
 		&apply_delta_instead, "apply a subversion-format delta", 1),
+	OPT_STRING(0, "ref", &args.ref, "refname",
+		"write to <refname> instead of refs/heads/master"),
 	OPT_END()
 };
 
@@ -51,9 +54,6 @@ static int apply_delta(int argc, const char *argv[])
 
 int main(int argc, const char *argv[])
 {
-	struct svndump_options args;
-
-	memset(&args, 0, sizeof(args));
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index f61113b4..638485f1 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -69,13 +69,14 @@ void fast_export_modify(const char *path, uint32_t mode, const char *dataref)
 	putchar('\n');
 }
 
-void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark,
+void fast_export_begin_commit(const struct strbuf *ref,
+			uint32_t mark, uint32_t prev_mark,
 			const char *author_name, const char *author_email,
 			const struct strbuf *log, unsigned long timestamp)
 {
-	if (!ref)
-		ref = "refs/heads/master";
-	printf("commit %s\n", ref);
+	printf("commit ");
+	fwrite(ref->buf, ref->len, 1, stdout);
+	putchar('\n');
 	if (mark)
 		printf("mark :%"PRIu32"\n", mark);
 	printf("committer %s <%s> %ld +0000\n",
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index f2baf99d..4987afbd 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -1,6 +1,8 @@
 #ifndef FAST_EXPORT_H_
 #define FAST_EXPORT_H_
 
+#define SVN_FE_DEFAULT_REF "refs/heads/master"
+
 struct strbuf;
 struct line_buffer;
 
@@ -10,7 +12,8 @@ void fast_export_reset(void);
 
 void fast_export_delete(const char *path);
 void fast_export_modify(const char *path, uint32_t mode, const char *dataref);
-void fast_export_begin_commit(const char *ref, uint32_t mark, uint32_t prev_mark,
+void fast_export_begin_commit(const struct strbuf *ref,
+			uint32_t mark, uint32_t prev_mark,
 			const char *author_name, const char *author_email,
 			const struct strbuf *log, unsigned long timestamp);
 void fast_export_progress(uint32_t revision);
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 0136747b..55c5b657 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -57,6 +57,7 @@ static struct {
 	uint32_t version;
 	struct strbuf uuid, url;
 	int first_commit_done;
+	struct strbuf ref;
 } dump_ctx;
 
 static void reset_node_ctx(char *fname)
@@ -82,7 +83,7 @@ static void reset_rev_ctx(uint32_t revision)
 	strbuf_reset(&rev_ctx.author);
 }
 
-static void reset_dump_ctx(const char *url)
+static void reset_dump_ctx(const char *ref, const char *url)
 {
 	strbuf_reset(&dump_ctx.url);
 	if (url)
@@ -90,6 +91,11 @@ static void reset_dump_ctx(const char *url)
 	dump_ctx.version = 1;
 	strbuf_reset(&dump_ctx.uuid);
 	dump_ctx.first_commit_done = 0;
+	strbuf_reset(&dump_ctx.ref);
+	if (ref)
+		strbuf_addstr(&dump_ctx.ref, ref);
+	else
+		strbuf_addstr(&dump_ctx.ref, SVN_FE_DEFAULT_REF);
 }
 
 static void handle_property(const struct strbuf *key_buf,
@@ -331,7 +337,7 @@ static void begin_revision(void)
 
 	add_metadata_trailer(&rev_ctx.log);
 
-	fast_export_begin_commit(NULL, rev_ctx.revision, prev,
+	fast_export_begin_commit(&dump_ctx.ref, rev_ctx.revision, prev,
 		author, email.buf, &rev_ctx.log, rev_ctx.timestamp);
 }
 
@@ -497,7 +503,7 @@ int svndump_init(const struct svndump_options *opts)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(opts->metadata_url);
+	reset_dump_ctx(opts->ref, opts->metadata_url);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
@@ -506,7 +512,7 @@ int svndump_init(const struct svndump_options *opts)
 void svndump_deinit(void)
 {
 	fast_export_deinit();
-	reset_dump_ctx(NULL);
+	reset_dump_ctx(NULL, NULL);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index b8f52172..4de98700 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -3,6 +3,7 @@
 
 struct svndump_options {
 	const char *dumpfile;
+	const char *ref;
 	const char *metadata_url;
 };
 
-- 
1.7.6

[PATCH v2 08/11] vcs-svn,svn-fe: convert REPORT_FILENO to an option

[Dmitry Ivankov]

svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
These come through a file descriptor number 3.

Sometimes it is easier to setup variable fd than a fixed one. It is
the case with pipe() call and even more fd=3 can be already taken.
On Windows file descriptors are not by default inherited by a child
process, nor there is an option to setup descriptors other than
standard stdin, stdout, stderr at a process creation time.

Add an option for this file descriptor number in vcs-svn/ and svn-fe,
add a simple test for it.

To be used like following:
$ svn-fe --read-blob-fd=7 ... 7<somewhere

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    3 ++
 contrib/svn-fe/svn-fe.txt |    8 ++++-
 t/t9010-svn-fe.sh         |   58 +++++++++++++++++++++++++++++++++++++++++---
 test-svn-fe.c             |    3 ++
 vcs-svn/svndump.c         |    4 +--
 vcs-svn/svndump.h         |    1 +
 6 files changed, 68 insertions(+), 9 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 11739bc..cd9e449 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -19,12 +19,15 @@ static struct option svn_fe_options[] = {
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
+		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
 };
 
 int main(int argc, const char **argv)
 {
 	args.ref = "refs/heads/master";
+	args.backflow_fd = 3;
 	argc = parse_options(argc, argv, NULL, svn_fe_options,
 						svn_fe_usage, 0);
 	if (argc == 1) {
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 20c3315..a7ad368 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -8,9 +8,9 @@ svn-fe - convert an SVN "dumpfile" to a fast-import stream
 SYNOPSIS
 --------
 [verse]
-mkfifo backchannel &&
+mkfifo backchannel && fd=3 &&
 svnadmin dump --deltas REPO |
-	svn-fe [options] [git-svn-id-url] 3<backchannel |
+	eval "svn-fe [options] [git-svn-id-url] $fd<backchannel" |
 	git fast-import --cat-blob-fd=3 3>backchannel
 
 DESCRIPTION
@@ -35,6 +35,10 @@ OPTIONS
 --ref=<dst_ref>::
 	Ref to be written by the generated stream.
 	Default is refs/heads/master.
+--read-blob-fd=<fd>::
+	Integer number of file descriptor from which
+	responses to 'ls' and 'cat-blob' requests will come.
+	Default is fd=3.
 
 INPUT FORMAT
 ------------
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index 52efabe..6dcad94 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -18,20 +18,21 @@ reinit_git () {
 
 try_dump_ext () {
 	args=$1 &&
-	shift &&
+	fd=${2:-3} &&
+	shift 2 &&
 	input=$1 &&
 	maybe_fail_svnfe=${2:+test_$2} &&
 	maybe_fail_fi=${3:+test_$3} &&
 
 	{
-		$maybe_fail_svnfe test-svn-fe $args "$input" >stream 3<backflow &
+		eval "$maybe_fail_svnfe test-svn-fe $args "$input" >stream $fd<backflow" &
 	} &&
 	$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&
 	wait $!
 }
 
 try_dump () {
-	try_dump_ext "" $@
+	try_dump_ext "" "" $@
 }
 
 properties () {
@@ -1120,10 +1121,59 @@ test_expect_success SVNREPO,PIPE 't9135/svn.dump' '
 
 test_expect_success PIPE 'import to notmaster ref' '
 	reinit_git &&
-	try_dump_ext "--ref=refs/heads/notmaster" emptyprop.dump &&
+	try_dump_ext "--ref=refs/heads/notmaster" 3 emptyprop.dump &&
 
 	git rev-parse --verify notmaster &&
 	test_must_fail git rev-parse --verify master
 '
 
+test_expect_success PIPE 'use different backflow fd' '
+	reinit_git &&
+	echo hi >hi &&
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "1999-02-01T00:01:002.000000Z" \
+			svn:log "add directory with some files in it" &&
+		echo PROPS-END
+	} >props &&
+	{
+		echo Prop-content-length: $(wc -c <props) &&
+		echo Content-length: $(wc -c <props) &&
+		echo &&
+		cat props
+	} >revprops &&
+	{
+		cat <<-EOF &&
+		SVN-fs-dump-format-version: 3
+
+		Revision-number: 1
+		EOF
+		cat revprops &&
+		cat <<-EOF &&
+		Node-path: directory
+		Node-kind: dir
+		Node-action: add
+		Node-path: directory/somefile
+		Node-kind: file
+		Node-action: add
+		EOF
+		text_no_props hi &&
+
+		echo "Revision-number: 2" &&
+		cat revprops &&
+		cat <<-\EOF
+		Node-path: otherfile
+		Node-kind: file
+		Node-action: add
+		Node-copyfrom-rev: 1
+		Node-copyfrom-path: directory/somefile
+		EOF
+	} >directory.dump &&
+	try_dump_ext "--read-blob-fd=7" 7 directory.dump &&
+
+	git checkout HEAD otherfile &&
+	test_cmp hi otherfile
+'
+
 test_done
diff --git a/test-svn-fe.c b/test-svn-fe.c
index bc437b3..43e19b2 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -22,6 +22,8 @@ static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
+		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
 };
 
@@ -54,6 +56,7 @@ static int apply_delta(int argc, const char *argv[])
 int main(int argc, const char *argv[])
 {
 	args.ref = "refs/heads/master";
+	args.backflow_fd = 3;
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 616f9ec..4194052 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -20,8 +20,6 @@
  */
 #define constcmp(s, ref) memcmp(s, ref, sizeof(ref) - 1)
 
-#define REPORT_FILENO 3
-
 #define NODEACT_REPLACE 4
 #define NODEACT_DELETE 3
 #define NODEACT_ADD 2
@@ -484,7 +482,7 @@ int svndump_init(const struct svndump_args *args)
 {
 	if (buffer_init(&input, args->filename))
 		return error("cannot open %s: %s", args->filename, strerror(errno));
-	fast_export_init(REPORT_FILENO);
+	fast_export_init(args->backflow_fd);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
 	strbuf_init(&rev_ctx.log, 4096);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index 904e628..d266b68 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -4,6 +4,7 @@
 struct svndump_args {
 	const char *filename, *url;
 	const char *ref;
+	int backflow_fd;
 };
 
 int svndump_init(const struct svndump_args *args);
-- 
1.7.3.4

Re: [PATCH v2 08/11] vcs-svn,svn-fe: convert REPORT_FILENO to an option

[Jonathan Nieder]

Dmitry Ivankov wrote:

> svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
> These come through a file descriptor number 3.
>
> Sometimes it is easier to setup variable fd than a fixed one. It is
> the case with pipe() call and even more fd=3 can be already taken.
> On Windows file descriptors are not by default inherited by a child
> process, nor there is an option to setup descriptors other than
> standard stdin, stdout, stderr at a process creation time.
>
> Add an option for this file descriptor number in vcs-svn/ and svn-fe,
> add a simple test for it.
>
> To be used like following:
> $ svn-fe --read-blob-fd=7 ... 7<somewhere

Thanks.  The above description covers the basics but I think it is out
of order.  Maybe it would make sense to say:

 . first, that Windows lacks fork() and has facilities to redirect
   stdin, stdout, and stderr and to inherit others in a child process
   but nothing more (by the way, does anyone on list know if this is
   true?)

 . second, that this patch should help to work around that by allowing
   the caller to tell what file descriptor number the reading end of
   the "feature cat-blob" pipe inherited

 . third, that being able to specify the fd number is more convenient
   anyway

 . lastly, that the option is plumbed into both test-svn-fe and
   contrib's svn-fe tool, and what usage looks like

That way, the motivation comes first.

It is also possible to motivate it by that third point instead
(hard-coded fds as part of a command's interface do not scale and are
just weird), so I'd be tempted to leave out the Windows stuff I am
uncertain about if I were writing it. :)

> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -19,12 +19,15 @@ static struct option svn_fe_options[] = {
>  		"append git-svn metadata line to commit messages"),
>  	OPT_STRING(0, "ref", &args.ref, "dst_ref",
>  		"write to dst_ref instead of refs/heads/master"),
> +	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
> +		"read blobs and trees from this fd instead of 3"),

>From the operator's point of view, I think this is just the other end
of the pipe that fast-import --cat-blob-fd writes to.  Maybe

	"read fast-import replies from file descriptor <n> (default: 3)"

[...]
>  	OPT_END()
>  };
>  
>  int main(int argc, const char **argv)
>  {
>  	args.ref = "refs/heads/master";
> +	args.backflow_fd = 3;

Like in the last patch, it is tempting to push this default to the
library so others can conveniently use "-1".

[...]
> --- a/contrib/svn-fe/svn-fe.txt
> +++ b/contrib/svn-fe/svn-fe.txt
[...]
> @@ -35,6 +35,10 @@ OPTIONS
>  --ref=<dst_ref>::
>  	Ref to be written by the generated stream.
>  	Default is refs/heads/master.
> +--read-blob-fd=<fd>::
> +	Integer number of file descriptor from which
> +	responses to 'ls' and 'cat-blob' requests will come.
> +	Default is fd=3.

Nit: more usual to use a verb phrase.

> --- a/t/t9010-svn-fe.sh
> +++ b/t/t9010-svn-fe.sh
> @@ -18,20 +18,21 @@ reinit_git () {
>  
>  try_dump_ext () {

If try_dump_ext from the previous patch gets removed, it would ripple
through here, too.  Demonstration of one possible approach below.

[...]
> +test_expect_success PIPE 'use different backflow fd' '
> +	reinit_git &&
> +	echo hi >hi &&
> +	{
> +		properties \
> +			svn:author author@example.com \
> +			svn:date "1999-02-01T00:01:002.000000Z" \
> +			svn:log "add directory with some files in it" &&

Is this dump copy/pasted from another test?  Would it be possible to
simplify or share the dumpfile?

Some but not all of the suggestions above implemented below (this is
just an example; if something looks crazy, please feel free to drop or
fix it, of course).

Sorry to take so long to look this over.  In broad strokes your
patches carry out very good changes.

-- >8 --
From: Dmitry Ivankov <divanorama@gmail.com>

svn-fe needs to read fast-import's responses to "ls" and "cat-blob".
These come through a file descriptor number 3.

Sometimes it is easier to setup variable fd than a fixed one. It is
the case with pipe() call and even more fd=3 can be already taken.
On Windows file descriptors are not by default inherited by a child
process, nor there is an option to setup descriptors other than
standard stdin, stdout, stderr at a process creation time.

Add an option for this file descriptor number in vcs-svn/ and svn-fe,
add a simple test for it.

To be used like following:
$ svn-fe --read-blob-fd=7 ... 7<somewhere

[jn: various style tweaks]

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    4 ++-
 contrib/svn-fe/svn-fe.txt |    8 +++++-
 t/t9010-svn-fe.sh         |   54 +++++++++++++++++++++++++++++++++++++++++++-
 test-svn-fe.c             |    6 ++--
 vcs-svn/fast_export.c     |    2 +
 vcs-svn/svndump.c         |    4 +--
 vcs-svn/svndump.h         |    3 ++
 7 files changed, 70 insertions(+), 11 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 5adb2706..eaab2c79 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -12,11 +12,13 @@ static const char * const svn_fe_usage[] = {
 	NULL
 };
 
-static struct svndump_options args;
+static struct svndump_options args = SVNDUMP_OPTIONS_INIT;
 
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &args.metadata_url, "url",
 		"add git-svn-id line to log messages, imitating git-svn"),
+	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
+		"read fast-import replies from file descriptor <n> (default: 3)"),
 	OPT_END()
 };
 
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 8c6d3471..13ab81b3 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -8,9 +8,9 @@ svn-fe - convert an SVN "dumpfile" to a fast-import stream
 SYNOPSIS
 --------
 [verse]
-mkfifo backchannel &&
+mkfifo backchannel && fd=3 &&
 svnadmin dump --deltas REPO |
-	svn-fe [options] [git-svn-id-url] 3<backchannel |
+	eval "svn-fe [options] [git-svn-id-url] $fd<backchannel" |
 	git fast-import --cat-blob-fd=3 3>backchannel
 
 DESCRIPTION
@@ -33,6 +33,10 @@ OPTIONS
 	metadata lines format. See NOTES for more detailed
 	description.
 
+--read-blob-fd=<fd>::
+	Read responses to 'ls' and 'cat-blob' requests from
+	this file descriptor.  The default is 3.
+
 INPUT FORMAT
 ------------
 Subversion's repository dump format is documented in full in
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed248..e8585260 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -17,12 +17,13 @@ reinit_git () {
 }
 
 try_dump () {
-	input=$1 &&
+	args=$1 &&
 	maybe_fail_svnfe=${2:+test_$2} &&
 	maybe_fail_fi=${3:+test_$3} &&
+	fd=${4:-3}
 
 	{
-		$maybe_fail_svnfe test-svn-fe "$input" >stream 3<backflow &
+		eval "$maybe_fail_svnfe test-svn-fe $args >stream $fd<backflow" &
 	} &&
 	$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&
 	wait $!
@@ -739,6 +740,55 @@ test_expect_success PIPE 'deltas supported' '
 	try_dump delta.dump
 '
 
+test_expect_success PIPE 'use different backflow fd' '
+	reinit_git &&
+	echo hi >hi &&
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "1999-02-01T00:01:002.000000Z" \
+			svn:log "add directory with some files in it" &&
+		echo PROPS-END
+	} >props &&
+	{
+		echo Prop-content-length: $(wc -c <props) &&
+		echo Content-length: $(wc -c <props) &&
+		echo &&
+		cat props
+	} >revprops &&
+	{
+		cat <<-EOF &&
+		SVN-fs-dump-format-version: 3
+
+		Revision-number: 1
+		EOF
+		cat revprops &&
+		cat <<-EOF &&
+		Node-path: directory
+		Node-kind: dir
+		Node-action: add
+		Node-path: directory/somefile
+		Node-kind: file
+		Node-action: add
+		EOF
+		text_no_props hi &&
+
+		echo "Revision-number: 2" &&
+		cat revprops &&
+		cat <<-\EOF
+		Node-path: otherfile
+		Node-kind: file
+		Node-action: add
+		Node-copyfrom-rev: 1
+		Node-copyfrom-path: directory/somefile
+		EOF
+	} >directory.dump &&
+	try_dump "--read-blob-fd=7 directory.dump" "" "" 7 &&
+
+	git checkout HEAD otherfile &&
+	test_cmp hi otherfile
+'
+
 test_expect_success PIPE 'property deltas supported' '
 	reinit_git &&
 	cat >expect <<-\EOF &&
diff --git a/test-svn-fe.c b/test-svn-fe.c
index e114bc7c..a399e183 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -15,11 +15,14 @@ static const char * const test_svnfe_usage[] = {
 	NULL
 };
 
+static struct svndump_options args = SVNDUMP_OPTIONS_INIT;
 static int apply_delta_instead;
 
 static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', "apply-delta",
 		&apply_delta_instead, "apply a subversion-format delta", 1),
+	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
+		"read fast-import replies from file descriptor <n> (default: 3)"),
 	OPT_END()
 };
 
@@ -51,9 +54,6 @@ static int apply_delta(int argc, const char *argv[])
 
 int main(int argc, const char *argv[])
 {
-	struct svndump_options args;
-
-	memset(&args, 0, sizeof(args));
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index f61113b4..ecb56e4b 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -28,6 +28,8 @@ static int init_postimage(void)
 
 void fast_export_init(int fd)
 {
+	if (fd < 0)
+		fd = 3;
 	if (buffer_fdinit(&report_buffer, fd))
 		die_errno("cannot read from file descriptor %d", fd);
 }
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 0136747b..a996fbda 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -20,8 +20,6 @@
  */
 #define constcmp(s, ref) memcmp(s, ref, sizeof(ref) - 1)
 
-#define REPORT_FILENO 3
-
 #define NODEACT_REPLACE 4
 #define NODEACT_DELETE 3
 #define NODEACT_ADD 2
@@ -490,7 +488,7 @@ int svndump_init(const struct svndump_options *opts)
 
 	if (buffer_init(&input, filename))
 		return error("cannot open %s: %s", filename, strerror(errno));
-	fast_export_init(REPORT_FILENO);
+	fast_export_init(opts->backflow_fd);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
 	strbuf_init(&rev_ctx.log, 4096);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index b8f52172..92ad9ba8 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -4,8 +4,11 @@
 struct svndump_options {
 	const char *dumpfile;
 	const char *metadata_url;
+	int backflow_fd;
 };
 
+#define SVNDUMP_OPTIONS_INIT { NULL, NULL, -1 }
+
 int svndump_init(const struct svndump_options *o);
 void svndump_read(void);
 void svndump_deinit(void);
-- 
1.7.6

[PATCH v2 09/11] vcs-svn,svn-fe: allow to disable 'progress' lines

[Dmitry Ivankov]

vcs-svn/ writes a progress line after each processed revision. It
is too noisy for big imports. That's a stress for a terminal and
any other output can be lost or scrolled away among these lines.
If svn-fe is invoked by a remote helper the import stream with
progress lines in it will go directly to the git fast-import which
always prints every progress line met in the stream.

For now just add a switch to turn progress lines off:
$ svn-fe --no-progress ...

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    3 +++
 contrib/svn-fe/svn-fe.txt |    3 +++
 test-svn-fe.c             |    1 +
 vcs-svn/svndump.c         |    6 +++++-
 vcs-svn/svndump.h         |    1 +
 5 files changed, 13 insertions(+), 1 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index cd9e449..a388750 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -15,6 +15,9 @@ static const char * const svn_fe_usage[] = {
 static struct svndump_args args;
 
 static struct option svn_fe_options[] = {
+	{ OPTION_BIT, 0, "progress", &args.progress,
+		NULL, "don't write a progress line after each commit",
+		PARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },
 	OPT_STRING(0, "git-svn-id-url", &args.url, "url",
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index a7ad368..f1a459e 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -39,6 +39,9 @@ OPTIONS
 	Integer number of file descriptor from which
 	responses to 'ls' and 'cat-blob' requests will come.
 	Default is fd=3.
+--[no-]progress::
+	Write 'progress' lines to fast-import stream. These
+	can be displayed by fast-import.
 
 INPUT FORMAT
 ------------
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 43e19b2..f2711e6 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -57,6 +57,7 @@ int main(int argc, const char *argv[])
 {
 	args.ref = "refs/heads/master";
 	args.backflow_fd = 3;
+	args.progress = 1;
 	argc = parse_options(argc, argv, NULL, test_svnfe_options,
 						test_svnfe_usage, 0);
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 4194052..6ad9f63 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -37,6 +37,8 @@
 
 #define MAX_GITSVN_LINE_LEN 4096
 
+static int print_progress;
+
 static struct line_buffer input = LINE_BUFFER_INIT;
 
 static struct {
@@ -332,7 +334,8 @@ static void end_revision(void)
 {
 	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
-		printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
+		if (print_progress)
+			printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
 		dump_ctx.first_commit_done = 1;
 	}
 }
@@ -482,6 +485,7 @@ int svndump_init(const struct svndump_args *args)
 {
 	if (buffer_init(&input, args->filename))
 		return error("cannot open %s: %s", args->filename, strerror(errno));
+	print_progress = args->progress;
 	fast_export_init(args->backflow_fd);
 	strbuf_init(&dump_ctx.uuid, 4096);
 	strbuf_init(&dump_ctx.url, 4096);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index d266b68..af63edb 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -5,6 +5,7 @@ struct svndump_args {
 	const char *filename, *url;
 	const char *ref;
 	int backflow_fd;
+	int progress;
 };
 
 int svndump_init(const struct svndump_args *args);
-- 
1.7.3.4

[PATCH v2 10/11] vcs-svn,svn-fe: add --incremental option

[Dmitry Ivankov]

This option is to make svn-fe write commits on top of the existing ref
instead of overwriting it. More precise, the first commit's parent is
set to be :(first_revision_in_current_dump - 1) mark.

Prerequisite is to (re)use import marks (from previous imports). It is
safe to use this option on a svn dump that starts with r0/r1. The svn
dump itself should be incremental too.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    3 +++
 contrib/svn-fe/svn-fe.txt |    5 +++++
 t/t9010-svn-fe.sh         |   34 +++++++++++++++++++++++++++++-----
 test-svn-fe.c             |    3 +++
 vcs-svn/svndump.c         |   13 +++++++++----
 vcs-svn/svndump.h         |    2 +-
 6 files changed, 50 insertions(+), 10 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index a388750..211dc4d 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -18,6 +18,9 @@ static struct option svn_fe_options[] = {
 	{ OPTION_BIT, 0, "progress", &args.progress,
 		NULL, "don't write a progress line after each commit",
 		PARSE_OPT_NOARG | PARSE_OPT_NEGHELP, NULL, 1 },
+	OPT_BIT(0, "incremental", &args.incremental,
+		"resume export, requires marks and incremental dump",
+		1),
 	OPT_STRING(0, "git-svn-id-url", &args.url, "url",
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index f1a459e..0b6c29e 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -42,6 +42,11 @@ OPTIONS
 --[no-]progress::
 	Write 'progress' lines to fast-import stream. These
 	can be displayed by fast-import.
+--incremental::
+	If the first revision in dump has number greater than
+	1, make :(revision - 1) it's parent. For this to work
+	fast-import must be supplied with import-marks file
+	and the dump must be incremental.
 
 INPUT FORMAT
 ------------
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index 6dcad94..e5b78a9 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -19,7 +19,8 @@ reinit_git () {
 try_dump_ext () {
 	args=$1 &&
 	fd=${2:-3} &&
-	shift 2 &&
+	fi_args=${3:-} &&
+	shift 3 &&
 	input=$1 &&
 	maybe_fail_svnfe=${2:+test_$2} &&
 	maybe_fail_fi=${3:+test_$3} &&
@@ -27,12 +28,12 @@ try_dump_ext () {
 	{
 		eval "$maybe_fail_svnfe test-svn-fe $args "$input" >stream $fd<backflow" &
 	} &&
-	$maybe_fail_fi git fast-import --cat-blob-fd=3 <stream 3>backflow &&
+	eval "$maybe_fail_fi git fast-import $fi_args --cat-blob-fd=3 <stream 3>backflow" &&
 	wait $!
 }
 
 try_dump () {
-	try_dump_ext "" "" $@
+	try_dump_ext "" "" "" $@
 }
 
 properties () {
@@ -76,6 +77,15 @@ Content-length: 10
 
 PROPS-END
 EOF
+cat >moreempty.dump <<-EOF &&
+SVN-fs-dump-format-version: 3
+
+Revision-number: 3
+Prop-content-length: 10
+Content-length: 10
+
+PROPS-END
+EOF
 
 test_expect_success 'setup: have pipes?' '
 	rm -f frob &&
@@ -1121,7 +1131,7 @@ test_expect_success SVNREPO,PIPE 't9135/svn.dump' '
 
 test_expect_success PIPE 'import to notmaster ref' '
 	reinit_git &&
-	try_dump_ext "--ref=refs/heads/notmaster" 3 emptyprop.dump &&
+	try_dump_ext "--ref=refs/heads/notmaster" 3 "" emptyprop.dump &&
 
 	git rev-parse --verify notmaster &&
 	test_must_fail git rev-parse --verify master
@@ -1170,10 +1180,24 @@ test_expect_success PIPE 'use different backflow fd' '
 		Node-copyfrom-path: directory/somefile
 		EOF
 	} >directory.dump &&
-	try_dump_ext "--read-blob-fd=7" 7 directory.dump &&
+	try_dump_ext "--read-blob-fd=7" 7 "" directory.dump &&
 
 	git checkout HEAD otherfile &&
 	test_cmp hi otherfile
 '
 
+test_expect_success PIPE 'incremental import' '
+	reinit_git &&
+	>./marks &&
+
+	try_dump_ext "--incremental" "" "--export-marks=./marks" emptyprop.dump &&
+	test_line_count = 2 ./marks &&
+
+	try_dump_ext "--incremental" "" "--import-marks=./marks --export-marks=./marks" moreempty.dump &&
+	test_line_count = 3 ./marks &&
+
+	git log --format=oneline >history &&
+	test_line_count = 3 ./history
+'
+
 test_done
diff --git a/test-svn-fe.c b/test-svn-fe.c
index f2711e6..8d3cc99 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -22,6 +22,9 @@ static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_BIT(0, "incremental", &args.incremental,
+		"resume export, requires marks and incremental dump",
+		1),
 	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
 		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 6ad9f63..2b11f96 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -58,6 +58,7 @@ static struct {
 	struct strbuf uuid, url;
 	int first_commit_done;
 	struct strbuf ref_name;
+	int incremental;
 } dump_ctx;
 
 static void reset_node_ctx(char *fname)
@@ -83,7 +84,7 @@ static void reset_rev_ctx(uint32_t revision)
 	strbuf_reset(&rev_ctx.author);
 }
 
-static void reset_dump_ctx(const char *url, const char *dst_ref)
+static void reset_dump_ctx(const char *url, const char *dst_ref, int incremental)
 {
 	strbuf_reset(&dump_ctx.url);
 	if (url)
@@ -93,6 +94,7 @@ static void reset_dump_ctx(const char *url, const char *dst_ref)
 	dump_ctx.first_commit_done = 0;
 	strbuf_reset(&dump_ctx.ref_name);
 	strbuf_addstr(&dump_ctx.ref_name, dst_ref);
+	dump_ctx.incremental = incremental;
 }
 
 static void handle_property(const struct strbuf *key_buf,
@@ -321,7 +323,10 @@ static void begin_revision(void)
 	} else {
 		*gitsvnline = 0;
 	}
-	from_mark = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0;
+	if (dump_ctx.incremental)
+		from_mark = rev_ctx.revision - 1;
+	else
+		from_mark = dump_ctx.first_commit_done ? rev_ctx.revision - 1 : 0;
 	author = *rev_ctx.author.buf ? rev_ctx.author.buf : "nobody";
 	domain = *dump_ctx.uuid.buf ? dump_ctx.uuid.buf : "local";
 
@@ -493,7 +498,7 @@ int svndump_init(const struct svndump_args *args)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(args->url, args->ref);
+	reset_dump_ctx(args->url, args->ref, args->incremental);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
@@ -502,7 +507,7 @@ int svndump_init(const struct svndump_args *args)
 void svndump_deinit(void)
 {
 	fast_export_deinit();
-	reset_dump_ctx(NULL, "");
+	reset_dump_ctx(NULL, "", 0);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index af63edb..f2bb58c 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -5,7 +5,7 @@ struct svndump_args {
 	const char *filename, *url;
 	const char *ref;
 	int backflow_fd;
-	int progress;
+	int progress, incremental;
 };
 
 int svndump_init(const struct svndump_args *args);
-- 
1.7.3.4

Re: [PATCH v2 10/11] vcs-svn,svn-fe: add --incremental option

[Jonathan Nieder]

Dmitry Ivankov wrote:

> This option is to make svn-fe write commits on top of the existing ref
> instead of overwriting it. More precise, the first commit's parent is
> set to be :(first_revision_in_current_dump - 1) mark.
>
> Prerequisite is to (re)use import marks (from previous imports). It is
> safe to use this option on a svn dump that starts with r0/r1. The svn
> dump itself should be incremental too.

In other words, this allows running svn-fe to resume a partial import or
to resume an import after the remote repository has added more history.
Hoorah!

[...]
> +	try_dump_ext "--incremental" "" "--export-marks=./marks" emptyprop.dump &&
> +	test_line_count = 2 ./marks &&
> +
> +	try_dump_ext "--incremental" "" "--import-marks=./marks --export-marks=./marks" moreempty.dump &&
> +	test_line_count = 3 ./marks &&

It should be possible to avoid this extra argument by making "reinit_git"
remove the marks file and using something like

	git fast-import --import-marks-if-exists=marks --export-marks=marks

> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
> @@ -22,6 +22,9 @@ static struct option test_svnfe_options[] = {
>  	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
>  	OPT_STRING(0, "ref", &args.ref, "dst_ref",
>  		"write to dst_ref instead of refs/heads/master"),
> +	OPT_BIT(0, "incremental", &args.incremental,
> +		"resume export, requires marks and incremental dump",
> +		1),

Why OPT_BIT?

Hope that helps,
Jonathan

[PATCH v2 11/11] vcs-svn,svn-fe: add an option to write svnrev notes

[Dmitry Ivankov]

There are already a few options to determine svn revision from which
a git commit imported with svn-fe came from. One is to make svn-fe
write a git-svn-id: line to commit messages. Another one is to calc
distance to the root commit. The former includes a "url" and is for
git-svn compatibility, the latter is obviously slow and a bit fragile.

$ svn-fe --notes_ref=notes_tree --ref=branch...
will write annotations for branch commits to the notes_tree, each
annotation is a simple "rN" string. Then these annotations can be
viewed manually or used in incremental import to detect the last
imported revision or to (re)create the import marks for further
imports.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 contrib/svn-fe/svn-fe.c   |    2 ++
 contrib/svn-fe/svn-fe.txt |    3 +++
 t/t9010-svn-fe.sh         |   32 ++++++++++++++++++++++++++++++++
 test-svn-fe.c             |    2 ++
 vcs-svn/svndump.c         |   28 ++++++++++++++++++++++++----
 vcs-svn/svndump.h         |    2 +-
 6 files changed, 64 insertions(+), 5 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 211dc4d..8410221 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -25,6 +25,8 @@ static struct option svn_fe_options[] = {
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &args.ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_STRING(0, "notes-ref", &args.notes_ref, "notes",
+		"write \"rN\" notes to the <notes> tree"),
 	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
 		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
diff --git a/contrib/svn-fe/svn-fe.txt b/contrib/svn-fe/svn-fe.txt
index 0b6c29e..ce0582d 100644
--- a/contrib/svn-fe/svn-fe.txt
+++ b/contrib/svn-fe/svn-fe.txt
@@ -35,6 +35,9 @@ OPTIONS
 --ref=<dst_ref>::
 	Ref to be written by the generated stream.
 	Default is refs/heads/master.
+--notes-ref=<notes_ref>::
+	Write "rN" notes to the notes_ref tree for each
+	imported commit.
 --read-blob-fd=<fd>::
 	Integer number of file descriptor from which
 	responses to 'ls' and 'cat-blob' requests will come.
diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index e5b78a9..2841a3e 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -1200,4 +1200,36 @@ test_expect_success PIPE 'incremental import' '
 	test_line_count = 3 ./history
 '
 
+test_expect_success PIPE 'write notes' '
+	reinit_git &&
+	cat >expect <<-EOF &&
+	r2
+
+	r1
+
+	EOF
+	try_dump_ext "--notes=refs/notes/test" "" "" emptyprop.dump &&
+	git log --show-notes=refs/notes/test --format=%N >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success PIPE 'write notes incremental' '
+	reinit_git &&
+	>./marks &&
+	cat >expect <<-EOF &&
+	r3
+
+	r2
+
+	r1
+
+	EOF
+
+	try_dump_ext "--notes=refs/notes/test" "" "--export-marks=marks" emptyprop.dump &&
+	try_dump_ext "--incremental --notes=refs/notes/test" "" "--import-marks=marks" moreempty.dump &&
+
+	git log --show-notes=refs/notes/test --format=%N >actual &&
+	test_cmp expect actual
+'
+
 test_done
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 8d3cc99..e6d9ae6 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -27,6 +27,8 @@ static struct option test_svnfe_options[] = {
 		1),
 	OPT_INTEGER(0, "read-blob-fd", &args.backflow_fd,
 		"read blobs and trees from this fd instead of 3"),
+	OPT_STRING(0, "notes-ref", &args.notes_ref, "notes",
+		"write \"rN\" notes to the <notes> tree"),
 	OPT_END()
 };
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index 2b11f96..514703f 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -57,7 +57,7 @@ static struct {
 	uint32_t version;
 	struct strbuf uuid, url;
 	int first_commit_done;
-	struct strbuf ref_name;
+	struct strbuf ref_name, notes_ref_name;
 	int incremental;
 } dump_ctx;
 
@@ -84,7 +84,7 @@ static void reset_rev_ctx(uint32_t revision)
 	strbuf_reset(&rev_ctx.author);
 }
 
-static void reset_dump_ctx(const char *url, const char *dst_ref, int incremental)
+static void reset_dump_ctx(const char *url, const char *dst_ref, int incremental, const char *dst_notes_ref)
 {
 	strbuf_reset(&dump_ctx.url);
 	if (url)
@@ -95,6 +95,9 @@ static void reset_dump_ctx(const char *url, const char *dst_ref, int incremental
 	strbuf_reset(&dump_ctx.ref_name);
 	strbuf_addstr(&dump_ctx.ref_name, dst_ref);
 	dump_ctx.incremental = incremental;
+	strbuf_reset(&dump_ctx.notes_ref_name);
+	if (dst_notes_ref)
+		strbuf_addstr(&dump_ctx.notes_ref_name, dst_notes_ref);
 }
 
 static void handle_property(const struct strbuf *key_buf,
@@ -337,8 +340,25 @@ static void begin_revision(void)
 
 static void end_revision(void)
 {
+	char buf[32];
+	char tmbuf[32];
 	if (rev_ctx.revision) {
 		fast_export_end_commit(rev_ctx.revision);
+		if (dump_ctx.notes_ref_name.len) {
+			datestamp(tmbuf, 32);
+			printf("commit %s\n", dump_ctx.notes_ref_name.buf);
+			printf("committer %s <%s@%s> %s\n",
+					"vcs-svn", "vcs-svn", "local", tmbuf);
+			printf("data <<EOF\n");
+			printf("imported r%d\n", rev_ctx.revision);
+			printf("EOF\n\n");
+			if (!dump_ctx.first_commit_done && dump_ctx.incremental && rev_ctx.revision > 1)
+				printf("from %s^0\n", dump_ctx.notes_ref_name.buf);
+			snprintf(buf, 32, "r%d", rev_ctx.revision);
+			printf("N inline :%d\n", rev_ctx.revision);
+			printf("data %ld\n", strlen(buf));
+			printf("%s\n", buf);
+		}
 		if (print_progress)
 			printf("progress Imported commit %"PRIu32".\n\n", rev_ctx.revision);
 		dump_ctx.first_commit_done = 1;
@@ -498,7 +518,7 @@ int svndump_init(const struct svndump_args *args)
 	strbuf_init(&rev_ctx.author, 4096);
 	strbuf_init(&node_ctx.src, 4096);
 	strbuf_init(&node_ctx.dst, 4096);
-	reset_dump_ctx(args->url, args->ref, args->incremental);
+	reset_dump_ctx(args->url, args->ref, args->incremental, args->notes_ref);
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	return 0;
@@ -507,7 +527,7 @@ int svndump_init(const struct svndump_args *args)
 void svndump_deinit(void)
 {
 	fast_export_deinit();
-	reset_dump_ctx(NULL, "", 0);
+	reset_dump_ctx(NULL, "", 0, "");
 	reset_rev_ctx(0);
 	reset_node_ctx(NULL);
 	strbuf_release(&rev_ctx.log);
diff --git a/vcs-svn/svndump.h b/vcs-svn/svndump.h
index f2bb58c..928bb0b 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -3,7 +3,7 @@
 
 struct svndump_args {
 	const char *filename, *url;
-	const char *ref;
+	const char *ref, *notes_ref;
 	int backflow_fd;
 	int progress, incremental;
 };
-- 
1.7.3.4

Re: [PATCH v2 11/11] vcs-svn,svn-fe: add an option to write svnrev notes

[Jonathan Nieder]

Hi,

Dmitry Ivankov wrote:

> There are already a few options to determine svn revision from which
> a git commit imported with svn-fe came from. One is to make svn-fe
> write a git-svn-id: line to commit messages. Another one is to calc
> distance to the root commit. The former includes a "url" and is for
> git-svn compatibility, the latter is obviously slow and a bit fragile.
>
> $ svn-fe --notes_ref=notes_tree --ref=branch...
> will write annotations for branch commits to the notes_tree, each
> annotation is a simple "rN" string. Then these annotations can be
> viewed manually or used in incremental import to detect the last
> imported revision or to (re)create the import marks for further
> imports.

Wouldn't another way be to look at the mark numbers?

I am not sure I like this.  svn-fe is supposed to be a generally
useful tool, and this patch hard-codes the particular note format rN.
If it is needed, maybe it would be possible to do something like

	--notes-ref=refs/notes/svn-rev --note='project foo, r%N'

As a bonus, that would allow including more information using
different flag characters in the note in the future.

Re: [PATCH v2 11/11] vcs-svn,svn-fe: add an option to write svnrev notes

[Dmitry Ivankov]

Sorry for a way too slow response. This patch is probably the most
unbaked one, so I'll start here.

On Tue, Jul 26, 2011 at 3:39 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> Hi,
>
> Dmitry Ivankov wrote:
>
>> There are already a few options to determine svn revision from which
>> a git commit imported with svn-fe came from. One is to make svn-fe
>> write a git-svn-id: line to commit messages. Another one is to calc
>> distance to the root commit. The former includes a "url" and is for
>> git-svn compatibility, the latter is obviously slow and a bit fragile.
>>
>> $ svn-fe --notes_ref=notes_tree --ref=branch...
>> will write annotations for branch commits to the notes_tree, each
>> annotation is a simple "rN" string. Then these annotations can be
>> viewed manually or used in incremental import to detect the last
>> imported revision or to (re)create the import marks for further
>> imports.
>
> Wouldn't another way be to look at the mark numbers?
If marks file is absent (after clone for example), we'll need to look
at notes anyway.
If it is present and has a mark for latest revision we can hope it is
valid and not
regenerate it.
If for some reason notes are lost and marks are present, it's possible
to recreate
notes from marks though, at least "rN" ones.

> I am not sure I like this.  svn-fe is supposed to be a generally
> useful tool, and this patch hard-codes the particular note format rN.
> If it is needed, maybe it would be possible to do something like
>
>        --notes-ref=refs/notes/svn-rev --note='project foo, r%N'
>
> As a bonus, that would allow including more information using
> different flag characters in the note in the future.
Format string looks nice. While the whole notes thing may need
more thinking.
The main reason I wrote these in svn-fe is that it's more atomic
to write note just after writing a commit. Also "checkpoint" will
create notes for current status (and will write marks too, but weren't
we going to consider notes as a primary data copy? it can be cloned
for example).

One more consideration is that copy-from information most likely
will be written by svn-fe (nothing else knows it anyway) to some
notes, so we'll need some notes writing in svn-fe.

Re: [PATCH v2 11/11] vcs-svn,svn-fe: add an option to write svnrev notes

[Jonathan Nieder]

Dmitry Ivankov wrote:
> On Tue, Jul 26, 2011 at 3:39 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>>        --notes-ref=refs/notes/svn-rev --note='project foo, r%N'
>>
>> As a bonus, that would allow including more information using
>> different flag characters in the note in the future.
>
> Format string looks nice.

Yes, it sounds pleasant to work with to me.  Open questions:

 - how fine-grained should the notes commits be?  E.g., should they
   be written one at a time, in batches of 10, or something else?

 - is there a way to make some commits get no note at all?  Is this
   template-based approach the right way to go?  (I guess yes, it is.)

 - what if someone wants multiple notes refs (e.g., revision numbers
   and revprops in separate notes refs)?  Will this support that?  If
   not, is it extensible enough to sensibly support that in the
   future?

[...]
> One more consideration is that copy-from information most likely
> will be written by svn-fe (nothing else knows it anyway) to some
> notes, so we'll need some notes writing in svn-fe.

Yep, copyfrom info has to get downstream somehow.  Which means

   a. in log messages
   b. in notes
   c. in comments or "progress" lines in the stream, or
or d. in a second output stream, with file descriptor number specified
      by the caller.

I was leaning towards (a) or (c), but (b) certainly also seems
sensible.