[PATCH 0/2] fast-import: allow more operations on root directory

[PATCH 0/2] fast-import: allow more operations on root directory

[Dmitry Ivankov]

[1/2] has been already reviewed in [1] where it didn't seem to have any effect,
but for this series it does, for 'ls ""' command.
So the change is about allowing to "ls" root directory and to copy or rename 
it to some subdirectory. The documentation doesn't deny these, but they don't
work. Make them work.

More details are in [2/2].

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

Dmitry Ivankov (2):
  fast-import: be saner with temporary trees
  fast-import: allow top directory as an argument for some commands

 fast-import.c          |   78 +++++++++++++++++++++++++++++++++++++++++------
 t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++
 2 files changed, 126 insertions(+), 10 deletions(-)

-- 
1.7.3.4

[PATCH 1/2] fast-import: be saner with temporary trees

[Dmitry Ivankov]

new_tree_entry/release_tree_entry manage a stack of tree_entry structs
to use as temporaries. Initializing them is the responsibility of the
caller, both after allocation with xmalloc() when existing temporaries
are exhausted and after used entries are pushed with release_tree_entry.

parse_ls doesn't set root->versions[0] fields, making root an invalid
entry. The problem could arise if store_tree(root) is called. parse_ls
calls store_tree on the node corresponding to the path it is called on.

Do initialize entry->versions[0]. As of now, ls command can not list
the topmost tree so this change is just to avoid surprises in case
things will change around ls or tree_content_get.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
Acked-by: Jonathan Nieder <jrnieder@gmail.com>
---
 fast-import.c |    3 +++
 1 files changed, 3 insertions(+), 0 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 7cc2262..3a0aaad 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -2951,6 +2951,9 @@ 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);
+		root->versions[0].mode = 0;
+		root->versions[1].mode = S_IFDIR;
 		hashcpy(root->versions[1].sha1, e->idx.sha1);
 		load_tree(root);
 		if (*p++ != ' ')
-- 
1.7.3.4

[PATCH 2/2] fast-import: allow top directory as an argument for some commands

[Dmitry Ivankov]

fast-import allows to operate on the toplevel directory. This includes
setting the value by sha1/mark, copying/renaming some subdirectory to
a toplevel one.

But it's not possible to 'ls' it or copy/rename it so somewhere, or
delete it with 'D' command.

For ls it's quite reasonable input so just handle it. The special case
is empty directory, for other empty directories we report 'missing' so
stick with this behaviour with toplevel too.

For copy/rename it looks like a normal operation too so handle it too.
But if it's empty, do nothing - there is nothing to copy/rename and the
destination is nonexistent because toplevel is empty.

For 'D ""' we already have deleteall command so probably it's not worth
adding one more way to do the same thing.

Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
---
 fast-import.c          |   75 +++++++++++++++++++++++++++++++++++++++++------
 t/t9300-fast-import.sh |   58 +++++++++++++++++++++++++++++++++++++
 2 files changed, 123 insertions(+), 10 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 3a0aaad..8643e23 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -1552,6 +1552,31 @@ static int tree_content_set(
 	return 1;
 }
 
+static int tree_content_clear(
+	struct tree_entry *root,
+	struct tree_entry *backup_leaf)
+{
+	if (backup_leaf)
+		memcpy(backup_leaf, root, sizeof(*backup_leaf));
+	else if (root->tree)
+		release_tree_content_recursive(root->tree);
+	root->tree = NULL;
+	hashclr(root->versions[1].sha1);
+	return 1;
+}
+
+static int is_empty_tree(struct tree_entry *root)
+{
+	int n;
+	if (!root->tree)
+		load_tree(root);
+	for (n = 0; n < root->tree->entry_count; n++) {
+		if (root->tree->entries[n]->versions[1].mode)
+			return 0;
+	}
+	return 1;
+}
+
 static int tree_content_remove(
 	struct tree_entry *root,
 	const char *p,
@@ -1587,11 +1612,9 @@ static int tree_content_remove(
 			if (!e->tree)
 				load_tree(e);
 			if (tree_content_remove(e, slash1 + 1, backup_leaf)) {
-				for (n = 0; n < e->tree->entry_count; n++) {
-					if (e->tree->entries[n]->versions[1].mode) {
-						hashclr(root->versions[1].sha1);
-						return 1;
-					}
+				if (!is_empty_tree(e)) {
+					hashclr(root->versions[1].sha1);
+					return 1;
 				}
 				backup_leaf = NULL;
 				goto del_entry;
@@ -1613,6 +1636,19 @@ del_entry:
 	return 1;
 }
 
+static int tree_content_copy_root(
+	struct tree_entry *root,
+	struct tree_entry *leaf
+)
+{
+	memcpy(leaf, root, sizeof(*leaf));
+	if (root->tree && is_null_sha1(root->versions[1].sha1))
+		leaf->tree = dup_tree_content(root->tree);
+	else
+		leaf->tree = NULL;
+	return 1;
+}
+
 static int tree_content_get(
 	struct tree_entry *root,
 	const char *p,
@@ -2329,10 +2365,17 @@ static void file_change_cr(struct branch *b, int rename)
 	}
 
 	memset(&leaf, 0, sizeof(leaf));
-	if (rename)
-		tree_content_remove(&b->branch_tree, s, &leaf);
-	else
-		tree_content_get(&b->branch_tree, s, &leaf);
+	if (rename) {
+		if (!*s)
+			tree_content_clear(&b->branch_tree, &leaf);
+		else
+			tree_content_remove(&b->branch_tree, s, &leaf);
+	} else {
+		if (!*s)
+			tree_content_copy_root(&b->branch_tree, &leaf);
+		else
+			tree_content_get(&b->branch_tree, s, &leaf);
+	}
 	if (!leaf.versions[1].mode)
 		die("Path %s not in branch", s);
 	if (!*d) {	/* C "path/to/subdir" "" */
@@ -2342,6 +2385,14 @@ static void file_change_cr(struct branch *b, int rename)
 			leaf.tree);
 		return;
 	}
+	/*
+	 * Git does not track empty, non-toplevel directories.
+	 * At this point destination is non-toplevel, so if source is
+	 * toplevel and empty, leave it as is all empty.
+	 */
+	if (!*s && is_empty_tree(&leaf)) {
+		return;
+	}
 	tree_content_set(&b->branch_tree, d,
 		leaf.versions[1].sha1,
 		leaf.versions[1].mode,
@@ -2969,7 +3020,11 @@ 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) {
+		if (!is_empty_tree(root))
+			tree_content_copy_root(root, &leaf);
+	} else
+		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 f256475..0e46cf0 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -978,6 +978,64 @@ test_expect_success PIPE 'N: empty directory reads as missing' '
 '
 
 test_expect_success \
+	'N: ls root directory' \
+	'cat >expect <<-\EOF &&
+	missing
+	040000
+	EOF
+	cat >input <<-EOF &&
+	commit refs/heads/ls-root
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	On empty tree: ls add file ls.
+	COMMIT
+	ls ""
+	M 644 inline file1
+	data 0
+	ls ""
+	EOF
+	git fast-import --quiet <input >tmp &&
+	cat tmp | cut -d " " -f 1 >actual &&
+	test_cmp expect actual'
+
+test_expect_success \
+	'N: move root directory' \
+	'echo "root/a/b" >expect &&
+	cat >input <<-EOF &&
+	commit refs/heads/move-root
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	Add stuff and move it.
+	COMMIT
+	M 644 inline a/b
+	data 0
+	R "" root
+	EOF
+	git fast-import <input &&
+	git ls-tree -r --name-only refs/heads/move-root >actual &&
+	test_cmp expect actual'
+
+test_expect_success \
+	'N: copy root directory' \
+	'cat <<-\EOF >expect &&
+	a/b
+	a/root/a/b
+	EOF
+	cat >input <<-EOF &&
+	commit refs/heads/copy-root
+	committer $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> $GIT_COMMITTER_DATE
+	data <<COMMIT
+	Add stuff and move it.
+	COMMIT
+	M 644 inline a/b
+	data 0
+	C "" a/root
+	EOF
+	git fast-import <input &&
+	git ls-tree -r --name-only refs/heads/copy-root >actual &&
+	test_cmp expect actual'
+
+test_expect_success \
 	'N: copy root directory by tree hash' \
 	'cat >expect <<-\EOF &&
 	:100755 000000 f1fb5da718392694d0076d677d6d0e364c79b0bc 0000000000000000000000000000000000000000 D	file3/newf
-- 
1.7.3.4

Re: [PATCH 1/2] fast-import: be saner with temporary trees

[Jonathan Nieder]

Hi,

Dmitry Ivankov wrote:

> new_tree_entry/release_tree_entry manage a stack of tree_entry structs
> to use as temporaries. Initializing them is the responsibility of the
> caller,
[...]
> Do initialize entry->versions[0]. As of now, ls command can not list
> the topmost tree so this change is just to avoid surprises in case
> things will change around ls or tree_content_get.
>
> Signed-off-by: Dmitry Ivankov <divanorama@gmail.com>
> Acked-by: Jonathan Nieder <jrnieder@gmail.com>
[...]
> +++ b/fast-import.c
> @@ -2951,6 +2951,9 @@ 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);
> +		root->versions[0].mode = 0;
> +		root->versions[1].mode = S_IFDIR;
>  		hashcpy(root->versions[1].sha1, e->idx.sha1);

I don't remember what this patch was about or why I acked it, which is
a bad sign.  (If the above explanation doesn't make me want to ack it
again, how is it going to help someone in the future understand the
harm in reverting this patch if some other bug bisects to it?)

I think part of the problem is that the explanation seems to be
phrased in terms of implementation details (root->versions[0] fields,
calling store_tree, initializing entry->versions[0]) instead of the
higher-level logic that these implement.

It sounds ("avoid surprises") like this is meant to avoid scary
behavior from the programmer's perspective, so: what are the symptoms
without this patch, and why is this a good way to eliminate them?

On the other hand, if we squash this in with David's [1], it starts
to make perfect sense.  The explanation can say:

	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.

	In the 'ls <dataref> ""' case, this requires some care, because
	parse_ls currently uses an uninitialized temporary tree entry
	from the new_tree_entry/release_tree_entry stack in a quick and
	dirty way without initializing its fields well enough for
	functions like store_tree that might calculate its tree name.

Sensible?  Alternatively, we could rush in David's [1] with the
tweak below to dodge the 'ls <dataref> ""' case, and include your fix
as part of a patch that just addresses that case.

Thanks very much for the patch and the reminder.

Sincerely,
Jonathan

[1] http://thread.gmane.org/gmane.comp.version-control.git/192517/focus=192520
---
 fast-import.c          |    2 +-
 t/t9300-fast-import.sh |   66 +++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 66 insertions(+), 2 deletions(-)

diff --git a/fast-import.c b/fast-import.c
index 8dbfd4cc..2748ced2 100644
--- a/fast-import.c
+++ b/fast-import.c
@@ -3019,7 +3019,7 @@ static void parse_ls(struct branch *b)
 			die("Garbage after path in: %s", command_buf.buf);
 		p = uq.buf;
 	}
-	if (*p) {
+	if (*p || root != &b->branch_tree) {
 		tree_content_get(root, p, &leaf);
 	} else {
 		leaf = *root;
diff --git a/t/t9300-fast-import.sh b/t/t9300-fast-import.sh
index 2558a2ed..2f411fbb 100755
--- a/t/t9300-fast-import.sh
+++ b/t/t9300-fast-import.sh
@@ -1400,8 +1400,72 @@ 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_failure '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:) &&
+	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 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
+	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
-- 
1.7.9.2