[PATCH] cache-tree: populate cache-tree on successful merge

[David Turner]

When we unpack trees into an existing index, we discard the old index
and replace it with the new, merged index.  Ensure that this index has
its cache-tree populated.  This will make subsequent git status and
commit commands faster.

Signed-off-by: David Turner dtur...@twopensource.com
Signed-off-by: Brian Degenhardt b...@bmdhacks.com
---

This patch is by my colleague, Brian Degenhardt (as part of his work
on git at Twitter).  I'm sending it with his and Twitter's approval.

---
 t/t0090-cache-tree.sh | 24 
 unpack-trees.c|  7 +++
 2 files changed, 31 insertions(+)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 601d02d..055cc19 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -199,6 +199,30 @@ test_expect_success 'checkout -B gives cache-tree' '
test_cache_tree
 '
 
+test_expect_success 'merge --ff-only maintains cache-tree' '
+   git checkout current 
+   git checkout -b changes 
+   test_commit llamas 
+   test_commit pachyderm 
+   test_cache_tree 
+   git checkout current 
+   test_cache_tree 
+   git merge --ff-only changes 
+   test_cache_tree
+'
+
+test_expect_success 'merge maintains cache-tree' '
+   git checkout current 
+   git checkout -b changes2 
+   test_commit alpacas 
+   test_cache_tree 
+   git checkout current 
+   test_commit struthio 
+   test_cache_tree 
+   git merge changes2 
+   test_cache_tree
+'
+
 test_expect_success 'partial commit gives cache-tree' '
git checkout -b partial no-children 
test_commit one 
diff --git a/unpack-trees.c b/unpack-trees.c
index 2927660..befc247 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
o-src_index = NULL;
ret = check_updates(o) ? (-2) : 0;
if (o-dst_index) {
+   if (!o-result.cache_tree)
+   o-result.cache_tree = cache_tree();
+
+   if (!cache_tree_fully_valid(o-result.cache_tree)) {
+   cache_tree_update(o-result, WRITE_TREE_SILENT | 
WRITE_TREE_REPAIR);
+   }
+
discard_index(o-dst_index);
*o-dst_index = o-result;
} else {
-- 
2.0.4.315.gad8727a-twtrsrc

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

 The work done to produce the cache-tree is work that the commit would
 otherwise have to do.  So we're spending extra time in one place to
 eliminate that work in a different place.

Good point.  Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

Junio C Hamano gits...@pobox.com writes:

 David Turner dtur...@twopensource.com writes:

 The work done to produce the cache-tree is work that the commit would
 otherwise have to do.  So we're spending extra time in one place to
 eliminate that work in a different place.

 Good point.  Thanks.

Hmm, I forgot about another codepath.  What about operations that
are purely done to pouplate the index, without necessarily creating
a tree out of the index?

The most worrisome is git checkout $branch (two-tree merge).
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

 When we unpack trees into an existing index, we discard the old index
 and replace it with the new, merged index.  Ensure that this index has
 its cache-tree populated.  This will make subsequent git status and
 commit commands faster.

Wouldn't it make repeated calls to git merge and friends to build
a long history slower, when the user does not run git status in
between?  E.g. git cherry-pick -4 $other_topic, where you would
not even have a chance to run git status in the middle.  What do
the pros-and-cons look like?
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[David Turner]

On Tue, 2015-07-28 at 12:50 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  When we unpack trees into an existing index, we discard the old index
  and replace it with the new, merged index.  Ensure that this index has
  its cache-tree populated.  This will make subsequent git status and
  commit commands faster.
 
 Wouldn't it make repeated calls to git merge and friends to build
 a long history slower, when the user does not run git status in
 between?  E.g. git cherry-pick -4 $other_topic, where you would
 not even have a chance to run git status in the middle.  What do
 the pros-and-cons look like?

I have not benchmarked, but I suspect it would not make those slower.

The work done to produce the cache-tree is work that the commit would
otherwise have to do.  So we're spending extra time in one place to
eliminate that work in a different place.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

 Git checkout $branch already populates the cache-tree; this is due to
 patches I added last year:

 commit aecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69

Heh, our mails crossed ;-)

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[David Turner]

On Tue, 2015-07-28 at 13:47 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  When we unpack trees into an existing index, we discard the old index
  and replace it with the new, merged index.  Ensure that this index has
  its cache-tree populated.  This will make subsequent git status and
  commit commands faster.
 
  Signed-off-by: David Turner dtur...@twopensource.com
  Signed-off-by: Brian Degenhardt b...@bmdhacks.com
  ---
 
  This patch is by my colleague, Brian Degenhardt (as part of his work
  on git at Twitter).  I'm sending it with his and Twitter's approval.
 
 I'd need to tweak the From:/Author: line then, and flip the order of
 the sign-off, as Brian wrote and signed off then David relayed (as
 attached).

Where do I put an Author: line? In the commit message above the
signoffs?  As an email header?  I didn't see an option to git send-email
that would do this.  I don't want to use the From: header because I want
to be the point-of-contact for these patches.

  diff --git a/unpack-trees.c b/unpack-trees.c
  index 2927660..befc247 100644
  --- a/unpack-trees.c
  +++ b/unpack-trees.c
  @@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
  struct unpack_trees_options
  o-src_index = NULL;
  ret = check_updates(o) ? (-2) : 0;
  if (o-dst_index) {
  +   if (!o-result.cache_tree)
  +   o-result.cache_tree = cache_tree();
  +
  +   if (!cache_tree_fully_valid(o-result.cache_tree)) {
  +   cache_tree_update(o-result, WRITE_TREE_SILENT | 
  WRITE_TREE_REPAIR);
  +   }
 
 This does the cache-tree thing unconditionally, not on successful
 merge.  cache_tree_update() would refuse when it sees an unmerged
 entry, but somehow the discrepancy between the title and the code
 bothers me.
 
 By the way, I wonder if we can lose/revert aecf567c (cache-tree:
 create/update cache-tree on checkout, 2014-07-05), now the
 underlying unpack_trees() does the necessary cache_tree_update()
 when a branch is checked out.

Well, the tests still pass, so I guess so. That is, we still need the
WRITE_TREE_REPAIR bit, but not the update check.

Will re-roll once I hear back on the author line.

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[David Turner]

On Tue, 2015-07-28 at 13:04 -0700, Junio C Hamano wrote:
 Junio C Hamano gits...@pobox.com writes:
 
  David Turner dtur...@twopensource.com writes:
 
  The work done to produce the cache-tree is work that the commit would
  otherwise have to do.  So we're spending extra time in one place to
  eliminate that work in a different place.
 
  Good point.  Thanks.
 
 Hmm, I forgot about another codepath.  What about operations that
 are purely done to pouplate the index, without necessarily creating
 a tree out of the index?
 
 The most worrisome is git checkout $branch (two-tree merge).

Git checkout $branch already populates the cache-tree; this is due to
patches I added last year:

commit aecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69
Author: David Turner dtur...@twopensource.com
Date:   Sat Jul 5 21:06:56 2014 -0700

cache-tree: create/update cache-tree on checkout

When git checkout checks out a branch, create or update the
cache-tree so that subsequent operations are faster.


Admittedly, we do not test for the case where we must do a two-way merge
during a checkout, but I just tested that case, and it appears that we
do already populate the cache-tree in that case.


--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

 When we unpack trees into an existing index, we discard the old index
 and replace it with the new, merged index.  Ensure that this index has
 its cache-tree populated.  This will make subsequent git status and
 commit commands faster.

 Signed-off-by: David Turner dtur...@twopensource.com
 Signed-off-by: Brian Degenhardt b...@bmdhacks.com
 ---

 This patch is by my colleague, Brian Degenhardt (as part of his work
 on git at Twitter).  I'm sending it with his and Twitter's approval.

I'd need to tweak the From:/Author: line then, and flip the order of
the sign-off, as Brian wrote and signed off then David relayed (as
attached).

 diff --git a/unpack-trees.c b/unpack-trees.c
 index 2927660..befc247 100644
 --- a/unpack-trees.c
 +++ b/unpack-trees.c
 @@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
 struct unpack_trees_options
   o-src_index = NULL;
   ret = check_updates(o) ? (-2) : 0;
   if (o-dst_index) {
 + if (!o-result.cache_tree)
 + o-result.cache_tree = cache_tree();
 +
 + if (!cache_tree_fully_valid(o-result.cache_tree)) {
 + cache_tree_update(o-result, WRITE_TREE_SILENT | 
 WRITE_TREE_REPAIR);
 + }

This does the cache-tree thing unconditionally, not on successful
merge.  cache_tree_update() would refuse when it sees an unmerged
entry, but somehow the discrepancy between the title and the code
bothers me.

By the way, I wonder if we can lose/revert aecf567c (cache-tree:
create/update cache-tree on checkout, 2014-07-05), now the
underlying unpack_trees() does the necessary cache_tree_update()
when a branch is checked out.

Thanks.

-- 8 --
From: Brian Degenhardt b...@bmdhacks.com
Date: Tue, 28 Jul 2015 15:30:40 -0400
Subject: [PATCH] unpack-trees: populate cache-tree on successful merge

When we unpack trees into an existing index, we discard the old
index and replace it with the new, merged index.  Ensure that this
index has its cache-tree populated.  This will make subsequent git
status and commit commands faster.

Signed-off-by: Brian Degenhardt b...@bmdhacks.com
Signed-off-by: David Turner dtur...@twopensource.com
Signed-off-by: Junio C Hamano gits...@pobox.com
---
 t/t0090-cache-tree.sh | 24 
 unpack-trees.c|  8 
 2 files changed, 32 insertions(+)

diff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh
index 601d02d..055cc19 100755
--- a/t/t0090-cache-tree.sh
+++ b/t/t0090-cache-tree.sh
@@ -199,6 +199,30 @@ test_expect_success 'checkout -B gives cache-tree' '
test_cache_tree
 '
 
+test_expect_success 'merge --ff-only maintains cache-tree' '
+   git checkout current 
+   git checkout -b changes 
+   test_commit llamas 
+   test_commit pachyderm 
+   test_cache_tree 
+   git checkout current 
+   test_cache_tree 
+   git merge --ff-only changes 
+   test_cache_tree
+'
+
+test_expect_success 'merge maintains cache-tree' '
+   git checkout current 
+   git checkout -b changes2 
+   test_commit alpacas 
+   test_cache_tree 
+   git checkout current 
+   test_commit struthio 
+   test_cache_tree 
+   git merge changes2 
+   test_cache_tree
+'
+
 test_expect_success 'partial commit gives cache-tree' '
git checkout -b partial no-children 
test_commit one 
diff --git a/unpack-trees.c b/unpack-trees.c
index be84ba2..d92f903 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -1155,6 +1155,14 @@ int unpack_trees(unsigned len, struct tree_desc *t, 
struct unpack_trees_options
o-src_index = NULL;
ret = check_updates(o) ? (-2) : 0;
if (o-dst_index) {
+   if (!ret) {
+   if (!o-result.cache_tree)
+   o-result.cache_tree = cache_tree();
+   if (!cache_tree_fully_valid(o-result.cache_tree))
+   cache_tree_update(o-result,
+ WRITE_TREE_SILENT |
+ WRITE_TREE_REPAIR);
+   }
discard_index(o-dst_index);
*o-dst_index = o-result;
} else {
-- 
2.5.0-370-gf964943

--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html

Re: [PATCH] cache-tree: populate cache-tree on successful merge

[Junio C Hamano]

David Turner dtur...@twopensource.com writes:

 On Tue, 2015-07-28 at 13:47 -0700, Junio C Hamano wrote:
 David Turner dtur...@twopensource.com writes:
 
  When we unpack trees into an existing index, we discard the old index
  and replace it with the new, merged index.  Ensure that this index has
  its cache-tree populated.  This will make subsequent git status and
  commit commands faster.
 
  Signed-off-by: David Turner dtur...@twopensource.com
  Signed-off-by: Brian Degenhardt b...@bmdhacks.com
  ---
 
  This patch is by my colleague, Brian Degenhardt (as part of his work
  on git at Twitter).  I'm sending it with his and Twitter's approval.
 
 I'd need to tweak the From:/Author: line then, and flip the order of
 the sign-off, as Brian wrote and signed off then David relayed (as
 attached).

 Where do I put an Author: line? In the commit message above the
 signoffs?  As an email header?  I didn't see an option to git send-email
 that would do this.  I don't want to use the From: header because I want
 to be the point-of-contact for these patches.

The message you are responding to would have been a good example of
forcing the author, subject and author-date to be different from the
e-mail headers.  That is, if you did git am -s -c on my message
you responded to, you would have seen a new commit authored by
Brian; and anybody responding to the message would have sent that
e-mail to me (and git@vger.kernel.org).

I think that is the arrangement you are looking for.

Delete everything before and including the -- 8 -- line from my
message you responded to and then the person who applies does not
have to say -c but just with git am -s the same thing would have
happened.  E-mail coming from (and reply going to) you, but resulting
commit would be authored by Brian.

git send-email, if you are sending somebody else's commit, should
automatically add the in-body header From: Brian ... as the first
line of the body, with a blank line and the body of the commit log.

 By the way, I wonder if we can lose/revert aecf567c (cache-tree:
 create/update cache-tree on checkout, 2014-07-05), now the
 underlying unpack_trees() does the necessary cache_tree_update()
 when a branch is checked out.

 Well, the tests still pass, so I guess so. That is, we still need the
 WRITE_TREE_REPAIR bit, but not the update check.

 Will re-roll once I hear back on the author line.

Let's not do the drop cache-tree generation from checkout in the
same patch.  It can be done as a separate patch but I do not think
it is a very high priority.

With that understanding, what I have received from you (with a minor
tweak shown in the message you are responding to) is already fine, I
think.

Thanks.
--
To unsubscribe from this list: send the line unsubscribe git in
the body of a message to majord...@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html