David Barr wrote:
> On Fri, Mar 9, 2012 at 7:33 AM, Jonathan Nieder <jrnieder@xxxxxxxxx> 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@xxxxxxxxxx>
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@xxxxxxxxx>
---
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
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html