{"thread":{"id":"42857","subject":"[PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs","startedAt":"2016-07-15T15:52:01Z","lastAt":"2016-07-16T08:34:39Z","messageCount":3,"participants":["Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":1},"messages":[{"id":"291544","messageId":"alpine.DEB.2.20.1607151749540.6426@virtualbox","threadId":"42857","inReplyTo":null,"subject":"[PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-15T15:51:51Z","receivedAt":"2016-07-15T15:52:01Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"\nThe entire point of the previous patch was to make sure that we look at\nabbreviated patch IDs (using the diff *headers* only, but avoiding\nto load the blobs into memory and diff them) first, and only look at\nfull patch IDs when the abbreviated patch IDs were not for the\n--cherry-pick test.\n\nLet's make sure that we actually avoid looking at the full patch ID,\nsimply by corrupting an object that is needed for the full patch ID, and\nthen seeing that --cherry-pick still works.\n\nSigned-off-by: Johannes Schindelin <johannes.schindelin@gmx.de>\n---\n\tOn Fri, 15 Jul 2016, Johannes Schindelin wrote:\n\n\t> I also think that this patch series could use a test that\n\t> verifies that we no longer generate unneeded diffs. Maybe by\n\t> rebasing a commit on top of an commit touching a different file,\n\t> after corrupting the blob of the latter one? Let me give it a\n\t> try.\n\n\tAnd here it is. I verified that this test passes with your patch\n\tand fails without it.\n\n t/t6007-rev-list-cherry-pick-file.sh | 19 +++++++++++++++++++\n 1 file changed, 19 insertions(+)\n\ndiff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh\nindex 28d4f6b..a5f7c2a 100755\n--- a/t/t6007-rev-list-cherry-pick-file.sh\n+++ b/t/t6007-rev-list-cherry-pick-file.sh\n@@ -207,4 +207,23 @@ test_expect_success '--count --left-right' '\n \ttest_cmp expect actual\n '\n \n+remove_loose_object () {\n+\tsha1=\"$(git rev-parse \"$1\")\" &&\n+\tremainder=${sha1#??} &&\n+\tfirsttwo=${sha1%$remainder} &&\n+\trm .git/objects/$firsttwo/$remainder\n+}\n+\n+test_expect_success '--cherry-pick avoids looking at full diffs' '\n+\tgit checkout -b shy-diff &&\n+\ttest_commit dont-look-at-me &&\n+\techo Hello >dont-look-at-me.t &&\n+\ttest_tick &&\n+\tgit commit -m tip dont-look-at-me.t &&\n+\tgit checkout -b mainline HEAD^ &&\n+\ttest_commit to-cherry-pick &&\n+\tremove_loose_object shy-diff^:dont-look-at-me.t &&\n+\tgit rev-list --cherry-pick ...shy-diff\n+'\n+\n test_done\n-- \n2.9.0.281.g286a8d9\n\n"},{"id":"291551","messageId":"xmqqwpkmn83r.fsf@gitster.mtv.corp.google.com","threadId":"42857","inReplyTo":"alpine.DEB.2.20.1607151749540.6426@virtualbox","subject":"Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2016-07-15T17:07:04Z","receivedAt":"2016-07-15T17:07:24Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n\n> The entire point of the previous patch was to make sure that we look at\n> abbreviated patch IDs (using the diff *headers* only, but avoiding\n> to load the blobs into memory and diff them) first, and only look at\n> full patch IDs when the abbreviated patch IDs were not for the\n> --cherry-pick test.\n>\n> Let's make sure that we actually avoid looking at the full patch ID,\n> simply by corrupting an object that is needed for the full patch ID, and\n> then seeing that --cherry-pick still works.\n\nI think \"Avoid looking at\" merely is the means to an end, and not\nthe goal by itself.  By not looking at them, you hopefully run\nfaster.\n\nSo I'd think a more useful addition under t/ would be to t/perf\nsomewhere, not \"now you can rev-list --cherry-pick even inside a\ncorrupt repository, as long as corruption happens to be with blobs\nand not the containing trees\".\n\n"},{"id":"291593","messageId":"alpine.DEB.2.20.1607161033370.6426@virtualbox","threadId":"42857","inReplyTo":"xmqqwpkmn83r.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2016-07-16T08:34:17Z","receivedAt":"2016-07-16T08:34:39Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi Junio,\n\nOn Fri, 15 Jul 2016, Junio C Hamano wrote:\n\n> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:\n> \n> > The entire point of the previous patch was to make sure that we look at\n> > abbreviated patch IDs (using the diff *headers* only, but avoiding\n> > to load the blobs into memory and diff them) first, and only look at\n> > full patch IDs when the abbreviated patch IDs were not for the\n> > --cherry-pick test.\n> >\n> > Let's make sure that we actually avoid looking at the full patch ID,\n> > simply by corrupting an object that is needed for the full patch ID, and\n> > then seeing that --cherry-pick still works.\n> \n> I think \"Avoid looking at\" merely is the means to an end, and not\n> the goal by itself.  By not looking at them, you hopefully run\n> faster.\n> \n> So I'd think a more useful addition under t/ would be to t/perf\n> somewhere, not \"now you can rev-list --cherry-pick even inside a\n> corrupt repository, as long as corruption happens to be with blobs\n> and not the containing trees\".\n\nI agree that we need a perf test.\n\nBut we *also* need this test that ensures that we avoid loading blobs into\nmemory, because that is the solution we do not want to see regress in the\nfuture.\n\nCiao,\nDscho\n"}]}