# [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs 3 messages from 2016-07-15 to 2016-07-16. Participants: Johannes Schindelin, Junio C Hamano. Thread: https://gitlist.dev/t/42857 ## Johannes Schindelin, 2016-07-15 15:51 Subject: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1607151749540.6426%40virtualbox ``` The entire point of the previous patch was to make sure that we look at abbreviated patch IDs (using the diff *headers* only, but avoiding to load the blobs into memory and diff them) first, and only look at full patch IDs when the abbreviated patch IDs were not for the --cherry-pick test. Let's make sure that we actually avoid looking at the full patch ID, simply by corrupting an object that is needed for the full patch ID, and then seeing that --cherry-pick still works. Signed-off-by: Johannes Schindelin --- On Fri, 15 Jul 2016, Johannes Schindelin wrote: > I also think that this patch series could use a test that > verifies that we no longer generate unneeded diffs. Maybe by > rebasing a commit on top of an commit touching a different file, > after corrupting the blob of the latter one? Let me give it a > try. And here it is. I verified that this test passes with your patch and fails without it. t/t6007-rev-list-cherry-pick-file.sh | 19 +++++++++++++++++++ 1 file changed, 19 insertions(+) diff --git a/t/t6007-rev-list-cherry-pick-file.sh b/t/t6007-rev-list-cherry-pick-file.sh index 28d4f6b..a5f7c2a 100755 --- a/t/t6007-rev-list-cherry-pick-file.sh +++ b/t/t6007-rev-list-cherry-pick-file.sh @@ -207,4 +207,23 @@ test_expect_success '--count --left-right' ' test_cmp expect actual ' +remove_loose_object () { + sha1="$(git rev-parse "$1")" && + remainder=${sha1#??} && + firsttwo=${sha1%$remainder} && + rm .git/objects/$firsttwo/$remainder +} + +test_expect_success '--cherry-pick avoids looking at full diffs' ' + git checkout -b shy-diff && + test_commit dont-look-at-me && + echo Hello >dont-look-at-me.t && + test_tick && + git commit -m tip dont-look-at-me.t && + git checkout -b mainline HEAD^ && + test_commit to-cherry-pick && + remove_loose_object shy-diff^:dont-look-at-me.t && + git rev-list --cherry-pick ...shy-diff +' + test_done -- 2.9.0.281.g286a8d9 ``` ## Junio C Hamano, 2016-07-15 17:07 Subject: Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs Message-ID: URL: https://gitlist.dev/e/xmqqwpkmn83r.fsf%40gitster.mtv.corp.google.com In-Reply-To: ``` Johannes Schindelin writes: > The entire point of the previous patch was to make sure that we look at > abbreviated patch IDs (using the diff *headers* only, but avoiding > to load the blobs into memory and diff them) first, and only look at > full patch IDs when the abbreviated patch IDs were not for the > --cherry-pick test. > > Let's make sure that we actually avoid looking at the full patch ID, > simply by corrupting an object that is needed for the full patch ID, and > then seeing that --cherry-pick still works. I think "Avoid looking at" merely is the means to an end, and not the goal by itself. By not looking at them, you hopefully run faster. So I'd think a more useful addition under t/ would be to t/perf somewhere, not "now you can rev-list --cherry-pick even inside a corrupt repository, as long as corruption happens to be with blobs and not the containing trees". ``` ## Johannes Schindelin, 2016-07-16 08:34 Subject: Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs Message-ID: URL: https://gitlist.dev/e/alpine.DEB.2.20.1607161033370.6426%40virtualbox In-Reply-To: ``` Hi Junio, On Fri, 15 Jul 2016, Junio C Hamano wrote: > Johannes Schindelin writes: > > > The entire point of the previous patch was to make sure that we look at > > abbreviated patch IDs (using the diff *headers* only, but avoiding > > to load the blobs into memory and diff them) first, and only look at > > full patch IDs when the abbreviated patch IDs were not for the > > --cherry-pick test. > > > > Let's make sure that we actually avoid looking at the full patch ID, > > simply by corrupting an object that is needed for the full patch ID, and > > then seeing that --cherry-pick still works. > > I think "Avoid looking at" merely is the means to an end, and not > the goal by itself. By not looking at them, you hopefully run > faster. > > So I'd think a more useful addition under t/ would be to t/perf > somewhere, not "now you can rev-list --cherry-pick even inside a > corrupt repository, as long as corruption happens to be with blobs > and not the containing trees". I agree that we need a perf test. But we *also* need this test that ensures that we avoid loading blobs into memory, because that is the solution we do not want to see regress in the future. Ciao, Dscho ```