{"thread":{"id":"39958","subject":"[PATCH] cache-tree: populate cache-tree on successful merge","startedAt":"2015-07-28T19:30:40Z","lastAt":"2015-07-28T21:38:18Z","messageCount":10,"participants":["David Turner","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"266971","messageId":"1438111840-6403-1-git-send-email-dturner@twopensource.com","threadId":"39958","inReplyTo":null,"subject":"[PATCH] cache-tree: populate cache-tree on successful merge","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-28T19:30:40Z","receivedAt":"2015-07-28T19:30:40Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"When we unpack trees into an existing index, we discard the old index\nand replace it with the new, merged index. Ensure that this index has\nits cache-tree populated. This will make subsequent git status and\ncommit commands faster.\n\nSigned-off-by: David Turner \nSigned-off-by: Brian Degenhardt \n---\n\nThis patch is by my colleague, Brian Degenhardt (as part of his work\non git at Twitter). I'm sending it with his and Twitter's approval.\n\n---\n t/t0090-cache-tree.sh | 24 ++++++++++++++++++++++++\n unpack-trees.c | 7 +++++++\n 2 files changed, 31 insertions(+)\n\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 601d02d..055cc19 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -199,6 +199,30 @@ test_expect_success 'checkout -B gives cache-tree' '\n \ttest_cache_tree\n '\n \n+test_expect_success 'merge --ff-only maintains cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -b changes &&\n+\ttest_commit llamas &&\n+\ttest_commit pachyderm &&\n+\ttest_cache_tree &&\n+\tgit checkout current &&\n+\ttest_cache_tree &&\n+\tgit merge --ff-only changes &&\n+\ttest_cache_tree\n+'\n+\n+test_expect_success 'merge maintains cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -b changes2 &&\n+\ttest_commit alpacas &&\n+\ttest_cache_tree &&\n+\tgit checkout current &&\n+\ttest_commit struthio &&\n+\ttest_cache_tree &&\n+\tgit merge changes2 &&\n+\ttest_cache_tree\n+'\n+\n test_expect_success 'partial commit gives cache-tree' '\n \tgit checkout -b partial no-children &&\n \ttest_commit one &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex 2927660..befc247 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n \tif (o->dst_index) {\n+\t\tif (!o->result.cache_tree)\n+\t\t\to->result.cache_tree = cache_tree();\n+\n+\t\tif (!cache_tree_fully_valid(o->result.cache_tree)) {\n+\t\t\tcache_tree_update(&o->result, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n+\t\t}\n+\n \t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n \t} else {\n-- \n2.0.4.315.gad8727a-twtrsrc\n"},{"id":"266973","messageId":"xmqqa8ugdpu7.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"1438111840-6403-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T19:50:08Z","receivedAt":"2015-07-28T19:50:08Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> When we unpack trees into an existing index, we discard the old index\n> and replace it with the new, merged index. Ensure that this index has\n> its cache-tree populated. This will make subsequent git status and\n> commit commands faster.\n\nWouldn't it make repeated calls to \"git merge\" and friends to build\na long history slower, when the user does not run \"git status\" in\nbetween? E.g. \"git cherry-pick -4 $other_topic\", where you would\nnot even have a chance to run \"git status\" in the middle. What do\nthe pros-and-cons look like?\n"},{"id":"266974","messageId":"1438113266.18134.26.camel@twopensource.com","threadId":"39958","inReplyTo":"xmqqa8ugdpu7.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-28T19:54:26Z","receivedAt":"2015-07-28T19:54:26Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-07-28 at 12:50 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > When we unpack trees into an existing index, we discard the old index\n> > and replace it with the new, merged index. Ensure that this index has\n> > its cache-tree populated. This will make subsequent git status and\n> > commit commands faster.\n> \n> Wouldn't it make repeated calls to \"git merge\" and friends to build\n> a long history slower, when the user does not run \"git status\" in\n> between? E.g. \"git cherry-pick -4 $other_topic\", where you would\n> not even have a chance to run \"git status\" in the middle. What do\n> the pros-and-cons look like?\n\nI have not benchmarked, but I suspect it would not make those slower.\n\nThe work done to produce the cache-tree is work that the commit would\notherwise have to do. So we're spending extra time in one place to\neliminate that work in a different place.\n"},{"id":"266975","messageId":"xmqq6154dpkt.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"1438113266.18134.26.camel@twopensource.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T19:55:46Z","receivedAt":"2015-07-28T19:55:46Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> The work done to produce the cache-tree is work that the commit would\n> otherwise have to do. So we're spending extra time in one place to\n> eliminate that work in a different place.\n\nGood point. Thanks.\n"},{"id":"266979","messageId":"xmqq1tfsdp60.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"xmqq6154dpkt.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T20:04:39Z","receivedAt":"2015-07-28T20:04:39Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> David Turner writes:\n>\n>> The work done to produce the cache-tree is work that the commit would\n>> otherwise have to do. So we're spending extra time in one place to\n>> eliminate that work in a different place.\n>\n> Good point. Thanks.\n\nHmm, I forgot about another codepath. What about operations that\nare purely done to pouplate the index, without necessarily creating\na tree out of the index?\n\nThe most worrisome is \"git checkout $branch\" (two-tree merge).\n"},{"id":"266990","messageId":"1438115304.18134.33.camel@twopensource.com","threadId":"39958","inReplyTo":"xmqq1tfsdp60.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-28T20:28:24Z","receivedAt":"2015-07-28T20:28:24Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-07-28 at 13:04 -0700, Junio C Hamano wrote:\n> Junio C Hamano writes:\n> \n> > David Turner writes:\n> >\n> >> The work done to produce the cache-tree is work that the commit would\n> >> otherwise have to do. So we're spending extra time in one place to\n> >> eliminate that work in a different place.\n> >\n> > Good point. Thanks.\n> \n> Hmm, I forgot about another codepath. What about operations that\n> are purely done to pouplate the index, without necessarily creating\n> a tree out of the index?\n> \n> The most worrisome is \"git checkout $branch\" (two-tree merge).\n\nGit checkout $branch already populates the cache-tree; this is due to\npatches I added last year:\n\ncommit aecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69\nAuthor: David Turner \nDate: Sat Jul 5 21:06:56 2014 -0700\n\n cache-tree: create/update cache-tree on checkout\n \n When git checkout checks out a branch, create or update the\n cache-tree so that subsequent operations are faster.\n----\n\nAdmittedly, we do not test for the case where we must do a two-way merge\nduring a checkout, but I just tested that case, and it appears that we\ndo already populate the cache-tree in that case.\n"},{"id":"266995","messageId":"xmqqsi88c8m9.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"1438111840-6403-1-git-send-email-dturner@twopensource.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T20:47:26Z","receivedAt":"2015-07-28T20:47:26Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> When we unpack trees into an existing index, we discard the old index\n> and replace it with the new, merged index. Ensure that this index has\n> its cache-tree populated. This will make subsequent git status and\n> commit commands faster.\n>\n> Signed-off-by: David Turner \n> Signed-off-by: Brian Degenhardt \n> ---\n>\n> This patch is by my colleague, Brian Degenhardt (as part of his work\n> on git at Twitter). I'm sending it with his and Twitter's approval.\n\nI'd need to tweak the From:/Author: line then, and flip the order of\nthe sign-off, as Brian wrote and signed off then David relayed (as\nattached).\n\n> diff --git a/unpack-trees.c b/unpack-trees.c\n> index 2927660..befc247 100644\n> --- a/unpack-trees.c\n> +++ b/unpack-trees.c\n> @@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> \to->src_index = NULL;\n> \tret = check_updates(o) ? (-2) : 0;\n> \tif (o->dst_index) {\n> +\t\tif (!o->result.cache_tree)\n> +\t\t\to->result.cache_tree = cache_tree();\n> +\n> +\t\tif (!cache_tree_fully_valid(o->result.cache_tree)) {\n> +\t\t\tcache_tree_update(&o->result, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> +\t\t}\n\nThis does the cache-tree thing unconditionally, not \"on successful\nmerge\". cache_tree_update() would refuse when it sees an unmerged\nentry, but somehow the discrepancy between the title and the code\nbothers me.\n\nBy the way, I wonder if we can lose/revert aecf567c (cache-tree:\ncreate/update cache-tree on checkout, 2014-07-05), now the\nunderlying unpack_trees() does the necessary cache_tree_update()\nwhen a branch is checked out.\n\nThanks.\n\n-- >8 --\nFrom: Brian Degenhardt \nDate: Tue, 28 Jul 2015 15:30:40 -0400\nSubject: [PATCH] unpack-trees: populate cache-tree on successful merge\n\nWhen we unpack trees into an existing index, we discard the old\nindex and replace it with the new, merged index. Ensure that this\nindex has its cache-tree populated. This will make subsequent git\nstatus and commit commands faster.\n\nSigned-off-by: Brian Degenhardt \nSigned-off-by: David Turner \nSigned-off-by: Junio C Hamano \n---\n t/t0090-cache-tree.sh | 24 ++++++++++++++++++++++++\n unpack-trees.c | 8 ++++++++\n 2 files changed, 32 insertions(+)\n\ndiff --git a/t/t0090-cache-tree.sh b/t/t0090-cache-tree.sh\nindex 601d02d..055cc19 100755\n--- a/t/t0090-cache-tree.sh\n+++ b/t/t0090-cache-tree.sh\n@@ -199,6 +199,30 @@ test_expect_success 'checkout -B gives cache-tree' '\n \ttest_cache_tree\n '\n \n+test_expect_success 'merge --ff-only maintains cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -b changes &&\n+\ttest_commit llamas &&\n+\ttest_commit pachyderm &&\n+\ttest_cache_tree &&\n+\tgit checkout current &&\n+\ttest_cache_tree &&\n+\tgit merge --ff-only changes &&\n+\ttest_cache_tree\n+'\n+\n+test_expect_success 'merge maintains cache-tree' '\n+\tgit checkout current &&\n+\tgit checkout -b changes2 &&\n+\ttest_commit alpacas &&\n+\ttest_cache_tree &&\n+\tgit checkout current &&\n+\ttest_commit struthio &&\n+\ttest_cache_tree &&\n+\tgit merge changes2 &&\n+\ttest_cache_tree\n+'\n+\n test_expect_success 'partial commit gives cache-tree' '\n \tgit checkout -b partial no-children &&\n \ttest_commit one &&\ndiff --git a/unpack-trees.c b/unpack-trees.c\nindex be84ba2..d92f903 100644\n--- a/unpack-trees.c\n+++ b/unpack-trees.c\n@@ -1155,6 +1155,14 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n \to->src_index = NULL;\n \tret = check_updates(o) ? (-2) : 0;\n \tif (o->dst_index) {\n+\t\tif (!ret) {\n+\t\t\tif (!o->result.cache_tree)\n+\t\t\t\to->result.cache_tree = cache_tree();\n+\t\t\tif (!cache_tree_fully_valid(o->result.cache_tree))\n+\t\t\t\tcache_tree_update(&o->result,\n+\t\t\t\t\t\t WRITE_TREE_SILENT |\n+\t\t\t\t\t\t WRITE_TREE_REPAIR);\n+\t\t}\n \t\tdiscard_index(o->dst_index);\n \t\t*o->dst_index = o->result;\n \t} else {\n-- \n2.5.0-370-gf964943\n"},{"id":"266996","messageId":"xmqqoaiwc831.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"1438115304.18134.33.camel@twopensource.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T20:58:58Z","receivedAt":"2015-07-28T20:58:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> Git checkout $branch already populates the cache-tree; this is due to\n> patches I added last year:\n>\n> commit aecf567cbfb6ab46e82f7f5df36fb6a2dd5bee69\n\nHeh, our mails crossed ;-)\n\nThanks.\n"},{"id":"267003","messageId":"1438118327.18134.42.camel@twopensource.com","threadId":"39958","inReplyTo":"xmqqsi88c8m9.fsf@gitster.dls.corp.google.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"David Turner","fromEmail":"dturner@twopensource.com","sentAt":"2015-07-28T21:18:47Z","receivedAt":"2015-07-28T21:18:47Z","isPatch":true,"sender":{"key":"novalis@novalis.org","avatar":"https://avatars.githubusercontent.com/u/77003?v=4"},"body":"On Tue, 2015-07-28 at 13:47 -0700, Junio C Hamano wrote:\n> David Turner writes:\n> \n> > When we unpack trees into an existing index, we discard the old index\n> > and replace it with the new, merged index. Ensure that this index has\n> > its cache-tree populated. This will make subsequent git status and\n> > commit commands faster.\n> >\n> > Signed-off-by: David Turner \n> > Signed-off-by: Brian Degenhardt \n> > ---\n> >\n> > This patch is by my colleague, Brian Degenhardt (as part of his work\n> > on git at Twitter). I'm sending it with his and Twitter's approval.\n> \n> I'd need to tweak the From:/Author: line then, and flip the order of\n> the sign-off, as Brian wrote and signed off then David relayed (as\n> attached).\n\nWhere do I put an Author: line? In the commit message above the\nsignoffs? As an email header? I didn't see an option to git send-email\nthat would do this. I don't want to use the From: header because I want\nto be the point-of-contact for these patches.\n\n> > diff --git a/unpack-trees.c b/unpack-trees.c\n> > index 2927660..befc247 100644\n> > --- a/unpack-trees.c\n> > +++ b/unpack-trees.c\n> > @@ -1156,6 +1156,13 @@ int unpack_trees(unsigned len, struct tree_desc *t, struct unpack_trees_options\n> > \to->src_index = NULL;\n> > \tret = check_updates(o) ? (-2) : 0;\n> > \tif (o->dst_index) {\n> > +\t\tif (!o->result.cache_tree)\n> > +\t\t\to->result.cache_tree = cache_tree();\n> > +\n> > +\t\tif (!cache_tree_fully_valid(o->result.cache_tree)) {\n> > +\t\t\tcache_tree_update(&o->result, WRITE_TREE_SILENT | WRITE_TREE_REPAIR);\n> > +\t\t}\n> \n> This does the cache-tree thing unconditionally, not \"on successful\n> merge\". cache_tree_update() would refuse when it sees an unmerged\n> entry, but somehow the discrepancy between the title and the code\n> bothers me.\n> \n> By the way, I wonder if we can lose/revert aecf567c (cache-tree:\n> create/update cache-tree on checkout, 2014-07-05), now the\n> underlying unpack_trees() does the necessary cache_tree_update()\n> when a branch is checked out.\n\nWell, the tests still pass, so I guess so. That is, we still need the\nWRITE_TREE_REPAIR bit, but not the update check.\n\nWill re-roll once I hear back on the author line.\n"},{"id":"267005","messageId":"xmqqbnewc69h.fsf@gitster.dls.corp.google.com","threadId":"39958","inReplyTo":"1438118327.18134.42.camel@twopensource.com","subject":"Re: [PATCH] cache-tree: populate cache-tree on successful merge","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2015-07-28T21:38:18Z","receivedAt":"2015-07-28T21:38:18Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"David Turner writes:\n\n> On Tue, 2015-07-28 at 13:47 -0700, Junio C Hamano wrote:\n>> David Turner writes:\n>> \n>> > When we unpack trees into an existing index, we discard the old index\n>> > and replace it with the new, merged index. Ensure that this index has\n>> > its cache-tree populated. This will make subsequent git status and\n>> > commit commands faster.\n>> >\n>> > Signed-off-by: David Turner \n>> > Signed-off-by: Brian Degenhardt \n>> > ---\n>> >\n>> > This patch is by my colleague, Brian Degenhardt (as part of his work\n>> > on git at Twitter). I'm sending it with his and Twitter's approval.\n>> \n>> I'd need to tweak the From:/Author: line then, and flip the order of\n>> the sign-off, as Brian wrote and signed off then David relayed (as\n>> attached).\n>\n> Where do I put an Author: line? In the commit message above the\n> signoffs? As an email header? I didn't see an option to git send-email\n> that would do this. I don't want to use the From: header because I want\n> to be the point-of-contact for these patches.\n\nThe message you are responding to would have been a good example of\nforcing the author, subject and author-date to be different from the\ne-mail headers. That is, if you did \"git am -s -c\" on my message\nyou responded to, you would have seen a new commit authored by\nBrian; and anybody responding to the message would have sent that\ne-mail to me (and git@vger.kernel.org).\n\nI think that is the arrangement you are looking for.\n\nDelete everything before and including the \"-- >8 --\" line from my\nmessage you responded to and then the person who applies does not\nhave to say \"-c\" but just with \"git am -s\" the same thing would have\nhappened. E-mail coming from (and reply going to) you, but resulting\ncommit would be authored by Brian.\n\n\"git send-email\", if you are sending somebody else's commit, should\nautomatically add the in-body header \"From: Brian ...\" as the first\nline of the body, with a blank line and the body of the commit log.\n\n>> By the way, I wonder if we can lose/revert aecf567c (cache-tree:\n>> create/update cache-tree on checkout, 2014-07-05), now the\n>> underlying unpack_trees() does the necessary cache_tree_update()\n>> when a branch is checked out.\n>\n> Well, the tests still pass, so I guess so. That is, we still need the\n> WRITE_TREE_REPAIR bit, but not the update check.\n>\n> Will re-roll once I hear back on the author line.\n\nLet's not do the \"drop cache-tree generation from checkout\" in the\nsame patch. It can be done as a separate patch but I do not think\nit is a very high priority.\n\nWith that understanding, what I have received from you (with a minor\ntweak shown in the message you are responding to) is already fine, I\nthink.\n\nThanks.\n"}]}