[GSoC][PATCH] t9160:modernize test path checking#2160
Open
HodaSalim wants to merge 1 commit intogit:masterfrom
Open
[GSoC][PATCH] t9160:modernize test path checking#2160HodaSalim wants to merge 1 commit intogit:masterfrom
HodaSalim wants to merge 1 commit intogit:masterfrom
Conversation
github-project-automation
bot
moved this to Todo
in Op-Amp Frequency Response and Applications Suite
Welcome to GitGitGadgetHi @HodaSalim, and welcome to GitGitGadget, the GitHub App to send patch series to the Git mailing list from GitHub Pull Requests. Please make sure that either:
You can CC potential reviewers by adding a footer to the PR description with the following syntax: NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description, Also, it is a good idea to review the commit messages one last time, as the Git project expects them in a quite specific form:
It is in general a good idea to await the automated test ("Checks") in this Pull Request before contributing the patches, e.g. to avoid trivial issues such as unportable code. Contributing the patchesBefore you can contribute the patches, your GitHub username needs to be added to the list of permitted users. Any already-permitted user can do that, by adding a comment to your PR of the form Both the person who commented An alternative is the channel Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment If you want to see what email(s) would be sent for a After you submit, GitGitGadget will respond with another comment that contains the link to the cover letter mail in the Git mailing list archive. Please make sure to monitor the discussion in that thread and to address comments and suggestions (while the comments and suggestions will be mirrored into the PR by GitGitGadget, you will still want to reply via mail). If you do not want to subscribe to the Git mailing list just to be able to respond to a mail, you can download the mbox from the Git mailing list archive (click the curl -g --user "<EMailAddress>:<Password>" \
--url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txtTo iterate on your change, i.e. send a revised patch or patch series, you will first want to (force-)push to the same branch. You probably also want to modify your Pull Request description (or title). It is a good idea to summarize the revision by adding something like this to the cover letter (read: by editing the first comment on the PR, i.e. the PR description): To send a new iteration, just add another PR comment with the contents: Need help?New contributors who want advice are encouraged to join git-mentoring@googlegroups.com, where volunteers who regularly contribute to Git are willing to answer newbie questions, give advice, or otherwise provide mentoring to interested contributors. You must join in order to post or view messages, but anyone can join. You may also be able to find help in real time in the developer IRC channel, |
HodaSalim
changed the title
Member
|
/allow |
|
User HodaSalim is now allowed to use GitGitGadget. |
Author
|
/preview |
|
Preview email sent as pull.2160.git.git.1767624256966.gitgitgadget@gmail.com |
Replace old-style path checks with Git's dedicated test helpers: - test -f → test_path_is_file - test -d → test_path_is_dir - test -s → test_file_not_empty Found using: git grep "test -[efd]" t/ This improves test readability and provides better error messages when path checks fail. Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
HodaSalim
force-pushed
the
microproject/modernize-t9160
branch
from
2ec9e93 to
a69555e
Compare
Author
|
/preview |
|
Preview email sent as pull.2160.git.git.1767624627377.gitgitgadget@gmail.com |
HodaSalim
changed the title
Author
|
/submit |
|
Submitted as pull.2160.git.git.1767625195071.gitgitgadget@gmail.com To fetch this version into To fetch this version to local tag |
|
Hoda Salim wrote on the Git mailing list (how to reply to this email): From: HodaSalim <hoda.s.salim@gmail.com>
Replace old-style path checks with Git's dedicated test helpers:
- test -f → test_path_is_file
- test -d → test_path_is_dir
- test -s → test_file_not_empty
Fix typos with the word "subsequent"
Found using: git grep "test -[efd]" t/
This improves test readability and provides better error messages
when path checks fail.
Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
---
t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
1 file changed, 13 insertions(+), 13 deletions(-)
diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
index 36c6b1a12f..de32cf2542 100755
--- a/t/t9160-git-svn-preserve-empty-dirs.sh
+++ b/t/t9160-git-svn-preserve-empty-dirs.sh
@@ -61,15 +61,15 @@ test_expect_success 'clone svn repo with --preserve-empty-dirs' '
# "$GIT_REPO"/1 should only contain the placeholder file.
test_expect_success 'directory empty from inception' '
- test -f "$GIT_REPO"/1/.gitignore &&
+ test_path_is_file "$GIT_REPO"/1/.gitignore &&
test $(find "$GIT_REPO"/1 -type f | wc -l) = "1"
'
# "$GIT_REPO"/2 and "$GIT_REPO"/3 should only contain the placeholder file.
test_expect_success 'directory empty from subsequent svn commit' '
- test -f "$GIT_REPO"/2/.gitignore &&
+ test_path_is_file "$GIT_REPO"/2/.gitignore &&
test $(find "$GIT_REPO"/2 -type f | wc -l) = "1" &&
- test -f "$GIT_REPO"/3/.gitignore &&
+ test_path_is_file "$GIT_REPO"/3/.gitignore &&
test $(find "$GIT_REPO"/3 -type f | wc -l) = "1"
'
@@ -77,7 +77,7 @@ test_expect_success 'directory empty from subsequent svn commit' '
# generated for every sub-directory at some point in the repo's history.
test_expect_success 'add entry to previously empty directory' '
test $(find "$GIT_REPO"/4 -type f | wc -l) = "1" &&
- test -f "$GIT_REPO"/4/a/b/c/foo
+ test_path_is_file "$GIT_REPO"/4/a/b/c/foo
'
# The HEAD~2 commit should not have introduced .gitignore placeholder files.
@@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
# "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
test_expect_success 'placeholder namespace conflict with file' '
- test -s "$GIT_REPO"/5/.placeholder
+ test_file_not_empty "$GIT_REPO"/5/.placeholder
'
# "$GIT_REPO"/6/.placeholder should be a directory, and the "$GIT_REPO"/6 tree
# should only contain one file: the placeholder.
test_expect_success 'placeholder namespace conflict with directory' '
- test -d "$GIT_REPO"/6/.placeholder &&
- test -f "$GIT_REPO"/6/.placeholder/.placeholder &&
+ test_path_is_dir "$GIT_REPO"/6/.placeholder &&
+ test_path_is_file "$GIT_REPO"/6/.placeholder/.placeholder &&
test $(find "$GIT_REPO"/6 -type f | wc -l) = "1"
'
@@ -133,19 +133,19 @@ test_expect_success 'second set of svn commits and rebase' '
# Check that --preserve-empty-dirs and --placeholder-file flag state
# stays persistent over multiple invocations.
-test_expect_success 'flag persistence during subsqeuent rebase' '
- test -f "$GIT_REPO"/7/.placeholder &&
+test_expect_success 'flag persistence during subsequent rebase' '
+ test_path_is_file "$GIT_REPO"/7/.placeholder &&
test $(find "$GIT_REPO"/7 -type f | wc -l) = "1"
'
# Check that placeholder files are properly removed when unnecessary,
# even across multiple invocations.
-test_expect_success 'placeholder list persistence during subsqeuent rebase' '
- test -f "$GIT_REPO"/1/file1.txt &&
+test_expect_success 'placeholder list persistence during subsequent rebase' '
+ test_path_is_file "$GIT_REPO"/1/file1.txt &&
test $(find "$GIT_REPO"/1 -type f | wc -l) = "1" &&
- test -f "$GIT_REPO"/5/file1.txt &&
- test -f "$GIT_REPO"/5/.placeholder &&
+ test_path_is_file "$GIT_REPO"/5/file1.txt &&
+ test_path_is_file "$GIT_REPO"/5/.placeholder &&
test $(find "$GIT_REPO"/5 -type f | wc -l) = "2"
'
--
2.50.1 (Apple Git-155)
|
|
Junio C Hamano wrote on the Git mailing list (how to reply to this email): Hoda Salim <hoda.s.salim@gmail.com> writes:
> From: HodaSalim <hoda.s.salim@gmail.com>
>
> Replace old-style path checks with Git's dedicated test helpers:
> - test -f → test_path_is_file
> - test -d → test_path_is_dir
> - test -s → test_file_not_empty
>
> Fix typos with the word "subsequent"
>
> Found using: git grep "test -[efd]" t/
>
> This improves test readability and provides better error messages
> when path checks fail.
For a small change like this, the above is sufficient as all the
necessary things are described, but for future reference, we prefer
to explain things in this order:
- Give an observation on how the current system works in the
present tense (so no need to say "Currently X is Y", or
"Previously X was Y" to describe the state before your change;
just "X is Y" is enough), and discuss what you perceive as a
problem in it.
- Propose a solution (optional---often, problem description
trivially leads to an obvious solution in reader's minds).
- Give commands to somebody editing the codebase to "make it so",
instead of saying "This commit does X".
You are going backwards ;-).
Will queue. Thanks (and thanks for all the reviewers who helped
polish the draft to get to this v2).
>
> Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
> ---
> t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
> 1 file changed, 13 insertions(+), 13 deletions(-)
>
> diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
> index 36c6b1a12f..de32cf2542 100755
> --- a/t/t9160-git-svn-preserve-empty-dirs.sh
> +++ b/t/t9160-git-svn-preserve-empty-dirs.sh
> @@ -61,15 +61,15 @@ test_expect_success 'clone svn repo with --preserve-empty-dirs' '
>
> # "$GIT_REPO"/1 should only contain the placeholder file.
> test_expect_success 'directory empty from inception' '
> - test -f "$GIT_REPO"/1/.gitignore &&
> + test_path_is_file "$GIT_REPO"/1/.gitignore &&
> test $(find "$GIT_REPO"/1 -type f | wc -l) = "1"
> '
>
> # "$GIT_REPO"/2 and "$GIT_REPO"/3 should only contain the placeholder file.
> test_expect_success 'directory empty from subsequent svn commit' '
> - test -f "$GIT_REPO"/2/.gitignore &&
> + test_path_is_file "$GIT_REPO"/2/.gitignore &&
> test $(find "$GIT_REPO"/2 -type f | wc -l) = "1" &&
> - test -f "$GIT_REPO"/3/.gitignore &&
> + test_path_is_file "$GIT_REPO"/3/.gitignore &&
> test $(find "$GIT_REPO"/3 -type f | wc -l) = "1"
> '
>
> @@ -77,7 +77,7 @@ test_expect_success 'directory empty from subsequent svn commit' '
> # generated for every sub-directory at some point in the repo's history.
> test_expect_success 'add entry to previously empty directory' '
> test $(find "$GIT_REPO"/4 -type f | wc -l) = "1" &&
> - test -f "$GIT_REPO"/4/a/b/c/foo
> + test_path_is_file "$GIT_REPO"/4/a/b/c/foo
> '
>
> # The HEAD~2 commit should not have introduced .gitignore placeholder files.
> @@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
>
> # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
> test_expect_success 'placeholder namespace conflict with file' '
> - test -s "$GIT_REPO"/5/.placeholder
> + test_file_not_empty "$GIT_REPO"/5/.placeholder
> '
>
> # "$GIT_REPO"/6/.placeholder should be a directory, and the "$GIT_REPO"/6 tree
> # should only contain one file: the placeholder.
> test_expect_success 'placeholder namespace conflict with directory' '
> - test -d "$GIT_REPO"/6/.placeholder &&
> - test -f "$GIT_REPO"/6/.placeholder/.placeholder &&
> + test_path_is_dir "$GIT_REPO"/6/.placeholder &&
> + test_path_is_file "$GIT_REPO"/6/.placeholder/.placeholder &&
> test $(find "$GIT_REPO"/6 -type f | wc -l) = "1"
> '
>
> @@ -133,19 +133,19 @@ test_expect_success 'second set of svn commits and rebase' '
>
> # Check that --preserve-empty-dirs and --placeholder-file flag state
> # stays persistent over multiple invocations.
> -test_expect_success 'flag persistence during subsqeuent rebase' '
> - test -f "$GIT_REPO"/7/.placeholder &&
> +test_expect_success 'flag persistence during subsequent rebase' '
> + test_path_is_file "$GIT_REPO"/7/.placeholder &&
> test $(find "$GIT_REPO"/7 -type f | wc -l) = "1"
> '
>
> # Check that placeholder files are properly removed when unnecessary,
> # even across multiple invocations.
> -test_expect_success 'placeholder list persistence during subsqeuent rebase' '
> - test -f "$GIT_REPO"/1/file1.txt &&
> +test_expect_success 'placeholder list persistence during subsequent rebase' '
> + test_path_is_file "$GIT_REPO"/1/file1.txt &&
> test $(find "$GIT_REPO"/1 -type f | wc -l) = "1" &&
>
> - test -f "$GIT_REPO"/5/file1.txt &&
> - test -f "$GIT_REPO"/5/.placeholder &&
> + test_path_is_file "$GIT_REPO"/5/file1.txt &&
> + test_path_is_file "$GIT_REPO"/5/.placeholder &&
> test $(find "$GIT_REPO"/5 -type f | wc -l) = "2"
> ' |
|
Hoda Salim wrote on the Git mailing list (how to reply to this email): > Will queue. Thanks (and thanks for all the reviewers who helped
> polish the draft to get to this v2).
Yeah, sorry about that. The patch was initially submitted using
GitGitGadget and I didn't really do a great job in the beginning to
reply to the GitGitGadget email.
Pushkar was the maintainer that kindly pointed out the misspelling. I
added him to the CC for visibility
Thank You,
Hoda
On Mon, Feb 2, 2026 at 9:00 PM Junio C Hamano <gitster@pobox.com> wrote:
>
> Hoda Salim <hoda.s.salim@gmail.com> writes:
>
> > From: HodaSalim <hoda.s.salim@gmail.com>
> >
> > Replace old-style path checks with Git's dedicated test helpers:
> > - test -f → test_path_is_file
> > - test -d → test_path_is_dir
> > - test -s → test_file_not_empty
> >
> > Fix typos with the word "subsequent"
> >
> > Found using: git grep "test -[efd]" t/
> >
> > This improves test readability and provides better error messages
> > when path checks fail.
>
> For a small change like this, the above is sufficient as all the
> necessary things are described, but for future reference, we prefer
> to explain things in this order:
>
> - Give an observation on how the current system works in the
> present tense (so no need to say "Currently X is Y", or
> "Previously X was Y" to describe the state before your change;
> just "X is Y" is enough), and discuss what you perceive as a
> problem in it.
>
> - Propose a solution (optional---often, problem description
> trivially leads to an obvious solution in reader's minds).
>
> - Give commands to somebody editing the codebase to "make it so",
> instead of saying "This commit does X".
>
> You are going backwards ;-).
>
> Will queue. Thanks (and thanks for all the reviewers who helped
> polish the draft to get to this v2).
>
>
> >
> > Signed-off-by: HodaSalim <hoda.s.salim@gmail.com>
> > ---
> > t/t9160-git-svn-preserve-empty-dirs.sh | 26 +++++++++++++-------------
> > 1 file changed, 13 insertions(+), 13 deletions(-)
> >
> > diff --git a/t/t9160-git-svn-preserve-empty-dirs.sh b/t/t9160-git-svn-preserve-empty-dirs.sh
> > index 36c6b1a12f..de32cf2542 100755
> > --- a/t/t9160-git-svn-preserve-empty-dirs.sh
> > +++ b/t/t9160-git-svn-preserve-empty-dirs.sh
> > @@ -61,15 +61,15 @@ test_expect_success 'clone svn repo with --preserve-empty-dirs' '
> >
> > # "$GIT_REPO"/1 should only contain the placeholder file.
> > test_expect_success 'directory empty from inception' '
> > - test -f "$GIT_REPO"/1/.gitignore &&
> > + test_path_is_file "$GIT_REPO"/1/.gitignore &&
> > test $(find "$GIT_REPO"/1 -type f | wc -l) = "1"
> > '
> >
> > # "$GIT_REPO"/2 and "$GIT_REPO"/3 should only contain the placeholder file.
> > test_expect_success 'directory empty from subsequent svn commit' '
> > - test -f "$GIT_REPO"/2/.gitignore &&
> > + test_path_is_file "$GIT_REPO"/2/.gitignore &&
> > test $(find "$GIT_REPO"/2 -type f | wc -l) = "1" &&
> > - test -f "$GIT_REPO"/3/.gitignore &&
> > + test_path_is_file "$GIT_REPO"/3/.gitignore &&
> > test $(find "$GIT_REPO"/3 -type f | wc -l) = "1"
> > '
> >
> > @@ -77,7 +77,7 @@ test_expect_success 'directory empty from subsequent svn commit' '
> > # generated for every sub-directory at some point in the repo's history.
> > test_expect_success 'add entry to previously empty directory' '
> > test $(find "$GIT_REPO"/4 -type f | wc -l) = "1" &&
> > - test -f "$GIT_REPO"/4/a/b/c/foo
> > + test_path_is_file "$GIT_REPO"/4/a/b/c/foo
> > '
> >
> > # The HEAD~2 commit should not have introduced .gitignore placeholder files.
> > @@ -102,14 +102,14 @@ test_expect_success 'clone svn repo with --placeholder-file specified' '
> >
> > # "$GIT_REPO"/5/.placeholder should be a file, and non-empty.
> > test_expect_success 'placeholder namespace conflict with file' '
> > - test -s "$GIT_REPO"/5/.placeholder
> > + test_file_not_empty "$GIT_REPO"/5/.placeholder
> > '
> >
> > # "$GIT_REPO"/6/.placeholder should be a directory, and the "$GIT_REPO"/6 tree
> > # should only contain one file: the placeholder.
> > test_expect_success 'placeholder namespace conflict with directory' '
> > - test -d "$GIT_REPO"/6/.placeholder &&
> > - test -f "$GIT_REPO"/6/.placeholder/.placeholder &&
> > + test_path_is_dir "$GIT_REPO"/6/.placeholder &&
> > + test_path_is_file "$GIT_REPO"/6/.placeholder/.placeholder &&
> > test $(find "$GIT_REPO"/6 -type f | wc -l) = "1"
> > '
> >
> > @@ -133,19 +133,19 @@ test_expect_success 'second set of svn commits and rebase' '
> >
> > # Check that --preserve-empty-dirs and --placeholder-file flag state
> > # stays persistent over multiple invocations.
> > -test_expect_success 'flag persistence during subsqeuent rebase' '
> > - test -f "$GIT_REPO"/7/.placeholder &&
> > +test_expect_success 'flag persistence during subsequent rebase' '
> > + test_path_is_file "$GIT_REPO"/7/.placeholder &&
> > test $(find "$GIT_REPO"/7 -type f | wc -l) = "1"
> > '
> >
> > # Check that placeholder files are properly removed when unnecessary,
> > # even across multiple invocations.
> > -test_expect_success 'placeholder list persistence during subsqeuent rebase' '
> > - test -f "$GIT_REPO"/1/file1.txt &&
> > +test_expect_success 'placeholder list persistence during subsequent rebase' '
> > + test_path_is_file "$GIT_REPO"/1/file1.txt &&
> > test $(find "$GIT_REPO"/1 -type f | wc -l) = "1" &&
> >
> > - test -f "$GIT_REPO"/5/file1.txt &&
> > - test -f "$GIT_REPO"/5/.placeholder &&
> > + test_path_is_file "$GIT_REPO"/5/file1.txt &&
> > + test_path_is_file "$GIT_REPO"/5/.placeholder &&
> > test $(find "$GIT_REPO"/5 -type f | wc -l) = "2"
> > ' |
|
This patch series was integrated into seen via 3f6c068. |
|
This patch series was integrated into seen via 831969a. |
|
This patch series was integrated into seen via 1168106. |
|
This branch is now known as |
|
This patch series was integrated into seen via e3bb321. |
|
This patch series was integrated into next via 0d40107. |
This file contains hidden or bidirectional Unicode text that may be interpreted or compiled differently than what appears below. To review, open the file in an editor that reveals hidden Unicode characters.
Learn more about bidirectional Unicode characters
Sign up for free
to join this conversation on GitHub.
Already have an account?
Sign in to comment
2 participants
Add this suggestion to a batch that can be applied as a single commit.This suggestion is invalid because no changes were made to the code.Suggestions cannot be applied while the pull request is closed.Suggestions cannot be applied while viewing a subset of changes.Only one suggestion per line can be applied in a batch.Add this suggestion to a batch that can be applied as a single commit.Applying suggestions on deleted lines is not supported.You must change the existing code in this line in order to create a valid suggestion.Outdated suggestions cannot be applied.This suggestion has been applied or marked resolved.Suggestions cannot be applied from pending reviews.Suggestions cannot be applied on multi-line comments.Suggestions cannot be applied while the pull request is queued to merge.Suggestion cannot be applied right now. Please check back later.
No description provided.