Re: [PATCH v2 2/5] t3034: add rename threshold tests

From: Junio C Hamano <gitster@pobox.com>
Date: Feb 22, 2016, 19:03 UTC
Message-ID: <xmqqh9h07ejt.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1456095545-20201-3-git-send-email-felipegassis@gmail.com>

"Felipe Gonçalves Assis"  <felipeg.assis@gmail.com> writes:

Show 52 quoted lines

> 10ae752 (merge-recursive: option to specify rename threshold,
> 2010-09-27) introduced this feature but did not include any tests.
>
> The tests use the new option --find-renames, which replaces the then
> introduced and now deprecated option --rename-threshold.
> ...
> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
> new file mode 100755
> index 0000000..7ae7f83
> --- /dev/null
> +++ b/t/t3034-merge-recursive-rename-options.sh
> @@ -0,0 +1,159 @@
> +#!/bin/sh
> +
> +test_description='merge-recursive rename options
> +
> +Test rename detection by examining rename/delete conflicts.
> +
> +Similarity index:
> +R100 a-old a-new
> +R075 b-old b-new
> +R050 c-old c-new
> +R025 d-old d-new
> +'
> ...
> +test_expect_success setup '
> +	cat <<-\EOF >a-old &&
> +	aa1
> +	aa2
> +	aa3
> +	aa4
> +	EOF
> +	sed s/aa/bb/ <a-old >b-old &&
> +	sed s/aa/cc/ <a-old >c-old &&
> +	sed s/aa/dd/ <a-old >d-old &&
> +	git add [a-d]-old &&
> +	git commit -m base &&
> +	git rm [a-d]-old &&
> +	git commit -m delete &&
> +	git checkout -b rename HEAD^ &&
> +	cp a-old a-new &&
> +	sed 1,1s/./x/ <b-old >b-new &&
> +	sed 1,2s/./x/ <c-old >c-new &&
> +	sed 1,3s/./x/ <d-old >d-new &&
> +	git add [a-d]-new &&
> +	git rm [a-d]-old &&
> +	git commit -m rename &&
> +	get_expected_stages a &&
> +	get_expected_stages b &&
> +	get_expected_stages c &&
> +	get_expected_stages d
> +'

I somehow doubt that it is wise to make the similarity index computed by the current heuristics as a hard promise like this test does.

This test specifies that turning the original bb1/bb2/bb3/bb4 into updated xb1/bb2/bb3/bb4 _MUST_ get 75% similarity, but that is not something we want to guarantee, ever. We may later update the algorithm and tweak such a change to register 70% or 78%, but such a change would break the expectation by this test. This test script however should not be interested in the exact similarity index assignment for a given filepair--it only wants to make sure that the option chooses the filepair with similarity index above the given value.

The test for --find-renames=<num> should instead only validate that the code works on the value given from the command line relative to the similarity index the code computed. I.e. first measure what the similarity index going from b-old to b-new is (e.g. it may say 76%, or 74%, depending on the version of Git being tested), then choose a value that is higher (or lower) than that similarity to give to the --find-renames=<num> option and ensure that merge-recursive does what is expected.

I am not very happy with this one.