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

[David Barr <davidbarr@google.com>]

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