[PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs

[PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs

[Johannes Schindelin]

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 <johannes.schindelin@gmx.de>
---
	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

Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> 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".

Re: [PATCH 2/1] Verify that --cherry-pick avoids looking at full diffs

[Johannes Schindelin]

Hi Junio,

On Fri, 15 Jul 2016, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> 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