threads / patch / 42857

patchVerify that --cherry-pick avoids looking at full diffs

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

## tl;dr

3 messages between Jul 15, 2016 and Jul 16, 2016. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

Johannes Schindelin· Jul 15, 2016, 15:51 UTC · lore

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(+)

Show changes to t/t6007-rev-list-cherry-pick-file.sh +19 −0

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· Jul 15, 2016, 17:07 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

Show 9 quoted lines

> 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· Jul 16, 2016, 08:34 UTC · re: Junio C Hamano · lore

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

Hi Junio,

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

Show 20 quoted lines

> 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