* [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers
@ 2024-12-18 11:17 Meet Soni
2024-12-18 15:23 ` karthik nayak
2024-12-20 13:06 ` [PATCH v2] " Meet Soni
0 siblings, 2 replies; 7+ messages in thread
From: Meet Soni @ 2024-12-18 11:17 UTC (permalink / raw)
To: git
Cc: christian.couder, karthik.188, kaartic.sivaraam, ps,
shyamthakkar001, shejialuo, chandrapratap3519, Meet Soni
test -f does not provide verbose error message on test failures, so use
test_path_is_file, test_path_is_missing instead.
Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index d6975ca48d..1a251485e1 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '
'
test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)"
'
test_expect_success 'fails without MERGE_HEAD (completed merge)' '
git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
# Merge successfully completed
post_merge_head="$(git rev-parse HEAD)" &&
test_must_fail git merge --abort 2>output &&
@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '
'
test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$post_merge_head" = "$(git rev-parse HEAD)"
'
@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '
test_expect_success 'Abort after --no-commit' '
# Redo merge, but stop before creating merge commit
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort non-conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '
test_expect_success 'Abort after conflicts' '
# Create conflicting merge
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '
git add foo &&
git diff --staged > expect &&
test_must_fail git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '
test_expect_success 'Conflicting merge with dirty index fails' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '
test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
echo xyzzy >> bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '
echo bart > bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '
echo barf > bar &&
git diff > expect &&
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
base-commit: d882f382b3d939d90cfa58d17b17802338f05d66
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers
2024-12-18 11:17 [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers Meet Soni
@ 2024-12-18 15:23 ` karthik nayak
2024-12-20 13:06 ` [PATCH v2] " Meet Soni
1 sibling, 0 replies; 7+ messages in thread
From: karthik nayak @ 2024-12-18 15:23 UTC (permalink / raw)
To: Meet Soni, git
Cc: christian.couder, kaartic.sivaraam, ps, shyamthakkar001,
shejialuo, chandrapratap3519
[-- Attachment #1: Type: text/plain, Size: 861 bytes --]
Meet Soni <meetsoni3017@gmail.com> writes:
> test -f does not provide verbose error message on test failures, so use
> test_path_is_file, test_path_is_missing instead.
While `test -f` checks to ensure that the file exists and is a regular
file, I also notice that the patch contains changing `test ! -f`. This
is a bit more tricky, since:
1. It can be used to check if a regular file doesn't exist
2. It can be used to check if a directory exists instead of a file
The commit message only talks about the former.
The patch itself look great, but I just noticed that the subject
mentions 'GSoC'. As Patrick has already mentioned in your previous email
to the list [1], we still don't have any plans with regards to GSoC 25.
So marking a patch in that context, doesn't make much sense (yet).
[1]: https://lore.kernel.org/git/Z2AwhvaE4DLAxzDy@pks.im/
[snip]
[-- Attachment #2: signature.asc --]
[-- Type: application/pgp-signature, Size: 690 bytes --]
^ permalink raw reply [flat|nested] 7+ messages in thread
* [PATCH v2] t7611: replace test -f with test_path_is* helpers
2024-12-18 11:17 [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers Meet Soni
2024-12-18 15:23 ` karthik nayak
@ 2024-12-20 13:06 ` Meet Soni
2024-12-20 14:16 ` Junio C Hamano
2024-12-27 10:53 ` [PATCH v3] " Meet Soni
1 sibling, 2 replies; 7+ messages in thread
From: Meet Soni @ 2024-12-20 13:06 UTC (permalink / raw)
To: git
Cc: christian.couder, karthik.188, kaartic.sivaraam, ps,
shyamthakkar001, shejialuo, chandrapratap3519, Meet Soni
Replace `test -f` and `test ! -f` with `test_path_is_file` and
`test_path_is_missing` for better verbosity.
While `test -f` ensures that the file exists and is a regular file,
`test_path_is_file` provides clearer error messages on failure. Similarly,
`test ! -f`, used to check either the absence of a regular file or the
presence of a directory, has been replaced with `test_path_is_missing` for
better readability and consistent handling of such cases.
Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
Updated commit message and subject according to review.
t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index d6975ca48d..1a251485e1 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '
'
test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)"
'
test_expect_success 'fails without MERGE_HEAD (completed merge)' '
git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
# Merge successfully completed
post_merge_head="$(git rev-parse HEAD)" &&
test_must_fail git merge --abort 2>output &&
@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '
'
test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$post_merge_head" = "$(git rev-parse HEAD)"
'
@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '
test_expect_success 'Abort after --no-commit' '
# Redo merge, but stop before creating merge commit
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort non-conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '
test_expect_success 'Abort after conflicts' '
# Create conflicting merge
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '
git add foo &&
git diff --staged > expect &&
test_must_fail git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '
test_expect_success 'Conflicting merge with dirty index fails' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '
test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
echo xyzzy >> bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '
echo bart > bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '
echo barf > bar &&
git diff > expect &&
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v2] t7611: replace test -f with test_path_is* helpers
2024-12-20 13:06 ` [PATCH v2] " Meet Soni
@ 2024-12-20 14:16 ` Junio C Hamano
2024-12-27 10:53 ` [PATCH v3] " Meet Soni
1 sibling, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2024-12-20 14:16 UTC (permalink / raw)
To: Meet Soni
Cc: git, christian.couder, karthik.188, kaartic.sivaraam, ps,
shyamthakkar001, shejialuo, chandrapratap3519
Meet Soni <meetsoni3017@gmail.com> writes:
> Replace `test -f` and `test ! -f` with `test_path_is_file` and
> `test_path_is_missing` for better verbosity.
OK. "verbosity" -> "debuggability" perhaps, but that is minor.
> While `test -f` ensures that the file exists and is a regular file,
> `test_path_is_file` provides clearer error messages on failure.
Correct.
> Similarly,
> `test ! -f`, used to check either the absence of a regular file or the
> presence of a directory, has been replaced with `test_path_is_missing` for
> better readability and consistent handling of such cases.
This is misleading. If you rewrite "test ! -f" that intends to be
happy when it sees a directory, you would be changing the meaning of
the program if you replace it with test_path_is_missing. Rather,
when you see "test ! -f foo", you should first think if the intent
of the test script there is to allow presence of "foo" that is not a
file, and if that is not the case, in other words, the "test ! -f"
you see should have been written "test ! -e" (i.e. "I do not want to
see 'foo' there on the filesystem, no matter what kind of filesystem
entity it is!"), it would give us the same meaning with better
debuggability to rewrite such "test ! -f" with test_path_is_missing.
IOW, unlike "test -f foo" that can pretty much blindly replaceable
with "test_path_is_file foo" without thinking at all, you must be a
bit more careful. And _if_ you did such a more careful analysis
before replacing "test ! -f", then you should restate the above,
perhaps something along the lines of ...
On the other hand, `test ! -f` literally means there should not
be a file (implication is that we are OK if there is a directory
or device nodes or other things at the given path). But by
looking at each of these in the test individually, many of them
should rather have said "test ! -e", i.e. "there shouldn't be
anything at the given path on the filesystem". Rewrite these
cases to test_path_is_missing for better debuggability.
I didn't check myself if all of the "test ! -f" you touched are
indeed the original should have said "test ! -e". Hopefully you
have done it yourself?
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread* [PATCH v3] t7611: replace test -f with test_path_is* helpers
2024-12-20 13:06 ` [PATCH v2] " Meet Soni
2024-12-20 14:16 ` Junio C Hamano
@ 2024-12-27 10:53 ` Meet Soni
2024-12-27 12:19 ` Ghanshyam Thakkar
1 sibling, 1 reply; 7+ messages in thread
From: Meet Soni @ 2024-12-27 10:53 UTC (permalink / raw)
To: git
Cc: christian.couder, karthik.188, kaartic.sivaraam, ps,
shyamthakkar001, shejialuo, chandrapratap3519, gitster, Meet Soni
Replace `test -f` and `test ! -f` with `test_path_is_file` and
`test_path_is_missing` for better debuggability.
While `test -f` ensures that the file exists and is a regular file,
`test_path_is_file` provides clearer error messages on failure. On the
other hand, `test ! -f`, used to check either the absence of a regular
file or the presence of any other filesystem object, but looking at
them in the test individually, all of them should've said `test ! e`,
i.e. "there shouldn't be anything at given path on filesystem."
Replaced these cases with `test_path_is_missing` for better
debuggability.
Helped-by: karthik nayak <karthik.188@gmail.com>
Helped-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Meet Soni <meetsoni3017@gmail.com>
---
Apologies for late response.
Updated commit message for better clarification of changes made.
t/t7611-merge-abort.sh | 34 +++++++++++++++++-----------------
1 file changed, 17 insertions(+), 17 deletions(-)
diff --git a/t/t7611-merge-abort.sh b/t/t7611-merge-abort.sh
index d6975ca48d..1a251485e1 100755
--- a/t/t7611-merge-abort.sh
+++ b/t/t7611-merge-abort.sh
@@ -54,13 +54,13 @@ test_expect_success 'fails without MERGE_HEAD (unstarted merge)' '
'
test_expect_success 'fails without MERGE_HEAD (unstarted merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)"
'
test_expect_success 'fails without MERGE_HEAD (completed merge)' '
git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
# Merge successfully completed
post_merge_head="$(git rev-parse HEAD)" &&
test_must_fail git merge --abort 2>output &&
@@ -68,7 +68,7 @@ test_expect_success 'fails without MERGE_HEAD (completed merge)' '
'
test_expect_success 'fails without MERGE_HEAD (completed merge): .git/MERGE_HEAD sanity' '
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$post_merge_head" = "$(git rev-parse HEAD)"
'
@@ -79,10 +79,10 @@ test_expect_success 'Forget previous merge' '
test_expect_success 'Abort after --no-commit' '
# Redo merge, but stop before creating merge commit
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort non-conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -91,10 +91,10 @@ test_expect_success 'Abort after --no-commit' '
test_expect_success 'Abort after conflicts' '
# Create conflicting merge
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort conflicting merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
test -z "$(git diff --staged)"
@@ -105,7 +105,7 @@ test_expect_success 'Clean merge with dirty index fails' '
git add foo &&
git diff --staged > expect &&
test_must_fail git merge clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -114,7 +114,7 @@ test_expect_success 'Clean merge with dirty index fails' '
test_expect_success 'Conflicting merge with dirty index fails' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff)" &&
git diff --staged > actual &&
@@ -129,10 +129,10 @@ test_expect_success 'Reset index (but preserve worktree changes)' '
test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
git merge --no-commit clean_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -141,10 +141,10 @@ test_expect_success 'Abort clean merge with non-conflicting dirty worktree' '
test_expect_success 'Abort conflicting merge with non-conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test -f .git/MERGE_HEAD &&
+ test_path_is_file .git/MERGE_HEAD &&
# Abort merge
git merge --abort &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -159,7 +159,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
echo xyzzy >> bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -168,7 +168,7 @@ test_expect_success 'Fail clean merge with conflicting dirty worktree' '
test_expect_success 'Fail conflicting merge with conflicting dirty worktree' '
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -183,7 +183,7 @@ test_expect_success 'Fail clean merge with matching dirty worktree' '
echo bart > bar &&
git diff > expect &&
test_must_fail git merge --no-commit clean_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
@@ -194,7 +194,7 @@ test_expect_success 'Fail conflicting merge with matching dirty worktree' '
echo barf > bar &&
git diff > expect &&
test_must_fail git merge conflict_branch &&
- test ! -f .git/MERGE_HEAD &&
+ test_path_is_missing .git/MERGE_HEAD &&
test "$pre_merge_head" = "$(git rev-parse HEAD)" &&
test -z "$(git diff --staged)" &&
git diff > actual &&
--
2.34.1
^ permalink raw reply related [flat|nested] 7+ messages in thread
* Re: [PATCH v3] t7611: replace test -f with test_path_is* helpers
2024-12-27 10:53 ` [PATCH v3] " Meet Soni
@ 2024-12-27 12:19 ` Ghanshyam Thakkar
2024-12-27 16:15 ` Junio C Hamano
0 siblings, 1 reply; 7+ messages in thread
From: Ghanshyam Thakkar @ 2024-12-27 12:19 UTC (permalink / raw)
To: Meet Soni, git
Cc: christian.couder, karthik.188, kaartic.sivaraam, ps, shejialuo,
chandrapratap3519, gitster
On Fri Dec 27, 2024 at 4:23 PM IST, Meet Soni wrote:
> Replace `test -f` and `test ! -f` with `test_path_is_file` and
> `test_path_is_missing` for better debuggability.
>
> While `test -f` ensures that the file exists and is a regular file,
> `test_path_is_file` provides clearer error messages on failure. On the
> other hand, `test ! -f`, used to check either the absence of a regular
> file or the presence of any other filesystem object, but looking at
> them in the test individually, all of them should've said `test ! e`,
> i.e. "there shouldn't be anything at given path on filesystem."
> Replaced these cases with `test_path_is_missing` for better
> debuggability.
'Replaced' -> 'Replace'. Cf. https://git-scm.com/docs/SubmittingPatches#imperative-mood
Other than that, this LGTM.
Thanks.
^ permalink raw reply [flat|nested] 7+ messages in thread
* Re: [PATCH v3] t7611: replace test -f with test_path_is* helpers
2024-12-27 12:19 ` Ghanshyam Thakkar
@ 2024-12-27 16:15 ` Junio C Hamano
0 siblings, 0 replies; 7+ messages in thread
From: Junio C Hamano @ 2024-12-27 16:15 UTC (permalink / raw)
To: Ghanshyam Thakkar
Cc: Meet Soni, git, christian.couder, karthik.188, kaartic.sivaraam,
ps, shejialuo, chandrapratap3519
"Ghanshyam Thakkar" <shyamthakkar001@gmail.com> writes:
> On Fri Dec 27, 2024 at 4:23 PM IST, Meet Soni wrote:
>> Replace `test -f` and `test ! -f` with `test_path_is_file` and
>> `test_path_is_missing` for better debuggability.
>>
>> While `test -f` ensures that the file exists and is a regular file,
>> `test_path_is_file` provides clearer error messages on failure. On the
>> other hand, `test ! -f`, used to check either the absence of a regular
>> file or the presence of any other filesystem object, but looking at
>> them in the test individually, all of them should've said `test ! e`,
>> i.e. "there shouldn't be anything at given path on filesystem."
>> Replaced these cases with `test_path_is_missing` for better
>> debuggability.
>
> 'Replaced' -> 'Replace'. Cf. https://git-scm.com/docs/SubmittingPatches#imperative-mood
>
> Other than that, this LGTM.
Thanks, both. Tweaked the log message before applying. No need to
resend.
Queued.
^ permalink raw reply [flat|nested] 7+ messages in thread
end of thread, other threads:[~2024-12-27 16:15 UTC | newest]
Thread overview: 7+ messages (download: mbox.gz follow: Atom feed
-- links below jump to the message on this page --
2024-12-18 11:17 [GSoC] [PATCH] t7611: replace test -f with test_path_is* helpers Meet Soni
2024-12-18 15:23 ` karthik nayak
2024-12-20 13:06 ` [PATCH v2] " Meet Soni
2024-12-20 14:16 ` Junio C Hamano
2024-12-27 10:53 ` [PATCH v3] " Meet Soni
2024-12-27 12:19 ` Ghanshyam Thakkar
2024-12-27 16:15 ` Junio C Hamano
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).