[PATCH 0/5] t1400: modernize style

[PATCH 0/5] t1400: modernize style

[Kyle Meyer]

These patches follow up on Peff's suggestion to modernize the style in
t1400-update-ref.sh.

    20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net

The first two commits aren't concerned with "modernizing" the tests,
but instead address issues that I noticed while looking more closely
at t1400.

I also considered

  * making the quoting/spacing/breaks around the test descriptions and
    bodies more consistent, but I think this leads to too much code
    churn.

  * moving the here-documents for log creation into the following
    tests, but I don't think it's worth it because it makes already
    long lines even longer.

  t1400: rename test descriptions to be unique
  t1400: set core.logAllRefUpdates in "logged by touch" tests
  t1400: use test_path_is_* helpers
  t1400: remove a set of unused output files
  t1400: use test_when_finished for cleanup

 t/t1400-update-ref.sh | 154 +++++++++++++++++++++++++-------------------------
 1 file changed, 78 insertions(+), 76 deletions(-)

-- 
2.12.0

[PATCH 1/5] t1400: rename test descriptions to be unique

[Kyle Meyer]

A few tests share their description with another test.  Extend the
descriptions to indicate how the tests differ.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 t/t1400-update-ref.sh | 8 ++++----
 1 file changed, 4 insertions(+), 4 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 825422341..fde5b98af 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -40,7 +40,7 @@ test_expect_success \
 	"git update-ref $m $A &&
 	 test $A"' = $(cat .git/'"$m"')'
 test_expect_success \
-	"create $m" \
+	"create $m with oldvalue verification" \
 	"git update-ref $m $B $A &&
 	 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "fail to delete $m with stale ref" '
@@ -72,7 +72,7 @@ test_expect_success \
 	"git update-ref HEAD $A &&
 	 test $A"' = $(cat .git/'"$m"')'
 test_expect_success \
-	"create $m (by HEAD)" \
+	"create $m (by HEAD) with oldvalue verification" \
 	"git update-ref HEAD $B $A &&
 	 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "fail to delete $m (by HEAD) with stale ref" '
@@ -307,7 +307,7 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
 EOF
 test_expect_success \
-	"verifying $m's log" \
+	"verifying $m's log (logged by touch)" \
 	"test_cmp expect .git/logs/$m"
 rm -rf .git/$m .git/logs expect
 
@@ -338,7 +338,7 @@ $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150380 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
 EOF
 test_expect_success \
-	"verifying $m's log" \
+	"verifying $m's log (logged by config)" \
 	'test_cmp expect .git/logs/$m'
 rm -f .git/$m .git/logs/$m expect
 
-- 
2.12.0

[PATCH 2/5] t1400: set core.logAllRefUpdates in "logged by touch" tests

[Kyle Meyer]

A group of update-ref tests verifies that logs are created when either
the log file for the ref already exists or core.logAllRefUpdates is
"true".  However, when the default for core.logAllRefUpdates was
changed in 0bee59186 (Enable reflogs by default in any repository with
a working directory., 2006-12-14), the setup for the tests was not
updated.  As a result, the "logged by touch" tests would pass even if
the log file did not exist (i.e., if "--create-reflog" was removed
from the first "git update-ref" call).

Update the "logged by touch" tests to disable core.logAllRefUpdates
explicitly so that the behavior does not depend on the default value.
While we're here, update the "logged by config" tests to use
test_config() rather than setting core.logAllRefUpdates to "true"
outside of the tests.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---

I'm confused about the setup for the "logged by touch" tests.
d0ab05849 (tests: remove some direct access to .git/logs, 2015-07-27)
changed the setup to delete the log file itself rather than its
contents.  The reflog was then recreated by using "--create-reflog" in
the "create $m (logged by touch)" test.  What I don't understand is
how this change fits with d0ab05849, which seems to be concerned with
loosening the assumption that the logs are stored in .git/logs.  For
these particular tests, we are still removing
.git/logs/refs/heads/master explictly, so why not leave the empty file
around so that the "create $m (logged by touch)" actually tests that
update-ref call is logged by the existence of the log rather than
using "--create-reflog", which overrides this?

 t/t1400-update-ref.sh | 23 ++++++++++++-----------
 1 file changed, 12 insertions(+), 11 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index fde5b98af..be8b113b1 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -260,17 +260,20 @@ rm -f .git/$m
 rm -f .git/logs/refs/heads/master
 test_expect_success \
 	"create $m (logged by touch)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:30" \
+	'test_config core.logAllRefUpdates false &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:30" \
 	 git update-ref --create-reflog HEAD '"$A"' -m "Initial Creation" &&
 	 test '"$A"' = $(cat .git/'"$m"')'
 test_expect_success \
 	"update $m (logged by touch)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:31" \
+	'test_config core.logAllRefUpdates false &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:31" \
 	 git update-ref HEAD'" $B $A "'-m "Switch" &&
 	 test '"$B"' = $(cat .git/'"$m"')'
 test_expect_success \
 	"set $m (logged by touch)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:41" \
+	'test_config core.logAllRefUpdates false &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:41" \
 	 git update-ref HEAD'" $A &&
 	 test $A"' = $(cat .git/'"$m"')'
 
@@ -312,23 +315,21 @@ test_expect_success \
 rm -rf .git/$m .git/logs expect
 
 test_expect_success \
-	'enable core.logAllRefUpdates' \
-	'git config core.logAllRefUpdates true &&
-	 test true = $(git config --bool --get core.logAllRefUpdates)'
-
-test_expect_success \
 	"create $m (logged by config)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:32" \
+	'test_config core.logAllRefUpdates true &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:32" \
 	 git update-ref HEAD'" $A "'-m "Initial Creation" &&
 	 test '"$A"' = $(cat .git/'"$m"')'
 test_expect_success \
 	"update $m (logged by config)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:33" \
+	'test_config core.logAllRefUpdates true &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:33" \
 	 git update-ref HEAD'" $B $A "'-m "Switch" &&
 	 test '"$B"' = $(cat .git/'"$m"')'
 test_expect_success \
 	"set $m (logged by config)" \
-	'GIT_COMMITTER_DATE="2005-05-26 23:43" \
+	'test_config core.logAllRefUpdates true &&
+	 GIT_COMMITTER_DATE="2005-05-26 23:43" \
 	 git update-ref HEAD '"$A &&
 	 test $A"' = $(cat .git/'"$m"')'
 
-- 
2.12.0

[PATCH 3/5] t1400: use test_path_is_* helpers

[Kyle Meyer]

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 t/t1400-update-ref.sh | 44 ++++++++++++++++++++++----------------------
 1 file changed, 22 insertions(+), 22 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index be8b113b1..c5c8e95fc 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -49,7 +49,7 @@ test_expect_success "fail to delete $m with stale ref" '
 '
 test_expect_success "delete $m" '
 	git update-ref -d $m $B &&
-	! test -f .git/$m
+	test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
@@ -57,7 +57,7 @@ test_expect_success "delete $m without oldvalue verification" "
 	git update-ref $m $A &&
 	test $A = \$(cat .git/$m) &&
 	git update-ref -d $m &&
-	! test -f .git/$m
+	test_path_is_missing .git/$m
 "
 rm -f .git/$m
 
@@ -81,7 +81,7 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" '
 '
 test_expect_success "delete $m (by HEAD)" '
 	git update-ref -d HEAD $B &&
-	! test -f .git/$m
+	test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
@@ -89,7 +89,7 @@ test_expect_success "deleting current branch adds message to HEAD's log" '
 	git update-ref $m $A &&
 	git symbolic-ref HEAD $m &&
 	git update-ref -m delete-$m -d $m &&
-	! test -f .git/$m &&
+	test_path_is_missing .git/$m &&
 	grep "delete-$m$" .git/logs/HEAD
 '
 rm -f .git/$m
@@ -98,7 +98,7 @@ test_expect_success "deleting by HEAD adds message to HEAD's log" '
 	git update-ref $m $A &&
 	git symbolic-ref HEAD $m &&
 	git update-ref -m delete-by-head -d HEAD &&
-	! test -f .git/$m &&
+	test_path_is_missing .git/$m &&
 	grep "delete-by-head$" .git/logs/HEAD
 '
 rm -f .git/$m
@@ -190,14 +190,14 @@ test_expect_success \
 test_expect_success "delete $m (by HEAD) should remove both packed and loose $m" '
 	git update-ref -d HEAD $B &&
 	! grep "$m" .git/packed-refs &&
-	! test -f .git/$m
+	test_path_is_missing .git/$m
 '
 rm -f .git/$m
 
 cp -f .git/HEAD .git/HEAD.orig
 test_expect_success "delete symref without dereference" '
 	git update-ref --no-deref -d HEAD &&
-	! test -f .git/HEAD
+	test_path_is_missing .git/HEAD
 '
 cp -f .git/HEAD.orig .git/HEAD
 
@@ -207,7 +207,7 @@ test_expect_success "delete symref without dereference when the referred ref is
 	git commit -m foo &&
 	git pack-refs --all &&
 	git update-ref --no-deref -d HEAD &&
-	! test -f .git/HEAD
+	test_path_is_missing .git/HEAD
 '
 cp -f .git/HEAD.orig .git/HEAD
 git update-ref -d $m
@@ -242,7 +242,7 @@ test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
 test_expect_success "(not) prior created .git/$m" "
-	! test -f .git/$m
+	test_path_is_missing .git/$m
 "
 rm -f .git/$m
 
@@ -280,13 +280,13 @@ test_expect_success \
 test_expect_success "empty directory removal" '
 	git branch d1/d2/r1 HEAD &&
 	git branch d1/r2 HEAD &&
-	test -f .git/refs/heads/d1/d2/r1 &&
-	test -f .git/logs/refs/heads/d1/d2/r1 &&
+	test_path_is_file .git/refs/heads/d1/d2/r1 &&
+	test_path_is_file .git/logs/refs/heads/d1/d2/r1 &&
 	git branch -d d1/d2/r1 &&
-	! test -e .git/refs/heads/d1/d2 &&
-	! test -e .git/logs/refs/heads/d1/d2 &&
-	test -f .git/refs/heads/d1/r2 &&
-	test -f .git/logs/refs/heads/d1/r2
+	test_path_is_missing .git/refs/heads/d1/d2 &&
+	test_path_is_missing .git/logs/refs/heads/d1/d2 &&
+	test_path_is_file .git/refs/heads/d1/r2 &&
+	test_path_is_file .git/logs/refs/heads/d1/r2
 '
 
 test_expect_success "symref empty directory removal" '
@@ -294,14 +294,14 @@ test_expect_success "symref empty directory removal" '
 	git branch e1/r2 HEAD &&
 	git checkout e1/e2/r1 &&
 	test_when_finished "git checkout master" &&
-	test -f .git/refs/heads/e1/e2/r1 &&
-	test -f .git/logs/refs/heads/e1/e2/r1 &&
+	test_path_is_file .git/refs/heads/e1/e2/r1 &&
+	test_path_is_file .git/logs/refs/heads/e1/e2/r1 &&
 	git update-ref -d HEAD &&
-	! test -e .git/refs/heads/e1/e2 &&
-	! test -e .git/logs/refs/heads/e1/e2 &&
-	test -f .git/refs/heads/e1/r2 &&
-	test -f .git/logs/refs/heads/e1/r2 &&
-	test -f .git/logs/HEAD
+	test_path_is_missing .git/refs/heads/e1/e2 &&
+	test_path_is_missing .git/logs/refs/heads/e1/e2 &&
+	test_path_is_file .git/refs/heads/e1/r2 &&
+	test_path_is_file .git/logs/refs/heads/e1/r2 &&
+	test_path_is_file .git/logs/HEAD
 '
 
 cat >expect <<EOF
-- 
2.12.0

[PATCH 4/5] t1400: remove a set of unused output files

[Kyle Meyer]

This test case redirects stdout and stderr to output files, but,
unlike the other cases of redirection in the t1400 tests, these files
are not examined downstream.  Remove the redirection so that the
output is visible when running the tests verbosely.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 t/t1400-update-ref.sh | 4 ++--
 1 file changed, 2 insertions(+), 2 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index c5c8e95fc..9e7e0227e 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -64,8 +64,8 @@ rm -f .git/$m
 test_expect_success \
 	"fail to create $n" \
 	"touch .git/$n_dir &&
-	 test_must_fail git update-ref $n $A >out 2>err"
-rm -f .git/$n_dir out err
+	 test_must_fail git update-ref $n $A"
+rm -f .git/$n_dir
 
 test_expect_success \
 	"create $m (by HEAD)" \
-- 
2.12.0

[PATCH 5/5] t1400: use test_when_finished for cleanup

[Kyle Meyer]

Move cleanup lines that occur after test blocks into
test_when_finished calls within the test bodies.  Don't move cleanup
lines that seem to be related to mutiple tests rather than a single
test.

Signed-off-by: Kyle Meyer <kyle@kyleam.com>
---
 t/t1400-update-ref.sh | 81 ++++++++++++++++++++++++++-------------------------
 1 file changed, 41 insertions(+), 40 deletions(-)

diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
index 9e7e0227e..a23cd7b57 100755
--- a/t/t1400-update-ref.sh
+++ b/t/t1400-update-ref.sh
@@ -48,24 +48,24 @@ test_expect_success "fail to delete $m with stale ref" '
 	test $B = "$(cat .git/$m)"
 '
 test_expect_success "delete $m" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref -d $m $B &&
 	test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
-test_expect_success "delete $m without oldvalue verification" "
+test_expect_success "delete $m without oldvalue verification" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref $m $A &&
-	test $A = \$(cat .git/$m) &&
+	test $A = $(cat .git/$m) &&
 	git update-ref -d $m &&
 	test_path_is_missing .git/$m
-"
-rm -f .git/$m
+'
 
-test_expect_success \
-	"fail to create $n" \
-	"touch .git/$n_dir &&
-	 test_must_fail git update-ref $n $A"
-rm -f .git/$n_dir
+test_expect_success "fail to create $n" '
+	test_when_finished "rm -f .git/$n_dir" &&
+	touch .git/$n_dir &&
+	test_must_fail git update-ref $n $A
+'
 
 test_expect_success \
 	"create $m (by HEAD)" \
@@ -80,28 +80,28 @@ test_expect_success "fail to delete $m (by HEAD) with stale ref" '
 	test $B = $(cat .git/$m)
 '
 test_expect_success "delete $m (by HEAD)" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref -d HEAD $B &&
 	test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
 test_expect_success "deleting current branch adds message to HEAD's log" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref $m $A &&
 	git symbolic-ref HEAD $m &&
 	git update-ref -m delete-$m -d $m &&
 	test_path_is_missing .git/$m &&
 	grep "delete-$m$" .git/logs/HEAD
 '
-rm -f .git/$m
 
 test_expect_success "deleting by HEAD adds message to HEAD's log" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref $m $A &&
 	git symbolic-ref HEAD $m &&
 	git update-ref -m delete-by-head -d HEAD &&
 	test_path_is_missing .git/$m &&
 	grep "delete-by-head$" .git/logs/HEAD
 '
-rm -f .git/$m
 
 test_expect_success 'update-ref does not create reflogs by default' '
 	test_when_finished "git update-ref -d $outside" &&
@@ -188,20 +188,21 @@ test_expect_success \
 	"git update-ref HEAD $B $A &&
 	 test $B"' = $(cat .git/'"$m"')'
 test_expect_success "delete $m (by HEAD) should remove both packed and loose $m" '
+	test_when_finished "rm -f .git/$m" &&
 	git update-ref -d HEAD $B &&
 	! grep "$m" .git/packed-refs &&
 	test_path_is_missing .git/$m
 '
-rm -f .git/$m
 
 cp -f .git/HEAD .git/HEAD.orig
 test_expect_success "delete symref without dereference" '
+	test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
 	git update-ref --no-deref -d HEAD &&
 	test_path_is_missing .git/HEAD
 '
-cp -f .git/HEAD.orig .git/HEAD
 
 test_expect_success "delete symref without dereference when the referred ref is packed" '
+	test_when_finished "cp -f .git/HEAD.orig .git/HEAD" &&
 	echo foo >foo.c &&
 	git add foo.c &&
 	git commit -m foo &&
@@ -209,7 +210,7 @@ test_expect_success "delete symref without dereference when the referred ref is
 	git update-ref --no-deref -d HEAD &&
 	test_path_is_missing .git/HEAD
 '
-cp -f .git/HEAD.orig .git/HEAD
+
 git update-ref -d $m
 
 test_expect_success 'update-ref -d is not confused by self-reference' '
@@ -241,10 +242,10 @@ test_expect_success 'update-ref --no-deref -d can delete reference to bad ref' '
 test_expect_success '(not) create HEAD with old sha1' "
 	test_must_fail git update-ref HEAD $A $B
 "
-test_expect_success "(not) prior created .git/$m" "
+test_expect_success "(not) prior created .git/$m" '
+	test_when_finished "rm -f .git/$m" &&
 	test_path_is_missing .git/$m
-"
-rm -f .git/$m
+'
 
 test_expect_success \
 	"create HEAD" \
@@ -252,10 +253,10 @@ test_expect_success \
 test_expect_success '(not) change HEAD with wrong SHA1' "
 	test_must_fail git update-ref HEAD $B $Z
 "
-test_expect_success "(not) changed .git/$m" "
-	! test $B"' = $(cat .git/'"$m"')
+test_expect_success "(not) changed .git/$m" '
+	test_when_finished "rm -f .git/$m" &&
+	! test $B = $(cat .git/$m)
 '
-rm -f .git/$m
 
 rm -f .git/logs/refs/heads/master
 test_expect_success \
@@ -309,10 +310,10 @@ $Z $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	Initial Creati
 $A $B $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150260 +0000	Switch
 $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150860 +0000
 EOF
-test_expect_success \
-	"verifying $m's log (logged by touch)" \
-	"test_cmp expect .git/logs/$m"
-rm -rf .git/$m .git/logs expect
+test_expect_success "verifying $m's log (logged by touch)" '
+	test_when_finished "rm -rf .git/$m .git/logs expect" &&
+	test_cmp expect .git/logs/$m
+'
 
 test_expect_success \
 	"create $m (logged by config)" \
@@ -340,8 +341,8 @@ $B $A $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150980 +0000
 EOF
 test_expect_success \
 	"verifying $m's log (logged by config)" \
-	'test_cmp expect .git/logs/$m'
-rm -f .git/$m .git/logs/$m expect
+	'test_when_finished "rm -f .git/$m .git/logs/$m expect" &&
+	 test_cmp expect .git/logs/$m'
 
 git update-ref $m $D
 cat >.git/logs/$m <<EOF
@@ -357,55 +358,55 @@ gd="Thu, 26 May 2005 18:33:00 -0500"
 ld="Thu, 26 May 2005 18:43:00 -0500"
 test_expect_success \
 	'Query "master@{May 25 2005}" (before history)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{May 25 2005}" >o 2>e &&
 	 test '"$C"' = $(cat o) &&
 	 test "warning: Log for '\'master\'' only goes back to $ed." = "$(cat e)"'
 test_expect_success \
 	"Query master@{2005-05-25} (before history)" \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify master@{2005-05-25} >o 2>e &&
 	 test '"$C"' = $(cat o) &&
 	 echo test "warning: Log for '\'master\'' only goes back to $ed." = "$(cat e)"'
 test_expect_success \
 	'Query "master@{May 26 2005 23:31:59}" (1 second before history)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{May 26 2005 23:31:59}" >o 2>e &&
 	 test '"$C"' = $(cat o) &&
 	 test "warning: Log for '\''master'\'' only goes back to $ed." = "$(cat e)"'
 test_expect_success \
 	'Query "master@{May 26 2005 23:32:00}" (exactly history start)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{May 26 2005 23:32:00}" >o 2>e &&
 	 test '"$C"' = $(cat o) &&
 	 test "" = "$(cat e)"'
 test_expect_success \
 	'Query "master@{May 26 2005 23:32:30}" (first non-creation change)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{May 26 2005 23:32:30}" >o 2>e &&
 	 test '"$A"' = $(cat o) &&
 	 test "" = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-26 23:33:01}" (middle of history with gap)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{2005-05-26 23:33:01}" >o 2>e &&
 	 test '"$B"' = $(cat o) &&
 	 test "warning: Log for ref '"$m has gap after $gd"'." = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-26 23:38:00}" (middle of history)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{2005-05-26 23:38:00}" >o 2>e &&
 	 test '"$Z"' = $(cat o) &&
 	 test "" = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-26 23:43:00}" (exact end of history)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{2005-05-26 23:43:00}" >o 2>e &&
 	 test '"$E"' = $(cat o) &&
 	 test "" = "$(cat e)"'
 test_expect_success \
 	'Query "master@{2005-05-28}" (past end of history)' \
-	'rm -f o e &&
+	'test_when_finished "rm -f o e" &&
 	 git rev-parse --verify "master@{2005-05-28}" >o 2>e &&
 	 test '"$D"' = $(cat o) &&
 	 test "warning: Log for ref '"$m unexpectedly ended on $ld"'." = "$(cat e)"'
@@ -415,7 +416,8 @@ rm -f .git/$m .git/logs/$m expect
 
 test_expect_success \
     'creating initial files' \
-    'echo TEST >F &&
+    'test_when_finished rm -f M &&
+     echo TEST >F &&
      git add F &&
 	 GIT_AUTHOR_DATE="2005-05-26 23:30" \
 	 GIT_COMMITTER_DATE="2005-05-26 23:30" git commit -m add -a &&
@@ -433,8 +435,7 @@ test_expect_success \
 	 echo $h_TEST >.git/MERGE_HEAD &&
 	 GIT_AUTHOR_DATE="2005-05-26 23:45" \
 	 GIT_COMMITTER_DATE="2005-05-26 23:45" git commit -F M &&
-	 h_MERGED=$(git rev-parse --verify HEAD) &&
-	 rm -f M'
+	 h_MERGED=$(git rev-parse --verify HEAD)'
 
 cat >expect <<EOF
 $Z $h_TEST $GIT_COMMITTER_NAME <$GIT_COMMITTER_EMAIL> 1117150200 +0000	commit (initial): add
-- 
2.12.0

Re: [PATCH 1/5] t1400: rename test descriptions to be unique

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:12PM -0400, Kyle Meyer wrote:

> A few tests share their description with another test.  Extend the
> descriptions to indicate how the tests differ.
> 
> Signed-off-by: Kyle Meyer <kyle@kyleam.com>

Makes sense, and your descriptions look good.

-Peff

Re: [PATCH 2/5] t1400: set core.logAllRefUpdates in "logged by touch" tests

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:13PM -0400, Kyle Meyer wrote:

> A group of update-ref tests verifies that logs are created when either
> the log file for the ref already exists or core.logAllRefUpdates is
> "true".  However, when the default for core.logAllRefUpdates was
> changed in 0bee59186 (Enable reflogs by default in any repository with
> a working directory., 2006-12-14), the setup for the tests was not
> updated.  As a result, the "logged by touch" tests would pass even if
> the log file did not exist (i.e., if "--create-reflog" was removed
> from the first "git update-ref" call).
> 
> Update the "logged by touch" tests to disable core.logAllRefUpdates
> explicitly so that the behavior does not depend on the default value.

Good catch.

> While we're here, update the "logged by config" tests to use
> test_config() rather than setting core.logAllRefUpdates to "true"
> outside of the tests.

Yeah, I agree that makes the result more obvious to read.

> I'm confused about the setup for the "logged by touch" tests.
> d0ab05849 (tests: remove some direct access to .git/logs, 2015-07-27)
> changed the setup to delete the log file itself rather than its
> contents.  The reflog was then recreated by using "--create-reflog" in
> the "create $m (logged by touch)" test.  What I don't understand is
> how this change fits with d0ab05849, which seems to be concerned with
> loosening the assumption that the logs are stored in .git/logs.

I suspect the answer is that the conversion was incomplete. That commit
was done for alternate ref backends, which is an ongoing saga.

I think it's OK to leave it as-is for now. It's not clear what "logged
by touch" will look like for backends that don't use the filesystem.
Probably it will need to call "update-ref --create-reflog" to kickstart
it, and then further updates will automatically write to it.

At that point the "rm -f" would need to become "tell the backend to
delete this reflog". There's no command for that now, but we can add one
later. Until then, I suspect the "rm -f" would be a noop. That means
that the first --create-reflog test is failing to test what it claims,
but the result passes anyway.

And that probably answers the question about why the conversion is
half-done. It was enough to get the tests to stop complaining when built
with an alternate ref backend. :)

> For these particular tests, we are still removing
> .git/logs/refs/heads/master explictly, so why not leave the empty file
> around so that the "create $m (logged by touch)" actually tests that
> update-ref call is logged by the existence of the log rather than
> using "--create-reflog", which overrides this?

I think the tests just wanted to start at a known state. I agree that
be correct (and would even work with an alternate backend), but it is a
bit subtle that we are relying on the leftover state from the previous
tests.

>  t/t1400-update-ref.sh | 23 ++++++++++++-----------
>  1 file changed, 12 insertions(+), 11 deletions(-)

Patch itself looks good.

-Peff

Re: [PATCH 3/5] t1400: use test_path_is_* helpers

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:14PM -0400, Kyle Meyer wrote:

> diff --git a/t/t1400-update-ref.sh b/t/t1400-update-ref.sh
> index be8b113b1..c5c8e95fc 100755
> --- a/t/t1400-update-ref.sh
> +++ b/t/t1400-update-ref.sh
> @@ -49,7 +49,7 @@ test_expect_success "fail to delete $m with stale ref" '
>  '
>  test_expect_success "delete $m" '
>  	git update-ref -d $m $B &&
> -	! test -f .git/$m
> +	test_path_is_missing .git/$m
>  '

I think this is an improvement for now. I suspect that all of these will
have to become "test_must_fail git rev-parse --verify $m" in the long
run, when we want to run the test suite against an alternate ref
backend. That can wait, though.

-Peff

Re: [PATCH 4/5] t1400: remove a set of unused output files

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:15PM -0400, Kyle Meyer wrote:

> This test case redirects stdout and stderr to output files, but,
> unlike the other cases of redirection in the t1400 tests, these files
> are not examined downstream.  Remove the redirection so that the
> output is visible when running the tests verbosely.

Makes sense.

-Peff

Re: [PATCH 5/5] t1400: use test_when_finished for cleanup

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:16PM -0400, Kyle Meyer wrote:

> Move cleanup lines that occur after test blocks into
> test_when_finished calls within the test bodies.  Don't move cleanup
> lines that seem to be related to mutiple tests rather than a single
> test.

Sounds logical. The patch looks good to me (though I'll admit my eyes
may have glazed a little in the middle).

-Peff

Re: [PATCH 0/5] t1400: modernize style

[Jeff King]

On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:

> These patches follow up on Peff's suggestion to modernize the style in
> t1400-update-ref.sh.
> 
>     20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net
> 
> The first two commits aren't concerned with "modernizing" the tests,
> but instead address issues that I noticed while looking more closely
> at t1400.

Looks good overall to me. Thanks for following up.

> I also considered
> 
>   * making the quoting/spacing/breaks around the test descriptions and
>     bodies more consistent, but I think this leads to too much code
>     churn.

I wouldn't mind the churn if you wanted to do it on top, but it's
definitely not necessary. There's nothing in 'pu' right now that touches
the file.

>   * moving the here-documents for log creation into the following
>     tests, but I don't think it's worth it because it makes already
>     long lines even longer.

Yeah, they're quite long. Probably something like:

  # arguments: <from> <to> <time> <msg>
  reflog () {
	printf '%s %s %s <%s> %s +0000\t%s' \
		"$1" "$2" \
		"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
		"$3" "$4"
  }

  test_expect_success 'verify $m log' '
	{
		reflog $Z $A 1117150200 "Initial Creation" &&
		reflog $A $B 1117150260 "Switch" &&
		reflog $B $A 1117150860 &&
	} >expect &&
	test_cmp expect .git/logs/$m
  '

wouldn't be too bad. Or maybe it's worse, because the actual format is
all tangled up in that printf statement. ;)

I'm OK with it either way.

-Peff

Re: [PATCH 0/5] t1400: modernize style

[Junio C Hamano]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:
>
>> These patches follow up on Peff's suggestion to modernize the style in
>> t1400-update-ref.sh.
>> 
>>     20170217082253.kxjezkxfqkfxjhzr@sigill.intra.peff.net
>> 
>> The first two commits aren't concerned with "modernizing" the tests,
>> but instead address issues that I noticed while looking more closely
>> at t1400.
>
> Looks good overall to me. Thanks for following up.

Thanks for a review.

>> I also considered
>> 
>>   * making the quoting/spacing/breaks around the test descriptions and
>>     bodies more consistent, but I think this leads to too much code
>>     churn.
>
> I wouldn't mind the churn if you wanted to do it on top, but it's
> definitely not necessary. There's nothing in 'pu' right now that touches
> the file.

Yes.  But I do not mind (actually I do prefer) if that "on top" came
as a separate follow-up after dust from these 5 settles ;-)

Thanks.

Re: [PATCH 2/5] t1400: set core.logAllRefUpdates in "logged by touch" tests

[Kyle Meyer]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 20, 2017 at 08:56:13PM -0400, Kyle Meyer wrote:

[...]

>> I'm confused about the setup for the "logged by touch" tests.
>> d0ab05849 (tests: remove some direct access to .git/logs, 2015-07-27)
>> changed the setup to delete the log file itself rather than its
>> contents.  The reflog was then recreated by using "--create-reflog" in
>> the "create $m (logged by touch)" test.  What I don't understand is
>> how this change fits with d0ab05849, which seems to be concerned with
>> loosening the assumption that the logs are stored in .git/logs.
>
> I suspect the answer is that the conversion was incomplete. That commit
> was done for alternate ref backends, which is an ongoing saga.
>
> I think it's OK to leave it as-is for now. It's not clear what "logged
> by touch" will look like for backends that don't use the filesystem.
> Probably it will need to call "update-ref --create-reflog" to kickstart
> it, and then further updates will automatically write to it.
>
> At that point the "rm -f" would need to become "tell the backend to
> delete this reflog". There's no command for that now, but we can add one
> later. Until then, I suspect the "rm -f" would be a noop. That means
> that the first --create-reflog test is failing to test what it claims,
> but the result passes anyway.
>
> And that probably answers the question about why the conversion is
> half-done. It was enough to get the tests to stop complaining when built
> with an alternate ref backend. :)

OK, thanks for the background.

-- 
Kyle

Re: [PATCH 0/5] t1400: modernize style

[Kyle Meyer]

Jeff King <peff@peff.net> writes:

> On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:

[...]

>>   * moving the here-documents for log creation into the following
>>     tests, but I don't think it's worth it because it makes already
>>     long lines even longer.
>
> Yeah, they're quite long. Probably something like:
>
>   # arguments: <from> <to> <time> <msg>
>   reflog () {
> 	printf '%s %s %s <%s> %s +0000\t%s' \
> 		"$1" "$2" \
> 		"$GIT_COMMITTER_NAME" "$GIT_COMMITTER_EMAIL" \
> 		"$3" "$4"
>   }
>
>   test_expect_success 'verify $m log' '
> 	{
> 		reflog $Z $A 1117150200 "Initial Creation" &&
> 		reflog $A $B 1117150260 "Switch" &&
> 		reflog $B $A 1117150860 &&
> 	} >expect &&
> 	test_cmp expect .git/logs/$m
>   '
>
> wouldn't be too bad. Or maybe it's worse, because the actual format is
> all tangled up in that printf statement. ;)
>
> I'm OK with it either way.

Heh, I didn't consider that option.  I suppose I'll stick with the
here-document for now because, to my eyes, it seems a bit easier to
read.

-- 
Kyle

Re: [PATCH 0/5] t1400: modernize style

[Kyle Meyer]

Junio C Hamano <gitster@pobox.com> writes:

> Jeff King <peff@peff.net> writes:
>
>> On Mon, Mar 20, 2017 at 08:56:11PM -0400, Kyle Meyer wrote:

[...]

>>> I also considered
>>> 
>>>   * making the quoting/spacing/breaks around the test descriptions and
>>>     bodies more consistent, but I think this leads to too much code
>>>     churn.
>>
>> I wouldn't mind the churn if you wanted to do it on top, but it's
>> definitely not necessary. There's nothing in 'pu' right now that touches
>> the file.
>
> Yes.  But I do not mind (actually I do prefer) if that "on top" came
> as a separate follow-up after dust from these 5 settles ;-)

OK, I follow-up with that later.  Thanks.

-- 
Kyle