diff mbox series

[v2] t4200-rerere.sh: use test_path_is_* functions in the script

Message ID pull.595.v2.git.1586444061576.gitgitgadget@gmail.com (mailing list archive)
State New, archived
Headers show
Series [v2] t4200-rerere.sh: use test_path_is_* functions in the script | expand

Commit Message

Linus Arver via GitGitGadget April 9, 2020, 2:54 p.m. UTC 
From: Angel Pan <dinoallosaurus1111@gmail.com>

The original code works fine. However, since the code base has already had help-functions(in test-lib-functions),
the change is able to help people understand the code better, as well as improve debug-friendliness.

* t4200-rerere.sh: replace "test -d" with "test_path_is_dir"
* t4200-rerere.sh: replace "test -f" with "test_path_is_file"

Signed-off-by: Angel Pan <dinoallosaurus1111@gmail.com>
---
    [GSoC/PATCH] t4200-rerere.sh: use test_path_is_* functions in the script
    
    Hello again. I thought I needed to improve my commit message since some
    of community members friendly reminded me last week. So I revised it
    again. Here is my patch with new commit message. If there is still
    something I miss, a reminder will be appreciated.
    
    Signed-off-by: Angel Pan dinoallosaurus1111@gmail.com
    [dinoallosaurus1111@gmail.com]

Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-595%2Fdinoallo%2Fmaster-v2
Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-595/dinoallo/master-v2
Pull-Request: https://github.com/gitgitgadget/git/pull/595

Range-diff vs v1:

 1:  7e0c8749016 ! 1:  c704627a465 t4200-rerere.sh: use test_path_is_* functions in the script
     @@ Metadata
       ## Commit message ##
          t4200-rerere.sh: use test_path_is_* functions in the script
      
     -    This is a microproject for GSoC.
     -    The original code works fine. However, since the code base has already have help-functions(in test-lib-functions),
     +    The original code works fine. However, since the code base has already had help-functions(in test-lib-functions),
          the change is able to help people understand the code better, as well as improve debug-friendliness.
      
     +    * t4200-rerere.sh: replace "test -d" with "test_path_is_dir"
     +    * t4200-rerere.sh: replace "test -f" with "test_path_is_file"
     +
          Signed-off-by: Angel Pan <dinoallosaurus1111@gmail.com>
      
       ## t/t4200-rerere.sh ##


 t/t4200-rerere.sh | 26 +++++++++++++-------------
 1 file changed, 13 insertions(+), 13 deletions(-)


base-commit: 9fadedd637b312089337d73c3ed8447e9f0aa775

Comments

Đoàn Trần Công Danh April 9, 2020, 3:21 p.m. UTC | #1 
On 2020-04-09 14:54:21+0000, Allo via GitGitGadget <gitgitgadget@gmail.com> wrote:
> From: Angel Pan <dinoallosaurus1111@gmail.com>
> 
> Subject: t4200-rerere.sh: use test_path_is_* functions in the script

"in the script" looks too verbose to me. Because the script has been
mentioned earlier.

> The original code works fine. However, since the code base has already had help-functions(in test-lib-functions),

We wrap lines at 72 characters per line.
And, missing space before open parenthese.

I'm not good at wording, but I think this commit message is too
verbose. I would say something like:

	Prefer "test_path_is_dir" to "test -d" and test_path_is_file
	to "test -f" because they're more friendly to debug.

That's not a good message, either. But, it's some what simpler and
cleaner.

> the change is able to help people understand the code better, as well as improve debug-friendliness.
> 
> * t4200-rerere.sh: replace "test -d" with "test_path_is_dir"
> * t4200-rerere.sh: replace "test -f" with "test_path_is_file"
>
Junio C Hamano April 9, 2020, 6:09 p.m. UTC | #2 
Danh Doan <congdanhqx@gmail.com> writes:

> On 2020-04-09 14:54:21+0000, Allo via GitGitGadget <gitgitgadget@gmail.com> wrote:
>> From: Angel Pan <dinoallosaurus1111@gmail.com>
>> ...
> That's not a good message, either. But, it's some what simpler and
> cleaner.

Thank you, Allo, for a patch, and thank you Danh for a good review.
diff mbox series

Patch

diff --git a/t/t4200-rerere.sh b/t/t4200-rerere.sh
index 831d424c472..da8be1c6254 100755
--- a/t/t4200-rerere.sh
+++ b/t/t4200-rerere.sh
@@ -70,7 +70,7 @@  test_expect_success 'nothing recorded without rerere' '
 	rm -rf .git/rr-cache &&
 	git config rerere.enabled false &&
 	test_must_fail git merge first &&
-	! test -d .git/rr-cache
+	! test_path_is_dir .git/rr-cache
 '
 
 test_expect_success 'activate rerere, old style (conflicting merge)' '
@@ -82,8 +82,8 @@  test_expect_success 'activate rerere, old style (conflicting merge)' '
 	sha1=$(perl -pe "s/	.*//" .git/MERGE_RR) &&
 	rr=.git/rr-cache/$sha1 &&
 	grep "^=======\$" $rr/preimage &&
-	! test -f $rr/postimage &&
-	! test -f $rr/thisimage
+	! test_path_is_file $rr/postimage &&
+	! test_path_is_file $rr/thisimage
 '
 
 test_expect_success 'rerere.enabled works, too' '
@@ -108,8 +108,8 @@  test_expect_success 'set up rr-cache' '
 
 test_expect_success 'rr-cache looks sane' '
 	# no postimage or thisimage yet
-	! test -f $rr/postimage &&
-	! test -f $rr/thisimage &&
+	! test_path_is_file $rr/postimage &&
+	! test_path_is_file $rr/thisimage &&
 
 	# preimage has right number of lines
 	cnt=$(sed -ne "/^<<<<<<</,/^>>>>>>>/p" $rr/preimage | wc -l) &&
@@ -165,7 +165,7 @@  test_expect_success 'first postimage wins' '
 	git show first:a1 | sed "s/To die: t/To die! T/" >expect &&
 
 	git commit -q -a -m "prefer first over second" &&
-	test -f $rr/postimage &&
+	test_path_is_file $rr/postimage &&
 
 	oldmtimepost=$(test-tool chmtime --get -60 $rr/postimage) &&
 
@@ -188,14 +188,14 @@  test_expect_success 'rerere clear' '
 	mv $rr/postimage .git/post-saved &&
 	echo "$sha1	a1" | perl -pe "y/\012/\000/" >.git/MERGE_RR &&
 	git rerere clear &&
-	! test -d $rr
+	! test_path_is_dir $rr
 '
 
 test_expect_success 'leftover directory' '
 	git reset --hard &&
 	mkdir -p $rr &&
 	test_must_fail git merge first &&
-	test -f $rr/preimage
+	test_path_is_file $rr/preimage
 '
 
 test_expect_success 'missing preimage' '
@@ -203,7 +203,7 @@  test_expect_success 'missing preimage' '
 	mkdir -p $rr &&
 	cp .git/post-saved $rr/postimage &&
 	test_must_fail git merge first &&
-	test -f $rr/preimage
+	test_path_is_file $rr/preimage
 '
 
 test_expect_success 'set up for garbage collection tests' '
@@ -228,16 +228,16 @@  test_expect_success 'set up for garbage collection tests' '
 
 test_expect_success 'gc preserves young or recently used records' '
 	git rerere gc &&
-	test -f $rr/preimage &&
-	test -f $rr2/preimage
+	test_path_is_file $rr/preimage &&
+	test_path_is_file $rr2/preimage
 '
 
 test_expect_success 'old records rest in peace' '
 	test-tool chmtime =$just_over_60_days_ago $rr/postimage &&
 	test-tool chmtime =$just_over_15_days_ago $rr2/preimage &&
 	git rerere gc &&
-	! test -f $rr/preimage &&
-	! test -f $rr2/preimage
+	! test_path_is_file $rr/preimage &&
+	! test_path_is_file $rr2/preimage
 '
 
 rerere_gc_custom_expiry_test () {