[PATCH 0/8] vcs-svn, svn-fe: add a couple of options

[PATCH 0/8] vcs-svn, svn-fe: add a couple of options

[Dmitry Ivankov]

This patch set adds a few options for svn-fe that can be used in
a svn remote helper and useful in general for vcs-svn/. The last
three patches add the options, others prepare the ground for them.

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

Dmitry Ivankov (8):
  vcs-svn: move url parameter from _read to _init
  svn-fe: add man target to Makefile
  svn-fe: add EXTLIBS needed for parse-options
  svn-fe: add usage and unpositional arguments versions
  test-svn-fe: use parse-options
  vcs-svn: allow to specify dump destination ref
  vcs-svn: convert REPORT_FILENO to an option
  vcs-svn: allow to disable 'progress' lines

 contrib/svn-fe/Makefile   |   18 ++++----
 contrib/svn-fe/svn-fe.c   |   42 +++++++++++++++++--
 contrib/svn-fe/svn-fe.txt |   29 +++++++++++--
 t/t9010-svn-fe.sh         |   99 +++++++++++++++++++++++++++++++++++++--------
 test-svn-fe.c             |   50 ++++++++++++++++-------
 vcs-svn/fast_export.c     |   13 +++++-
 vcs-svn/fast_export.h     |    2 +-
 vcs-svn/svndump.c         |   11 ++---
 vcs-svn/svndump.h         |    4 +-
 9 files changed, 205 insertions(+), 63 deletions(-)

-- 
1.7.3.4

[PATCH 1/8] 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. More parameters
will arise and all will go to svndump_init to setup the module for
dumping.

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

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 35db24f..11d7128 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -8,9 +8,9 @@
 
 int main(int argc, char **argv)
 {
-	if (svndump_init(NULL))
+	if (svndump_init(NULL, (argc > 1) ? argv[1] : NULL))
 		return 1;
-	svndump_read((argc > 1) ? argv[1] : NULL);
+	svndump_read();
 	svndump_deinit();
 	svndump_reset();
 	return 0;
diff --git a/test-svn-fe.c b/test-svn-fe.c
index 332a5f7..3ee5559 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -40,9 +40,9 @@ static int apply_delta(int argc, char *argv[])
 int main(int argc, char *argv[])
 {
 	if (argc == 2) {
-		if (svndump_init(argv[1]))
+		if (svndump_init(argv[1], NULL))
 			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..a88d392 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -313,14 +313,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,7 +454,7 @@ void svndump_read(const char *url)
 		end_revision();
 }
 
-int svndump_init(const char *filename)
+int svndump_init(const char *filename, const char *url)
 {
 	if (buffer_init(&input, filename))
 		return error("cannot open %s: %s", filename, strerror(errno));
@@ -466,7 +465,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(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..1bcaab6 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,8 +1,8 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-int svndump_init(const char *filename);
-void svndump_read(const char *url);
+int svndump_init(const char *filename, const char *url);
+void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
 
-- 
1.7.3.4

[PATCH 2/8] 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

[PATCH 3/8] 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

[PATCH 4/8] 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 11d7128..033dd5b 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -3,12 +3,36 @@
  * 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)
 {
-	if (svndump_init(NULL, (argc > 1) ? argv[1] : NULL))
+	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, url))
 		return 1;
 	svndump_read();
 	svndump_deinit();
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

[PATCH 5/8] 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 3ee5559..603d3fb 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], NULL))
+	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], NULL))
 			return 1;
 		svndump_read();
 		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

[PATCH 6/8] vcs-svn: 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   |    5 +++-
 contrib/svn-fe/svn-fe.txt |    3 ++
 t/t9010-svn-fe.sh         |   49 +++++++++++++++++++++++++++++---------------
 test-svn-fe.c             |    7 ++++-
 vcs-svn/fast_export.c     |    8 +++++-
 vcs-svn/fast_export.h     |    2 +-
 vcs-svn/svndump.c         |    4 +-
 vcs-svn/svndump.h         |    2 +-
 8 files changed, 54 insertions(+), 26 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 033dd5b..78c7cd0 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -13,10 +13,13 @@ static const char * const svn_fe_usage[] = {
 };
 
 static const char *url;
+static const char *ref = "refs/heads/master";
 
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &url, "url",
 		"append git-svn metadata line to commit messages"),
+	OPT_STRING(0, "ref", &ref, "dst_ref",
+		"write to dst_ref instead of refs/heads/master"),
 	OPT_END()
 };
 
@@ -32,7 +35,7 @@ int main(int argc, const char **argv)
 		url = argv[0];
 	} else if (argc)
 		usage_with_options(svn_fe_usage, svn_fe_options);
-	if (svndump_init(NULL, url))
+	if (svndump_init(NULL, url, ref))
 		return 1;
 	svndump_read();
 	svndump_deinit();
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 603d3fb..afa551c 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -10,14 +10,17 @@
 #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
 };
 
+static const char *ref = "refs/heads/master";
 static int d;
 
 static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
+	OPT_STRING(0, "ref", &ref, "dst_ref",
+		"write to dst_ref instead of refs/heads/master"),
 	OPT_END()
 };
 
@@ -56,7 +59,7 @@ int main(int argc, const char *argv[])
 		return apply_delta(argc, argv);
 
 	if (argc == 1) {
-		if (svndump_init(argv[0], NULL))
+		if (svndump_init(argv[0], NULL, ref))
 			return 1;
 		svndump_read();
 		svndump_deinit();
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34..cfb0f2b 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -18,6 +18,7 @@
 static uint32_t first_commit_done;
 static struct line_buffer postimage = LINE_BUFFER_INIT;
 static struct line_buffer report_buffer = LINE_BUFFER_INIT;
+static struct strbuf ref_name = STRBUF_INIT;
 
 /* NEEDSWORK: move to fast_export_init() */
 static int init_postimage(void)
@@ -29,15 +30,18 @@ static int init_postimage(void)
 	return buffer_tmpfile_init(&postimage);
 }
 
-void fast_export_init(int fd)
+void fast_export_init(int fd, const char *dst_ref)
 {
 	first_commit_done = 0;
+	strbuf_reset(&ref_name);
+	strbuf_addstr(&ref_name, dst_ref);
 	if (buffer_fdinit(&report_buffer, fd))
 		die_errno("cannot read from file descriptor %d", fd);
 }
 
 void fast_export_deinit(void)
 {
+	strbuf_release(&ref_name);
 	if (buffer_deinit(&report_buffer))
 		die_errno("error closing fast-import feedback stream");
 }
@@ -89,7 +93,7 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 	} else {
 		*gitsvnline = '\0';
 	}
-	printf("commit refs/heads/master\n");
+	printf("commit %s\n", ref_name.buf);
 	printf("mark :%"PRIu32"\n", revision);
 	printf("committer %s <%s@%s> %ld +0000\n",
 		   *author ? author : "nobody",
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b6..d7eb7cc 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -4,7 +4,7 @@
 struct strbuf;
 struct line_buffer;
 
-void fast_export_init(int fd);
+void fast_export_init(int fd, const char *dst_ref);
 void fast_export_deinit(void);
 void fast_export_reset(void);
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index a88d392..f0df381 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -454,11 +454,11 @@ void svndump_read(void)
 		end_revision();
 }
 
-int svndump_init(const char *filename, const char *url)
+int svndump_init(const char *filename, const char *url, const char *dst_ref)
 {
 	if (buffer_init(&input, filename))
 		return error("cannot open %s: %s", filename, strerror(errno));
-	fast_export_init(REPORT_FILENO);
+	fast_export_init(REPORT_FILENO, dst_ref);
 	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 1bcaab6..08922fb 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,7 +1,7 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-int svndump_init(const char *filename, const char *url);
+int svndump_init(const char *filename, const char *url, const char *dst_ref);
 void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
-- 
1.7.3.4

[PATCH 7/8] vcs-svn: 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   |    5 +++-
 contrib/svn-fe/svn-fe.txt |    8 ++++-
 t/t9010-svn-fe.sh         |   58 +++++++++++++++++++++++++++++++++++++++++---
 test-svn-fe.c             |    5 +++-
 vcs-svn/svndump.c         |    6 +---
 vcs-svn/svndump.h         |    2 +-
 6 files changed, 71 insertions(+), 13 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 78c7cd0..32755b1 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -14,12 +14,15 @@ static const char * const svn_fe_usage[] = {
 
 static const char *url;
 static const char *ref = "refs/heads/master";
+static int backflow_fd = 3;
 
 static struct option svn_fe_options[] = {
 	OPT_STRING(0, "git-svn-id-url", &url, "url",
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_INTEGER(0, "read-blob-fd", &backflow_fd,
+		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
 };
 
@@ -35,7 +38,7 @@ int main(int argc, const char **argv)
 		url = argv[0];
 	} else if (argc)
 		usage_with_options(svn_fe_usage, svn_fe_options);
-	if (svndump_init(NULL, url, ref))
+	if (svndump_init(NULL, url, ref, backflow_fd))
 		return 1;
 	svndump_read();
 	svndump_deinit();
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 afa551c..7885eb1 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -16,11 +16,14 @@ static const char * const test_svnfe_usage[] = {
 
 static const char *ref = "refs/heads/master";
 static int d;
+static int backflow_fd = 3;
 
 static struct option test_svnfe_options[] = {
 	OPT_SET_INT('d', NULL, &d, "test apply_delta", 1),
 	OPT_STRING(0, "ref", &ref, "dst_ref",
 		"write to dst_ref instead of refs/heads/master"),
+	OPT_INTEGER(0, "read-blob-fd", &backflow_fd,
+		"read blobs and trees from this fd instead of 3"),
 	OPT_END()
 };
 
@@ -59,7 +62,7 @@ int main(int argc, const char *argv[])
 		return apply_delta(argc, argv);
 
 	if (argc == 1) {
-		if (svndump_init(argv[0], NULL, ref))
+		if (svndump_init(argv[0], NULL, ref, backflow_fd))
 			return 1;
 		svndump_read();
 		svndump_deinit();
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index f0df381..c52faf1 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -19,8 +19,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
@@ -454,11 +452,11 @@ void svndump_read(void)
 		end_revision();
 }
 
-int svndump_init(const char *filename, const char *url, const char *dst_ref)
+int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno)
 {
 	if (buffer_init(&input, filename))
 		return error("cannot open %s: %s", filename, strerror(errno));
-	fast_export_init(REPORT_FILENO, dst_ref);
+	fast_export_init(report_fileno, dst_ref);
 	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 08922fb..85c82c5 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,7 +1,7 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-int svndump_init(const char *filename, const char *url, const char *dst_ref);
+int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno);
 void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
-- 
1.7.3.4

[PATCH 8/8] vcs-svn: 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.

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   |    6 +++++-
 contrib/svn-fe/svn-fe.txt |    3 +++
 test-svn-fe.c             |    2 +-
 vcs-svn/fast_export.c     |    7 +++++--
 vcs-svn/fast_export.h     |    2 +-
 vcs-svn/svndump.c         |    4 ++--
 vcs-svn/svndump.h         |    2 +-
 7 files changed, 18 insertions(+), 8 deletions(-)

diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
index 32755b1..792ffad 100644
--- a/contrib/svn-fe/svn-fe.c
+++ b/contrib/svn-fe/svn-fe.c
@@ -15,8 +15,12 @@ static const char * const svn_fe_usage[] = {
 static const char *url;
 static const char *ref = "refs/heads/master";
 static int backflow_fd = 3;
+static int progress = 1;
 
 static struct option svn_fe_options[] = {
+	OPT_BIT(0, "progress", &progress,
+		"write a progress line after each commit",
+		1),
 	OPT_STRING(0, "git-svn-id-url", &url, "url",
 		"append git-svn metadata line to commit messages"),
 	OPT_STRING(0, "ref", &ref, "dst_ref",
@@ -38,7 +42,7 @@ int main(int argc, const char **argv)
 		url = argv[0];
 	} else if (argc)
 		usage_with_options(svn_fe_usage, svn_fe_options);
-	if (svndump_init(NULL, url, ref, backflow_fd))
+	if (svndump_init(NULL, url, ref, backflow_fd, progress))
 		return 1;
 	svndump_read();
 	svndump_deinit();
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 7885eb1..e4bfda0 100644
--- a/test-svn-fe.c
+++ b/test-svn-fe.c
@@ -62,7 +62,7 @@ int main(int argc, const char *argv[])
 		return apply_delta(argc, argv);
 
 	if (argc == 1) {
-		if (svndump_init(argv[0], NULL, ref, backflow_fd))
+		if (svndump_init(argv[0], NULL, ref, backflow_fd, 1))
 			return 1;
 		svndump_read();
 		svndump_deinit();
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index cfb0f2b..a8b8603 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -19,6 +19,7 @@ static uint32_t first_commit_done;
 static struct line_buffer postimage = LINE_BUFFER_INIT;
 static struct line_buffer report_buffer = LINE_BUFFER_INIT;
 static struct strbuf ref_name = STRBUF_INIT;
+static int print_progress;
 
 /* NEEDSWORK: move to fast_export_init() */
 static int init_postimage(void)
@@ -30,9 +31,10 @@ static int init_postimage(void)
 	return buffer_tmpfile_init(&postimage);
 }
 
-void fast_export_init(int fd, const char *dst_ref)
+void fast_export_init(int fd, const char *dst_ref, int progress)
 {
 	first_commit_done = 0;
+	print_progress = progress;
 	strbuf_reset(&ref_name);
 	strbuf_addstr(&ref_name, dst_ref);
 	if (buffer_fdinit(&report_buffer, fd))
@@ -112,7 +114,8 @@ void fast_export_begin_commit(uint32_t revision, const char *author,
 
 void fast_export_end_commit(uint32_t revision)
 {
-	printf("progress Imported commit %"PRIu32".\n\n", revision);
+	if (print_progress)
+		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 d7eb7cc..7cab7b3 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -4,7 +4,7 @@
 struct strbuf;
 struct line_buffer;
 
-void fast_export_init(int fd, const char *dst_ref);
+void fast_export_init(int fd, const char *dst_ref, int progress);
 void fast_export_deinit(void);
 void fast_export_reset(void);
 
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index c52faf1..5e3a44d 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -452,11 +452,11 @@ void svndump_read(void)
 		end_revision();
 }
 
-int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno)
+int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno, int progress)
 {
 	if (buffer_init(&input, filename))
 		return error("cannot open %s: %s", filename, strerror(errno));
-	fast_export_init(report_fileno, dst_ref);
+	fast_export_init(report_fileno, dst_ref, progress);
 	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 85c82c5..becea11 100644
--- a/vcs-svn/svndump.h
+++ b/vcs-svn/svndump.h
@@ -1,7 +1,7 @@
 #ifndef SVNDUMP_H_
 #define SVNDUMP_H_
 
-int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno);
+int svndump_init(const char *filename, const char *url, const char *dst_ref, int report_fileno, int progress);
 void svndump_read(void);
 void svndump_deinit(void);
 void svndump_reset(void);
-- 
1.7.3.4

Re: [PATCH 0/8] vcs-svn, svn-fe: add a couple of options

[Sverre Rabbelier]

Heya,

[+Ram]

On Sun, Jul 3, 2011 at 19:57, Dmitry Ivankov <divanorama@gmail.com> wrote:
> This patch set adds a few options for svn-fe that can be used in
> a svn remote helper and useful in general for vcs-svn/. The last
> three patches add the options, others prepare the ground for them.
>
> The patch base is svn-fe branch at git://repo.or.cz/git/jrn.git
>
> Dmitry Ivankov (8):
>  vcs-svn: move url parameter from _read to _init
>  svn-fe: add man target to Makefile
>  svn-fe: add EXTLIBS needed for parse-options
>  svn-fe: add usage and unpositional arguments versions
>  test-svn-fe: use parse-options
>  vcs-svn: allow to specify dump destination ref
>  vcs-svn: convert REPORT_FILENO to an option
>  vcs-svn: allow to disable 'progress' lines
>
>  contrib/svn-fe/Makefile   |   18 ++++----
>  contrib/svn-fe/svn-fe.c   |   42 +++++++++++++++++--
>  contrib/svn-fe/svn-fe.txt |   29 +++++++++++--
>  t/t9010-svn-fe.sh         |   99 +++++++++++++++++++++++++++++++++++++--------
>  test-svn-fe.c             |   50 ++++++++++++++++-------
>  vcs-svn/fast_export.c     |   13 +++++-
>  vcs-svn/fast_export.h     |    2 +-
>  vcs-svn/svndump.c         |   11 ++---
>  vcs-svn/svndump.h         |    4 +-
>  9 files changed, 205 insertions(+), 63 deletions(-)
>
> --
> 1.7.3.4

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH 8/8] vcs-svn: allow to disable 'progress' lines

[Ramkumar Ramachandra]

Hi Dmitry,

Dmitry Ivankov writes:
> 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.
>
> For now just add a switch to turn progress lines off:
> $ svn-fe --no-progress ...

Agreed.  Sounds like a good change.

> diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
> index 32755b1..792ffad 100644
> --- a/contrib/svn-fe/svn-fe.c
> +++ b/contrib/svn-fe/svn-fe.c
> @@ -38,7 +42,7 @@ int main(int argc, const char **argv)
>                url = argv[0];
>        } else if (argc)
>                usage_with_options(svn_fe_usage, svn_fe_options);
> -       if (svndump_init(NULL, url, ref, backflow_fd))
> +       if (svndump_init(NULL, url, ref, backflow_fd, progress))

You're modifying the svndump_init API for every new option that's
added.  This'll clearly break down when you have many options -- how
about wrapping it up in an options structure and then passing that?

> 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.

Hm, this will make it a little too silent for the end-user.  What do
you feel about printing something minimalistic like a '.' for each
imported revision?  Atleast it won't look like it's hung.  Also, how
does this interact with the 'progress' option of fast-import protocol?

>  INPUT FORMAT
>  ------------
> diff --git a/test-svn-fe.c b/test-svn-fe.c
> index 7885eb1..e4bfda0 100644
> --- a/test-svn-fe.c
> +++ b/test-svn-fe.c
> @@ -62,7 +62,7 @@ int main(int argc, const char *argv[])
>                return apply_delta(argc, argv);
>
>        if (argc == 1) {
> -               if (svndump_init(argv[0], NULL, ref, backflow_fd))
> +               if (svndump_init(argv[0], NULL, ref, backflow_fd, 1))
>                        return 1;
>                svndump_read();
>                svndump_deinit();
> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
> index cfb0f2b..a8b8603 100644
> --- a/vcs-svn/fast_export.c
> +++ b/vcs-svn/fast_export.c
> @@ -19,6 +19,7 @@ static uint32_t first_commit_done;
>  static struct line_buffer postimage = LINE_BUFFER_INIT;
>  static struct line_buffer report_buffer = LINE_BUFFER_INIT;
>  static struct strbuf ref_name = STRBUF_INIT;
> +static int print_progress;

> @@ -30,9 +31,10 @@ static int init_postimage(void)
>        return buffer_tmpfile_init(&postimage);
>  }
>
> -void fast_export_init(int fd, const char *dst_ref)
> +void fast_export_init(int fd, const char *dst_ref, int progress)
>  {
>        first_commit_done = 0;
> +       print_progress = progress;

The only reason you're modifying the API of fast_export_init is so
that it can set a global static variable?  Also, this change seems
more absurd because progress reporting isn't directly related to
fast_export_init.  How about having a dedicated function for option
parsing that sets all the global statics?

I'm sorry I haven't been more involved with this project.  Still, I
hope this review helps.

-- Ram

Re: [PATCH 7/8] vcs-svn: convert REPORT_FILENO to an option

[Ramkumar Ramachandra]

Hi again,

Dmitry Ivankov writes:
> 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

Except for the fact that you're changing the API to accommodate an
extra option as I already pointed out earlier, I like this patch.
This was one of the things that I'd wanted to do myself, but never
actually got around to doing.

Thanks for working on this.

-- Ram

Re: [PATCH 8/8] vcs-svn: allow to disable 'progress' lines

[Dmitry Ivankov]

Hi!

[...]
>> diff --git a/contrib/svn-fe/svn-fe.c b/contrib/svn-fe/svn-fe.c
[...]
>> -       if (svndump_init(NULL, url, ref, backflow_fd))
>> +       if (svndump_init(NULL, url, ref, backflow_fd, progress))
>
> You're modifying the svndump_init API for every new option that's
> added.  This'll clearly break down when you have many options -- how
> about wrapping it up in an options structure and then passing that?
Well, there has to be a function to init that structure then. And the
structure will become a part of API.
So don't know if it's worthy.

>> 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.
>
> Hm, this will make it a little too silent for the end-user.  What do
> you feel about printing something minimalistic like a '.' for each
> imported revision? Atleast it won't look like it's hung.
For a medium 8k commit repo it is 100 lines - still too much.
A single line for the first revision seem harmless and will indicate
that the remote connection succeeded
(helps to see that it's not a connection timeout, probably caused by
dns lookup or a firewall).

>  Also, how does this interact with the 'progress' option of fast-import protocol?
git fast-import --quiet prints any progress line produced by a helper.
transport-helper.c tries to set the option
but doesn't fail if it is not accepted or if helper doesn't support
options at all.
For now the helper doesn't use this protocol option.

A better solution could be to use progress.o api to display progress.
Or an ad-hoc hack with adaptive progress step, say report a progress
on each "power of two"-th revision.

As a starting point for tests let there bust a simple switch-off.

[...]
>> diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
>> index cfb0f2b..a8b8603 100644
>> --- a/vcs-svn/fast_export.c
>> +++ b/vcs-svn/fast_export.c
>> @@ -19,6 +19,7 @@ static uint32_t first_commit_done;
>>  static struct line_buffer postimage = LINE_BUFFER_INIT;
>>  static struct line_buffer report_buffer = LINE_BUFFER_INIT;
>>  static struct strbuf ref_name = STRBUF_INIT;
>> +static int print_progress;
[...]
>> -void fast_export_init(int fd, const char *dst_ref)
>> +void fast_export_init(int fd, const char *dst_ref, int progress)
>>  {
>>        first_commit_done = 0;
>> +       print_progress = progress;
>
> The only reason you're modifying the API of fast_export_init is so
> that it can set a global static variable?
Looked once more at how these new variables are used. We can move
progress lines generation to svndump.c. And also move ref_name from
_init and being global static to a parameter in fast_export_begin_commit(),
and to be more sane s/revision/target_mark/ s/revision - 1/from_mark/ and
add a from_mark parameter, move first_commit_done logic to svndump.o.
This way fast_export.o can operate on single commits and maybe it'll be
easier to use it to apply svn branches layout in svn-fe in one run, though I'm
not sure I'll use svn-fe to manage svn branches.

>  Also, this change seems
> more absurd because progress reporting isn't directly related to
> fast_export_init.  How about having a dedicated function for option
> parsing that sets all the global statics?
>
> I'm sorry I haven't been more involved with this project.  Still, I
> hope this review helps.
This is very helpful, thanks.

>
> -- Ram
>

Re: [PATCH 8/8] vcs-svn: allow to disable 'progress' lines

[Ramkumar Ramachandra]

Hi again,

Dmitry Ivankov writes:
>>> -       if (svndump_init(NULL, url, ref, backflow_fd))
>>> +       if (svndump_init(NULL, url, ref, backflow_fd, progress))
>>
>> You're modifying the svndump_init API for every new option that's
>> added.  This'll clearly break down when you have many options -- how
>> about wrapping it up in an options structure and then passing that?
> Well, there has to be a function to init that structure then. And the
> structure will become a part of API.
> So don't know if it's worthy.

Well, you'll be wrapping up url, ref, backflow_fd, progress, and all
other future command-line parameters in one variable.  Think about it.
 You can use the parse-options API to fill in default arguments
anyway.

>>> 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.
>>
>> Hm, this will make it a little too silent for the end-user.  What do
>> you feel about printing something minimalistic like a '.' for each
>> imported revision? Atleast it won't look like it's hung.
> For a medium 8k commit repo it is 100 lines - still too much.
> A single line for the first revision seem harmless and will indicate
> that the remote connection succeeded
> (helps to see that it's not a connection timeout, probably caused by
> dns lookup or a firewall).

Ah, right. I agree with you.

>>  Also, how does this interact with the 'progress' option of fast-import protocol?
> git fast-import --quiet prints any progress line produced by a helper.
> transport-helper.c tries to set the option
> but doesn't fail if it is not accepted or if helper doesn't support
> options at all.
> For now the helper doesn't use this protocol option.
>
> A better solution could be to use progress.o api to display progress.
> Or an ad-hoc hack with adaptive progress step, say report a progress
> on each "power of two"-th revision.
> As a starting point for tests let there bust a simple switch-off.

Yeah, that makes sense -- would love to see that in future.

>>> -void fast_export_init(int fd, const char *dst_ref)
>>> +void fast_export_init(int fd, const char *dst_ref, int progress)
>>>  {
>>>        first_commit_done = 0;
>>> +       print_progress = progress;
>>
>> The only reason you're modifying the API of fast_export_init is so
>> that it can set a global static variable?
> Looked once more at how these new variables are used. We can move
> progress lines generation to svndump.c. And also move ref_name from
> _init and being global static to a parameter in fast_export_begin_commit(),
> and to be more sane s/revision/target_mark/ s/revision - 1/from_mark/ and
> add a from_mark parameter, move first_commit_done logic to svndump.o.
> This way fast_export.o can operate on single commits and maybe it'll be
> easier to use it to apply svn branches layout in svn-fe in one run, though I'm
> not sure I'll use svn-fe to manage svn branches.

Makes sense.
Thanks.

-- Ram