{"thread":{"id":"41219","subject":"git filter-branch not removing commits when it should in 2.7.0","startedAt":"2016-01-19T20:48:40Z","lastAt":"2016-01-20T04:14:48Z","messageCount":19,"participants":["John Fultz","Junio C Hamano","Jeff King","Jonathan Nieder"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"276362","messageId":"AF975DD2-988F-47A8-BFC3-3BBC27419305@wolfram.com","threadId":"41219","inReplyTo":null,"subject":"git filter-branch not removing commits when it should in 2.7.0","fromName":"John Fultz","fromEmail":"jfultz@wolfram.com","sentAt":"2016-01-19T20:48:40Z","receivedAt":"2016-01-19T20:48:40Z","isPatch":false,"sender":{"key":"jfultz@wolfram.com","avatar":null},"body":"This seems to be a 2.7.0 regression in filter-branch. The bug is reproducible on Mac/Windows (haven't tried Linux) in the 2.7.0 production releases.\n\nMake an empty repo and put an empty commit in the history. E.g.,\n\necho > foo && git add . && git commit -m \"commit 1\" && git commit --allow-empty -m \"commit 2\"\n\nNow try to use filter-branch to remove the empty commit. Both of the following methods leave master unchanged, but both worked in 2.6.4:\n\ngit filter-branch --prune-empty\ngit filter-branch --commit-filter 'git_commit_non_empty_tree \"$@\"'\n\nLet me know if you need any more information. Thanks.\n\nSincerely,\n\nJohn Fultz\njfultz@wolfram.com\nUser Interface Group\nWolfram Research, Inc.\n"},{"id":"276365","messageId":"xmqqbn8h9squ.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"AF975DD2-988F-47A8-BFC3-3BBC27419305@wolfram.com","subject":"Re: git filter-branch not removing commits when it should in 2.7.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T21:14:33Z","receivedAt":"2016-01-19T21:14:33Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"John Fultz writes:\n\n> This seems to be a 2.7.0 regression in filter-branch. The bug is reproducible on Mac/Windows (haven't tried Linux) in the 2.7.0 production releases.\n>\n> Make an empty repo and put an empty commit in the history. E.g.,\n>\n> echo > foo && git add . && git commit -m \"commit 1\" && git commit --allow-empty -m \"commit 2\"\n>\n> Now try to use filter-branch to remove the empty commit. Both of the following methods leave master unchanged, but both worked in 2.6.4:\n>\n> git filter-branch --prune-empty\n> git filter-branch --commit-filter 'git_commit_non_empty_tree \"$@\"'\n>\n> Let me know if you need any more information. Thanks.\n>\n> Sincerely,\n>\n> John Fultz\n> jfultz@wolfram.com\n> User Interface Group\n> Wolfram Research, Inc.\n\nThanks.\n\nSince there were only 5 changes to git-filter-branch.sh between\nv2.6.0 and v2.7.0, it was fairly easy to pinpoint.\n\nReverting the following commit from v2.7.0 seems to give the same\nresult as v2.6.0 for \"--prune-empty\" experiment.\n\ncommit 348d4f2fc5d3c4f7ba47079b96676b4e2dd831fc\nAuthor: Jeff King \nDate: Fri Nov 6 01:24:29 2015 -0500\n\n filter-branch: skip index read/write when possible\n \n If the user specifies an index filter but not a tree filter,\n filter-branch cleverly avoids checking out the tree\n entirely. But we don't do the next level of optimization: if\n you have no index or tree filter, we do not need to read the\n index at all.\n \n This can greatly speed up cases where we are only changing\n the commit objects (e.g., cementing a graft into place).\n Here are numbers from the newly-added perf test:\n \n Test HEAD^ HEAD\n ---------------------------------------------------------------\n 7000.2: noop filter 13.81(4.95+0.83) 5.43(0.42+0.43) -60.7%\n \n Signed-off-by: Jeff King \n Signed-off-by: Junio C Hamano \n"},{"id":"276366","messageId":"xmqq7fj59rs2.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"xmqqbn8h9squ.fsf@gitster.mtv.corp.google.com","subject":"Re: git filter-branch not removing commits when it should in 2.7.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T21:35:25Z","receivedAt":"2016-01-19T21:35:25Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> John Fultz writes:\n>\n>> This seems to be a 2.7.0 regression in filter-branch. The bug is reproducible on Mac/Windows (haven't tried Linux) in the 2.7.0 production releases.\n>>\n>> Make an empty repo and put an empty commit in the history. E.g.,\n>>\n>> echo > foo && git add . && git commit -m \"commit 1\" && git commit --allow-empty -m \"commit 2\"\n>>\n>> Now try to use filter-branch to remove the empty commit. Both of the following methods leave master unchanged, but both worked in 2.6.4:\n>>\n>> git filter-branch --prune-empty\n>> git filter-branch --commit-filter 'git_commit_non_empty_tree \"$@\"'\n>\n> Thanks.\n>\n> Since there were only 5 changes to git-filter-branch.sh between\n> v2.6.0 and v2.7.0, it was fairly easy to pinpoint.\n>\n> Reverting the following commit from v2.7.0 seems to give the same\n> result as v2.6.0 for \"--prune-empty\" experiment.\n>\n> commit 348d4f2fc5d3c4f7ba47079b96676b4e2dd831fc\n> Author: Jeff King \n> Date: Fri Nov 6 01:24:29 2015 -0500\n>\n> filter-branch: skip index read/write when possible\n> \n> If the user specifies an index filter but not a tree filter,\n> filter-branch cleverly avoids checking out the tree\n> entirely. But we don't do the next level of optimization: if\n> you have no index or tree filter, we do not need to read the\n> index at all.\n> \n> This can greatly speed up cases where we are only changing\n> the commit objects (e.g., cementing a graft into place).\n> Here are numbers from the newly-added perf test:\n> \n> Test HEAD^ HEAD\n> ---------------------------------------------------------------\n> 7000.2: noop filter 13.81(4.95+0.83) 5.43(0.42+0.43) -60.7%\n> \n> Signed-off-by: Jeff King \n> Signed-off-by: Junio C Hamano \n\nOK, is this because git_commit_non_empty_tree() does this to decide\nthat it should skip the commit:\n\ngit_commit_non_empty_tree()\n{\n if test $# = 3 && test \"$1\" = $(git rev-parse \"$3^{tree}\");\n then\n map \"$3\"\n else\n git commit-tree \"$@\"\n fi\n}\n\nwhere its parameters when --prune-empty is in use (or when the\nfunction is used as the commit-filter), $1 is \"$tree\", $2 and $3 are\n\"-p\" and its sole commit object name $commit (which is read from the\nrevs file, which is an output from rev-list, so it is known to be\n40-hex) and tree after the said patch is computed like so:\n\n if test -n \"$need_index\"\n then\n tree=$(git write-tree)\n else\n tree=\"$commit^{tree}\"\n fi\n\ni.e. the helper does textual comparison between \"$FOURTY_HEX^{tree}\"\nand 40-hex from rev-parse \"$3^{tree}\"?\n\nIn other words, would the fix be a one-liner like this?\n\n git-filter-branch.sh | 2 +-\n 1 file changed, 1 insertion(+), 1 deletion(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 98f1779..86b2ff1 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -404,7 +404,7 @@ while read commit parents; do\n \tthen\n \t\ttree=$(git write-tree)\n \telse\n-\t\ttree=\"$commit^{tree}\"\n+\t\ttree=$(git rev-parse \"$commit^{tree}\")\n \tfi\n \tworkdir=$workdir @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n \t\t\"$tree\" $parentstr < ../message > ../map/$commit ||\n"},{"id":"276367","messageId":"20160119213705.GA28656@sigill.intra.peff.net","threadId":"41219","inReplyTo":"xmqq7fj59rs2.fsf@gitster.mtv.corp.google.com","subject":"Re: git filter-branch not removing commits when it should in 2.7.0","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T21:37:05Z","receivedAt":"2016-01-19T21:37:05Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 01:35:25PM -0800, Junio C Hamano wrote:\n\n> In other words, would the fix be a one-liner like this?\n> [...]\n> -\t\ttree=\"$commit^{tree}\"\n> +\t\ttree=$(git rev-parse \"$commit^{tree}\")\n\nYes, I was just writing up the commit message for it. :-/\n\nIt _is_ slower, though, because it introduces an extra rev-parse. When\nwe could in fact be getting rid of one. Give me a moment to complete a\nfew timing tests and post the results.\n\n-Peff\n"},{"id":"276369","messageId":"xmqq37tt9r9g.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"20160119213705.GA28656@sigill.intra.peff.net","subject":"Re: git filter-branch not removing commits when it should in 2.7.0","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T21:46:35Z","receivedAt":"2016-01-19T21:46:35Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 19, 2016 at 01:35:25PM -0800, Junio C Hamano wrote:\n>\n>> In other words, would the fix be a one-liner like this?\n>> [...]\n>> -\t\ttree=\"$commit^{tree}\"\n>> +\t\ttree=$(git rev-parse \"$commit^{tree}\")\n>\n> Yes, I was just writing up the commit message for it. :-/\n>\n> It _is_ slower, though, because it introduces an extra rev-parse. When\n> we could in fact be getting rid of one. Give me a moment to complete a\n> few timing tests and post the results.\n\nGood point.\n\nWe should do that rev-parse in the helper function. That rev-parse\nis there only because the skip-empty code wants to know the exact\nobject name when comparing. There is no reason for this code to do\nit for the helper--the helper, if (and only if) it is called, can\ndo the rev-parse itself, and we can still omit the overhead when\nwe are not skipping empty ones.\n"},{"id":"276370","messageId":"20160119215100.GB28656@sigill.intra.peff.net","threadId":"41219","inReplyTo":"xmqq37tt9r9g.fsf@gitster.mtv.corp.google.com","subject":"[PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T21:51:00Z","receivedAt":"2016-01-19T21:51:00Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 01:46:35PM -0800, Junio C Hamano wrote:\n\n> > It _is_ slower, though, because it introduces an extra rev-parse. When\n> > we could in fact be getting rid of one. Give me a moment to complete a\n> > few timing tests and post the results.\n> \n> Good point.\n> \n> We should do that rev-parse in the helper function. That rev-parse\n> is there only because the skip-empty code wants to know the exact\n> object name when comparing. There is no reason for this code to do\n> it for the helper--the helper, if (and only if) it is called, can\n> do the rev-parse itself, and we can still omit the overhead when\n> we are not skipping empty ones.\n\nHere's the patch I came up with. It takes the conservative choice (see\nthe argument below), and shows the performance impact. I'll work up the\nnon-conservative one on top, which I think can do even better than the\noriginal.\n\n-- >8 --\nSubject: filter-branch: resolve $commit^{tree} in no-index case\n\nCommit 348d4f2 (filter-branch: skip index read/write when\npossible, 2015-11-06) taught filter-branch to optimize out\nthe final \"git write-tree\" when we know we haven't touched\nthe tree with any of our filters. It does by simply putting\nthe literal text \"$commit^{tree}\" into the \"$tree\" variable,\navoiding a useless rev-parse call.\n\nHowever, when we pass this to git_commit_non_empty_tree(),\nit gets confused; it resolves \"$commit^{tree}\" itself, and\ncompares our string to the 40-hex sha1, which obviously\ndoesn't match. As a result, \"--prune-empty\" (or any custom\nfilter using git_commit_non_empty_tree) will fail to drop\nan empty commit (when filter-branch is used without a tree\nor index filter).\n\nLet's resolve $tree to the 40-hex ourselves, so that\ngit_commit_non_empty_tree can work. Unfortunately, this is a\nbit slower due to the extra process overhead:\n\n $ cd t/perf && ./run 348d4f2 HEAD p7000-filter-branch.sh\n [...]\n Test 348d4f2 HEAD\n --------------------------------------------------------------\n 7000.2: noop filter 3.76(0.24+0.26) 4.54(0.28+0.24) +20.7%\n\nHowever, the value of $tree here is technically\nuser-visible. The user can provide arbitrary shell code at\nthis stage, which could itself have a similar assumption to\nwhat is in git_commit_non_empty_tree. So the conservative\nchoice to fix this regression is to take the 20% hit and\ngive the pre-348d4f2 behavior. We still end up much faster\nthan before the optimization:\n\n $ cd t/perf && ./run 348d4f2^ HEAD p7000-filter-branch.sh\n [...]\n Test 348d4f2^ HEAD\n --------------------------------------------------------------\n 7000.2: noop filter 9.51(4.32+0.40) 4.51(0.28+0.23) -52.6%\n\nSigned-off-by: Jeff King \n---\n git-filter-branch.sh | 2 +-\n t/t7003-filter-branch.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex d61f9ba..5e094ce 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -404,7 +404,7 @@ while read commit parents; do\n \tthen\n \t\ttree=$(git write-tree)\n \telse\n-\t\ttree=\"$commit^{tree}\"\n+\t\ttree=$(git rev-parse \"$commit^{tree}\")\n \tfi\n \tworkdir=$workdir @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n \t\t\"$tree\" $parentstr < ../message > ../map/$commit ||\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 377c648..97c23c2 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -333,6 +333,14 @@ test_expect_success 'prune empty collapsed merges' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'prune empty works even without index/tree filters' '\n+\tgit rev-list HEAD >expect &&\n+\tgit commit --allow-empty -m empty &&\n+\tgit filter-branch -f --prune-empty HEAD &&\n+\tgit rev-list HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--remap-to-ancestor with filename filters' '\n \tgit checkout master &&\n \tgit reset --hard A &&\n-- \n2.7.0.248.g5eafd77\n"},{"id":"276371","messageId":"20160119215928.GA6556@sigill.intra.peff.net","threadId":"41219","inReplyTo":"20160119215100.GB28656@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T21:59:28Z","receivedAt":"2016-01-19T21:59:28Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 04:51:00PM -0500, Jeff King wrote:\n\n> Here's the patch I came up with. It takes the conservative choice (see\n> the argument below), and shows the performance impact. I'll work up the\n> non-conservative one on top, which I think can do even better than the\n> original.\n\nOh, never mind. I had thought we could do away with _both_ rev-parses\nand compare \"foo^{tree}\" to \"foo^{tree}\".\n\nBut of course that does not work. Because it is really \"$commit^{tree}\"\nversus \"$parent^{tree}\" here. I need to update the commit message below.\n\nWe could get away \"git diff --exit-code $1 $2\" to do a single process\ninvocation (rather than two rev-parses), but I don't know if it is worth\nthe complexity.\n\n-Peff\n"},{"id":"276372","messageId":"20160119220722.GB6556@sigill.intra.peff.net","threadId":"41219","inReplyTo":"20160119215928.GA6556@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T22:07:22Z","receivedAt":"2016-01-19T22:07:22Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 04:59:28PM -0500, Jeff King wrote:\n\n> Oh, never mind. I had thought we could do away with _both_ rev-parses\n> and compare \"foo^{tree}\" to \"foo^{tree}\".\n> \n> But of course that does not work. Because it is really \"$commit^{tree}\"\n> versus \"$parent^{tree}\" here. I need to update the commit message below.\n\nActually, I ended up not including the part about doing that comparison.\nSo the commit message isn't _wrong_, but it is incomplete. This\nparagraph:\n\n However, the value of $tree here is technically\n user-visible. The user can provide arbitrary shell code at\n this stage, which could itself have a similar assumption to\n what is in git_commit_non_empty_tree. So the conservative\n choice to fix this regression is to take the 20% hit and\n give the pre-348d4f2 behavior.\n\nshould start with something like:\n\n We could try to make git_commit_non_empty_tree more clever.\n\nto make it clear why we care about the user-visibility of the variable\nin the first place.\n\nHere is a re-roll with that fixup.\n\n-- >8 --\nSubject: [PATCH v2] filter-branch: resolve $commit^{tree} in no-index case\n\nCommit 348d4f2 (filter-branch: skip index read/write when\npossible, 2015-11-06) taught filter-branch to optimize out\nthe final \"git write-tree\" when we know we haven't touched\nthe tree with any of our filters. It does by simply putting\nthe literal text \"$commit^{tree}\" into the \"$tree\" variable,\navoiding a useless rev-parse call.\n\nHowever, when we pass this to git_commit_non_empty_tree(),\nit gets confused; it resolves \"$commit^{tree}\" itself, and\ncompares our string to the 40-hex sha1, which obviously\ndoesn't match. As a result, \"--prune-empty\" (or any custom\nfilter using git_commit_non_empty_tree) will fail to drop\nan empty commit (when filter-branch is used without a tree\nor index filter).\n\nLet's resolve $tree to the 40-hex ourselves, so that\ngit_commit_non_empty_tree can work. Unfortunately, this is a\nbit slower due to the extra process overhead:\n\n $ cd t/perf && ./run 348d4f2 HEAD p7000-filter-branch.sh\n [...]\n Test 348d4f2 HEAD\n --------------------------------------------------------------\n 7000.2: noop filter 3.76(0.24+0.26) 4.54(0.28+0.24) +20.7%\n\nWe could try to make git_commit_non_empty_tree more clever.\nHowever, the value of $tree here is technically\nuser-visible. The user can provide arbitrary shell code at\nthis stage, which could itself have a similar assumption to\nwhat is in git_commit_non_empty_tree. So the conservative\nchoice to fix this regression is to take the 20% hit and\ngive the pre-348d4f2 behavior. We still end up much faster\nthan before the optimization:\n\n $ cd t/perf && ./run 348d4f2^ HEAD p7000-filter-branch.sh\n [...]\n Test 348d4f2^ HEAD\n --------------------------------------------------------------\n 7000.2: noop filter 9.51(4.32+0.40) 4.51(0.28+0.23) -52.6%\n\nSigned-off-by: Jeff King \n---\n git-filter-branch.sh | 2 +-\n t/t7003-filter-branch.sh | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletion(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex d61f9ba..5e094ce 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -404,7 +404,7 @@ while read commit parents; do\n \tthen\n \t\ttree=$(git write-tree)\n \telse\n-\t\ttree=\"$commit^{tree}\"\n+\t\ttree=$(git rev-parse \"$commit^{tree}\")\n \tfi\n \tworkdir=$workdir @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n \t\t\"$tree\" $parentstr < ../message > ../map/$commit ||\ndiff --git a/t/t7003-filter-branch.sh b/t/t7003-filter-branch.sh\nindex 377c648..97c23c2 100755\n--- a/t/t7003-filter-branch.sh\n+++ b/t/t7003-filter-branch.sh\n@@ -333,6 +333,14 @@ test_expect_success 'prune empty collapsed merges' '\n \ttest_cmp expect actual\n '\n \n+test_expect_success 'prune empty works even without index/tree filters' '\n+\tgit rev-list HEAD >expect &&\n+\tgit commit --allow-empty -m empty &&\n+\tgit filter-branch -f --prune-empty HEAD &&\n+\tgit rev-list HEAD >actual &&\n+\ttest_cmp expect actual\n+'\n+\n test_expect_success '--remap-to-ancestor with filename filters' '\n \tgit checkout master &&\n \tgit reset --hard A &&\n-- \n2.7.0.248.g5eafd77\n"},{"id":"276374","messageId":"xmqqr3hd8azs.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"20160119220722.GB6556@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-19T22:23:19Z","receivedAt":"2016-01-19T22:23:19Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Actually, I ended up not including the part about doing that comparison.\n> So the commit message isn't _wrong_, but it is incomplete. This\n> paragraph:\n>\n> However, the value of $tree here is technically\n> user-visible. The user can provide arbitrary shell code at\n> this stage, which could itself have a similar assumption to\n> what is in git_commit_non_empty_tree. So the conservative\n> choice to fix this regression is to take the 20% hit and\n> give the pre-348d4f2 behavior.\n>\n> should start with something like:\n>\n> We could try to make git_commit_non_empty_tree more clever.\n>\n> to make it clear why we care about the user-visibility of the variable\n> in the first place.\n>\n> Here is a re-roll with that fixup.\n\nThanks. I failed to spot and realize that $tree is also part of\nwhat we expose and end-user scripts may already be looking at when I\ncommented, and I agree with the reasoning behind this conservative\nroute.\n\nWill queue.\n"},{"id":"276375","messageId":"20160119222802.GC6556@sigill.intra.peff.net","threadId":"41219","inReplyTo":"20160119215928.GA6556@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T22:28:02Z","receivedAt":"2016-01-19T22:28:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 04:59:28PM -0500, Jeff King wrote:\n\n> We could get away \"git diff --exit-code $1 $2\" to do a single process\n> invocation (rather than two rev-parses), but I don't know if it is worth\n> the complexity.\n\nAnd here's that patch.\n\nI'm actually a little iffy on it because it switches to \"diff-tree\" from\na raw-sha1 comparison. For a well-formed repo, that shouldn't matter.\nBut what if you had a commit that was replacing a malformed tree object,\nbut not otherwise changing the diff? We might drop it as \"empty\", even\nthough you'd prefer to keep it.\n\nI guess some alternative ways of doing the same thing are:\n\n 1. Use \"git rev-parse $1 $3^{tree}\", parse the output and compare.\n Surprisingly tedious to do in a shell without invoking any extra\n sub-processes.\n\n 2. When we do the initial rev-list, output the tree, too. Then we get\n the old tree sha1 for \"free\" (no extra process, but at the cost of\n slightly more I/O by the shell).\n\nI suspect we could also cache the tree of the parent and avoid the\nrev-parse in git_commit_non_empty_tree(), too.\n\nI dunno. I'm inclined to say that none of this is worth it to try to\ndrop one or two processes. Writing filter-branch in a better language\n(or just using BFG) would probably be a more productive use of time.\n20% looks like a lot, but that's because it's pretty fast in the first\nplace. The timings I showed earlier (and below) are for git.git, which\nis not that huge. But the savings from 348d4f2 are really about avoiding\nlooking at the trees entirely; the bigger your tree, the more you save.\nRunning it on linux.git should show that we're still reclaiming most of\nthe original optimization.\n\nSo I'm inclined to go with the \"conservative\" fix I sent initially, and\ndrop this micro-optimization.\n\n-- >8 --\nSubject: [PATCH] filter-branch: optimize out rev-parse for unchanged tree\n\nThe prior commit noticed that we started feeding\n\"$commit^{tree}\" to git_commit_non_empty_tree instead of its\nresolved 40-hex sha1. It made the conservative fix of\nresolving and passing the 40-hex sha1, even though it does\nadd some overhead.\n\nThis patch takes the less conservative choice, under the\nassumption that we are unlikely to break anybody's\ncommit-filter by showing them the unresolved name\n\"$commit^{tree}\" (which is probably the case, because most\npeople would end their filter with \"git commit-tree\" or\n\"git_commit_non_empty_tree\").\n\nWe can make the comparison in the latter function more\nclever by using \"diff-tree --quiet\", which will compare the\ntwo trees with only a single process invocation.\n\nHere are the resulting numbers from p7000:\n\n Test 348d4f2^ 348d4f2 HEAD^ HEAD\n ----------------------------------------------------------------------------------------------------------------\n 7000.2: noop filter 9.48(4.29+0.41) 3.73(0.22+0.26) -60.7% 4.56(0.27+0.26) -51.9% 3.74(0.25+0.25) -60.5%\n\nSigned-off-by: Jeff King \n---\n git-filter-branch.sh | 4 ++--\n 1 file changed, 2 insertions(+), 2 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 5e094ce..d1879e3 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -44,7 +44,7 @@ skip_commit()\n # it will skip commits that leave the tree untouched, commit the other.\n git_commit_non_empty_tree()\n {\n-\tif test $# = 3 && test \"$1\" = $(git rev-parse \"$3^{tree}\"); then\n+\tif test $# = 3 && git diff-tree --quiet \"$1\" \"$3\"; then\n \t\tmap \"$3\"\n \telse\n \t\tgit commit-tree \"$@\"\n@@ -404,7 +404,7 @@ while read commit parents; do\n \tthen\n \t\ttree=$(git write-tree)\n \telse\n-\t\ttree=$(git rev-parse \"$commit^{tree}\")\n+\t\ttree=\"$commit^{tree}\"\n \tfi\n \tworkdir=$workdir @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n \t\t\"$tree\" $parentstr < ../message > ../map/$commit ||\n-- \n2.7.0.248.g5eafd77\n"},{"id":"276376","messageId":"20160119224857.GD6556@sigill.intra.peff.net","threadId":"41219","inReplyTo":"20160119222802.GC6556@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-19T22:48:58Z","receivedAt":"2016-01-19T22:48:58Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 05:28:02PM -0500, Jeff King wrote:\n\n> I dunno. I'm inclined to say that none of this is worth it to try to\n> drop one or two processes. Writing filter-branch in a better language\n> (or just using BFG) would probably be a more productive use of time.\n> 20% looks like a lot, but that's because it's pretty fast in the first\n> place. The timings I showed earlier (and below) are for git.git, which\n> is not that huge. But the savings from 348d4f2 are really about avoiding\n> looking at the trees entirely; the bigger your tree, the more you save.\n> Running it on linux.git should show that we're still reclaiming most of\n> the original optimization.\n\nIn case anyone is curious, here are the linux.git numbers (re-wrapped,\nbecause t/perf produces some really long lines; that might be worth\naddressing):\n\n348d4f2^ 295.32(269.61+14.36)\n348d4f2 7.92( 0.85+ 0.72) -97.3%\nHEAD^ 9.37( 0.87+ 0.80) -96.8%\nHEAD 7.71( 0.92+ 0.62) -97.4%\n\nSo yes, the conservative fix costs us about 1.5 seconds, or 18%, over\nthe micro-optimized one. But the original point was to save over 280\nseconds, which we still do.\n\n-Peff\n"},{"id":"276390","messageId":"20160120004719.GC16090@google.com","threadId":"41219","inReplyTo":"20160119215100.GB28656@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-01-20T00:47:19Z","receivedAt":"2016-01-20T00:47:19Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n\n> Signed-off-by: Jeff King \n> ---\n> git-filter-branch.sh | 2 +-\n> t/t7003-filter-branch.sh | 8 ++++++++\n> 2 files changed, 9 insertions(+), 1 deletion(-)\n\nReviewed-by: Jonathan Nieder \n"},{"id":"276391","messageId":"20160120012253.GD16090@google.com","threadId":"41219","inReplyTo":"20160119222802.GC6556@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2016-01-20T01:22:53Z","receivedAt":"2016-01-20T01:22:53Z","isPatch":true,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Jeff King wrote:\n> On Tue, Jan 19, 2016 at 04:59:28PM -0500, Jeff King wrote:\n\n>> We could get away \"git diff --exit-code $1 $2\" to do a single process\n>> invocation (rather than two rev-parses), but I don't know if it is worth\n>> the complexity.\n>\n> And here's that patch.\n>\n> I'm actually a little iffy on it because it switches to \"diff-tree\" from\n> a raw-sha1 comparison. For a well-formed repo, that shouldn't matter.\n> But what if you had a commit that was replacing a malformed tree object,\n> but not otherwise changing the diff? We might drop it as \"empty\", even\n> though you'd prefer to keep it.\n\nMph. We could get the best of both worlds by introducing a \"git\nrev-parse --compare \" that compares object ids. Actually...\n\nHow about something like this?\n\nSigned-off-by: Jonathan Nieder \n---\n git-filter-branch.sh | 12 ++++++++++--\n 1 file changed, 10 insertions(+), 2 deletions(-)\n\ndiff --git a/git-filter-branch.sh b/git-filter-branch.sh\nindex 86b2ff1..06f4e0f 100755\n--- a/git-filter-branch.sh\n+++ b/git-filter-branch.sh\n@@ -44,7 +44,15 @@ skip_commit()\n # it will skip commits that leave the tree untouched, commit the other.\n git_commit_non_empty_tree()\n {\n-\tif test $# = 3 && test \"$1\" = $(git rev-parse \"$3^{tree}\"); then\n+\tif\n+\t\ttest $# = 3 &&\n+\t\tgit rev-parse \"$1\" \"$3^{tree}\" |\n+\t\t{\n+\t\t\tread a\n+\t\t\tread b\n+\t\t\ttest \"$a\" = \"$b\"\n+\t\t}\n+\tthen\n \t\tmap \"$3\"\n \telse\n \t\tgit commit-tree \"$@\"\n@@ -404,7 +412,7 @@ while read commit parents; do\n \tthen\n \t\ttree=$(git write-tree)\n \telse\n-\t\ttree=$(git rev-parse \"$commit^{tree}\")\n+\t\ttree=\"$commit^{tree}\"\n \tfi\n \tworkdir=$workdir @SHELL_PATH@ -c \"$filter_commit\" \"git commit-tree\" \\\n \t\t\"$tree\" $parentstr < ../message > ../map/$commit ||\n-- \n2.7.0\n"},{"id":"276392","messageId":"20160120013436.GA24541@sigill.intra.peff.net","threadId":"41219","inReplyTo":"20160120012253.GD16090@google.com","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-20T01:34:36Z","receivedAt":"2016-01-20T01:34:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 05:22:53PM -0800, Jonathan Nieder wrote:\n\n> > I'm actually a little iffy on it because it switches to \"diff-tree\" from\n> > a raw-sha1 comparison. For a well-formed repo, that shouldn't matter.\n> > But what if you had a commit that was replacing a malformed tree object,\n> > but not otherwise changing the diff? We might drop it as \"empty\", even\n> > though you'd prefer to keep it.\n> \n> Mph. We could get the best of both worlds by introducing a \"git\n> rev-parse --compare \" that compares object ids. Actually...\n> \n> How about something like this?\n\nThanks. I had in my head that we could do something like that, but\nhadn't quite worked it out. I think what you wrote works.\n\nIf you want to wrap it up into a patch, I'd be OK with it, but note that\nit still falls afoul of changing $tree in a user-visible way (so you\nshould note that in the commit message).\n\n-Peff\n"},{"id":"276396","messageId":"xmqqbn8h6mrl.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"20160120013436.GA24541@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T01:51:58Z","receivedAt":"2016-01-20T01:51:58Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n>> Mph. We could get the best of both worlds by introducing a \"git\n>> rev-parse --compare \" that compares object ids. Actually...\n>> \n>> How about something like this?\n>\n> Thanks. I had in my head that we could do something like that, but\n> hadn't quite worked it out. I think what you wrote works.\n\nBut wouldn't \"diff-tree --quiet\" essentially be that command?\n\n> If you want to wrap it up into a patch, I'd be OK with it, but note that\n> it still falls afoul of changing $tree in a user-visible way (so you\n> should note that in the commit message).\n\nYes, I think we should take your conservative variant for that exact\nreason.\n"},{"id":"276399","messageId":"20160120020039.GD24541@sigill.intra.peff.net","threadId":"41219","inReplyTo":"xmqqbn8h6mrl.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-20T02:00:39Z","receivedAt":"2016-01-20T02:00:39Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 05:51:58PM -0800, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> >> Mph. We could get the best of both worlds by introducing a \"git\n> >> rev-parse --compare \" that compares object ids. Actually...\n> >> \n> >> How about something like this?\n> >\n> > Thanks. I had in my head that we could do something like that, but\n> > hadn't quite worked it out. I think what you wrote works.\n> \n> But wouldn't \"diff-tree --quiet\" essentially be that command?\n\nI think Jonathan was responding to my point that \"diff-tree --quiet\"\n_isn't_ quite the same, if you have mis-formatted tree objects. If the\nsha1s are different, a rev-parse comparison will keep the commit. But\n\"diff-tree\" will actually do the diff, and may consider different sha1s\nto have the same content, dropping the second one.\n\nIt's a minor point, but I find one of my primary uses for filter-branch\nthese days is massaging out bogus objects made by older or buggy git\nclients (not that I see _that_ many of them; I think it speaks more to\nthe fact that I don't really use filter-branch much these days).\n\n> > If you want to wrap it up into a patch, I'd be OK with it, but note that\n> > it still falls afoul of changing $tree in a user-visible way (so you\n> > should note that in the commit message).\n> \n> Yes, I think we should take your conservative variant for that exact\n> reason.\n\nI'm fine with that, too. We could also do the conservative variant for\n\"maint\", and then do the other with a deprecation warning. I think I\ndecided I don't care enough to go through those motions myself, but I\ndon't mind if somebody else wants to.\n\n-Peff\n"},{"id":"276403","messageId":"xmqq4me96kd2.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"20160120020039.GD24541@sigill.intra.peff.net","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T02:43:53Z","receivedAt":"2016-01-20T02:43:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> On Tue, Jan 19, 2016 at 05:51:58PM -0800, Junio C Hamano wrote:\n>\n>> Jeff King writes:\n>> \n>> >> Mph. We could get the best of both worlds by introducing a \"git\n>> >> rev-parse --compare \" that compares object ids. Actually...\n>> >> \n>> >> How about something like this?\n>> >\n>> > Thanks. I had in my head that we could do something like that, but\n>> > hadn't quite worked it out. I think what you wrote works.\n>> \n>> But wouldn't \"diff-tree --quiet\" essentially be that command?\n>\n> I think Jonathan was responding to my point that \"diff-tree --quiet\"\n> _isn't_ quite the same, if you have mis-formatted tree objects. If the\n> sha1s are different, a rev-parse comparison will keep the commit. But\n> \"diff-tree\" will actually do the diff, and may consider different sha1s\n> to have the same content, dropping the second one.\n>\n> It's a minor point, but I find one of my primary uses for filter-branch\n> these days is massaging out bogus objects made by older or buggy git\n> clients (not that I see _that_ many of them; I think it speaks more to\n> the fact that I don't really use filter-branch much these days).\n\nI (think I) understand that use case, but this function compares the\nparent tree and the tree of the commit we just created, and it does\nso in order to skip the one we just created (when --prune-empty is\ngiven). It is not about \"tree-filter returned a tree, let's compare\nthem and if the old one and the new one looks the same (even though\nthey do not have identical object name) and replace the old one with\nthe new before giving it to commit-filter, so that we have a larger\nchance of noticing a case where we did not have to rewrite and instead\nwere able to reuse the old commit and tree\".\n\nIn other words, I do not think \"broken tree object may look the same\nto diff-tree, but I do want to replace it\" is relevant to this\ncodepath; it is not something this function handles, I think.\n\nWhat am I not seeing?\n"},{"id":"276404","messageId":"xmqqziw153yg.fsf@gitster.mtv.corp.google.com","threadId":"41219","inReplyTo":"xmqq4me96kd2.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-01-20T03:23:35Z","receivedAt":"2016-01-20T03:23:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano writes:\n\n> In other words, I do not think \"broken tree object may look the same\n> to diff-tree, but I do want to replace it\" is relevant to this\n> codepath; it is not something this function handles, I think.\n>\n> What am I not seeing?\n\nOne thing that I was not seeing is that \"diff-tree --quiet\" (without\nany magic options like -ignore-whitespace that may make two\ndifferent tree object judged to be the same, and without -C -C that\nforces it to recurse into identical subtrees) lacks an obvious and\nstupid optimization to say \"they are the same\" or \"they are\ndifferent when given two tree object names without opening the\ntree(s). So if you make it compare a rewritten and corrected tree\nand an incorrect and possibly broken one, it may try to read the\ntree data from the latter and comparison can lead to incorrect\nresults.\n\nA consequence of this is if you are running filter-branch without\nany tree filters (i.e. no_index) but with \"--prune-empty\", and a\ncommit and its parent actually have different trees but one (or\nboth) of them are broken, \"diff-tree\" _might_ say \"they are the\nsame\" and you end up skipping a commit when you do not want to. If\nyour plan was to run another round of filter-branch, this time with\na \"broken tree encoding correction\" tree-filter, on the result of\nthe first \"--prune-empty\" filtering, we would definitely end up with\na wrong history.\n\nBut for such a case, I would say you should be running the\ncorrection filter as the very first thing. So I am not sure if it\nmatters in practice.\n\nOne possible action item out of this is that we might want to think\nabout giving the obvious and stupid optimization to such invocation\nof \"diff-tree --quiet\". I _think_ we correctly avoid descending\ninto the identical subtrees while doing a recursive diff-tree by\nsaying \"hey these two corresponding directories have the same tree\nobject names\", but there is no fundamental reason why we shouldn't\nbe doing the same optimization at the top-level of the comparison.\n"},{"id":"276413","messageId":"20160120041447.GA12811@sigill.intra.peff.net","threadId":"41219","inReplyTo":"xmqqziw153yg.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH] filter-branch: resolve $commit^{tree} in no-index case","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2016-01-20T04:14:48Z","receivedAt":"2016-01-20T04:14:48Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Tue, Jan 19, 2016 at 07:23:35PM -0800, Junio C Hamano wrote:\n\n> A consequence of this is if you are running filter-branch without\n> any tree filters (i.e. no_index) but with \"--prune-empty\", and a\n> commit and its parent actually have different trees but one (or\n> both) of them are broken, \"diff-tree\" _might_ say \"they are the\n> same\" and you end up skipping a commit when you do not want to. If\n> your plan was to run another round of filter-branch, this time with\n> a \"broken tree encoding correction\" tree-filter, on the result of\n> the first \"--prune-empty\" filtering, we would definitely end up with\n> a wrong history.\n\nYeah, that is the case I was thinking about.\n\n> But for such a case, I would say you should be running the\n> correction filter as the very first thing. So I am not sure if it\n> matters in practice.\n\nI'd also agree with that.\n\n> One possible action item out of this is that we might want to think\n> about giving the obvious and stupid optimization to such invocation\n> of \"diff-tree --quiet\". I _think_ we correctly avoid descending\n> into the identical subtrees while doing a recursive diff-tree by\n> saying \"hey these two corresponding directories have the same tree\n> object names\", but there is no fundamental reason why we shouldn't\n> be doing the same optimization at the top-level of the comparison.\n\nYeah, I just assumed we did, but a simple strace shows that is not the\ncase. I doubt it matters _too_ much in practice, but it would probably\nbe easy to do.\n\n-Peff\n"}]}