[NON-PATCH] vcs-svn: avoid 'ls' and filedelete with empty path

[Jonathan Nieder <jrnieder@gmail.com>]

Jonathan Nieder wrote:

> svn-fe should probably also be tweaked to handle this case without
> demanding support for the (nice) 'ls empty path' extension in
> fast-import backends, and this could even happen before 1.7.10.
> I can't promise to get to that quickly enough, though.

On second thought, svn-fe will need the 'ls empty path' extension
before it can handle this case again. :(

-- >8 --
There is a pathological Subversion operation that svn-fe handles
incorrectly due to an unexpected response from fast-import:

  svn cp $SVN_ROOT $SVN_ROOT/subdirectory

svn-fe tries to handle this as a copy from "" to "subdirectory", in
two steps: first, "ls :1 " to retrieve a <dataref> for the root, and
then "M 040000 <dataref> subdirectory" to use it in the active commit.

In git 1.7.9.3 and earlier, the fast-import "ls" command does not
understand that by the empty path we mean the root of the tree.  The
unrecognized path is reported as "missing", so svn-fe emits "D
subdirectory" and continues unaware of the miscommunication that has
taken place.

This is a regression introduced by commit 723b7a27 (vcs-svn: eliminate
repo_tree structure, 2010-12-10).

A patch in flight teaches fast-import to error out, which is a little
better, but still does not win us a successful and accurate import.
Because the meaning of empty paths was not specified in the
fast-import manual until recently, other backends are likely to handle
this construct inconsistently, too.

svn-fe never actually needs to pass "" as an argument to 'ls',
'C', 'R', or 'D'. (*)  There is always another way to spell what it is
trying to do:

. Making a 'ls <foo> ""' request and waiting for a response is a
  complicated way to spell the identity operation.  When <foo> is
  a commit, <foo> can be used directly to name the root of the
  corresponding tree. (*)

. Emitting 'ls ""' to get a name for the root of the current tree
  would be useful in general and there is no other command that
  does that.  Svn-fe never does that: translating non-copy nodes from
  the Subversion dump format does not involve retrieving current
  directory listings to apply a delta to them, and for copies, the
  copyfrom information refers to a specific previous revision.

. svn-fe does not use the filerename (R) and filecopy (C) commands.

. The intent of the command 'D ""' is more clearly written as
  'deleteall'.

Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
Explained-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>

Toy patch, not intended for application.

(*) Lies.
---
 t/t9010-svn-fe.sh     |   69 +++++++++++++++++++++++++++++++++++++++++++++++++
 vcs-svn/fast_export.c |   25 +++++++++++++++++-
 vcs-svn/fast_export.h |    2 +-
 vcs-svn/repo_tree.c   |    6 +++--
 vcs-svn/repo_tree.h   |    2 +-
 vcs-svn/svndump.c     |    3 ++-
 6 files changed, 101 insertions(+), 6 deletions(-)

diff --git a/t/t9010-svn-fe.sh b/t/t9010-svn-fe.sh
index b7eed248..45706bde 100755
--- a/t/t9010-svn-fe.sh
+++ b/t/t9010-svn-fe.sh
@@ -271,6 +271,75 @@ test_expect_success PIPE 'directory with files' '
 	test_cmp hi directory/file2
 '
 
+test_expect_success PIPE 'copy from root to directory' '
+	reinit_git &&
+	echo hello >hello &&
+	hello_blob=$(git hash-object -w -t blob hello) &&
+	subtree=$(
+		echo "100644 blob $hello_blob	README.txt" |
+		git mktree
+	) &&
+	expect=$(
+		git mktree <<-EOF
+			100644 blob $hello_blob	README.txt
+			040000 tree $subtree	trunk
+		EOF
+	) &&
+
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "2012-10-10T00:01:003.000000Z" \
+			svn:log "created README.txt" &&
+		echo PROPS-END
+	} >r1.props &&
+	{
+		properties \
+			svn:author author@example.com \
+			svn:date "2012-10-10T00:02:005.000000Z" \
+			svn:log "created trunk" &&
+		echo PROPS-END
+	} >r2.props &&
+	{
+		cat <<-EOF &&
+		SVN-fs-dump-format-version: 3
+
+		Revision-number: 1
+		EOF
+		echo Prop-content-length: $(wc -c <r1.props) &&
+		echo Content-length: $(wc -c <r1.props) &&
+		echo &&
+		cat r1.props &&
+		cat <<-\EOF &&
+
+		Node-path: README.txt
+		Node-kind: file
+		Node-action: add
+		EOF
+		text_no_props hello &&
+		echo Revision-number: 2
+		echo Prop-content-length: $(wc -c <r2.props) &&
+		echo Content-length: $(wc -c <r2.props) &&
+		echo &&
+		cat r2.props &&
+		sed -e "s/X\$//" <<-\EOF
+
+		Node-path: trunk
+		Node-kind: dir
+		Node-action: add
+		Node-copyfrom-rev: 1
+		Node-copyfrom-path: X
+		Prop-content-length: 10
+		Content-length: 10
+
+		PROPS-END
+		EOF
+	} >copy-root.dump &&
+	try_dump copy-root.dump &&
+
+	git diff-tree --exit-code $expect HEAD
+'
+
 test_expect_success PIPE 'branch name with backslash' '
 	reinit_git &&
 	sort <<-\EOF >expect.branch-files &&
diff --git a/vcs-svn/fast_export.c b/vcs-svn/fast_export.c
index 19d7c34c..24232618 100644
--- a/vcs-svn/fast_export.c
+++ b/vcs-svn/fast_export.c
@@ -49,6 +49,12 @@ void fast_export_reset(void)
 
 void fast_export_delete(const char *path)
 {
+	/* delete("") means to return to a clean slate. */
+	if (!*path) {
+		printf("deleteall\n");
+		return;
+	}
+
 	putchar('D');
 	putchar(' ');
 	quote_c_style(path, NULL, stdout, 0);
@@ -113,6 +119,8 @@ void fast_export_end_commit(uint32_t revision)
 
 static void ls_from_rev(uint32_t rev, const char *path)
 {
+	assert(*path);
+
 	/* ls :5 path/to/old/file */
 	printf("ls :%"PRIu32" ", rev);
 	quote_c_style(path, NULL, stdout, 0);
@@ -122,6 +130,8 @@ static void ls_from_rev(uint32_t rev, const char *path)
 
 static void ls_from_active_commit(const char *path)
 {
+	assert(*path);
+
 	/* ls "path/to/file" */
 	printf("ls \"");
 	quote_c_style(path, NULL, stdout, 1);
@@ -285,12 +295,25 @@ static int parse_ls_response(const char *response, uint32_t *mode,
 int fast_export_ls_rev(uint32_t rev, const char *path,
 				uint32_t *mode, struct strbuf *dataref)
 {
+	if (!*path) {
+		/*
+		 * The easy case: when path is "", the caller can use
+		 * :<rev> directly to refer to the root of the tree. (*)
+		 */
+		strbuf_addf(dataref, ":%"PRIu32, rev);
+		*mode = REPO_MODE_DIR;
+		return 0;
+	}
+
 	ls_from_rev(rev, path);
 	return parse_ls_response(get_response_line(), mode, dataref);
 }
 
-int fast_export_ls(const char *path, uint32_t *mode, struct strbuf *dataref)
+int fast_export_ls_nonroot(const char *path, uint32_t *mode,
+				struct strbuf *dataref)
 {
+	assert(*path);
+
 	ls_from_active_commit(path);
 	return parse_ls_response(get_response_line(), mode, dataref);
 }
diff --git a/vcs-svn/fast_export.h b/vcs-svn/fast_export.h
index 43d05b65..53e208e2 100644
--- a/vcs-svn/fast_export.h
+++ b/vcs-svn/fast_export.h
@@ -22,7 +22,7 @@ void fast_export_blob_delta(uint32_t mode,
 /* If there is no such file at that rev, returns -1, errno == ENOENT. */
 int fast_export_ls_rev(uint32_t rev, const char *path,
 			uint32_t *mode_out, struct strbuf *dataref_out);
-int fast_export_ls(const char *path,
+int fast_export_ls_nonroot(const char *path,
 			uint32_t *mode_out, struct strbuf *dataref_out);
 
 #endif
diff --git a/vcs-svn/repo_tree.c b/vcs-svn/repo_tree.c
index 67d27f0b..1547b8ce 100644
--- a/vcs-svn/repo_tree.c
+++ b/vcs-svn/repo_tree.c
@@ -8,13 +8,15 @@
 #include "repo_tree.h"
 #include "fast_export.h"
 
-const char *repo_read_path(const char *path, uint32_t *mode_out)
+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out)
 {
 	int err;
 	static struct strbuf buf = STRBUF_INIT;
 
+	assert(*path);
+
 	strbuf_reset(&buf);
-	err = fast_export_ls(path, mode_out, &buf);
+	err = fast_export_ls_nonroot(path, mode_out, &buf);
 	if (err) {
 		if (errno != ENOENT)
 			die_errno("BUG: unexpected fast_export_ls error");
diff --git a/vcs-svn/repo_tree.h b/vcs-svn/repo_tree.h
index 889c6a3c..466d5a63 100644
--- a/vcs-svn/repo_tree.h
+++ b/vcs-svn/repo_tree.h
@@ -11,7 +11,7 @@ struct strbuf;
 uint32_t next_blob_mark(void);
 void repo_copy(uint32_t revision, const char *src, const char *dst);
 void repo_add(const char *path, uint32_t mode, uint32_t blob_mark);
-const char *repo_read_path(const char *path, uint32_t *mode_out);
+const char *repo_read_nonroot_path(const char *path, uint32_t *mode_out);
 void repo_delete(const char *path);
 void repo_commit(uint32_t revision, const char *author,
 		const struct strbuf *log, const char *uuid, const char *url,
diff --git a/vcs-svn/svndump.c b/vcs-svn/svndump.c
index ca63760f..d610184d 100644
--- a/vcs-svn/svndump.c
+++ b/vcs-svn/svndump.c
@@ -248,7 +248,8 @@ static void handle_node(void)
 		old_data = NULL;
 	} else if (node_ctx.action == NODEACT_CHANGE) {
 		uint32_t mode;
-		old_data = repo_read_path(node_ctx.dst.buf, &mode);
+		assert(*node_ctx.dst.buf);
+		old_data = repo_read_nonroot_path(node_ctx.dst.buf, &mode);
 		if (mode == REPO_MODE_DIR && type != REPO_MODE_DIR)
 			die("invalid dump: cannot modify a directory into a file");
 		if (mode != REPO_MODE_DIR && type == REPO_MODE_DIR)
-- 
1.7.9.2