* [PATCH v2 0/3] Tests for merge-recursive rename options
@ 2016-02-23 23:48 Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 1/3] t3034: add rename threshold tests Felipe Gonçalves Assis
` (2 more replies)
0 siblings, 3 replies; 7+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-23 23:48 UTC (permalink / raw)
To: git; +Cc: Johannes.Schindelin, gitster, sunshine, Felipe Gonçalves Assis
Get rid of blatant bash-isms in favour of simple and portable constructions.
Felipe Gonçalves Assis (3):
t3034: add rename threshold tests
t3034: test option to disable renames
t3034: test deprecated interface
...s.sh => t3032-merge-recursive-space-options.sh} | 2 +-
t/t3034-merge-recursive-rename-options.sh | 309 +++++++++++++++++++++
2 files changed, 310 insertions(+), 1 deletion(-)
rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)
create mode 100755 t/t3034-merge-recursive-rename-options.sh
--
2.7.1.492.gd821b20
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH 1/3] t3034: add rename threshold tests
2016-02-23 23:48 [PATCH v2 0/3] Tests for merge-recursive rename options Felipe Gonçalves Assis
@ 2016-02-23 23:48 ` Felipe Gonçalves Assis
2016-02-24 0:50 ` Eric Sunshine
2016-02-23 23:48 ` [PATCH 2/3] t3034: test option to disable renames Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 3/3] t3034: test deprecated interface Felipe Gonçalves Assis
2 siblings, 1 reply; 7+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-23 23:48 UTC (permalink / raw)
To: git
Cc: Johannes.Schindelin, gitster, sunshine,
Felipe Gonçalves Assis, Felipe Gonçalves Assis
From: Felipe Gonçalves Assis <felipeg.assis@gmail.com>
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.
Also update name and description of t3032 for consistency:
"merge-recursive options" -> "merge-recursive space options"
Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
...s.sh => t3032-merge-recursive-space-options.sh} | 2 +-
t/t3034-merge-recursive-rename-options.sh | 235 +++++++++++++++++++++
2 files changed, 236 insertions(+), 1 deletion(-)
rename t/{t3032-merge-recursive-options.sh => t3032-merge-recursive-space-options.sh} (99%)
create mode 100755 t/t3034-merge-recursive-rename-options.sh
diff --git a/t/t3032-merge-recursive-options.sh b/t/t3032-merge-recursive-space-options.sh
similarity index 99%
rename from t/t3032-merge-recursive-options.sh
rename to t/t3032-merge-recursive-space-options.sh
index 4029c9c..b56180e 100755
--- a/t/t3032-merge-recursive-options.sh
+++ b/t/t3032-merge-recursive-space-options.sh
@@ -1,6 +1,6 @@
#!/bin/sh
-test_description='merge-recursive options
+test_description='merge-recursive space options
* [master] Clarify
! [remote] Remove cruft
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
new file mode 100755
index 0000000..fbec68c
--- /dev/null
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -0,0 +1,235 @@
+#!/bin/sh
+
+test_description='merge-recursive rename options
+
+Test rename detection by examining rename/delete conflicts.
+
+* (HEAD -> rename) rename
+| * (master) delete
+|/
+* base
+
+git diff --name-status base master
+D 0-old
+D 1-old
+D 2-old
+D 3-old
+
+git diff --name-status -M01 base rename
+R025 0-old 0-new
+R050 1-old 1-new
+R075 2-old 2-new
+R100 3-old 3-new
+
+Actual similarity indices are parsed from diff output. We rely on the fact that
+they are rounded down (see, e.g., Documentation/diff-generate-patch.txt, which
+mentions this in a different context).
+'
+
+. ./test-lib.sh
+
+get_expected_stages () {
+ git checkout rename -- $1-new &&
+ git ls-files --stage $1-new >expected-stages-undetected-$1 &&
+ sed "s/ 0 / 2 /" <expected-stages-undetected-$1 \
+ >expected-stages-detected-$1 &&
+ git read-tree -u --reset HEAD
+}
+
+rename_detected () {
+ git ls-files --stage $1-old $1-new >stages-actual-$1 &&
+ test_cmp expected-stages-detected-$1 stages-actual-$1
+}
+
+rename_undetected () {
+ git ls-files --stage $1-old $1-new >stages-actual-$1 &&
+ test_cmp expected-stages-undetected-$1 stages-actual-$1
+}
+
+check_common () {
+ git ls-files --stage >stages-actual &&
+ test_line_count = 4 stages-actual
+}
+
+check_threshold_0 () {
+ check_common &&
+ rename_detected 0 &&
+ rename_detected 1 &&
+ rename_detected 2 &&
+ rename_detected 3
+}
+
+check_threshold_1 () {
+ check_common &&
+ rename_undetected 0 &&
+ rename_detected 1 &&
+ rename_detected 2 &&
+ rename_detected 3
+}
+
+check_threshold_2 () {
+ check_common &&
+ rename_undetected 0 &&
+ rename_undetected 1 &&
+ rename_detected 2 &&
+ rename_detected 3
+}
+
+check_exact_renames () {
+ check_common &&
+ rename_undetected 0 &&
+ rename_undetected 1 &&
+ rename_undetected 2 &&
+ rename_detected 3
+}
+
+test_expect_success 'setup 1/2: basic setup' '
+ cat <<-\EOF >3-old &&
+ 33a
+ 33b
+ 33c
+ 33d
+ EOF
+ sed s/33/22/ <3-old >2-old &&
+ sed s/33/11/ <3-old >1-old &&
+ sed s/33/00/ <3-old >0-old &&
+ git add [0-3]-old &&
+ git commit -m base &&
+ git rm [0-3]-old &&
+ git commit -m delete &&
+ git checkout -b rename HEAD^ &&
+ cp 3-old 3-new &&
+ sed 1,1s/./x/ <2-old >2-new &&
+ sed 1,2s/./x/ <1-old >1-new &&
+ sed 1,3s/./x/ <0-old >0-new &&
+ git add [0-3]-new &&
+ git rm [0-3]-old &&
+ git commit -m rename &&
+ get_expected_stages 0 &&
+ get_expected_stages 1 &&
+ get_expected_stages 2 &&
+ get_expected_stages 3 &&
+ check_50="false" &&
+ tail="HEAD^ -- HEAD master"
+'
+
+test_expect_success 'setup 2/2: threshold array' '
+ git diff --name-status -M01 HEAD^ HEAD >diff-output &&
+ test_debug "cat diff-output" &&
+ test_line_count = 4 diff-output &&
+ grep "R[0-9]\{3\} \([0-3]\)-old \1-new" diff-output \
+ >grep-output &&
+ test_cmp diff-output grep-output &&
+ th0=$(sed -n "s/R\(...\) 0-old 0-new/\1/p" <diff-output) &&
+ th1=$(sed -n "s/R\(...\) 1-old 1-new/\1/p" <diff-output) &&
+ th2=$(sed -n "s/R\(...\) 2-old 2-new/\1/p" <diff-output) &&
+ th3=$(sed -n "s/R\(...\) 3-old 3-new/\1/p" <diff-output) &&
+ test "$th0" -lt "$th1" &&
+ test "$th1" -lt "$th2" &&
+ test "$th2" -lt "$th3" &&
+ test "$th3" = 100 &&
+ if [ 50 -le "$th0" ]; then
+ check_50=check_threshold_0
+ elif [ 50 -le "$th1" ]; then
+ check_50=check_threshold_1
+ elif [ 50 -le "$th2" ]; then
+ check_50=check_threshold_2
+ fi &&
+ th0="$th0%" &&
+ th1="$th1%" &&
+ th2="$th2%" &&
+ th3="$th3%"
+'
+
+test_expect_success 'assumption for tests: rename detection with diff' '
+ git diff --name-status -M$th0 --diff-filter=R HEAD^ HEAD \
+ >diff-output-0 &&
+ git diff --name-status -M$th1 --diff-filter=R HEAD^ HEAD \
+ >diff-output-1 &&
+ git diff --name-status -M$th2 --diff-filter=R HEAD^ HEAD \
+ >diff-output-2 &&
+ git diff --name-status -M100% --diff-filter=R HEAD^ HEAD \
+ >diff-output-3 &&
+ test_line_count = 4 diff-output-0 &&
+ test_line_count = 3 diff-output-1 &&
+ test_line_count = 2 diff-output-2 &&
+ test_line_count = 1 diff-output-3
+'
+
+test_expect_success 'default similarity threshold is 50%' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive $tail &&
+ $check_50
+'
+
+test_expect_success 'low rename threshold' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=$th0 $tail &&
+ check_threshold_0
+'
+
+test_expect_success 'medium rename threshold' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=$th1 $tail &&
+ check_threshold_1
+'
+
+test_expect_success 'high rename threshold' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=$th2 $tail &&
+ check_threshold_2
+'
+
+test_expect_success 'exact renames only' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=100% $tail &&
+ check_exact_renames
+'
+
+test_expect_success 'rename threshold is truncated' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=200% $tail &&
+ check_exact_renames
+'
+
+test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive \
+ --find-renames=$th0 --find-renames=$th2 $tail &&
+ check_threshold_2
+'
+
+test_expect_success '--find-renames resets threshold' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive \
+ --find-renames=$th0 --find-renames $tail &&
+ $check_50
+'
+
+test_expect_success 'assumption for further tests: trivial merge succeeds' '
+ git read-tree --reset -u HEAD &&
+ git merge-recursive HEAD -- HEAD HEAD &&
+ git diff --quiet --cached &&
+ git merge-recursive --find-renames=$th0 HEAD -- HEAD HEAD &&
+ git diff --quiet --cached &&
+ git merge-recursive --find-renames=$th2 HEAD -- HEAD HEAD &&
+ git diff --quiet --cached &&
+ git merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&
+ git diff --quiet --cached
+'
+
+test_expect_success '--find-renames rejects negative argument' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=-25 \
+ HEAD -- HEAD HEAD &&
+ git diff --quiet --cached
+'
+
+test_expect_success '--find-renames rejects non-numbers' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --find-renames=0xf \
+ HEAD -- HEAD HEAD &&
+ git diff --quiet --cached
+'
+
+test_done
--
2.7.1.492.gd821b20
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 2/3] t3034: test option to disable renames
2016-02-23 23:48 [PATCH v2 0/3] Tests for merge-recursive rename options Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 1/3] t3034: add rename threshold tests Felipe Gonçalves Assis
@ 2016-02-23 23:48 ` Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 3/3] t3034: test deprecated interface Felipe Gonçalves Assis
2 siblings, 0 replies; 7+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-23 23:48 UTC (permalink / raw)
To: git
Cc: Johannes.Schindelin, gitster, sunshine,
Felipe Gonçalves Assis, Felipe Gonçalves Assis
From: Felipe Gonçalves Assis <felipeg.assis@gmail.com>
Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
t/t3034-merge-recursive-rename-options.sh | 28 ++++++++++++++++++++++++++++
1 file changed, 28 insertions(+)
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index fbec68c..c07a4cb 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -83,6 +83,14 @@ check_exact_renames () {
rename_detected 3
}
+check_no_renames () {
+ check_common &&
+ rename_undetected 0 &&
+ rename_undetected 1 &&
+ rename_undetected 2 &&
+ rename_undetected 3
+}
+
test_expect_success 'setup 1/2: basic setup' '
cat <<-\EOF >3-old &&
33a
@@ -192,6 +200,12 @@ test_expect_success 'rename threshold is truncated' '
check_exact_renames
'
+test_expect_success 'disabled rename detection' '
+ git read-tree --reset -u HEAD &&
+ git merge-recursive --no-renames $tail &&
+ check_no_renames
+'
+
test_expect_success 'last wins in --find-renames=<m> --find-renames=<n>' '
git read-tree --reset -u HEAD &&
test_must_fail git merge-recursive \
@@ -206,6 +220,18 @@ test_expect_success '--find-renames resets threshold' '
$check_50
'
+test_expect_success 'last wins in --no-renames --find-renames' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --no-renames --find-renames $tail &&
+ $check_50
+'
+
+test_expect_success 'last wins in --find-renames --no-renames' '
+ git read-tree --reset -u HEAD &&
+ git merge-recursive --find-renames --no-renames $tail &&
+ check_no_renames
+'
+
test_expect_success 'assumption for further tests: trivial merge succeeds' '
git read-tree --reset -u HEAD &&
git merge-recursive HEAD -- HEAD HEAD &&
@@ -215,6 +241,8 @@ test_expect_success 'assumption for further tests: trivial merge succeeds' '
git merge-recursive --find-renames=$th2 HEAD -- HEAD HEAD &&
git diff --quiet --cached &&
git merge-recursive --find-renames=100% HEAD -- HEAD HEAD &&
+ git diff --quiet --cached &&
+ git merge-recursive --no-renames HEAD -- HEAD HEAD &&
git diff --quiet --cached
'
--
2.7.1.492.gd821b20
^ permalink raw reply related [flat|nested] 7+ messages in thread
* [PATCH 3/3] t3034: test deprecated interface
2016-02-23 23:48 [PATCH v2 0/3] Tests for merge-recursive rename options Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 1/3] t3034: add rename threshold tests Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 2/3] t3034: test option to disable renames Felipe Gonçalves Assis
@ 2016-02-23 23:48 ` Felipe Gonçalves Assis
2 siblings, 0 replies; 7+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-23 23:48 UTC (permalink / raw)
To: git
Cc: Johannes.Schindelin, gitster, sunshine,
Felipe Gonçalves Assis, Felipe Gonçalves Assis
From: Felipe Gonçalves Assis <felipeg.assis@gmail.com>
--find-renames= and --rename-threshold= should be aliases.
Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
---
t/t3034-merge-recursive-rename-options.sh | 46 +++++++++++++++++++++++++++++++
1 file changed, 46 insertions(+)
diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
index c07a4cb..96d8767 100755
--- a/t/t3034-merge-recursive-rename-options.sh
+++ b/t/t3034-merge-recursive-rename-options.sh
@@ -260,4 +260,50 @@ test_expect_success '--find-renames rejects non-numbers' '
git diff --quiet --cached
'
+test_expect_success 'rename-threshold=<n> is a synonym for find-renames=<n>' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --rename-threshold=$th0 $tail &&
+ check_threshold_0
+'
+
+test_expect_success 'last wins in --no-renames --rename-threshold=<n>' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --no-renames --rename-threshold=$th0 $tail &&
+ check_threshold_0
+'
+
+test_expect_success 'last wins in --rename-threshold=<n> --no-renames' '
+ git read-tree --reset -u HEAD &&
+ git merge-recursive --rename-threshold=$th0 --no-renames $tail &&
+ check_no_renames
+'
+
+test_expect_success '--rename-threshold=<n> rejects negative argument' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --rename-threshold=-25 \
+ HEAD -- HEAD HEAD &&
+ git diff --quiet --cached
+'
+
+test_expect_success '--rename-threshold=<n> rejects non-numbers' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive --rename-threshold=0xf \
+ HEAD -- HEAD HEAD &&
+ git diff --quiet --cached
+'
+
+test_expect_success 'last wins in --rename-threshold=<m> --find-renames=<n>' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive \
+ --rename-threshold=$th0 --find-renames=$th2 $tail &&
+ check_threshold_2
+'
+
+test_expect_success 'last wins in --find-renames=<m> --rename-threshold=<n>' '
+ git read-tree --reset -u HEAD &&
+ test_must_fail git merge-recursive \
+ --find-renames=$th2 --rename-threshold=$th0 $tail &&
+ check_threshold_0
+'
+
test_done
--
2.7.1.492.gd821b20
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] t3034: add rename threshold tests
2016-02-23 23:48 ` [PATCH 1/3] t3034: add rename threshold tests Felipe Gonçalves Assis
@ 2016-02-24 0:50 ` Eric Sunshine
2016-02-24 0:55 ` Eric Sunshine
2016-02-24 1:15 ` Felipe Gonçalves Assis
0 siblings, 2 replies; 7+ messages in thread
From: Eric Sunshine @ 2016-02-24 0:50 UTC (permalink / raw)
To: Felipe Gonçalves Assis
Cc: Git List, Johannes Schindelin, Junio C Hamano,
Felipe Gonçalves Assis
On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
<felipeg.assis@gmail.com> wrote:
> 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.
>
> Also update name and description of t3032 for consistency:
> "merge-recursive options" -> "merge-recursive space options"
A few superficial comments below...
> Signed-off-by: Felipe Gonçalves Assis <felipegassis@gmail.com>
> ---
> diff --git a/t/t3034-merge-recursive-rename-options.sh b/t/t3034-merge-recursive-rename-options.sh
> @@ -0,0 +1,235 @@
> +test_expect_success 'setup 1/2: basic setup' '
If someone ever adds a third setup test, it's likely that that person
will forget to update this title to say "1/3" (ditto regarding "setup
2/2" below). Perhaps just call this "setup repo" and 2/2 "setup
thresholds" or something.
> + cat <<-\EOF >3-old &&
> + 33a
> + 33b
> + 33c
> + 33d
> + EOF
> + sed s/33/22/ <3-old >2-old &&
> + sed s/33/11/ <3-old >1-old &&
> + sed s/33/00/ <3-old >0-old &&
> + git add [0-3]-old &&
> + git commit -m base &&
> + git rm [0-3]-old &&
> + git commit -m delete &&
> + git checkout -b rename HEAD^ &&
> + cp 3-old 3-new &&
> + sed 1,1s/./x/ <2-old >2-new &&
> + sed 1,2s/./x/ <1-old >1-new &&
> + sed 1,3s/./x/ <0-old >0-new &&
> + git add [0-3]-new &&
> + git rm [0-3]-old &&
> + git commit -m rename &&
> + get_expected_stages 0 &&
> + get_expected_stages 1 &&
> + get_expected_stages 2 &&
> + get_expected_stages 3 &&
> + check_50="false" &&
Why isn't this assignment done in setup 2/2 where all the other
assignments to 'check_50' are done?
> + tail="HEAD^ -- HEAD master"
> +'
> +
> +test_expect_success 'setup 2/2: threshold array' '
> + git diff --name-status -M01 HEAD^ HEAD >diff-output &&
> + test_debug "cat diff-output" &&
> + test_line_count = 4 diff-output &&
> + grep "R[0-9]\{3\} \([0-3]\)-old \1-new" diff-output \
> + >grep-output &&
I'd be slightly concerned about the use of \{3\} with older grep's
such as on Solaris, and would just code it as "[0-9][0-9][0-9]" and be
done with it, but perhaps it's not worth worrying about until someone
complains.
> + test_cmp diff-output grep-output &&
So, this is asserting that you only see "renames" and nothing else. Okay.
> + th0=$(sed -n "s/R\(...\) 0-old 0-new/\1/p" <diff-output) &&
> + th1=$(sed -n "s/R\(...\) 1-old 1-new/\1/p" <diff-output) &&
> + th2=$(sed -n "s/R\(...\) 2-old 2-new/\1/p" <diff-output) &&
> + th3=$(sed -n "s/R\(...\) 3-old 3-new/\1/p" <diff-output) &&
> + test "$th0" -lt "$th1" &&
> + test "$th1" -lt "$th2" &&
> + test "$th2" -lt "$th3" &&
> + test "$th3" = 100 &&
It's very slightly odd to see '=' rather than '-eq' among all these
other algebraic operators ('-lt', '-le'), but not so odd that I'd
mention it (unless I just did), and not necessarily worth changing.
> + if [ 50 -le "$th0" ]; then
> + check_50=check_threshold_0
> + elif [ 50 -le "$th1" ]; then
> + check_50=check_threshold_1
> + elif [ 50 -le "$th2" ]; then
> + check_50=check_threshold_2
> + fi &&
> + th0="$th0%" &&
> + th1="$th1%" &&
> + th2="$th2%" &&
> + th3="$th3%"
> +'
> +
> +test_expect_success 'assumption for tests: rename detection with diff' '
> + git diff --name-status -M$th0 --diff-filter=R HEAD^ HEAD \
> + >diff-output-0 &&
> + git diff --name-status -M$th1 --diff-filter=R HEAD^ HEAD \
> + >diff-output-1 &&
> + git diff --name-status -M$th2 --diff-filter=R HEAD^ HEAD \
> + >diff-output-2 &&
> + git diff --name-status -M100% --diff-filter=R HEAD^ HEAD \
> + >diff-output-3 &&
> + test_line_count = 4 diff-output-0 &&
> + test_line_count = 3 diff-output-1 &&
> + test_line_count = 2 diff-output-2 &&
> + test_line_count = 1 diff-output-3
> +'
> +
> +test_expect_success 'default similarity threshold is 50%' '
> + git read-tree --reset -u HEAD &&
> + test_must_fail git merge-recursive $tail &&
> + $check_50
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] t3034: add rename threshold tests
2016-02-24 0:50 ` Eric Sunshine
@ 2016-02-24 0:55 ` Eric Sunshine
2016-02-24 1:15 ` Felipe Gonçalves Assis
1 sibling, 0 replies; 7+ messages in thread
From: Eric Sunshine @ 2016-02-24 0:55 UTC (permalink / raw)
To: Felipe Gonçalves Assis
Cc: Git List, Johannes Schindelin, Junio C Hamano,
Felipe Gonçalves Assis
On Tue, Feb 23, 2016 at 7:50 PM, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> + if [ 50 -le "$th0" ]; then
>> + check_50=check_threshold_0
>> + elif [ 50 -le "$th1" ]; then
>> + check_50=check_threshold_1
>> + elif [ 50 -le "$th2" ]; then
>> + check_50=check_threshold_2
>> + fi &&
Oh, I totally forgot to mention a few style issues:
1. use 'test' rather than '['
2. place 'then' on its own line
3. drop semicolon
if test 50 -le "$th0"
then
...
elif test ...
then
...
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH 1/3] t3034: add rename threshold tests
2016-02-24 0:50 ` Eric Sunshine
2016-02-24 0:55 ` Eric Sunshine
@ 2016-02-24 1:15 ` Felipe Gonçalves Assis
1 sibling, 0 replies; 7+ messages in thread
From: Felipe Gonçalves Assis @ 2016-02-24 1:15 UTC (permalink / raw)
To: Eric Sunshine
Cc: Git List, Johannes Schindelin, Junio C Hamano,
Felipe Gonçalves Assis
On 23 February 2016 at 21:50, Eric Sunshine <sunshine@sunshineco.com> wrote:
> On Tue, Feb 23, 2016 at 6:48 PM, Felipe Gonçalves Assis
> <felipeg.assis@gmail.com> wrote:
>> 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.
>>
>> Also update name and description of t3032 for consistency:
>> "merge-recursive options" -> "merge-recursive space options"
>
> A few superficial comments below...
>
>> + cat <<-\EOF >3-old &&
>> + 33a
>> + 33b
>> + 33c
>> + 33d
>> + EOF
>> + sed s/33/22/ <3-old >2-old &&
>> + sed s/33/11/ <3-old >1-old &&
>> + sed s/33/00/ <3-old >0-old &&
>> + git add [0-3]-old &&
>> + git commit -m base &&
>> + git rm [0-3]-old &&
>> + git commit -m delete &&
>> + git checkout -b rename HEAD^ &&
>> + cp 3-old 3-new &&
>> + sed 1,1s/./x/ <2-old >2-new &&
>> + sed 1,2s/./x/ <1-old >1-new &&
>> + sed 1,3s/./x/ <0-old >0-new &&
>> + git add [0-3]-new &&
>> + git rm [0-3]-old &&
>> + git commit -m rename &&
>> + get_expected_stages 0 &&
>> + get_expected_stages 1 &&
>> + get_expected_stages 2 &&
>> + get_expected_stages 3 &&
>> + check_50="false" &&
>
> Why isn't this assignment done in setup 2/2 where all the other
> assignments to 'check_50' are done?
>
Oh, it might be a minor thing, but I would like to make tests using
check_50 fail if setup 2/2 is skipped, instead of succeeding without
any actual checks.
>> + th0=$(sed -n "s/R\(...\) 0-old 0-new/\1/p" <diff-output) &&
>> + th1=$(sed -n "s/R\(...\) 1-old 1-new/\1/p" <diff-output) &&
>> + th2=$(sed -n "s/R\(...\) 2-old 2-new/\1/p" <diff-output) &&
>> + th3=$(sed -n "s/R\(...\) 3-old 3-new/\1/p" <diff-output) &&
>> + test "$th0" -lt "$th1" &&
>> + test "$th1" -lt "$th2" &&
>> + test "$th2" -lt "$th3" &&
>> + test "$th3" = 100 &&
>
> It's very slightly odd to see '=' rather than '-eq' among all these
> other algebraic operators ('-lt', '-le'), but not so odd that I'd
> mention it (unless I just did), and not necessarily worth changing.
>
Here I favoured the stricter test. If the string is not exactly "100",
then a lot should be reviewed.
Once more, thanks for the review. Your other comments should be
included in the next version of the patches.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2016-02-24 1:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2016-02-23 23:48 [PATCH v2 0/3] Tests for merge-recursive rename options Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 1/3] t3034: add rename threshold tests Felipe Gonçalves Assis
2016-02-24 0:50 ` Eric Sunshine
2016-02-24 0:55 ` Eric Sunshine
2016-02-24 1:15 ` Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 2/3] t3034: test option to disable renames Felipe Gonçalves Assis
2016-02-23 23:48 ` [PATCH 3/3] t3034: test deprecated interface Felipe Gonçalves Assis
This is an external index of several public inboxes,
see mirroring instructions on how to clone and mirror
all data and code used by this external index.