[PATCH 4/5] t7610-mergetool: prefer test_config over git config

[David Aguilar]

Signed-off-by: David Aguilar <***@gmail.com>
---
t/t7610-mergetool.sh | 20 +++++++++-----------
1 file changed, 9 insertions(+), 11 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1a15e06..7eeb207 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -14,7 +14,7 @@ Testing basic merge tool invocation'
# running mergetool

test_expect_success 'setup' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
echo master >file1 &&
echo master spaced >"spaced name" &&
echo master file11 >file11 &&
@@ -112,7 +112,7 @@ test_expect_success 'custom mergetool' '
'

test_expect_success 'mergetool crlf' '
- git config core.autocrlf true &&
+ test_config core.autocrlf true &&
git checkout -b test2 branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
@@ -129,7 +129,7 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&
- git config core.autocrlf false &&
+ test_config core.autocrlf false &&
git reset --hard
'

@@ -176,7 +176,7 @@ test_expect_success 'mergetool skips autoresolved' '
test_expect_success 'mergetool merges all from subdir' '
(
cd subdir &&
- git config rerere.enabled false &&
+ test_config rerere.enabled false &&
test_must_fail git merge master &&
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&
@@ -190,7 +190,7 @@ test_expect_success 'mergetool merges all from subdir' '
'

test_expect_success 'mergetool skips resolved paths when rerere is active' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test5 branch1 &&
git submodule update -N &&
@@ -204,7 +204,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
'

test_expect_success 'conflicted stash sets up rerere' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
git stash &&
@@ -232,7 +232,7 @@ test_expect_success 'conflicted stash sets up rerere' '

test_expect_success 'mergetool takes partial path' '
git reset --hard &&
- git config rerere.enabled false &&
+ test_config rerere.enabled false &&
git checkout -b test12 branch1 &&
git submodule update -N &&
test_must_fail git merge master &&
@@ -505,14 +505,12 @@ test_expect_success 'file with no base' '

test_expect_success 'custom commands override built-ins' '
git checkout -b test14 branch1 &&
- git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
- git config mergetool.defaults.trustExitCode true &&
+ test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
+ test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool defaults -- both &&
echo master both added >expected &&
test_cmp both expected &&
- git config --unset mergetool.defaults.cmd &&
- git config --unset mergetool.defaults.trustExitCode &&
git reset --hard master >/dev/null 2>&1
'

[David Aguilar]

Signed-off-by: David Aguilar <***@gmail.com>
---
t/t7610-mergetool.sh | 10 ++--------
1 file changed, 2 insertions(+), 8 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 875c8af..214edfb 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -192,7 +192,7 @@ test_expect_success 'mergetool merges all from subdir' '
test_expect_success 'mergetool skips resolved paths when rerere is active' '
git config rerere.enabled true &&
rm -rf .git/rr-cache &&
- git checkout -b test5 branch1
+ git checkout -b test5 branch1 &&
git submodule update -N &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "l" | git mergetool --no-prompt submod >/dev/null 2>&1 ) &&
@@ -231,18 +231,12 @@ test_expect_success 'conflicted stash sets up rerere' '
'

test_expect_success 'mergetool takes partial path' '
- git reset --hard
+ git reset --hard &&
git config rerere.enabled false &&
git checkout -b test12 branch1 &&
git submodule update -N &&
test_must_fail git merge master &&

- #should not need these lines
- #( yes "d" | git mergetool file11 >/dev/null 2>&1 ) &&
- #( yes "d" | git mergetool file12 >/dev/null 2>&1 ) &&
- #( yes "l" | git mergetool submod >/dev/null 2>&1 ) &&
- #( yes "" | git mergetool file1 file2 >/dev/null 2>&1 ) &&
-
( yes "" | git mergetool subdir ) &&

test "$(cat subdir/file3)" = "master new sub" &&

[David Aguilar]

Add tests to ensure that filenames start with "./" when
mergetool.writeToTemp is false and do not start with "./" when
mergetool.writeToTemp is true.

Signed-off-by: David Aguilar <***@gmail.com>
---
t/t7610-mergetool.sh | 23 +++++++++++++++++++++++
1 file changed, 23 insertions(+)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 214edfb..1a15e06 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -516,4 +516,27 @@ test_expect_success 'custom commands override built-ins' '
git reset --hard master >/dev/null 2>&1
'

+test_expect_success 'filenames seen by tools start with ./' '
+ git checkout -b test15 branch1 &&
+ test_config mergetool.writeToTemp false &&
+ test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+ test_config mergetool.myecho.trustExitCode true &&
+ test_must_fail git merge master &&
+ git mergetool --no-prompt --tool myecho -- both >actual &&
+ grep ^\./both_LOCAL_ actual >/dev/null &&
+ git reset --hard master >/dev/null 2>&1
+'
+
+test_expect_success 'temporary filenames are used with mergetool.writeToTemp' '
+ git checkout -b test16 branch1 &&
+ test_config mergetool.writeToTemp true &&
+ test_config mergetool.myecho.cmd "echo \"\$LOCAL\"" &&
+ test_config mergetool.myecho.trustExitCode true &&
+ test_must_fail git merge master &&
+ git mergetool --no-prompt --tool myecho -- both >actual &&
+ test_must_fail grep ^\./both_LOCAL_ actual >/dev/null &&
+ grep /both_LOCAL_ actual >/dev/null &&
+ git reset --hard master >/dev/null 2>&1
+'
+
test_done

[David Aguilar]

Prefer "test" over "[ ]" for conditionals.
Prefer "$()" over backticks for command substitutions.
Avoid control structures on a single line with semicolons.

Signed-off-by: David Aguilar <***@gmail.com>
---
t/test-lib-functions.sh | 30 ++++++++++++++++++------------
1 file changed, 18 insertions(+), 12 deletions(-)

diff --git a/t/test-lib-functions.sh b/t/test-lib-functions.sh
index dafd6ad..fc77cc6 100644
--- a/t/test-lib-functions.sh
+++ b/t/test-lib-functions.sh
@@ -413,7 +413,7 @@ test_external () {
# test_run_, but keep its stdout on our stdout even in
# non-verbose mode.
"$@" 2>&4
- if [ "$?" = 0 ]
+ if test "$?" = 0
then
if test $test_external_has_tap -eq 0; then
test_ok_ "$descr"
@@ -440,11 +440,12 @@ test_external_without_stderr () {
tmp=${TMPDIR:-/tmp}
stderr="$tmp/git-external-stderr.$$.tmp"
test_external "$@" 4> "$stderr"
- [ -f "$stderr" ] || error "Internal error: $stderr disappeared."
+ test -f "$stderr" || error "Internal error: $stderr disappeared."
descr="no stderr: $1"
shift
say >&3 "# expecting no stderr from previous command"
- if [ ! -s "$stderr" ]; then
+ if test ! -s "$stderr"
+ then
rm "$stderr"

if test $test_external_has_tap -eq 0; then
@@ -454,8 +455,9 @@ test_external_without_stderr () {
test_success=$(($test_success + 1))
fi
else
- if [ "$verbose" = t ]; then
- output=`echo; echo "# Stderr is:"; cat "$stderr"`
+ if test "$verbose" = t
+ then
+ output=$(echo; echo "# Stderr is:"; cat "$stderr")
else
output=
fi
@@ -474,7 +476,7 @@ test_external_without_stderr () {
# The commands test the existence or non-existence of $1. $2 can be
# given to provide a more precise diagnosis.
test_path_is_file () {
- if ! [ -f "$1" ]
+ if ! test -f "$1"
then
echo "File $1 doesn't exist. $*"
false
@@ -482,7 +484,7 @@ test_path_is_file () {
}

test_path_is_dir () {
- if ! [ -d "$1" ]
+ if ! test -d "$1"
then
echo "Directory $1 doesn't exist. $*"
false
@@ -501,11 +503,12 @@ test_dir_is_empty () {
}

test_path_is_missing () {
- if [ -e "$1" ]
+ if test -e "$1"
then
echo "Path exists:"
ls -ld "$1"
- if [ $# -ge 1 ]; then
+ if test $# -ge 1
+ then
echo "$*"
fi
false
@@ -657,9 +660,12 @@ test_cmp_rev () {
# similar to GNU seq(1), but the latter might not be available
# everywhere (and does not do letters). It may be used like:
#
-# for i in `test_seq 100`; do
-# for j in `test_seq 10 20`; do
-# for k in `test_seq a z`; do
+# for i in $(test_seq 100)
+# do
+# for j in $(test_seq 10 20)
+# do
+# for k in $(test_seq a z)
+# do
# echo $i-$j-$k
# done
# done

[Junio C Hamano]

Post by David Aguilar
---

The reason why this change favours "test_config" over "git config"
is not described here, but if we think about that, some of the
changes in this may become questionable.

Post by David Aguilar
diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1a15e06..7eeb207 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -14,7 +14,7 @@ Testing basic merge tool invocation'
# running mergetool
test_expect_success 'setup' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
echo master >file1 &&
echo master spaced >"spaced name" &&
echo master file11 >file11 &&

This one does not have corresponding "git config" to remove the
configuration when the setup is done, so this changes the
behaviour. The remainder of the tests will run without
rerere.enabled set.

If this change does not make a difference, perhaps we do not even
need to set rerere.enabled here in the first place?

But do later tests perform merges that can potentially be affected
by the setting of rerere.enabled? If so, this change can break
them. If this change does not break existing tests, I would say
this is a good change that anticipates that we may add more tests in
the future that work in subdir and that makes sure to leave subdir
in a predictable state for them.

Post by David Aguilar
@@ -112,7 +112,7 @@ test_expect_success 'custom mergetool' '
'
test_expect_success 'mergetool crlf' '
- git config core.autocrlf true &&
+ test_config core.autocrlf true &&
git checkout -b test2 branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
@@ -129,7 +129,7 @@ test_expect_success 'mergetool crlf' '
git submodule update -N &&
test "$(cat submod/bar)" = "master submodule" &&
git commit -m "branch1 resolved with mergetool - autocrlf" &&
- git config core.autocrlf false &&
+ test_config core.autocrlf false &&
git reset --hard
'

This one wants to set core.autocrlf to true while it runs, and then
wants to reset to the original. When the test fails in the middle,
however, we may abort this test and move on to the next one, while
leaving core.autcrlf still set to "true", which may break later
tests, hence use of test_config to ensure that the setting is
reverted at the end of the test is the right thing to do.

So the hunk #112 is correct, but the hunk #129 is questionable and
even misleading.

Post by David Aguilar
@@ -176,7 +176,7 @@ test_expect_success 'mergetool skips autoresolved' '
test_expect_success 'mergetool merges all from subdir' '
(
cd subdir &&
- git config rerere.enabled false &&
+ test_config rerere.enabled false &&
test_must_fail git merge master &&
( yes "r" | git mergetool ../submod ) &&
( yes "d" "d" | git mergetool --no-prompt ) &&

Same comment as the one for hunk #14 applies here. In principle,
this change will make this test more isolated and is a good thing.

Post by David Aguilar
@@ -190,7 +190,7 @@ test_expect_success 'mergetool merges all from subdir' '
'
test_expect_success 'mergetool skips resolved paths when rerere is active' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
rm -rf .git/rr-cache &&
git checkout -b test5 branch1 &&
git submodule update -N &&

Ditto.

Post by David Aguilar
@@ -204,7 +204,7 @@ test_expect_success 'mergetool skips resolved paths when rerere is active' '
'
test_expect_success 'conflicted stash sets up rerere' '
- git config rerere.enabled true &&
+ test_config rerere.enabled true &&
git checkout stash1 &&
echo "Conflicting stash content" >file11 &&
git stash &&

Ditto.

Post by David Aguilar
@@ -232,7 +232,7 @@ test_expect_success 'conflicted stash sets up rerere' '
test_expect_success 'mergetool takes partial path' '
git reset --hard &&
- git config rerere.enabled false &&
+ test_config rerere.enabled false &&
git checkout -b test12 branch1 &&
git submodule update -N &&
test_must_fail git merge master &&

Ditto.

Post by David Aguilar
@@ -505,14 +505,12 @@ test_expect_success 'file with no base' '
test_expect_success 'custom commands override built-ins' '
git checkout -b test14 branch1 &&
- git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
- git config mergetool.defaults.trustExitCode true &&
+ test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
+ test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool defaults -- both &&
echo master both added >expected &&
test_cmp both expected &&
- git config --unset mergetool.defaults.cmd &&
- git config --unset mergetool.defaults.trustExitCode &&
git reset --hard master >/dev/null 2>&1
'

This one is good; with test_config, you do not have to do the
unsetting yourself.

[David Aguilar]

Signed-off-by: David Aguilar <***@gmail.com>
---
This is a replacement patch for
"t7610-mergetool: prefer test_config over git config"
in da/mergetool-tests.

Changes since v1:

The changes are more surgical and the commit message mentions
why we are using test_config.

t/t7610-mergetool.sh | 8 +++-----
1 file changed, 3 insertions(+), 5 deletions(-)

diff --git a/t/t7610-mergetool.sh b/t/t7610-mergetool.sh
index 1a15e06..4fec633 100755
--- a/t/t7610-mergetool.sh
+++ b/t/t7610-mergetool.sh
@@ -112,7 +112,7 @@ test_expect_success 'custom mergetool' '
'

test_expect_success 'mergetool crlf' '
- git config core.autocrlf true &&
+ test_config core.autocrlf true &&
git checkout -b test2 branch1 &&
test_must_fail git merge master >/dev/null 2>&1 &&
( yes "" | git mergetool file1 >/dev/null 2>&1 ) &&
@@ -505,14 +505,12 @@ test_expect_success 'file with no base' '

test_expect_success 'custom commands override built-ins' '
git checkout -b test14 branch1 &&
- git config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
- git config mergetool.defaults.trustExitCode true &&
+ test_config mergetool.defaults.cmd "cat \"\$REMOTE\" >\"\$MERGED\"" &&
+ test_config mergetool.defaults.trustExitCode true &&
test_must_fail git merge master &&
git mergetool --no-prompt --tool defaults -- both &&
echo master both added >expected &&
test_cmp both expected &&
- git config --unset mergetool.defaults.cmd &&
- git config --unset mergetool.defaults.trustExitCode &&
git reset --hard master >/dev/null 2>&1
'