# [PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing. 8 messages from 2025-01-01 to 2025-01-03. Participants: matteobagnolini, Patrick Steinhardt, Matteo Bagnolini, Junio C Hamano. Thread: https://gitlist.dev/t/62721 ## matteobagnolini, 2025-01-01 22:59 Subject: [PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing. Message-ID: <20250101225915.65185-1-matteobagnolini2003@gmail.com> URL: https://gitlist.dev/e/20250101225915.65185-1-matteobagnolini2003%40gmail.com ``` `test -f` and `! test -f` do not provide clear error messages when they fail. To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`, which instead provides more informative error messages. Note that `! test -f` checks if a path is not a file, while `test_path_is_missing` verifies that a path does not exist. In this specific case the tests are meant to check the absence of the path, making `test_path_is_missing` a valid replacement. Signed-off-by: matteobagnolini --- t/t7110-reset-merge.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh index 61669a2d21..9a335071af 100755 --- a/t/t7110-reset-merge.sh +++ b/t/t7110-reset-merge.sh @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && git reset --merge HEAD && - ! test -f file3 && - ! test -f file2 && + test_path_is_missing file3 && + test_path_is_missing file2 && git diff --exit-code --cached ' @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && test_must_fail git reset --keep HEAD 2>err.log && -- 2.39.2 (Apple Git-143) ``` ## Patrick Steinhardt, 2025-01-03 11:41 Subject: Re: [PATCH] t7110: Replace `test -f` and `! test -f` with `test_path_is_file` and `test_path_is_missing` for improved debuggability when failing. Message-ID: URL: https://gitlist.dev/e/Z3fM1bT2Z_0GoD4w%40pks.im In-Reply-To: <20250101225915.65185-1-matteobagnolini2003@gmail.com> ``` On Wed, Jan 01, 2025 at 11:59:14PM +0100, matteobagnolini wrote: The commit subject is overly long. We typically try to restrict them to at most 72 characters. Furthermore, the part after the subsystem should start with a lower-case letter. Suggestion: t7110: replace `test -f` with `test_path_is_*` helpers > `test -f` and `! test -f` do not provide clear error messages when they fail. > To enanche debuggability, use `test_path_is_file` and `test_path_is_missing`, s/enanche/enhance > which instead provides more informative error messages. s/provides/provide It looks like you intended to start a new paragraph here. These should typically be separated by an empty line. > Note that `! test -f` checks if a path is not a file, while > `test_path_is_missing` verifies that a path does not exist. In this specific > case the tests are meant to check the absence of the path, making > `test_path_is_missing` a valid replacement. Makes sense. > Signed-off-by: matteobagnolini We typically prefer proper legal names in the SOB. > diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh > index 61669a2d21..9a335071af 100755 > --- a/t/t7110-reset-merge.sh > +++ b/t/t7110-reset-merge.sh > @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' ' > git reset --hard third && > rm -f file2 && > test_must_fail git merge branch3 && > - ! test -f file2 && > - test -f file3 && > + test_path_is_missing file2 && > + test_path_is_file file3 && > git diff --exit-code file3 && > git diff --exit-code branch3 file3 && > git reset --merge HEAD && > - ! test -f file3 && > - ! test -f file2 && > + test_path_is_missing file3 && > + test_path_is_missing file2 && > git diff --exit-code --cached > ' > > @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' ' > git reset --hard third && > rm -f file2 && > test_must_fail git merge branch3 && > - ! test -f file2 && > - test -f file3 && > + test_path_is_missing file2 && > + test_path_is_file file3 && > git diff --exit-code file3 && > git diff --exit-code branch3 file3 && > test_must_fail git reset --keep HEAD 2>err.log && The changes themselves look obviously good to me, thanks! Patrick ``` ## Matteo Bagnolini, 2025-01-03 13:00 Subject: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: <20250103130035.79376-1-matteobagnolini2003@gmail.com> URL: https://gitlist.dev/e/20250103130035.79376-1-matteobagnolini2003%40gmail.com In-Reply-To: ``` From: matteobagnolini `test -f` and `! test -f` do not provide clear error messages when they fail. To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`, which instead provide more informative error messages. Note that `! test -f` checks if a path is not a file, while `test_path_is_missing` verifies that a path does not exist. In this specific case the tests are meant to check the absence of the path, making `test_path_is_missing` a valid replacement. Signed-off-by: Matteo Bagnolini --- Sorry for avoidable spelling mistakes. Updated commit message according to review. t/t7110-reset-merge.sh | 12 ++++++------ 1 file changed, 6 insertions(+), 6 deletions(-) diff --git a/t/t7110-reset-merge.sh b/t/t7110-reset-merge.sh index 61669a2d21..9a335071af 100755 --- a/t/t7110-reset-merge.sh +++ b/t/t7110-reset-merge.sh @@ -270,13 +270,13 @@ test_expect_success '--merge is ok with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && git reset --merge HEAD && - ! test -f file3 && - ! test -f file2 && + test_path_is_missing file3 && + test_path_is_missing file2 && git diff --exit-code --cached ' @@ -284,8 +284,8 @@ test_expect_success '--keep fails with added/deleted merge' ' git reset --hard third && rm -f file2 && test_must_fail git merge branch3 && - ! test -f file2 && - test -f file3 && + test_path_is_missing file2 && + test_path_is_file file3 && git diff --exit-code file3 && git diff --exit-code branch3 file3 && test_must_fail git reset --keep HEAD 2>err.log && -- 2.39.2 (Apple Git-143) ``` ## Patrick Steinhardt, 2025-01-03 13:12 Subject: Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: URL: https://gitlist.dev/e/Z3fiREGwXdILl-M1%40pks.im In-Reply-To: <20250103130035.79376-1-matteobagnolini2003@gmail.com> ``` On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote: > From: matteobagnolini > > `test -f` and `! test -f` do not provide clear error messages when they fail. > To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`, > which instead provide more informative error messages. > > Note that `! test -f` checks if a path is not a file, while > `test_path_is_missing` verifies that a path does not exist. In this specific > case the tests are meant to check the absence of the path, making > `test_path_is_missing` a valid replacement. Thanks, this version looks good to me. Patrick ``` ## Junio C Hamano, 2025-01-03 16:25 Subject: Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: URL: https://gitlist.dev/e/xmqqfrlzsw4j.fsf%40gitster.g In-Reply-To: ``` Patrick Steinhardt writes: > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote: >> From: matteobagnolini This must match the author ident on the Signed-off-by: line. >> `test -f` and `! test -f` do not provide clear error messages when they fail. >> To enhance debuggability, use `test_path_is_file` and `test_path_is_missing`, >> which instead provide more informative error messages. >> >> Note that `! test -f` checks if a path is not a file, while >> `test_path_is_missing` verifies that a path does not exist. In this specific >> case the tests are meant to check the absence of the path, making >> `test_path_is_missing` a valid replacement. > > Thanks, this version looks good to me. > > Patrick Thanks for writing, and thanks for reviewing. ``` ## Matteo Bagnolini, 2025-01-03 17:27 Subject: Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: URL: https://gitlist.dev/e/CADyPGv0T7Wwrf_FidWDtLGOH7A88DcisAG4-jU3eEax106XEcQ%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Patrick Steinhardt writes: > > > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote: > >> From: matteobagnolini > > This must match the author ident on the Signed-off-by: line. So should I send another patch with this correction? Sorry for questioning, but as you might have noticed, this is my first time contributing and I'm slowly trying to learn the process. Matteo ``` ## Junio C Hamano, 2025-01-03 18:06 Subject: Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: URL: https://gitlist.dev/e/xmqq7c7bsrel.fsf%40gitster.g In-Reply-To: ``` Matteo Bagnolini writes: > Junio C Hamano writes: > >> Patrick Steinhardt writes: >> >> > On Fri, Jan 03, 2025 at 02:00:35PM +0100, Matteo Bagnolini wrote: >> >> From: matteobagnolini >> >> This must match the author ident on the Signed-off-by: line. > > So should I send another patch with this correction? > Sorry for questioning, but as you might have noticed, this is my first time > contributing and I'm slowly trying to learn the process. Thanks for asking; if there are no other things that need fixing, I can fix up while queuing. ``` ## Matteo Bagnolini, 2025-01-03 19:07 Subject: Re: [PATCH v2] t7110: replace `test -f` with `test_path_is_*` helpers Message-ID: URL: https://gitlist.dev/e/CADyPGv2onyb-n80LMp%2BLNRij8oGZLstUkNpJXQcLKbVqcPzpEQ%40mail.gmail.com In-Reply-To: ``` Junio C Hamano writes: > Thanks for asking; if there are no other things that need fixing, I > can fix up while queuing. Yes, everything else should be OK. Thank you. Matteo ```