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

[Jonathan Nieder <jrnieder@gmail.com>]

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