Github Patch

Github Patch

[Zakariyah Ali]

[-- Attachment #1.1: Type: text/plain, Size: 1 bytes --]



[-- Attachment #1.2: Type: text/html, Size: 26 bytes --]

[-- Attachment #2: 0001-t-t2000-modernize-path-checks-to-use-test_path-helpe.patch --]
[-- Type: text/x-patch, Size: 2203 bytes --]

From 91a3ccf496cdb61149e3c031265fe252c6c8ef3c Mon Sep 17 00:00:00 2001
From: alibaba0010 <zakariyahali100@gmail.com>
Date: Tue, 24 Mar 2026 21:04:38 +0100
Subject: [PATCH] t/t2000: modernize path checks to use test_path helpers
MIME-Version: 1.0
Content-Type: text/plain; charset=UTF-8
Content-Transfer-Encoding: 8bit

Replace old-style path checks using `test -f`, `test -d`,
and `test ! -h` with dedicated test helper functions for
improved test clarity and consistency.

This modernization improves test script readability by using
Git's dedicated test helpers:
- `test -f` → `test_path_is_file`
- `test -d` → `test_path_is_dir`
- `test ! -h && test -f` →
  `test_path_is_file_not_symlink`
- `test ! -h && test -d` →
  `test_path_is_dir_not_symlink`

Found instances using:
  git grep 'test -[efd]' t/ | grep 'test -[efd].*&&'

Converted 5 instances in
t/t2000-conflict-when-checking-files-out.sh

Signed-off-by: alibaba0010 <zakariyahali100@gmail.com>
---
 t/t2000-conflict-when-checking-files-out.sh | 10 +++++-----
 1 file changed, 5 insertions(+), 5 deletions(-)

diff --git a/t/t2000-conflict-when-checking-files-out.sh b/t/t2000-conflict-when-checking-files-out.sh
index f18616ad2b..b535bb002a 100755
--- a/t/t2000-conflict-when-checking-files-out.sh
+++ b/t/t2000-conflict-when-checking-files-out.sh
@@ -58,7 +58,7 @@ test_expect_success \
 
 test_expect_success \
     'git checkout-index conflicting paths.' \
-    'test -f path0 && test -d path1 && test -f path1/file1'
+    'test_path_is_file path0 && test_path_is_dir path1 && test_path_is_file path1/file1'
 
 test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
 	mkdir -p tar/get &&
@@ -127,9 +127,9 @@ test_debug 'show_files $tree2'
 
 test_expect_success \
     'checking out conflicting path with -f' \
-    'test ! -h path2 && test -d path2 &&
-     test ! -h path3 && test -d path3 &&
-     test ! -h path2/file0 && test -f path2/file0 &&
-     test ! -h path3/file1 && test -f path3/file1'
+    'test_path_is_dir_not_symlink path2 &&
+     test_path_is_dir_not_symlink path3 &&
+     test_path_is_file_not_symlink path2/file0 &&
+     test_path_is_file_not_symlink path3/file1'
 
 test_done
-- 
2.43.0

Re: Github Patch

[Pablo]

Zakariyah Ali (<zakariyahali100@gmail.com>) escribió:

The mail is empty but for the attached patch.

The patch should be sent inline in the email body, not as an attachment.
Please read Documentation/SubmittingPatches [1] and
Documentation/MyFirstContribution [2] about how to send patches.

Now, about the patch:

> From: alibaba0010 <zakariyahali100@gmail.com>
> Signed-off-by: alibaba0010 <zakariyahali100@gmail.com>

Your email says "Zakariyah Ali" but the From and Signed-off-by says
"alibaba0010. These should be consistent.
See Documentation/SubmittingPatches [1].

 "It is common, but not required, to use some form of your real name.
We realize that some contributors are not comfortable doing so or
prefer to contribute under a pseudonym or preferred name and we can
accept your patch either way, as long as the name and email you use
are distinctive, identifying, and not misleading."

> Replace old-style path checks using `test -f`, `test -d`,
> and `test ! -h` with dedicated test helper functions for
> improved test clarity and consistency.
>
> This modernization improves test script readability by using
> Git's dedicated test helpers: > - `test -f` → `test_path_is_file`
> - `test -d` → `test_path_is_dir`
> - `test ! -h && test -f` →
> `test_path_is_file_not_symlink`
> - `test ! -h && test -d` →
> `test_path_is_dir_not_symlink`

Try explaining why have you done this, why are these helpers better
than what was before rather than clarity and consistency, what do
these new helpers do to be considered valuable to refactor it.

> Found instances using: > git grep 'test -[efd]' t/ | grep 'test -[efd].*&&'

Even though it is mentioned on the microprojects to mention what have
you used to find the file, because it is only one file, including the
search command is not very useful. Drop it.

Given the date and that this seems a microproject I guess this is for
GSoC, you should add to the subject [GSoC PATCH] and CC your possible
co mentors.

I encourage you the same that it's being encourage for newcomers, read:

      https://lore.kernel.org/git/

There you'll find other microprojects similar to yours where you can learn from.

Code seems OK.

[1]: https://github.com/git/git/blob/master/Documentation/SubmittingPatches
[2]: https://github.com/git/git/blob/master/Documentation/MyFirstContribution.adoc

[GSoC PATCH v2] t2000: modernize path checks with test_path_is_* helpers

[Zakariyah Ali]

Replace bare 'test -f/-d' and 'test ! -h' assertions with dedicated
helpers. These helpers report loudly what expectation wasn't met,
therefore making debugging easier.

Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
---
 t/t2000-conflict-when-checking-files-out.sh | 12 +++++++-----
 1 file changed, 7 insertions(+), 5 deletions(-)

diff --git a/t/t2000-conflict-when-checking-files-out.sh b/t/t2000-conflict-when-checking-files-out.sh
index f18616ad2b..96bae6c53d 100755
--- a/t/t2000-conflict-when-checking-files-out.sh
+++ b/t/t2000-conflict-when-checking-files-out.sh
@@ -58,7 +58,9 @@ test_expect_success \
 
 test_expect_success \
     'git checkout-index conflicting paths.' \
-    'test -f path0 && test -d path1 && test -f path1/file1'
+    'test_path_is_file path0 &&
+     test_path_is_dir path1 &&
+     test_path_is_file path1/file1'
 
 test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
 	mkdir -p tar/get &&
@@ -127,9 +129,9 @@ test_debug 'show_files $tree2'
 
 test_expect_success \
     'checking out conflicting path with -f' \
-    'test ! -h path2 && test -d path2 &&
-     test ! -h path3 && test -d path3 &&
-     test ! -h path2/file0 && test -f path2/file0 &&
-     test ! -h path3/file1 && test -f path3/file1'
+    'test_path_is_dir_not_symlink path2 &&
+     test_path_is_dir_not_symlink path3 &&
+     test_path_is_file_not_symlink path2/file0 &&
+     test_path_is_file_not_symlink path3/file1'
 
 test_done
-- 
2.43.0

Re: [GSoC PATCH v2] t2000: modernize path checks with test_path_is_* helpers

[Junio C Hamano]

Zakariyah Ali <zakariyahali100@gmail.com> writes:

> Replace bare 'test -f/-d' and 'test ! -h' assertions with dedicated
> helpers. These helpers report loudly what expectation wasn't met,
> therefore making debugging easier.
>
> Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
> ---
>  t/t2000-conflict-when-checking-files-out.sh | 12 +++++++-----
>  1 file changed, 7 insertions(+), 5 deletions(-)

There is nothing in the patch text or in the proposed log message
that is wrong per-se, but looking at the entire test script, it
needs a major clean-up to match the modern testing standard.

On top of the patch we see here, we may want to do a follow-up patch
series to clean them.

Here is how such a patch may start out.  I think the remainder of
the file needs to be cleaned up similarly with about the same amount
of work.

---- >8 ----
Subject: t2000: modernise overall structure

This test script that dates back to 2005 certainly shows its age and
both its style and the way the tests are laid out do not match the
modern standard.

 * Executables that prepare the data used to test the command
   should be inside the test_expect_success block in modern tests.

 * In modern tests, running a command that is being tested, making
   sure it succeeds, and inspecting other side effects that are
   expected, are all done in a single test_expect_success block.

 * A test_expect_success block in modern tests are laid out as

	test_expect_success 'title of the test' '
		body of the test &&
		...
		body of the test
	'

   not as

	test_expect_success \
		'title of the test' \
                'body of the test &&
		...
		body of the test'

   which is in a prehistoric style.

 * In modern tests, each &&-chained statement in the body of the
   test_expect_success block are indented with a horizontal tab,
   unlike prehistoric style that used 4-space indent.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 t/t2000-conflict-when-checking-files-out.sh | 43 ++++++++++++++---------------
 1 file changed, 21 insertions(+), 22 deletions(-)

diff --git c/t/t2000-conflict-when-checking-files-out.sh w/t/t2000-conflict-when-checking-files-out.sh
index 96bae6c53d..39c80e80ea 100755
--- c/t/t2000-conflict-when-checking-files-out.sh
+++ w/t/t2000-conflict-when-checking-files-out.sh
@@ -35,32 +35,31 @@ show_files() {
 	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
 }
 
-date >path0
-mkdir path1
-date >path1/file1
+test_expect_success 'prepare files path0 and path1/file1' '
+	date >path0 &&
+	mkdir path1 &&
+	date >path1/file1 &&
 
-test_expect_success \
-    'git update-index --add various paths.' \
-    'git update-index --add path0 path1/file1'
-
-rm -fr path0 path1
-mkdir path0
-date >path0/file0
-date >path1
+	git update-index --add path0 path1/file1
+'
 
-test_expect_success \
-    'git checkout-index without -f should fail on conflicting work tree.' \
-    'test_must_fail git checkout-index -a'
+test_expect_success 'prepare working tree files with D/F conflicts' '
+	rm -fr path0 path1 &&
+	mkdir path0 &&
+	date >path0/file0 &&
+	date >path1
+'
 
-test_expect_success \
-    'git checkout-index with -f should succeed.' \
-    'git checkout-index -f -a'
+test_expect_success 'git checkout-index without -f should fail on conflicting work tree.' '
+	test_must_fail git checkout-index -a
+'
 
-test_expect_success \
-    'git checkout-index conflicting paths.' \
-    'test_path_is_file path0 &&
-     test_path_is_dir path1 &&
-     test_path_is_file path1/file1'
+test_expect_success 'git checkout-index with -f should succeed.' '
+	git checkout-index -f -a &&
+	test_path_is_file path0 &&
+	test_path_is_dir path1 &&
+	test_path_is_file path1/file1
+'
 
 test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
 	mkdir -p tar/get &&

[GSoC][PATCH v3] t2000: modernise overall structure

[Zakariyah Ali]

This test script that dates back to 2005 certainly shows its age and
both its style and the way the tests are laid out do not match the
modern standard.

 * Executables that prepare the data used to test the command should
   be inside the test_expect_success block in modern tests.

 * In modern tests, running a command that is being tested, making
   sure it succeeds, and inspecting other side effects that are
   expected, are all done in a single test_expect_success block.

 * A test_expect_success block in modern tests are laid out as

        test_expect_success 'title of the test' '
                body of the test &&
                ...
                body of the test
        '

   not as

        test_expect_success \
                'title of the test' \
                'body of the test &&
                ...
                body of the test'

   which is in a prehistoric style.

 * In modern tests, each &&-chained statement in the body of the
   test_expect_success block are indented with a horizontal tab,
   unlike prehistoric style that used 4-space indent.

Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com>
---
 t/t2000-conflict-when-checking-files-out.sh | 122 +++++++++++---------
 1 file changed, 66 insertions(+), 56 deletions(-)

diff --git a/t/t2000-conflict-when-checking-files-out.sh b/t/t2000-conflict-when-checking-files-out.sh
index f18616ad2b..af199d8191 100755
--- a/t/t2000-conflict-when-checking-files-out.sh
+++ b/t/t2000-conflict-when-checking-files-out.sh
@@ -35,30 +35,30 @@ show_files() {
 	sed -e 's/^\([0-9]*\)	[^ ]*	[0-9a-f]*	/tr: \1 /'
 }
 
-date >path0
-mkdir path1
-date >path1/file1
-
-test_expect_success \
-    'git update-index --add various paths.' \
-    'git update-index --add path0 path1/file1'
-
-rm -fr path0 path1
-mkdir path0
-date >path0/file0
-date >path1
+test_expect_success 'prepare files path0 and path1/file1' '
+	date >path0 &&
+	mkdir path1 &&
+	date >path1/file1 &&
+	git update-index --add path0 path1/file1
+'
 
-test_expect_success \
-    'git checkout-index without -f should fail on conflicting work tree.' \
-    'test_must_fail git checkout-index -a'
+test_expect_success 'prepare working tree files with D/F conflicts' '
+	rm -fr path0 path1 &&
+	mkdir path0 &&
+	date >path0/file0 &&
+	date >path1
+'
 
-test_expect_success \
-    'git checkout-index with -f should succeed.' \
-    'git checkout-index -f -a'
+test_expect_success 'git checkout-index without -f should fail on conflicting work tree.' '
+	test_must_fail git checkout-index -a
+'
 
-test_expect_success \
-    'git checkout-index conflicting paths.' \
-    'test -f path0 && test -d path1 && test -f path1/file1'
+test_expect_success 'git checkout-index with -f should succeed.' '
+	git checkout-index -f -a &&
+	test_path_is_file path0 &&
+	test_path_is_dir path1 &&
+	test_path_is_file path1/file1
+'
 
 test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
 	mkdir -p tar/get &&
@@ -83,53 +83,63 @@ test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' '
 # path path3 is occupied by a non-directory.  With "-f" it should remove
 # the symlink path3 and create directory path3 and file path3/file1.
 
-mkdir path2
-date >path2/file0
-test_expect_success \
-    'git update-index --add path2/file0' \
-    'git update-index --add path2/file0'
-test_expect_success \
-    'writing tree out with git write-tree' \
-    'tree1=$(git write-tree)'
+test_expect_success 'prepare path2/file0 and index' '
+	mkdir path2 &&
+	date >path2/file0 &&
+	git update-index --add path2/file0
+'
+
+test_expect_success 'write tree with path2/file0' '
+	tree1=$(git write-tree)
+'
+
 test_debug 'show_files $tree1'
 
-mkdir path3
-date >path3/file1
-test_expect_success \
-    'git update-index --add path3/file1' \
-    'git update-index --add path3/file1'
-test_expect_success \
-    'writing tree out with git write-tree' \
-    'tree2=$(git write-tree)'
+test_expect_success 'prepare path3/file1 and index' '
+	mkdir path3 &&
+	date >path3/file1 &&
+	git update-index --add path3/file1
+'
+
+test_expect_success 'write tree with path3/file1' '
+	tree2=$(git write-tree)
+'
+
 test_debug 'show_files $tree2'
 
-rm -fr path3
-test_expect_success \
-    'read previously written tree and checkout.' \
-    'git read-tree -m $tree1 && git checkout-index -f -a'
+test_expect_success 'read previously written tree and checkout.' '
+	rm -fr path3 &&
+	git read-tree -m $tree1 &&
+	git checkout-index -f -a
+'
+
 test_debug 'show_files $tree1'
 
-test_expect_success \
-    'add a symlink' \
-    'test_ln_s_add path2 path3'
-test_expect_success \
-    'writing tree out with git write-tree' \
-    'tree3=$(git write-tree)'
+test_expect_success 'add a symlink' '
+	test_ln_s_add path2 path3
+'
+
+test_expect_success 'write tree with symlink path3' '
+	tree3=$(git write-tree)
+'
+
 test_debug 'show_files $tree3'
 
 # Morten says "Got that?" here.
 # Test begins.
 
-test_expect_success \
-    'read previously written tree and checkout.' \
-    'git read-tree $tree2 && git checkout-index -f -a'
+test_expect_success 'read previously written tree and checkout.' '
+	git read-tree $tree2 &&
+	git checkout-index -f -a
+'
+
 test_debug 'show_files $tree2'
 
-test_expect_success \
-    'checking out conflicting path with -f' \
-    'test ! -h path2 && test -d path2 &&
-     test ! -h path3 && test -d path3 &&
-     test ! -h path2/file0 && test -f path2/file0 &&
-     test ! -h path3/file1 && test -f path3/file1'
+test_expect_success 'checking out conflicting path with -f' '
+	test_path_is_dir_not_symlink path2 &&
+	test_path_is_dir_not_symlink path3 &&
+	test_path_is_file_not_symlink path2/file0 &&
+	test_path_is_file_not_symlink path3/file1
+'
 
 test_done
-- 
2.43.0