[BUG] fast-import: ls command on commit root returns missing (was: Bug in svn-fe: copying the root directory acts as if it's an empty directory)

[BUG] fast-import: ls command on commit root returns missing (was: Bug in svn-fe: copying the root directory acts as if it's an empty directory)

[David Barr]

On Thu, Mar 8, 2012 at 11:46 AM, David Barr <davidbarr@google.com> wrote:
> Hi Andrew,
>
> On Thu, Mar 8, 2012 at 10:13 AM, Andrew Sayers
> <andrew-git@pileofstuff.org> wrote:
>> Here's a bug with svn-fe that I stumbled over while snorkelling through
>> repo madness.  I've tested it with the version of svn-fe in git.git's
>> master branch.
>>
>> Copying the root directory to a sub-directory (e.g. doing `svn cp .
>> trunk` to standardise your layout) doesn't correctly initialise the new
>> directory.
>
> This issue sounds very familiar, I wonder if there's an existing test
> or pending patch for it? Maybe Dmitry or Jonathan can recall.

I've stepped through the reproduction and the bug seems to arise when
the following command is sent to git-fast-import:

  'ls' SP ':1' SP LF

The expected output in this example is:

  '400000' SP 'tree' SP 'dd59323fe27c5647cb7ef15ce4637faae199c5f0' HT LF

The actual output is:

  'missing' SP LF

--
David Barr

[PATCH] fast-import: fix ls command with empty path

[David Barr]

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

When the following command is sent to fast-import:

 'ls' SP ':1' SP LF

The expected output is:

 '040000' SP 'tree' SP <dataref> HT LF

The actual output is:

 'missing' SP LF

This is because tree_content_get() is called but expects a non-empty
path. Instead, copy the root entry and force the mode to S_IFDIR.

Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
Signed-off-by: David Barr <davidbarr@google.com>
---
 fast-import.c          |    7 ++++++-
 t/t9300-fast-import.sh |   31 +++++++++++++++++++++++++++++++
 2 files changed, 37 insertions(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index c1486ca..8dbfd4c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3019,7 +3019,12 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	tree_content_get(root, p, &leaf);
+	if (*p) {
+		tree_content_get(root, p, &leaf);
+	} else {
+		leaf = *root;
+		leaf.versions[1].mode = S_IFDIR;
+	}
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 438aaf6..2558a2e 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1400,6 +1400,37 @@ test_expect_success \
 	 test_cmp expect.qux actual.qux &&
 	 test_cmp expect.qux actual.quux'
 
+test_expect_success PIPE 'N: read and copy root' '
+	cat >expect <<-\EOF
+	:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	file3/file2/newf
+	:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	file3/file2/oldf
+	:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	file3/file4
+	:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	file3/newdir/exec.sh
+	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/newdir/interesting
+	EOF
+	git update-ref -d refs/heads/N12 &&
+	rm -f backflow &&
+	mkfifo backflow &&
+	(
+		exec <backflow &&
+		cat <<-EOF &&
+		commit refs/heads/N12
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy root directory by tree hash read via ls
+		COMMIT
+
+		from refs/heads/branch^0
+		ls ""
+		EOF
+		read mode type tree filename &&
+		echo "M 040000 $tree file3"
+	) |
+	git fast-import --cat-blob-fd=3 3>backflow &&
+	git diff-tree -C --find-copies-harder -r N12^ N12 >actual &&
+	compare_diff_raw expect actual
+'
+
 ###
 ### series O
 ###
-- 
1.7.9.3

Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder]

(+cc: Sverre)
David Barr wrote:

> When the following command is sent to fast-import:
>
>  'ls' SP ':1' SP LF
>
> The expected output is:
>
>  '040000' SP 'tree' SP <dataref> HT LF
>
> The actual output is:
>
>  'missing' SP LF
>
> This is because tree_content_get() is called but expects a non-empty
> path. Instead, copy the root entry and force the mode to S_IFDIR.
>
> Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
> Signed-off-by: David Barr <davidbarr@google.com>

For what it's worth,
Acked-by: Jonathan Nieder <jrnieder@gmail.com>

Thanks very much for taking care of it.

> [Subject: fast-import: fix ls command with empty path]

I would s/fix/accept/ to be more precise about the nature of the
breakage.  (In other words: rather than mishandling ls with an empty
path, fast-import was not handling it at all.)

[...]
> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -3019,7 +3019,12 @@ static void parse_ls(struct branch *b)
>  			die("Garbage after path in: %s", command_buf.buf);
>  		p = uq.buf;
>  	}
> -	tree_content_get(root, p, &leaf);
> +	if (*p) {
> +		tree_content_get(root, p, &leaf);
> +	} else {
> +		leaf = *root;
> +		leaf.versions[1].mode = S_IFDIR;
> +	}

If this special case were implemented in tree_content_get(), we would
get support for paths with a trailing '/' (allows useless
incompatibility, bad) and support for requests like

	C "" some/subdir

(good).  What do you think?

-- >8 --
Subject: fast-import: allow filecopy to copy from root

Some subversion users apparently use "svn copy $SVN_ROOT
$SVN_ROOT/subdirectory" from time to time.  svn-fe handles this fine
already since it translates the subversion copy instruction to

	ls ""
	M 040000 <returned tree name> subdirectory

We can easily imagine an alternate importer that would write

	C "" subdirectory

instead, so handle that, too.

A naive implementation would also mean gaining support for copies
where the source has a trailing '/', as in

	C onedir/ anotherdir

but in the spirit of 34215783 (fast-import: tighten M 040000 syntax,
2010-10-17), this patch is careful to reject that syntax to avoid
making it too easy for frontends to introduce unnecessary
incompatibilities with git fast-import 1.7.8 and older and other
fast-import consumers.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c          |   32 ++++++++++++++-----------
 t/t9300-fast-import.sh |   62 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 79 insertions(+), 15 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 8dbfd4cc..5ce61bca 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1636,6 +1636,10 @@ static int tree_content_get(
 	unsigned int i, n;
 	struct tree_entry *e;
 
+	if (!*p) {
+		e = root;
+		goto last_component;
+	}
 	slash1 = strchr(p, '/');
 	if (slash1)
 		n = slash1 - p;
@@ -1648,14 +1652,10 @@ static int tree_content_get(
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
-			if (!slash1) {
-				memcpy(leaf, e, sizeof(*leaf));
-				if (e->tree && is_null_sha1(e->versions[1].sha1))
-					leaf->tree = dup_tree_content(e->tree);
-				else
-					leaf->tree = NULL;
-				return 1;
-			}
+			if (!slash1)
+				goto last_component;
+			if (!slash1[1])	/* paths with trailing '/' do not match */
+				return 0;
 			if (!S_ISDIR(e->versions[1].mode))
 				return 0;
 			if (!e->tree)
@@ -1664,6 +1664,14 @@ static int tree_content_get(
 		}
 	}
 	return 0;
+
+last_component:
+	memcpy(leaf, e, sizeof(*leaf));
+	if (e->tree && is_null_sha1(e->versions[1].sha1))
+		leaf->tree = dup_tree_content(e->tree);
+	else
+		leaf->tree = NULL;
+	return 1;
 }
 
 static int update_branch(struct branch *b)
@@ -3005,6 +3013,7 @@ static void parse_ls(struct branch *b)
 		struct object_entry *e = parse_treeish_dataref(&p);
 		root = new_tree_entry();
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		root->versions[1].mode = S_IFDIR;
 		load_tree(root);
 		if (*p++ != ' ')
 			die("Missing space after tree-ish: %s", command_buf.buf);
@@ -3019,12 +3028,7 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	if (*p) {
-		tree_content_get(root, p, &leaf);
-	} else {
-		leaf = *root;
-		leaf.versions[1].mode = S_IFDIR;
-	}
+	tree_content_get(root, p, &leaf);
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2558a2ed..5316b73c 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1047,6 +1047,66 @@ test_expect_success \
 	 git diff-tree -C --find-copies-harder -r N1^ N1 >actual &&
 	 compare_diff_raw expect actual'
 
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+copy to subdir
+COMMIT
+
+from refs/heads/branch^0
+C "" subdir
+
+INPUT_END
+
+cat >expect <<\EOF
+:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	subdir/file2/newf
+:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	subdir/file2/oldf
+:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	subdir/file4
+:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	subdir/newdir/exec.sh
+:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	subdir/newdir/interesting
+EOF
+test_expect_success \
+	'N: copy with empty source path' \
+	'git fast-import <input &&
+	 git diff-tree -C -C -r --no-commit-id N-root-to-subdir >actual &&
+	 compare_diff_raw expect actual'
+
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-unquoted-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+increase nesting
+COMMIT
+
+from refs/heads/branch^0
+C  subdir
+
+INPUT_END
+test_expect_success \
+	'N: copy with unquoted empty source path' \
+	'git fast-import <input &&
+	 git diff --exit-code N-root-to-subdir N-unquoted-root-to-subdir'
+
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-trailing-slash-in-src
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+copy newdir to newerdir
+COMMIT
+
+from refs/heads/branch^0
+C newdir/ newerdir
+
+INPUT_END
+test_expect_success \
+	'N: reject foo/ as source path' \
+	'# fatal: path not in branch
+	 test_must_fail git fast-import <input'
+
 cat >input <<INPUT_END
 commit refs/heads/N2
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
@@ -1293,7 +1353,7 @@ test_expect_success \
 	 compare_diff_raw expect actual'
 
 test_expect_success \
-	'N: reject foo/ syntax' \
+	'N: filemodify: reject foo/ syntax' \
 	'subdir=$(git rev-parse refs/heads/branch^0:file2) &&
 	 test_must_fail git fast-import <<-INPUT_END
 	commit refs/heads/N5B
-- 
1.7.9.2

Re: [PATCH] fast-import: fix ls command with empty path

[Sverre Rabbelier]

On Thu, Mar 8, 2012 at 01:09, Jonathan Nieder <jrnieder@gmail.com> wrote:
>> [Subject: fast-import: fix ls command with empty path]
>
> I would s/fix/accept/ to be more precise about the nature of the
> breakage.  (In other words: rather than mishandling ls with an empty
> path, fast-import was not handling it at all.)

Makes sense to me :)

-- 
Cheers,

Sverre Rabbelier

Re: [PATCH] fast-import: fix ls command with empty path

[Junio C Hamano]

Jonathan Nieder <jrnieder@gmail.com> writes:

> For what it's worth,
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>
> Thanks very much for taking care of it.
>
>> [Subject: fast-import: fix ls command with empty path]
>
> I would s/fix/accept/ to be more precise about the nature of the
> breakage.  (In other words: rather than mishandling ls with an empty
> path, fast-import was not handling it at all.)
>
> ...
> (good).  What do you think?
>
> -- >8 --
> Subject: fast-import: allow filecopy to copy from root

So what do you guys want to do with topic?  My gut feeling is that
this is not a new regression and can wait until the next cycle.  I
could certainly carry David's patch in 'pu' if doing so helps the
discussion to come up with the right solution, though.

Re: [PATCH] fast-import: fix ls command with empty path

[Dmitry Ivankov]

Don't quite have the time to run tests, but maybe the issue is solved
 in a stalled series [1]. At least [1] is worth looking at with regard
 to this bug.

 One more quick thought. "force the root mode to S_IFDIR" part doesn't
 look obviously good for me. First, isn't it already ensured that the
 root is a directory? Second, if it is allowed to be a file I'm not
 sure it's ok to silently make it a directory on a root-to-subtree
 operation, do we do it for subtree-to-subsubtree?

 P.S. looking at [1] now I'd say the commit messages could be improved there

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

On Thu, Mar 8, 2012 at 10:39 PM, Junio C Hamano <gitster@pobox.com> wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:
>
>> For what it's worth,
>> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
>>
>> Thanks very much for taking care of it.
>>
>>> [Subject: fast-import: fix ls command with empty path]
>>
>> I would s/fix/accept/ to be more precise about the nature of the
>> breakage.  (In other words: rather than mishandling ls with an empty
>> path, fast-import was not handling it at all.)
>>
>> ...
>> (good).  What do you think?
>>
>> -- >8 --
>> Subject: fast-import: allow filecopy to copy from root
>
> So what do you guys want to do with topic?  My gut feeling is that
> this is not a new regression and can wait until the next cycle.  I
> could certainly carry David's patch in 'pu' if doing so helps the
> discussion to come up with the right solution, though.
>
>

Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder]

Hi,

Junio C Hamano wrote:

> So what do you guys want to do with topic?  My gut feeling is that
> this is not a new regression and can wait until the next cycle.

Yes, I think you are very right.

. I have a vague fear that my "allow filecopy to copy from root" patch
  on top of David's is missing some handling of the empty src case,
  along the same lines as 8fe533f6 (fast-import: treat filemodify with
  empty tree as delete.

. After looking closer at David's patch, it does not seem to handle
  'ls <tree> ""' carefully enough.  It probably needs something
  like Dmitry's [1].

So please backburner this, and we can try for something better by
next cycle.

Thanks for a sanity check.
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/179426/focus=179425

Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder]

Dmitry Ivankov wrote:

>  One more quick thought. "force the root mode to S_IFDIR" part doesn't
>  look obviously good for me.

It was just a problematic and incomplete version of what your "be
saner with temporary trees" does properly. ;-)

[...]
>  P.S. looking at [1] now I'd say the commit messages could be improved there
>
>  [1] http://thread.gmane.org/gmane.comp.version-control.git/179426

Yes, please.  Or patch 2/2 could be split into multiple patches,
perhaps along the following lines:

 - ls "" support, as in David's patch
 - ls <dataref> "" support, which requires the "be saner with
   temporaries" fix
 - D "" support, maybe.  (As you mentioned, we have deleteall so
   compatibility would dictate not supporting it unless some frontend
   is using it already by mistake in some circumstance.)
 - C "" <dest> support, as in my reply to David's patch
 - R "" <dest> support

Thanks,
Jonathan

[PATCH v2 0/2] Re: fast-import: fix ls command with empty path

[Jonathan Nieder]

David Barr wrote:

> When the following command is sent to fast-import:
>
>  'ls' SP ':1' SP LF
>
> The expected output is:
>
>  '040000' SP 'tree' SP <dataref> HT LF
>
> The actual output is:
>
>  'missing' SP LF
>
> This is because tree_content_get() is called but expects a non-empty
> path. Instead, copy the root entry and force the mode to S_IFDIR.

Thanks again for your help.  I've pushed the following changes to

  git://repo.or.cz/git/jrn.git fast-import-pu

Testing, review, and improvements welcome.

David Barr (1):
  fast-import: teach ls command to accept empty path

Jonathan Nieder (1):
  fast-import: plug leak of dirty trees in 'ls' command

 fast-import.c          |   19 +++++++++-
 t/t9300-fast-import.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 113 insertions(+), 1 deletion(-)

[PATCH 1/2] fast-import: plug leak of dirty trees in 'ls' command

[Jonathan Nieder]

When the named directory has changed since it was last written to
pack, "tree_content_get" makes a deep copy of the list of tree entries
which we forgot to free.

This memory leak has been present since the "ls" command was
introduced in v1.7.5-rc0~3^2~33 (fast-import: add 'ls' command,
2010-12-02).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
After rediscovering this, I found [1] which mentions the same bug.

[1] http://thread.gmane.org/gmane.comp.version-control.git/178007/focus=178044

 fast-import.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fast-import.c b/fast-import.c
index c1486cab..1758da94 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3028,6 +3028,8 @@ static void parse_ls(struct branch *b)
 		store_tree(&leaf);
 
 	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
+	if (leaf.tree)
+		release_tree_content_recursive(leaf.tree);
 	if (!b || root != &b->branch_tree)
 		release_tree_entry(root);
 }
-- 
1.7.9.2

[PATCH 2/2] fast-import: teach ls command to accept empty path

[Jonathan Nieder]

From: David Barr <davidbarr@google.com>

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

When the following command is sent to fast-import:

 'ls' SP ':1' SP LF

The expected output is:

 '040000' SP 'tree' SP <dataref> HT LF

The actual output is:

 'missing' SP LF

This is because tree_content_get() is called but expects a non-empty
path. Instead, copy the root entry.

[jn: using a deep copy; w/ more tests]
[jn: with a fix from Dmitry to fully initialize root->versions[0]
 and versions[1] now that root can be passed to store_tree]

Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c          |   17 ++++++++-
 t/t9300-fast-import.sh |   95 ++++++++++++++++++++++++++++++++++++++++++++++++
 2 files changed, 111 insertions(+), 1 deletion(-)

diff --git a/fast-import.c b/fast-import.c
index 1758da94..31857e95 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3004,7 +3004,10 @@ static void parse_ls(struct branch *b)
 	} else {
 		struct object_entry *e = parse_treeish_dataref(&p);
 		root = new_tree_entry();
+		hashclr(root->versions[0].sha1);
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		root->versions[0].mode = 0;
+		root->versions[1].mode = S_IFDIR;
 		load_tree(root);
 		if (*p++ != ' ')
 			die("Missing space after tree-ish: %s", command_buf.buf);
@@ -3019,7 +3022,19 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	tree_content_get(root, p, &leaf);
+	if (*p) {
+		tree_content_get(root, p, &leaf);
+	} else {
+		memcpy(&leaf, root, sizeof(leaf));
+		/*
+		 * store_tree scribbles over version[0] in leaf.tree's
+		 * entries, so we need a deep copy.
+		 */
+		if (root->tree && is_null_sha1(root->versions[1].sha1))
+			leaf.tree = dup_tree_content(root->tree);
+		else
+			leaf.tree = NULL;
+	}
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 438aaf6b..635bfadb 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1400,6 +1400,101 @@ test_expect_success \
 	 test_cmp expect.qux actual.qux &&
 	 test_cmp expect.qux actual.quux'
 
+test_expect_success 'N: root of unborn branch reads as present and empty' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo "040000 tree $empty_tree	" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-empty
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read empty root directory via ls
+	COMMIT
+
+	ls ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: empty root reads as present and empty' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo "040000 tree $empty_tree	" >expect &&
+	echo empty >msg &&
+	cmit=$(git commit-tree "$empty_tree" -p refs/heads/branch^0 <msg) &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-empty-existing
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read empty root directory via ls
+	COMMIT
+
+	ls ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: "ls" command can read subdir of named tree' '
+	branch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&
+	subdir_tree=$(git rev-parse $branch_cmit:newdir) &&
+	echo "040000 tree $subdir_tree	newdir" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-subdir-of-named-tree
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read from commit with ls
+	COMMIT
+
+	ls $branch_cmit "newdir"
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: "ls" command can read root of named commit' '
+	branch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&
+	branch_tree=$(git rev-parse --verify $branch_cmit^{tree}) &&
+	echo "040000 tree $branch_tree	" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-root-of-named-tree
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read root directory of commit with ls
+	COMMIT
+
+	ls $branch_cmit ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success PIPE 'N: read and copy root' '
+	cat >expect <<-\EOF &&
+	:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	file3/file2/newf
+	:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	file3/file2/oldf
+	:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	file3/file4
+	:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	file3/newdir/exec.sh
+	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/newdir/interesting
+	EOF
+	git update-ref -d refs/heads/N12 &&
+	rm -f backflow &&
+	mkfifo backflow &&
+	(
+		exec <backflow &&
+		cat <<-EOF &&
+		commit refs/heads/N12
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy root directory by tree hash read via ls
+		COMMIT
+
+		from refs/heads/branch^0
+		ls ""
+		EOF
+		read mode type tree filename &&
+		echo "M 040000 $tree file3"
+	) |
+	git fast-import --cat-blob-fd=3 3>backflow &&
+	git diff-tree -C --find-copies-harder -r N12^ N12 >actual &&
+	compare_diff_raw expect actual
+'
+
 ###
 ### series O
 ###
-- 
1.7.9.2

Re: [PATCH 2/2] fast-import: teach ls command to accept empty path

[David Barr]

On Fri, Mar 9, 2012 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:
> From: David Barr <davidbarr@google.com>
>
> 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
>
> When the following command is sent to fast-import:
>
>  'ls' SP ':1' SP LF
>
> The expected output is:
>
>  '040000' SP 'tree' SP <dataref> HT LF
>
> The actual output is:
>
>  'missing' SP LF
>
> This is because tree_content_get() is called but expects a non-empty
> path. Instead, copy the root entry.
>
> [jn: using a deep copy; w/ more tests]
> [jn: with a fix from Dmitry to fully initialize root->versions[0]
>  and versions[1] now that root can be passed to store_tree]
>
> Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
> Signed-off-by: David Barr <davidbarr@google.com>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
> ---
> +               /*
> +                * store_tree scribbles over version[0] in leaf.tree's
> +                * entries, so we need a deep copy.
> +                */
> +               if (root->tree && is_null_sha1(root->versions[1].sha1))
> +                       leaf.tree = dup_tree_content(root->tree);

Is it ok to call store_tree(root)? If so, could we not introduce a
pointer rather than a deep copy?

diff --git a/fast-import.c b/fast-import.c
index 94d7037..eab24f3 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2993,7 +2993,8 @@ static void parse_ls(struct branch *b)
 {
 	const char *p;
 	struct tree_entry *root = NULL;
-	struct tree_entry leaf = {NULL};
+	struct tree_entry tmp_tree = {NULL};
+	struct tree_entry *leaf = &tmp_tree;

 	/* ls SP (<treeish> SP)? <path> */
 	p = command_buf.buf + strlen("ls ");
@@ -3022,27 +3023,18 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	if (*p) {
-		tree_content_get(root, p, &leaf);
-	} else {
-		memcpy(&leaf, root, sizeof(leaf));
-		/*
-		 * store_tree scribbles over version[0] in leaf.tree's
-		 * entries, so we need a deep copy.
-		 */
-		if (root->tree && is_null_sha1(root->versions[1].sha1))
-			leaf.tree = dup_tree_content(root->tree);
-		else
-			leaf.tree = NULL;
-	}
+	if (*p)
+		tree_content_get(root, p, leaf);
+	else
+		leaf = root;
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
 	 */
-	if (S_ISDIR(leaf.versions[1].mode))
-		store_tree(&leaf);
+	if (S_ISDIR(leaf->versions[1].mode))
+		store_tree(leaf);

-	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
+	print_ls(leaf->versions[1].mode, leaf->versions[1].sha1, p);
 	if (!b || root != &b->branch_tree)
 		release_tree_entry(root);
 }

--
David Barr

Re: [PATCH 2/2] fast-import: teach ls command to accept empty path

[Jonathan Nieder]

David Barr wrote:
> On Fri, Mar 9, 2012 at 7:33 AM, Jonathan Nieder <jrnieder@gmail.com> wrote:

>> +               /*
>> +                * store_tree scribbles over version[0] in leaf.tree's
>> +                * entries, so we need a deep copy.
>> +                */
>> +               if (root->tree && is_null_sha1(root->versions[1].sha1))
>> +                       leaf.tree = dup_tree_content(root->tree);
>
> Is it ok to call store_tree(root)?

Yes.

>                                     If so, could we not introduce a
> pointer rather than a deep copy?

If using 'ls' with an empty path after dirtying the root tree is
common, then that would work as an optimization.  The fussy bit is
making sure the call to

	release_tree_content_recursive(leaf.tree);

is skipped in this case and not skipped when tree_content_get() made a
copy.  That is, something like this (patch against fast-import-pu on
repo.or.cz/git/jrn.git):

-- >8 --
From: David Barr <davidbarr@google.com>
Subject: fast-import: optimize 'ls' command with empty path to avoid a copy

fast-import's "ls" command normally copies a tree (implicitly, by
calling tree_content_get) before passing it to store_tree.  Otherwise:

 - after versions[0] is overwritten by versions[1] in child
   directories, it would be impossible to rebuild the tree object for
   version 0 of the current tree, so parse_ls would need to

	hashcpy(leaf.versions[0].sha1, leaf.versions[1].sha1)

   so version 0 points to a tree that can be rebuilt.

 - in turn, that would make it impossible to rebuild the tree object
   for version 0 of the parent tree.  And so on.

The above considerations do not apply when the tree we are examining
with 'ls' has no parent.  Avoid a copy in that case.

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c |   22 +++++++++++++++-------
 1 file changed, 15 insertions(+), 7 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 1e5d59b4..28fe4c35 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3002,7 +3002,8 @@ static void parse_ls(struct branch *b)
 {
 	const char *p;
 	struct tree_entry *root = NULL;
-	struct tree_entry leaf = {NULL};
+	struct tree_entry leaf_storage = {NULL};
+	struct tree_entry *leaf = &leaf_storage;
 
 	/* ls SP (<treeish> SP)? <path> */
 	p = command_buf.buf + strlen("ls ");
@@ -3029,17 +3030,24 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	tree_content_get(root, p, &leaf);
+	if (*p)
+		tree_content_get(root, p, leaf);
+	else
+		leaf = root;
+
 	/*
 	 * A directory in preparation would have a sha1 of zero
 	 * until it is saved.  Save, for simplicity.
 	 */
-	if (S_ISDIR(leaf.versions[1].mode))
-		store_tree(&leaf);
+	if (S_ISDIR(leaf->versions[1].mode)
+	    && is_null_sha1(leaf->versions[1].sha1)) {
+		store_tree(leaf);
+		hashcpy(leaf->versions[0].sha1, leaf->versions[1].sha1);
+	}
 
-	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
-	if (leaf.tree)
-		release_tree_content_recursive(leaf.tree);
+	print_ls(leaf->versions[1].mode, leaf->versions[1].sha1, p);
+	if (*p && leaf->tree)
+		release_tree_content_recursive(leaf->tree);
 	if (!b || root != &b->branch_tree)
 		release_tree_entry(root);
 }
-- 
1.7.9.2

Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder]

Junio C Hamano wrote:
> Jonathan Nieder <jrnieder@gmail.com> writes:

>>> [Subject: fast-import: fix ls command with empty path]
>>
>> I would s/fix/accept/ to be more precise about the nature of the
>> breakage.  (In other words: rather than mishandling ls with an empty
>> path, fast-import was not handling it at all.)
[...]
> So what do you guys want to do with topic?  My gut feeling is that
> this is not a new regression and can wait until the next cycle.

Thanks again for the advice so far.

After sleeping on it, here are two patches for 'maint'.  One plugs a
memory leak.  The other makes my above comment actually true, so
trying to use this missing feature results in an error message that
can help the frontend author instead of the silently broken conversion
Andrew found.

Then we can carefully add 'ls ""' support in 1.7.11.

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.

Sensible?

Jonathan

[PATCH maint-1.7.6] fast-import: leakfix for 'ls' of dirty trees

[Jonathan Nieder]

When the chosen directory has changed since it was last written to
pack, "tree_content_get" makes a deep copy of its content to scribble
on while computing the tree name, which we forgot to free.

This leak has been present since the 'ls' command was introduced in
v1.7.5-rc0~3^2~33 (fast-import: add 'ls' command, 2010-12-02).

Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Sorry to have missed this buglet for so long.  (Doubly so because it
was noticed and commented on half a year ago before being promptly
forgotten.)  Patch is against commit 8dc6a373d2 which introduced the
leaky 'ls' support.

 fast-import.c |    2 ++
 1 file changed, 2 insertions(+)

diff --git a/fast-import.c b/fast-import.c
index 6c37b840..fff285cd 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2987,6 +2987,8 @@ static void parse_ls(struct branch *b)
 		store_tree(&leaf);
 
 	print_ls(leaf.versions[1].mode, leaf.versions[1].sha1, p);
+	if (leaf.tree)
+		release_tree_content_recursive(leaf.tree);
 	if (!b || root != &b->branch_tree)
 		release_tree_entry(root);
 }
-- 
1.7.9.2

[PATCH maint-1.7.6] fast-import: don't allow 'ls' of path with empty components

[Jonathan Nieder]

As the fast-import manual explains:

	The value of <path> must be in canonical form. That is it must
	not:
	. contain an empty directory component (e.g. foo//bar is invalid),
	. end with a directory separator (e.g. foo/ is invalid),
	. start with a directory separator (e.g. /foo is invalid),

Unfortunately the "ls" command accepts these invalid syntaxes and
responds by declaring that the indicated path is missing.  This is too
subtle and causes importers to silently misbehave; better to error out
so the operator knows what's happening.

The C, R, and M commands already error out for such paths.

Based on initial analysis by David Barr.

Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
---
Also against on 8dc6a373d (fast-import: add 'ls' command, 2010-12-02).

 fast-import.c          |    2 ++
 t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 41 insertions(+)

diff --git a/fast-import.c b/fast-import.c
index fff285cd..47f61f3c 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1640,6 +1640,8 @@ static int tree_content_get(
 		n = slash1 - p;
 	else
 		n = strlen(p);
+	if (!n)
+		die("Empty path component found in input");
 
 	if (!root->tree)
 		load_tree(root);
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 6b1ba6c8..2cd0f061 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1088,6 +1088,45 @@ test_expect_success \
 	INPUT_END'
 
+test_expect_success \
+	'N: reject foo/ syntax in copy source' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5C
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	C file2/ file3
+	INPUT_END'
+
+test_expect_success \
+	'N: reject foo/ syntax in rename source' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5D
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	rename with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	R file2/ file3
+	INPUT_END'
+
+test_expect_success \
+	'N: reject foo/ syntax in ls argument' \
+	'test_must_fail git fast-import <<-INPUT_END
+	commit refs/heads/N5E
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy with invalid syntax
+	COMMIT
+
+	from refs/heads/branch^0
+	ls "file2/"
+	INPUT_END'
+
 test_expect_success \
 	'N: copy to root by id and modify' \
 	'echo "hello, world" >expect.foo &&
 	 echo hello >expect.bar &&
-- 
1.7.9.2

[PULL maint] two fast-import "ls" fixes

[Jonathan Nieder]

Jonathan Nieder wrote:

> After sleeping on it, here are two patches for 'maint'.

For your convenience, these changes can also be found at:

  git://repo.or.cz/git/jrn.git tags/fast-import-ls-fixes

      fast-import: leakfix for 'ls' of dirty trees
      fast-import: don't allow 'ls' of path with empty components

 fast-import.c          |    4 ++++
 t/t9300-fast-import.sh |   39 +++++++++++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+)

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

[Jonathan Nieder]

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

[PATCH v3] fast-import: allow 'ls' and filecopy to read the root

[Jonathan Nieder]

In the same spirit as v1.7.4-rc0~177 (fast-import: Allow filemodify to
set the root, 2010-10-10), teach the 'ls' and 'C' commands the
following syntax:

	ls ""
	ls <dataref> ""
	C "" <path>

All three are requests to read from the directory at the top of the
hierarchy.

The potential usefulness of this extension was discovered by using
svn-fe to import from a repository whose history included a
pathological Subversion operation:

  svn cp $SVN_ROOT $SVN_ROOT/subdirectory

Since v1.7.10-rc0~118^2~4^2~5^2~4 (vcs-svn: eliminate repo_tree
structure, 2010-12-10) svn-fe handles this by sending the command
'ls :1 ' to fast-import, expecting output in the form

  '040000' SP 'tree' SP <dataref> HT LF

describing the toplevel directory so it can be copied.  After this
patch, the import works, with no modification to svn-fe needed.

Subtleties:

The 'ls <dataref> ""' command involves printing the makeshift "root"
tree that represents <dataref>, so we need to initialize its mode.

The 'C "" <path>' command needs to be careful not to copy an empty
tree to a subdirectory, as explained in v1.7.4~2^2~2^2 (fast-import:
treat filemodify with empty tree as delete, 2011-01-27).

Based on a patch by David Barr that made the same change at the
parse_ls level.  David's tests were carried over and some new ones
added.

Reported-by: Andrew Sayers <andrew-git@pileofstuff.org>
Signed-off-by: David Barr <davidbarr@google.com>
Signed-off-by: Jonathan Nieder <jrnieder@gmail.com>
Improved-by: Dmitry Ivankov <divanorama@gmail.com>
---
Ok, here's a patch for the svn-fe bug that I could live with.

It has a semantic conflict with the fast-import-ls-fixes series that
I sent separately, which is fixed by adding

			if (!slash1[1])
				die("Empty path component found in input");

after

			if (!slash1)
				goto last_component;

and removing the now-useless

	if (!n)
		die("Empty path component found in input");

I'll send a fixup patch as a reply, for squashing into the merge or
this patch, whichever is the first commit that contains both topics.

 fast-import.c          |   43 ++++++++----
 t/t9010-svn-fe.sh      |   69 +++++++++++++++++++
 t/t9300-fast-import.sh |  174 ++++++++++++++++++++++++++++++++++++++++++++++++
 3 files changed, 272 insertions(+), 14 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index c1486cab..75da2954 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1473,6 +1473,9 @@ static void tree_content_replace(
 	root->tree = newtree;
 }
 
+static int tree_content_remove(struct tree_entry *,
+					const char *, struct tree_entry *);
+
 static int tree_content_set(
 	struct tree_entry *root,
 	const char *p,
@@ -1495,6 +1498,15 @@ static int tree_content_set(
 	if (!slash1 && !S_ISDIR(mode) && subtree)
 		die("Non-directories cannot have subtrees");
 
+	/* Git does not track empty directories. */
+	if (S_ISDIR(mode)) {
+		if ((is_null_sha1(sha1) && !subtree->entry_count)
+		    || !memcmp(sha1, EMPTY_TREE_SHA1_BIN, 20)) {
+			tree_content_remove(root, p, NULL);
+			return 1;
+		}
+	}
+
 	if (!root->tree)
 		load_tree(root);
 	t = root->tree;
@@ -1636,6 +1648,11 @@ static int tree_content_get(
 	unsigned int i, n;
 	struct tree_entry *e;
 
+	if (!*p) {
+		e = root;
+		goto last_component;
+	}
+
 	slash1 = strchr(p, '/');
 	if (slash1)
 		n = slash1 - p;
@@ -1648,14 +1665,8 @@ static int tree_content_get(
 	for (i = 0; i < t->entry_count; i++) {
 		e = t->entries[i];
 		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
-			if (!slash1) {
-				memcpy(leaf, e, sizeof(*leaf));
-				if (e->tree && is_null_sha1(e->versions[1].sha1))
-					leaf->tree = dup_tree_content(e->tree);
-				else
-					leaf->tree = NULL;
-				return 1;
-			}
+			if (!slash1)
+				goto last_component;
 			if (!S_ISDIR(e->versions[1].mode))
 				return 0;
 			if (!e->tree)
@@ -1664,6 +1675,14 @@ static int tree_content_get(
 		}
 	}
 	return 0;
+
+last_component:
+	memcpy(leaf, e, sizeof(*leaf));
+	if (e->tree && is_null_sha1(e->versions[1].sha1))
+		leaf->tree = dup_tree_content(e->tree);
+	else
+		leaf->tree = NULL;
+	return 1;
 }
 
 static int update_branch(struct branch *b)
@@ -2256,12 +2275,6 @@ static void file_change_m(struct branch *b)
 		p = uq.buf;
 	}
 
-	/* Git does not track empty, non-toplevel directories. */
-	if (S_ISDIR(mode) && !memcmp(sha1, EMPTY_TREE_SHA1_BIN, 20) && *p) {
-		tree_content_remove(&b->branch_tree, p, NULL);
-		return;
-	}
-
 	if (S_ISGITLINK(mode)) {
 		if (inline_data)
 			die("Git links cannot be specified 'inline': %s",
@@ -2369,6 +2382,7 @@ static void file_change_cr(struct branch *b, int rename)
 			leaf.tree);
 		return;
 	}
+
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
@@ -3005,6 +3019,7 @@ static void parse_ls(struct branch *b)
 		struct object_entry *e = parse_treeish_dataref(&p);
 		root = new_tree_entry();
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
+		root->versions[1].mode = S_IFDIR;
 		load_tree(root);
 		if (*p++ != ' ')
 			die("Missing space after tree-ish: %s", command_buf.buf);
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/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 438aaf6b..e5460994 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1047,6 +1047,49 @@ test_expect_success \
 	 git diff-tree -C --find-copies-harder -r N1^ N1 >actual &&
 	 compare_diff_raw expect actual'
 
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+copy to subdir
+COMMIT
+
+from refs/heads/branch^0
+C "" subdir
+
+INPUT_END
+
+cat >expect <<\EOF
+:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	subdir/file2/newf
+:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	subdir/file2/oldf
+:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	subdir/file4
+:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	subdir/newdir/exec.sh
+:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	subdir/newdir/interesting
+EOF
+test_expect_success \
+	'N: copy with empty source path' \
+	'git fast-import <input &&
+	 git diff-tree -C -C -r --no-commit-id N-root-to-subdir >actual &&
+	 compare_diff_raw expect actual'
+
+test_tick
+cat >input <<INPUT_END
+commit refs/heads/N-unquoted-root-to-subdir
+committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+data <<COMMIT
+increase nesting
+COMMIT
+
+from refs/heads/branch^0
+C  subdir
+
+INPUT_END
+test_expect_success \
+	'N: copy with unquoted empty source path' \
+	'git fast-import <input &&
+	 git diff --exit-code N-root-to-subdir N-unquoted-root-to-subdir'
+
 cat >input <<INPUT_END
 commit refs/heads/N2
 committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
@@ -1400,6 +1443,137 @@ test_expect_success \
 	 test_cmp expect.qux actual.qux &&
 	 test_cmp expect.qux actual.quux'
 
+test_expect_success 'N: root of unborn branch reads as present and empty' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo "040000 tree $empty_tree	" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-empty
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read empty root directory via ls
+	COMMIT
+
+	ls ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: copying unborn branch root has no effect' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo tree $empty_tree >expect &&
+	git fast-import <<-EOF &&
+	commit refs/heads/N-copy-unborn
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy empty root directory
+	COMMIT
+
+	C "" subdir
+	EOF
+	git cat-file commit N-copy-unborn >cmit &&
+	head -n1 cmit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'N: empty root reads as present and empty' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo "040000 tree $empty_tree	" >expect &&
+	echo empty >msg &&
+	cmit=$(git commit-tree "$empty_tree" -p refs/heads/branch^0 <msg) &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-empty-existing
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read empty root directory via ls
+	COMMIT
+
+	ls ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: copying empty root has no effect' '
+	empty_tree=$(git mktree </dev/null) &&
+	echo tree $empty_tree >expect &&
+	echo empty >msg &&
+	cmit=$(git commit-tree "$empty_tree" -p refs/heads/branch^0 <msg) &&
+	git fast-import <<-EOF &&
+	commit refs/heads/N-copy-empty
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	copy empty root directory
+	COMMIT
+
+	C "" subdir
+	EOF
+	git cat-file commit N-copy-empty >cmit &&
+	head -n1 cmit >actual &&
+	test_cmp expect actual
+'
+
+test_expect_success 'N: "ls" command can read subdir of named tree' '
+	branch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&
+	subdir_tree=$(git rev-parse $branch_cmit:newdir) &&
+	echo "040000 tree $subdir_tree	newdir" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-subdir-of-named-tree
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read from commit with ls
+	COMMIT
+
+	ls $branch_cmit "newdir"
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success 'N: "ls" command can read root of named commit' '
+	branch_cmit=$(git rev-parse --verify refs/heads/branch^0) &&
+	branch_tree=$(git rev-parse --verify $branch_cmit^{tree}) &&
+	echo "040000 tree $branch_tree	" >expect &&
+	git fast-import --cat-blob-fd=3 3>actual <<-EOF &&
+	commit refs/heads/N-root-of-named-tree
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	read root directory of commit with ls
+	COMMIT
+
+	ls $branch_cmit ""
+	EOF
+	test_cmp expect actual
+'
+
+test_expect_success PIPE 'N: read and copy root' '
+	cat >expect <<-\EOF &&
+	:100755 100755 f1fb5da718392694d0076d677d6d0e364c79b0bc f1fb5da718392694d0076d677d6d0e364c79b0bc C100	file2/newf	file3/file2/newf
+	:100644 100644 7123f7f44e39be127c5eb701e5968176ee9d78b1 7123f7f44e39be127c5eb701e5968176ee9d78b1 C100	file2/oldf	file3/file2/oldf
+	:100755 100755 85df50785d62d3b05ab03d9cbf7e4a0b49449730 85df50785d62d3b05ab03d9cbf7e4a0b49449730 C100	file4	file3/file4
+	:100755 100755 e74b7d465e52746be2b4bae983670711e6e66657 e74b7d465e52746be2b4bae983670711e6e66657 C100	newdir/exec.sh	file3/newdir/exec.sh
+	:100644 100644 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 fcf778cda181eaa1cbc9e9ce3a2e15ee9f9fe791 C100	newdir/interesting	file3/newdir/interesting
+	EOF
+	git update-ref -d refs/heads/N12 &&
+	rm -f backflow &&
+	mkfifo backflow &&
+	(
+		exec <backflow &&
+		cat <<-EOF &&
+		commit refs/heads/N12
+		committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+		data <<COMMIT
+		copy root directory by tree hash read via ls
+		COMMIT
+
+		from refs/heads/branch^0
+		ls ""
+		EOF
+		read mode type tree filename &&
+		echo "M 040000 $tree file3"
+	) |
+	git fast-import --cat-blob-fd=3 3>backflow &&
+	git diff-tree -C --find-copies-harder -r N12^ N12 >actual &&
+	compare_diff_raw expect actual
+'
+
 ###
 ### series O
 ###
-- 
1.7.9.2

Re: [PATCH v3] fast-import: allow 'ls' and filecopy to read the root

[Jonathan Nieder]

Jonathan Nieder wrote:

> It has a semantic conflict with the fast-import-ls-fixes series that
> I sent separately
[...]
> I'll send a fixup patch as a reply, for squashing into the merge or
> this patch, whichever is the first commit that contains both topics.

With this tweak, the merge passes the merged set of tests.

diff --git i/fast-import.c c/fast-import.c
index 51cdda29..fe1c8643 100644
--- i/fast-import.c
+++ c/fast-import.c
@@ -1658,8 +1658,6 @@ static int tree_content_get(
 		n = slash1 - p;
 	else
 		n = strlen(p);
-	if (!n)
-		die("Empty path component found in input");
 
 	if (!root->tree)
 		load_tree(root);
@@ -1669,6 +1667,8 @@ static int tree_content_get(
 		if (e->name->str_len == n && !strncmp_icase(p, e->name->str_dat, n)) {
 			if (!slash1)
 				goto last_component;
+			if (!slash1[1])
+				die("Empty path component found in input");
 			if (!S_ISDIR(e->versions[1].mode))
 				return 0;
 			if (!e->tree)

[PATCH 2/1] fixup! fast-import: allow 'ls' and filecopy to read the root

[Jonathan Nieder]

Jonathan Nieder wrote:

> --- a/fast-import.c
> +++ b/fast-import.c
> @@ -2369,6 +2382,7 @@ static void file_change_cr(struct branch *b, int rename)
>  			leaf.tree);
>  		return;
>  	}
> +
>  	tree_content_set(&b->branch_tree, d,
>  		leaf.versions[1].sha1,
>  		leaf.versions[1].mode,

Maybe next time I will send the patch to myself and bounce it to the
list.  Sorry for the noise.

diff --git i/fast-import.c w/fast-import.c
index 51cdda29..013cbd5e 100644
--- i/fast-import.c
+++ w/fast-import.c
@@ -2384,7 +2384,6 @@ static void file_change_cr(struct branch *b, int rename)
 			leaf.tree);
 		return;
 	}
-
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,

Re: [PATCH] fast-import: fix ls command with empty path

[Jonathan Nieder]

Jonathan Nieder wrote:
> Dmitry Ivankov wrote:

>>  One more quick thought. "force the root mode to S_IFDIR" part doesn't
>>  look obviously good for me. First, isn't it already ensured that the
>>  root is a directory?
[...]
> It was just a problematic and incomplete version of what your "be
> saner with temporary trees" does properly. 

To tie up this loose end: looks like David's patch was ok in this
respect and my worries unfounded.  What I was missing is that
store_tree() does nothing unless its tree argument is dirty, and the
temporary tree used to repesent <treeish> in "ls <treeish> <path>" is
never dirty.

Of course, the reminder of the "be saner" patch and the tree delta
discussion was still very useful.

Thanks for your thoughtfulness.
Jonathan

Re: [PATCH] fast-import: fix ls command with empty path

[Dave Abrahams]

Jonathan Nieder <jrnieder <at> gmail.com> writes:

> 
> After sleeping on it, here are two patches for 'maint'.  One plugs a
> memory leak.  The other makes my above comment actually true, so
> trying to use this missing feature results in an error message that
> can help the frontend author instead of the silently broken conversion
> Andrew found.
> 
> Then we can carefully add 'ls ""' support in 1.7.11.

The support for 'ls ""' was nevre actually added, was it?