Skip to content

[Outreachy][PATCH] t7300-clean.sh: use test_path_* helper functions for error logging #1811

New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom
Closed
wants to merge 1 commit into from
Closed

[Outreachy][PATCH] t7300-clean.sh: use test_path_* helper functions for error logging #1811

wants to merge 1 commit into from

Conversation

devdekunle
Copy link

@devdekunle devdekunle commented Oct 7, 2024 •
edited
Loading

This test script uses "test - [def]", but when a test fails because the file passed to it does not exist, it fails silently without an error message.

Use test_path_* helper functions, which are designed to give better error messages when their expectations are not met.

I have added a mechanical validation that applies the same transformation done in this patch, when the script is passed to a sed script as shown below

#!/bin/bash
sed -e 's/^\(	*\)test -f /\1test_path_is_file /' \
    -e 's/^\(	*\)test -d /\1test_path_is_dir /' \
    -e 's/^\(	*\)test -e /\1test_path_exists /' \
    -e 's/^\(	*\)! test -[edf] /\1test_path_is_missing /' \
    -e 's/^\(	*\)test ! -[edf] /\1test_path_is_missing /' \
       "$1" >foo.sh

Reviewers can use the sed script to transform the original test script and compare the results in foo.sh with the results of applying this patch
You will see an instance of "! (test -e 3)" which was manually replaced with "test_path_is_missing", and everything else should match.
I have carefully studied the instances where "test ! - [df] foo" were used in the script to make sure that the test instances were expecting a file or a directory to not exist as the case may be before replacing with "test_path_is_missing".

Signed-off-by: Abraham Samuel Adekunle [email protected]
cc: Usman Akinyemi [email protected]
cc: Patrick Steinhardt [email protected]
cc: Junio C Hamano [email protected]
cc: Phillip Wood [email protected]

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 7, 2024

Welcome to GitGitGadget

Hi @devdekunle, 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:

  • Your Pull Request has a good description, if it consists of multiple commits, as it will be used as cover letter.
  • Your Pull Request description is empty, if it consists of a single commit, as the commit message should be descriptive enough by itself.

You can CC potential reviewers by adding a footer to the PR description with the following syntax:

CC: Revi Ewer <[email protected]>, Ill Takalook <[email protected]>

NOTE: DO NOT copy/paste your CC list from a previous GGG PR's description,
because it will result in a malformed CC list on the mailing list. See
example.

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:

  • the lines should not exceed 76 columns,
  • the first line should be like a header and typically start with a prefix like "tests:" or "revisions:" to state which subsystem the change is about, and
  • the commit messages' body should be describing the "why?" of the change.
  • Finally, the commit messages should end in a Signed-off-by: line matching the commits' author.

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 patches

Before 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 /allow. A good way to find other contributors is to locate recent pull requests where someone has been /allowed:

Both the person who commented /allow and the PR author are able to /allow you.

An alternative is the channel #git-devel on the Libera Chat IRC network:

<newcontributor> I've just created my first PR, could someone please /allow me? https://github.com/gitgitgadget/git/pull/12345
<veteran> newcontributor: it is done
<newcontributor> thanks!

Once on the list of permitted usernames, you can contribute the patches to the Git mailing list by adding a PR comment /submit.

If you want to see what email(s) would be sent for a /submit request, add a PR comment /preview to have the email(s) sent to you. You must have a public GitHub email address for this. Note that any reviewers CC'd via the list in the PR description will not actually be sent emails.

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 (raw) link), then import it into your mail program. If you use GMail, you can do this via:

curl -g --user "<EMailAddress>:<Password>" \
    --url "imaps://imap.gmail.com/INBOX" -T /path/to/raw.txt

To 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):

Changes since v1:
- Fixed a typo in the commit message (found by ...)
- Added a code comment to ... as suggested by ...
...

To send a new iteration, just add another PR comment with the contents: /submit.

Need help?

New contributors who want advice are encouraged to join [email protected], 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, #git-devel on Libera Chat. Remember that IRC does not support offline messaging, so if you send someone a private message and log out, they cannot respond to you. The scrollback of #git-devel is archived, though.

@gitgitgadget gitgitgadget bot added the new user label Oct 7, 2024
@dscho
Copy link
Member

dscho commented Oct 7, 2024

/allow

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 7, 2024

User devdekunle is now allowed to use GitGitGadget.

@dscho
Copy link
Member

dscho commented Oct 7, 2024

@devdekunle please edit the initial comment; It will be sent as a "cover letter" and should not repeat the commit message (nor include the initial advice you did not write).

@devdekunle
Copy link
Author

@devdekunle please edit the initial comment; It will be sent as a "cover letter" and should not repeat the commit message (nor include the initial advice you did not write).

Thank you very much @dscho for your assistance.
Please just to be clear on '(nor include the initial advice you did not write)'. Do you mean I should not include anything I did not include in my commit message?

@dscho
Copy link
Member

dscho commented Oct 7, 2024

I am referring to this:

Thanks for taking the time to contribute to Git! Please be advised that the Git community does not use github.com for their contributions. Instead, we use a mailing list ([email protected]) for code submissions, code reviews, and bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/) to conveniently send your Pull Requests commits to our mailing list.

For a single-commit pull request, please leave the pull request description empty: your commit message itself should describe your changes.

Please read the "guidelines for contributing" linked above!

It would not read well if this was included in the cover letter.

@devdekunle
Copy link
Author

I am referring to this:

Thanks for taking the time to contribute to Git! Please be advised that the Git community does not use github.com for their contributions. Instead, we use a mailing list ([email protected]) for code submissions, code reviews, and bug reports. Nevertheless, you can use GitGitGadget (https://gitgitgadget.github.io/) to conveniently send your Pull Requests commits to our mailing list.
For a single-commit pull request, please leave the pull request description empty: your commit message itself should describe your changes.
Please read the "guidelines for contributing" linked above!

It would not read well if this was included in the cover letter.

Ohhh okay thank you very much once again. I will remove that now

@devdekunle
Copy link
Author

@dscho I have edited it. Please is it safe to submit now?

@dscho
Copy link
Member

dscho commented Oct 7, 2024

@dscho I have edited it. Please is it safe to submit now?

It should be safe now, yes.

@devdekunle
Copy link
Author

@dscho I have edited it. Please is it safe to submit now?

It should be safe now, yes.

Okay thank you very much. I really appreciate.

@devdekunle
Copy link
Author

/submit

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 7, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1811/devdekunle/update_tests-v1

To fetch this version to local tag pr-1811/devdekunle/update_tests-v1:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1811/devdekunle/update_tests-v1

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 7, 2024

On the Git mailing list, Usman Akinyemi wrote (reply to this):

On Mon, Oct 7, 2024 at 7:19 PM Samuel Adekunle Abraham via
GitGitGadget <[email protected]> wrote:
>
> From: Abraham Samuel Adekunle <[email protected]>
>
> The test_path_* helper functions provide error messages which show the cause
> of the test failures. Hence they are used to replace every instance of
> test - [def] uses in the script.
Maybe also adding what they are being replaced with might make the
description much clearer.
>
> Signed-off-by: Abraham Samuel Adekunle <[email protected]>
> ---
>     [Outreachy] [PATCH] t7300-clean.sh: replace instances of test - [def]
>     with test_path_* helper functions.
Hello Samuel,

Good Job here, just a simple observation.
I think it might be much clearer if you used test -(d|e|f) instead of
test - [def], as it much clearer.
Overall it looks good to me.
>
>     The test_path_* helper functions provide error messages which show the
>     cause of the test failure should a failure occur. This is more useful
>     and helpful when debugging errors.
>
>     Signed-off-by: Abraham Samuel Adekunle [email protected]
>
> Published-As: https://github.com/gitgitgadget/git/releases/tag/pr-1811%2Fdevdekunle%2Fupdate_tests-v1
> Fetch-It-Via: git fetch https://github.com/gitgitgadget/git pr-1811/devdekunle/update_tests-v1
> Pull-Request: https://github.com/gitgitgadget/git/pull/1811
>
>  t/t7300-clean.sh | 370 +++++++++++++++++++++++------------------------
>  1 file changed, 185 insertions(+), 185 deletions(-)
>
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 0aae0dee670..5c97eb0dfe9 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -29,15 +29,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so &&
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so &&
>         git update-index --no-skip-worktree .gitignore &&
>         git checkout .gitignore
>  '
> @@ -47,15 +47,15 @@ test_expect_success 'git clean' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -64,15 +64,15 @@ test_expect_success 'git clean src/' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean src/ &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -81,15 +81,15 @@ test_expect_success 'git clean src/ src/' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean src/ src/ &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -98,16 +98,16 @@ test_expect_success 'git clean with prefix' '
>         mkdir -p build docs src/test &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&
>         (cd src/ && git clean) &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f src/test/1.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file src/test/1.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -163,16 +163,16 @@ test_expect_success 'git clean -d with prefix and path' '
>         mkdir -p build docs src/feature &&
>         touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so &&
>         (cd src/ && git clean -d feature/) &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test -f src/part3.c &&
> -       test ! -f src/feature/file.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_missing src/feature/file.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -182,16 +182,16 @@ test_expect_success SYMLINKS 'git clean symbolic link' '
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         ln -s docs/manual.txt src/part4.c &&
>         git clean &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test ! -f src/part4.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_missing src/part4.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -199,13 +199,13 @@ test_expect_success 'git clean with wildcard' '
>
>         touch a.clean b.clean other.c &&
>         git clean "*.clean" &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.clean &&
> -       test ! -f b.clean &&
> -       test -f other.c
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.clean &&
> +       test_path_is_missing b.clean &&
> +       test_path_is_file other.c
>
>  '
>
> @@ -214,15 +214,15 @@ test_expect_success 'git clean -n' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -n &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -231,15 +231,15 @@ test_expect_success 'git clean -d' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test ! -d docs &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_missing docs &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -248,16 +248,16 @@ test_expect_success 'git clean -d src/ examples/' '
>         mkdir -p build docs examples &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c &&
>         git clean -d src/ examples/ &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test ! -f examples/1.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_missing examples/1.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -266,15 +266,15 @@ test_expect_success 'git clean -x' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -x &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test ! -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -283,15 +283,15 @@ test_expect_success 'git clean -d -x' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -x &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test ! -d docs &&
> -       test ! -f obj.o &&
> -       test ! -d build
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_missing docs &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_missing build
>
>  '
>
> @@ -300,15 +300,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -x -e src &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test -f src/part3.c &&
> -       test ! -d docs &&
> -       test ! -f obj.o &&
> -       test ! -d build
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_missing docs &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_missing build
>
>  '
>
> @@ -317,15 +317,15 @@ test_expect_success 'git clean -X' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -X &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test ! -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -334,15 +334,15 @@ test_expect_success 'git clean -d -X' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -X &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test ! -f obj.o &&
> -       test ! -d build
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_missing build
>
>  '
>
> @@ -351,15 +351,15 @@ test_expect_success 'git clean -d -X with ignored tracked directory' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -X -e src &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test ! -f obj.o &&
> -       test ! -d build
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_missing obj.o &&
> +       test_path_is_missing build
>
>  '
>
> @@ -382,29 +382,29 @@ test_expect_success 'clean.requireForce and -n' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -n &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file Makefile &&
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_file a.out &&
> +       test_path_is_file src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
>  test_expect_success 'clean.requireForce and -f' '
>
>         git clean -f &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test -f obj.o &&
> -       test -f build/lib.so
> +       test_path_is_file README &&
> +       test_path_is_file src/part1.c &&
> +       test_path_is_file src/part2.c &&
> +       test_path_is_missing a.out &&
> +       test_path_is_missing src/part3.c &&
> +       test_path_is_file docs/manual.txt &&
> +       test_path_is_file obj.o &&
> +       test_path_is_file build/lib.so
>
>  '
>
> @@ -453,11 +453,11 @@ test_expect_success 'nested git work tree' '
>                 test_commit deeply.nested deeper.world
>         ) &&
>         git clean -f -d &&
> -       test -f foo/.git/index &&
> -       test -f foo/hello.world &&
> -       test -f baz/boo/.git/index &&
> -       test -f baz/boo/deeper.world &&
> -       ! test -d bar
> +       test_path_is_file foo/.git/index &&
> +       test_path_is_file foo/hello.world &&
> +       test_path_is_file baz/boo/.git/index &&
> +       test_path_is_file baz/boo/deeper.world &&
> +       test_path_is_missing bar
>  '
>
>  test_expect_success 'should clean things that almost look like git but are not' '
> @@ -624,9 +624,9 @@ test_expect_success 'force removal of nested git work tree' '
>                 test_commit deeply.nested deeper.world
>         ) &&
>         git clean -f -f -d &&
> -       ! test -d foo &&
> -       ! test -d bar &&
> -       ! test -d baz
> +       test_path_is_missing foo &&
> +       test_path_is_missing bar &&
> +       test_path_is_missing baz
>  '
>
>  test_expect_success 'git clean -e' '
> @@ -638,10 +638,10 @@ test_expect_success 'git clean -e' '
>                 touch known 1 2 3 &&
>                 git add known &&
>                 git clean -f -e 1 -e 2 &&
> -               test -e 1 &&
> -               test -e 2 &&
> -               ! (test -e 3) &&
> -               test -e known
> +               test_path_exists 1 &&
> +               test_path_exists 2 &&
> +               test_path_is_missing 3 &&
> +               test_path_exists known
>         )
>  '
>
> @@ -649,7 +649,7 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '
>         mkdir foo &&
>         chmod a= foo &&
>         git clean -dfx foo &&
> -       ! test -d foo
> +       test_path_is_missing foo
>  '
>
>  test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' '
>
> base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1
> --
> gitgitgadget
>

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 7, 2024

User Usman Akinyemi <[email protected]> has been added to the cc: list.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Samuel Adekunle Abraham via GitGitGadget" <[email protected]>
writes:

> From: Abraham Samuel Adekunle <[email protected]>
>
> The test_path_* helper functions provide error messages which show the cause
> of the test failures.

> Hence they are used to replace every instance of
> test - [def] uses in the script.

It is unclear the use of present tense "they are used" describes the
status quo, or the way you wish the test script were written.

The usual way to compose a log message of this project is to

 - Give an observation on how the current system work in the present
   tense (so no need to say "Currently X is Y", just "X is Y"), 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 the codebase to "become like so".

in this order.  

Also, for a patch like this one, which is rather large, repetitious,
and tire reviewers to miss simple typos easily, giving a procedure
to mechanically validate the patch would help.  Instead of having to
match up "test -f" that was removed with "test_is_file" that was
added, while ensuring the pathname given to them are the same, a
reader can reason about what the mechanical rewrite is doing, and
when the reader is convinced that the mechanical rewrite is doing
the right thing, being able to mechanically compare the result of
the procedure with the result of applying a patch is a really
powerful thing.

I probably would have written your two paragraphs more like the
first two paragraphs below, followed by the validation procedure,
like this:

    This test script uses "test -[edf]", but when a test fails
    because a file given to "test -f" does not exist, it fails
    silently.

    Use test_path_* helpers, which are designed to give better error
    messages when their expectations are not met.

    If you pass the current test script through

	sed -e 's/^\(	*\)test -f /\1test_path_is_file /' \
	    -e 's/^\(	*\)test -d /\1test_path_is_dir /' \
	    -e 's/^\(	*\)test -e /\1test_path_exists /' \
	    -e 's/^\(	*\)! test -[edf] /\1test_path_is_missing /' \
	    -e 's/^\(	*\)test ! -[edf] /\1test_path_is_missing /'

    and then compare the result with the result of applying this
    patch, you will see an instance of "! (test -e 3)", which was
    manually replaced with "test_path_is_missing 3", and everything
    else should match.

And I did perform the sed script, aka "how would I reproduce what is
in this patch" procedure, and compared the result with this patch.
The patch seems to be doing the right thing.

Manual validation is still needed for "test ! -f foo".  If the
original expects 'foo' to be gone, and has a reason to expect 'foo'
to be a file when the code being tested is broken, then rewriting it
into "test_path_is_missing" is OK.  But otherwise, the original
would have been happy when 'foo' existed as a directory and
rewriting it into "test_path_is_missing" is not quite right.

This check cannot be done mechanically, unfortunately.  Hits from

   $ git show | grep -e 'test ! -[df]'

need to be eyeballed to make sure that it is reasonable to rewrite
"test ! -f foo" into "test_path_is_missing foo".

For example:

>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean &&
> ...
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&

this test creates a.out and src/part3.c as regular files before
running "git clean", so the expected failure modes do not include
a.out to be a directory (which would also make "test ! -f a.out"
happy), and rewriting it to "test_path_is_missing a.out" is fine.

I did *not* go through all the instances of test_path_is_missing
in the postimage of this patch.  Instead of forcing reviewers to
do so on their own, mentioning that the author did that already
would probably help the process.

Thanks.

@devdekunle
Copy link
Author

devdekunle commented Oct 8, 2024 via email

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

On the Git mailing list, Samuel Abraham wrote (reply to this):

On Tue, Oct 8, 2024 at 2:48 AM Junio C Hamano <[email protected]> wrote:
>
> "Samuel Adekunle Abraham via GitGitGadget" <[email protected]>
> writes:
>
> > From: Abraham Samuel Adekunle <[email protected]>
> >
> > The test_path_* helper functions provide error messages which show the cause
> > of the test failures.
>
> > Hence they are used to replace every instance of
> > test - [def] uses in the script.
>
> It is unclear the use of present tense "they are used" describes the
> status quo, or the way you wish the test script were written.
>
> The usual way to compose a log message of this project is to
>
>  - Give an observation on how the current system work in the present
>    tense (so no need to say "Currently X is Y", just "X is Y"), 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 the codebase to "become like so".
>
> in this order.
>
> Also, for a patch like this one, which is rather large, repetitious,
> and tire reviewers to miss simple typos easily, giving a procedure
> to mechanically validate the patch would help.  Instead of having to
> match up "test -f" that was removed with "test_is_file" that was
> added, while ensuring the pathname given to them are the same, a
> reader can reason about what the mechanical rewrite is doing, and
> when the reader is convinced that the mechanical rewrite is doing
> the right thing, being able to mechanically compare the result of
> the procedure with the result of applying a patch is a really
> powerful thing.
>
> I probably would have written your two paragraphs more like the
> first two paragraphs below, followed by the validation procedure,
> like this:
>
>     This test script uses "test -[edf]", but when a test fails
>     because a file given to "test -f" does not exist, it fails
>     silently.
>
>     Use test_path_* helpers, which are designed to give better error
>     messages when their expectations are not met.
>
>     If you pass the current test script through
>
>         sed -e 's/^\(   *\)test -f /\1test_path_is_file /' \
>             -e 's/^\(   *\)test -d /\1test_path_is_dir /' \
>             -e 's/^\(   *\)test -e /\1test_path_exists /' \
>             -e 's/^\(   *\)! test -[edf] /\1test_path_is_missing /' \
>             -e 's/^\(   *\)test ! -[edf] /\1test_path_is_missing /'
>
>     and then compare the result with the result of applying this
>     patch, you will see an instance of "! (test -e 3)", which was
>     manually replaced with "test_path_is_missing 3", and everything
>     else should match.
>
> And I did perform the sed script, aka "how would I reproduce what is
> in this patch" procedure, and compared the result with this patch.
> The patch seems to be doing the right thing.
>
> Manual validation is still needed for "test ! -f foo".  If the
> original expects 'foo' to be gone, and has a reason to expect 'foo'
> to be a file when the code being tested is broken, then rewriting it
> into "test_path_is_missing" is OK.  But otherwise, the original
> would have been happy when 'foo' existed as a directory and
> rewriting it into "test_path_is_missing" is not quite right.
>
> This check cannot be done mechanically, unfortunately.  Hits from
>
>    $ git show | grep -e 'test ! -[df]'
>
> need to be eyeballed to make sure that it is reasonable to rewrite
> "test ! -f foo" into "test_path_is_missing foo".
>
> For example:
>
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean &&
> > ...
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
>
> this test creates a.out and src/part3.c as regular files before
> running "git clean", so the expected failure modes do not include
> a.out to be a directory (which would also make "test ! -f a.out"
> happy), and rewriting it to "test_path_is_missing a.out" is fine.
>
> I did *not* go through all the instances of test_path_is_missing
> in the postimage of this patch.  Instead of forcing reviewers to
> do so on their own, mentioning that the author did that already
> would probably help the process.
>
> Thanks.

Hi Junio,
Thank you very much for your time and very detailed review. I will
make corrections in my next patch.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

On the Git mailing list, Samuel Abraham wrote (reply to this):

On Tue, Oct 8, 2024 at 2:48 AM Junio C Hamano <[email protected]> wrote:
>
> "Samuel Adekunle Abraham via GitGitGadget" <[email protected]>
> writes:
>
> > From: Abraham Samuel Adekunle <[email protected]>
> >
> Manual validation is still needed for "test ! -f foo".  If the
> original expects 'foo' to be gone, and has a reason to expect 'foo'
> to be a file when the code being tested is broken, then rewriting it
> into "test_path_is_missing" is OK.  But otherwise, the original
> would have been happy when 'foo' existed as a directory and
> rewriting it into "test_path_is_missing" is not quite right.
>
> This check cannot be done mechanically, unfortunately.  Hits from
>
>    $ git show | grep -e 'test ! -[df]'
>
> need to be eyeballed to make sure that it is reasonable to rewrite
> "test ! -f foo" into "test_path_is_missing foo".
>
> For example:
>
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean &&
> > ...
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
>
> this test creates a.out and src/part3.c as regular files before
> running "git clean", so the expected failure modes do not include
> a.out to be a directory (which would also make "test ! -f a.out"
> happy), and rewriting it to "test_path_is_missing a.out" is fine.
>

Hi Junio,
Thanks again for your time and review.
I have gone through all the instances of "test ! - [df]" and for each
test case where "test ! -f foo" was used, foo was first created as a
regular file in the control flow before "git clean" was called and is
expected not to exist afterwards.
a few more examples are to the one you referenced above are shown below;

>         mkdir -p build docs src/test &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&
>         (cd src/ && git clean) &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&

and

>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -X -e src &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test -f a.out &&
> -       test ! -f src/part3.c &&
> -       test -f docs/manual.txt &&
> -       test ! -f obj.o &&

Also for the tests where "test ! -d foo" was used, the control flow
followed similar pattern as mentioned above where foo was created as a
directory and then "git clean -d" was called. The control flow
expected foo to be a directory which could have been removed
afterwards as can be seen below.

> @@ -300,15 +300,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' '
>         mkdir -p build docs &&
>         touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>         git clean -d -x -e src &&
> -       test -f Makefile &&
> -       test -f README &&
> -       test -f src/part1.c &&
> -       test -f src/part2.c &&
> -       test ! -f a.out &&
> -       test -f src/part3.c &&
> -       test ! -d docs &&
> -       test ! -f obj.o &&
> -       test ! -d build

and

>  test_expect_success 'should clean things that almost look like git but are not' '
> @@ -624,9 +624,9 @@ test_expect_success 'force removal of nested git work tree' '
>                 test_commit deeply.nested deeper.world
>         ) &&
>         git clean -f -f -d &&
> -       ! test -d foo &&
> -       ! test -d bar &&
> -       ! test -d baz

 This was the reason for replacing "test ! -[df]" with
"test_path_is_missing" where I think is appropriate. I will appreciate
it and be very grateful if test instances in this script where
"test_path_is_missing" is inappropriate to be used are pointed out by
other maintainers as well.
Thanks once again for your time.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

There are issues in commit 1d21b02:
t7300-clean.sh: use test_path_* helper functions for error logging
Commit not signed off
Lines in the body of the commit messages should be wrapped between 60 and 76 characters.
Indented lines, and lines without whitespace, are exempt

@devdekunle devdekunle changed the title [Outreachy] [PATCH] t7300-clean.sh: replace instances of test - [def] with test_path_* helper functions. [Outreachy][PATCH] t7300-clean.sh: use test_path_* helper functions for error logging Oct 8, 2024
@devdekunle
Copy link
Author

/submit

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1811/devdekunle/update_tests-v2

To fetch this version to local tag pr-1811/devdekunle/update_tests-v2:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1811/devdekunle/update_tests-v2

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

Samuel Abraham <[email protected]> writes:

> ...
>  This was the reason for replacing "test ! -[df]" with
> "test_path_is_missing" where I think is appropriate.

Telling that concisely in the proposed log message will help those
who are reviewing the patch and those who are reading "git log -p"
later, and that is what I would want to see after a review exchange
like this.

Thanks.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 8, 2024

This patch series was integrated into seen via git@5f752d4.

@gitgitgadget gitgitgadget bot added the seen label Oct 8, 2024
@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

On the Git mailing list, Samuel Abraham wrote (reply to this):

On Tue, Oct 8, 2024 at 7:13 PM Junio C Hamano <[email protected]> wrote:
>
> Samuel Abraham <[email protected]> writes:
>
> > ...
> >  This was the reason for replacing "test ! -[df]" with
> > "test_path_is_missing" where I think is appropriate.
>
> Telling that concisely in the proposed log message will help those
> who are reviewing the patch and those who are reading "git log -p"
> later, and that is what I would want to see after a review exchange
> like this.
>
> Thanks.
Hi, Junio
I want to express my gratitude to you and every member for your time,
 guidance and patience and to my Outreachy mentors Patrick and Phillip.
It has been a great learning experience.  I can see the patch has been
integrated into seen.
I look forward to working on #leftoverbits projects to enhance my understanding
of the git codebase. Thank you very much once again.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

On the Git mailing list, Patrick Steinhardt wrote (reply to this):

On Wed, Oct 09, 2024 at 04:35:04AM +0100, Samuel Abraham wrote:
> On Tue, Oct 8, 2024 at 7:13 PM Junio C Hamano <[email protected]> wrote:
> >
> > Samuel Abraham <[email protected]> writes:
> >
> > > ...
> > >  This was the reason for replacing "test ! -[df]" with
> > > "test_path_is_missing" where I think is appropriate.
> >
> > Telling that concisely in the proposed log message will help those
> > who are reviewing the patch and those who are reading "git log -p"
> > later, and that is what I would want to see after a review exchange
> > like this.
> >
> > Thanks.
> Hi, Junio
> I want to express my gratitude to you and every member for your time,
>  guidance and patience and to my Outreachy mentors Patrick and Phillip.
> It has been a great learning experience.  I can see the patch has been
> integrated into seen.
> I look forward to working on #leftoverbits projects to enhance my understanding
> of the git codebase. Thank you very much once again.

Note that a patch that has been merged into "seen" does not yet say that
it will be part of the next release. "seen" is only an integration
branch for topics which are currently in-flight on the mailing list and
in the process of being reviewed. The intent is that we can catch any
incompatibilities between two different in-flight patch series early.

So declaring victory is a bit too early :) A better indicator is that
the patch has been merged to "next". This is described in
Documentation/MyFirstContribution.txt, section "After Review Approval",
and more in-depth in Documentation/howto/maintain-git.txt.

I think that your v2 isn't quite there yet. As Junio mentions, he'd like
to see an updated commit message that includes your explanations why you
have done certain conversions the way you did. The fact that some parts
of the patch required discussion to arrive at a common understanding is
a good telling factor that a summarized form of the discussion should
likely be part of the commit message such that future readers of the
patch will get the same context.

Patrick

@devdekunle
Copy link
Author

/submit

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1811/devdekunle/update_tests-v3

To fetch this version to local tag pr-1811/devdekunle/update_tests-v3:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1811/devdekunle/update_tests-v3

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

Patrick Steinhardt <[email protected]> writes:

> I think that your v2 isn't quite there yet. As Junio mentions, he'd like
> to see an updated commit message that includes your explanations why you
> have done certain conversions the way you did. The fact that some parts
> of the patch required discussion to arrive at a common understanding is
> a good telling factor that a summarized form of the discussion should
> likely be part of the commit message such that future readers of the
> patch will get the same context.

What v2 had after the three-dash line seemed to me an OK material
for a proposed commit log message, but it should have been before
the line.  I suspect that it was from typical GGG gotcha?

Thanks.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

On the Git mailing list, Junio C Hamano wrote (reply to this):

"Samuel Adekunle Abraham via GitGitGadget" <[email protected]>
writes:

> From: Abraham Samuel Adekunle <[email protected]>
>
> This test script uses "test - [def]", but when a test fails because
> the file passed to it does not exist,
> it fails silently without an error message.
> Use test_path_* helper functions, which are designed to give better
> error messages when their expectations are not met.
>
> I have added a mechanical validation that applies the same transformation
> done in this patch, when the test script is passed to a sed script as shown
> below.
>
> sed -e 's/^\(	*\)test -f /\1test_path_is_file /' \
>     -e 's/^\(	*\)test -d /\1test_path_is_dir /' \
>     -e 's/^\(	*\)test -e /\1test_path_exists /' \
>     -e 's/^\(	*\)! test -[edf] /\1test_path_is_missing /' \
>     -e 's/^\(	*\)test ! -[edf] /\1test_path_is_missing /' \
>        "$1" >foo.sh
>
> Reviewers can use the sed script to tranform the original test script and
> compare the result in foo.sh with the results of applying the patch.
> You will see an instance of "!(test -e 3)" which was manually replaced with
> ""test_path_is_missing 3", and everything else should match.
>
> Careful and deliberate observation was done to check instances where
> "test ! - [df] foo" was used in the test script to make sure that the test
> instances were expecting foo to EITHER be a file or a directory, and NOT a
> possibility of being both as this would make replacing "test ! -f foo" with
> "test_path_is_missing foo" unreasonable.
> In the tests control flow, foo has been created as EITHER a
> reguar file OR a directory and should NOT exist
> after "git clean" or "git clean -d", as the case maybe, has been called.
> This made it reasonable to replace
> "test ! -[df] foo" with "test_path_is_missing foo".

This is a good place to stop (but perhaps have a paragraph break
before "In the tests control").  The "examples" you have below is
like telling readers to go read the patch and verify the correctness
of it themselves, which is not adding much value.

Other than that, looking very good.

Thanks.


>  t/t7300-clean.sh | 370 +++++++++++++++++++++++------------------------
>  1 file changed, 185 insertions(+), 185 deletions(-)
>
> diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> index 0aae0dee670..5c97eb0dfe9 100755
> --- a/t/t7300-clean.sh
> +++ b/t/t7300-clean.sh
> @@ -29,15 +29,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so &&
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so &&
>  	git update-index --no-skip-worktree .gitignore &&
>  	git checkout .gitignore
>  '
> @@ -47,15 +47,15 @@ test_expect_success 'git clean' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -64,15 +64,15 @@ test_expect_success 'git clean src/' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean src/ &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -81,15 +81,15 @@ test_expect_success 'git clean src/ src/' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean src/ src/ &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -98,16 +98,16 @@ test_expect_success 'git clean with prefix' '
>  	mkdir -p build docs src/test &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&
>  	(cd src/ && git clean) &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f src/test/1.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file src/test/1.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -163,16 +163,16 @@ test_expect_success 'git clean -d with prefix and path' '
>  	mkdir -p build docs src/feature &&
>  	touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so &&
>  	(cd src/ && git clean -d feature/) &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test -f src/part3.c &&
> -	test ! -f src/feature/file.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_missing src/feature/file.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -182,16 +182,16 @@ test_expect_success SYMLINKS 'git clean symbolic link' '
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	ln -s docs/manual.txt src/part4.c &&
>  	git clean &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test ! -f src/part4.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_missing src/part4.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -199,13 +199,13 @@ test_expect_success 'git clean with wildcard' '
>  
>  	touch a.clean b.clean other.c &&
>  	git clean "*.clean" &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.clean &&
> -	test ! -f b.clean &&
> -	test -f other.c
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.clean &&
> +	test_path_is_missing b.clean &&
> +	test_path_is_file other.c
>  
>  '
>  
> @@ -214,15 +214,15 @@ test_expect_success 'git clean -n' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -n &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -231,15 +231,15 @@ test_expect_success 'git clean -d' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -d &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test ! -d docs &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_missing docs &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -248,16 +248,16 @@ test_expect_success 'git clean -d src/ examples/' '
>  	mkdir -p build docs examples &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c &&
>  	git clean -d src/ examples/ &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test ! -f src/part3.c &&
> -	test ! -f examples/1.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_missing examples/1.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -266,15 +266,15 @@ test_expect_success 'git clean -x' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -x &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test ! -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -283,15 +283,15 @@ test_expect_success 'git clean -d -x' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -d -x &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test ! -d docs &&
> -	test ! -f obj.o &&
> -	test ! -d build
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_missing docs &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_missing build
>  
>  '
>  
> @@ -300,15 +300,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -d -x -e src &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test -f src/part3.c &&
> -	test ! -d docs &&
> -	test ! -f obj.o &&
> -	test ! -d build
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_missing docs &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_missing build
>  
>  '
>  
> @@ -317,15 +317,15 @@ test_expect_success 'git clean -X' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -X &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test ! -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -334,15 +334,15 @@ test_expect_success 'git clean -d -X' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -d -X &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test ! -f obj.o &&
> -	test ! -d build
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_missing build
>  
>  '
>  
> @@ -351,15 +351,15 @@ test_expect_success 'git clean -d -X with ignored tracked directory' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -d -X -e src &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test ! -f obj.o &&
> -	test ! -d build
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_missing obj.o &&
> +	test_path_is_missing build
>  
>  '
>  
> @@ -382,29 +382,29 @@ test_expect_success 'clean.requireForce and -n' '
>  	mkdir -p build docs &&
>  	touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
>  	git clean -n &&
> -	test -f Makefile &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test -f a.out &&
> -	test -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file Makefile &&
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_file a.out &&
> +	test_path_is_file src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
>  test_expect_success 'clean.requireForce and -f' '
>  
>  	git clean -f &&
> -	test -f README &&
> -	test -f src/part1.c &&
> -	test -f src/part2.c &&
> -	test ! -f a.out &&
> -	test ! -f src/part3.c &&
> -	test -f docs/manual.txt &&
> -	test -f obj.o &&
> -	test -f build/lib.so
> +	test_path_is_file README &&
> +	test_path_is_file src/part1.c &&
> +	test_path_is_file src/part2.c &&
> +	test_path_is_missing a.out &&
> +	test_path_is_missing src/part3.c &&
> +	test_path_is_file docs/manual.txt &&
> +	test_path_is_file obj.o &&
> +	test_path_is_file build/lib.so
>  
>  '
>  
> @@ -453,11 +453,11 @@ test_expect_success 'nested git work tree' '
>  		test_commit deeply.nested deeper.world
>  	) &&
>  	git clean -f -d &&
> -	test -f foo/.git/index &&
> -	test -f foo/hello.world &&
> -	test -f baz/boo/.git/index &&
> -	test -f baz/boo/deeper.world &&
> -	! test -d bar
> +	test_path_is_file foo/.git/index &&
> +	test_path_is_file foo/hello.world &&
> +	test_path_is_file baz/boo/.git/index &&
> +	test_path_is_file baz/boo/deeper.world &&
> +	test_path_is_missing bar
>  '
>  
>  test_expect_success 'should clean things that almost look like git but are not' '
> @@ -624,9 +624,9 @@ test_expect_success 'force removal of nested git work tree' '
>  		test_commit deeply.nested deeper.world
>  	) &&
>  	git clean -f -f -d &&
> -	! test -d foo &&
> -	! test -d bar &&
> -	! test -d baz
> +	test_path_is_missing foo &&
> +	test_path_is_missing bar &&
> +	test_path_is_missing baz
>  '
>  
>  test_expect_success 'git clean -e' '
> @@ -638,10 +638,10 @@ test_expect_success 'git clean -e' '
>  		touch known 1 2 3 &&
>  		git add known &&
>  		git clean -f -e 1 -e 2 &&
> -		test -e 1 &&
> -		test -e 2 &&
> -		! (test -e 3) &&
> -		test -e known
> +		test_path_exists 1 &&
> +		test_path_exists 2 &&
> +		test_path_is_missing 3 &&
> +		test_path_exists known
>  	)
>  '
>  
> @@ -649,7 +649,7 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '
>  	mkdir foo &&
>  	chmod a= foo &&
>  	git clean -dfx foo &&
> -	! test -d foo
> +	test_path_is_missing foo
>  '
>  
>  test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' '
>
> base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1

@devdekunle
t7300-clean.sh: use test_path_* helper functions for error logging
ad61cbe 
This test script uses "test - [def]", but when a test fails because
the file passed to it does not exist,
it fails silently without an error message.
Use test_path_* helper functions, which are designed to give better
error messages when their expectations are not met.

I have added a mechanical validation that applies the same transformation
done in this patch, when the test script is passed to a sed script as shown
below.

sed -e 's/^\(	*\)test -f /\1test_path_is_file /' \
    -e 's/^\(	*\)test -d /\1test_path_is_dir /' \
    -e 's/^\(	*\)test -e /\1test_path_exists /' \
    -e 's/^\(	*\)! test -[edf] /\1test_path_is_missing /' \
    -e 's/^\(	*\)test ! -[edf] /\1test_path_is_missing /' \
       "$1" >foo.sh

Reviewers can use the sed script to tranform the original test script and
compare the result in foo.sh with the results of applying the patch.
You will see an instance of "!(test -e 3)" which was manually replaced with
""test_path_is_missing 3", and everything else should match.

Careful and deliberate observation was done to check instances where
"test ! - [df] foo" was used in the test script to make sure that the test
instances were expecting foo to EITHER be a file or a directory, and NOT a
possibility of being both as this would make replacing "test ! -f foo" with
"test_path_is_missing foo" unreasonable.

In the tests control flow, foo has been created as EITHER a
reguar file OR a directory and should NOT exist
after "git clean" or "git clean -d", as the case maybe, has been called.
This made it reasonable to replace
"test ! -[df] foo" with "test_path_is_missing foo".

Signed-off-by: Abraham Samuel Adekunle <[email protected]>
@devdekunle
Copy link
Author

/submit

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

Submitted as [email protected]

To fetch this version into FETCH_HEAD:

git fetch https://github.com/gitgitgadget/git/ pr-1811/devdekunle/update_tests-v4

To fetch this version to local tag pr-1811/devdekunle/update_tests-v4:

git fetch --no-tags https://github.com/gitgitgadget/git/ tag pr-1811/devdekunle/update_tests-v4

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 9, 2024

On the Git mailing list, Samuel Abraham wrote (reply to this):

On Wed, Oct 9, 2024 at 6:47 PM Junio C Hamano <[email protected]> wrote:
>
> "Samuel Adekunle Abraham via GitGitGadget" <[email protected]>
> writes:
>
> > From: Abraham Samuel Adekunle <[email protected]>
> >
> > This test script uses "test - [def]", but when a test fails because
> > the file passed to it does not exist,
> > it fails silently without an error message.
> > Use test_path_* helper functions, which are designed to give better
> > error messages when their expectations are not met.
> >
> > I have added a mechanical validation that applies the same transformation
> > done in this patch, when the test script is passed to a sed script as shown
> > below.
> >
> > sed -e 's/^\( *\)test -f /\1test_path_is_file /' \
> >     -e 's/^\( *\)test -d /\1test_path_is_dir /' \
> >     -e 's/^\( *\)test -e /\1test_path_exists /' \
> >     -e 's/^\( *\)! test -[edf] /\1test_path_is_missing /' \
> >     -e 's/^\( *\)test ! -[edf] /\1test_path_is_missing /' \
> >        "$1" >foo.sh
> >
> > Reviewers can use the sed script to tranform the original test script and
> > compare the result in foo.sh with the results of applying the patch.
> > You will see an instance of "!(test -e 3)" which was manually replaced with
> > ""test_path_is_missing 3", and everything else should match.
> >
> > Careful and deliberate observation was done to check instances where
> > "test ! - [df] foo" was used in the test script to make sure that the test
> > instances were expecting foo to EITHER be a file or a directory, and NOT a
> > possibility of being both as this would make replacing "test ! -f foo" with
> > "test_path_is_missing foo" unreasonable.
> > In the tests control flow, foo has been created as EITHER a
> > reguar file OR a directory and should NOT exist
> > after "git clean" or "git clean -d", as the case maybe, has been called.
> > This made it reasonable to replace
> > "test ! -[df] foo" with "test_path_is_missing foo".
>
> This is a good place to stop (but perhaps have a paragraph break
> before "In the tests control").  The "examples" you have below is
> like telling readers to go read the patch and verify the correctness
> of it themselves, which is not adding much value.
>
> Other than that, looking very good.
>
> Thanks.

Thank you very much, I have submitted an updated patch.
>
> >  t/t7300-clean.sh | 370 +++++++++++++++++++++++------------------------
> >  1 file changed, 185 insertions(+), 185 deletions(-)
> >
> > diff --git a/t/t7300-clean.sh b/t/t7300-clean.sh
> > index 0aae0dee670..5c97eb0dfe9 100755
> > --- a/t/t7300-clean.sh
> > +++ b/t/t7300-clean.sh
> > @@ -29,15 +29,15 @@ test_expect_success 'git clean with skip-worktree .gitignore' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so &&
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so &&
> >       git update-index --no-skip-worktree .gitignore &&
> >       git checkout .gitignore
> >  '
> > @@ -47,15 +47,15 @@ test_expect_success 'git clean' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -64,15 +64,15 @@ test_expect_success 'git clean src/' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean src/ &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -81,15 +81,15 @@ test_expect_success 'git clean src/ src/' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean src/ src/ &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -98,16 +98,16 @@ test_expect_success 'git clean with prefix' '
> >       mkdir -p build docs src/test &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so src/test/1.c &&
> >       (cd src/ && git clean) &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f src/test/1.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file src/test/1.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -163,16 +163,16 @@ test_expect_success 'git clean -d with prefix and path' '
> >       mkdir -p build docs src/feature &&
> >       touch a.out src/part3.c src/feature/file.c docs/manual.txt obj.o build/lib.so &&
> >       (cd src/ && git clean -d feature/) &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test -f src/part3.c &&
> > -     test ! -f src/feature/file.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_missing src/feature/file.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -182,16 +182,16 @@ test_expect_success SYMLINKS 'git clean symbolic link' '
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       ln -s docs/manual.txt src/part4.c &&
> >       git clean &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test ! -f src/part4.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_missing src/part4.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -199,13 +199,13 @@ test_expect_success 'git clean with wildcard' '
> >
> >       touch a.clean b.clean other.c &&
> >       git clean "*.clean" &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.clean &&
> > -     test ! -f b.clean &&
> > -     test -f other.c
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.clean &&
> > +     test_path_is_missing b.clean &&
> > +     test_path_is_file other.c
> >
> >  '
> >
> > @@ -214,15 +214,15 @@ test_expect_success 'git clean -n' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -n &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -231,15 +231,15 @@ test_expect_success 'git clean -d' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -d &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test ! -d docs &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_missing docs &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -248,16 +248,16 @@ test_expect_success 'git clean -d src/ examples/' '
> >       mkdir -p build docs examples &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so examples/1.c &&
> >       git clean -d src/ examples/ &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test ! -f examples/1.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_missing examples/1.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -266,15 +266,15 @@ test_expect_success 'git clean -x' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -x &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test ! -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -283,15 +283,15 @@ test_expect_success 'git clean -d -x' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -d -x &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test ! -d docs &&
> > -     test ! -f obj.o &&
> > -     test ! -d build
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_missing docs &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_missing build
> >
> >  '
> >
> > @@ -300,15 +300,15 @@ test_expect_success 'git clean -d -x with ignored tracked directory' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -d -x -e src &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test -f src/part3.c &&
> > -     test ! -d docs &&
> > -     test ! -f obj.o &&
> > -     test ! -d build
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_missing docs &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_missing build
> >
> >  '
> >
> > @@ -317,15 +317,15 @@ test_expect_success 'git clean -X' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -X &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test ! -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -334,15 +334,15 @@ test_expect_success 'git clean -d -X' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -d -X &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test ! -f obj.o &&
> > -     test ! -d build
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_missing build
> >
> >  '
> >
> > @@ -351,15 +351,15 @@ test_expect_success 'git clean -d -X with ignored tracked directory' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -d -X -e src &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test ! -f obj.o &&
> > -     test ! -d build
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_missing obj.o &&
> > +     test_path_is_missing build
> >
> >  '
> >
> > @@ -382,29 +382,29 @@ test_expect_success 'clean.requireForce and -n' '
> >       mkdir -p build docs &&
> >       touch a.out src/part3.c docs/manual.txt obj.o build/lib.so &&
> >       git clean -n &&
> > -     test -f Makefile &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test -f a.out &&
> > -     test -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file Makefile &&
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_file a.out &&
> > +     test_path_is_file src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> >  test_expect_success 'clean.requireForce and -f' '
> >
> >       git clean -f &&
> > -     test -f README &&
> > -     test -f src/part1.c &&
> > -     test -f src/part2.c &&
> > -     test ! -f a.out &&
> > -     test ! -f src/part3.c &&
> > -     test -f docs/manual.txt &&
> > -     test -f obj.o &&
> > -     test -f build/lib.so
> > +     test_path_is_file README &&
> > +     test_path_is_file src/part1.c &&
> > +     test_path_is_file src/part2.c &&
> > +     test_path_is_missing a.out &&
> > +     test_path_is_missing src/part3.c &&
> > +     test_path_is_file docs/manual.txt &&
> > +     test_path_is_file obj.o &&
> > +     test_path_is_file build/lib.so
> >
> >  '
> >
> > @@ -453,11 +453,11 @@ test_expect_success 'nested git work tree' '
> >               test_commit deeply.nested deeper.world
> >       ) &&
> >       git clean -f -d &&
> > -     test -f foo/.git/index &&
> > -     test -f foo/hello.world &&
> > -     test -f baz/boo/.git/index &&
> > -     test -f baz/boo/deeper.world &&
> > -     ! test -d bar
> > +     test_path_is_file foo/.git/index &&
> > +     test_path_is_file foo/hello.world &&
> > +     test_path_is_file baz/boo/.git/index &&
> > +     test_path_is_file baz/boo/deeper.world &&
> > +     test_path_is_missing bar
> >  '
> >
> >  test_expect_success 'should clean things that almost look like git but are not' '
> > @@ -624,9 +624,9 @@ test_expect_success 'force removal of nested git work tree' '
> >               test_commit deeply.nested deeper.world
> >       ) &&
> >       git clean -f -f -d &&
> > -     ! test -d foo &&
> > -     ! test -d bar &&
> > -     ! test -d baz
> > +     test_path_is_missing foo &&
> > +     test_path_is_missing bar &&
> > +     test_path_is_missing baz
> >  '
> >
> >  test_expect_success 'git clean -e' '
> > @@ -638,10 +638,10 @@ test_expect_success 'git clean -e' '
> >               touch known 1 2 3 &&
> >               git add known &&
> >               git clean -f -e 1 -e 2 &&
> > -             test -e 1 &&
> > -             test -e 2 &&
> > -             ! (test -e 3) &&
> > -             test -e known
> > +             test_path_exists 1 &&
> > +             test_path_exists 2 &&
> > +             test_path_is_missing 3 &&
> > +             test_path_exists known
> >       )
> >  '
> >
> > @@ -649,7 +649,7 @@ test_expect_success SANITY 'git clean -d with an unreadable empty directory' '
> >       mkdir foo &&
> >       chmod a= foo &&
> >       git clean -dfx foo &&
> > -     ! test -d foo
> > +     test_path_is_missing foo
> >  '
> >
> >  test_expect_success 'git clean -d respects pathspecs (dir is prefix of pathspec)' '
> >
> > base-commit: 90fe3800b92a49173530828c0a17951abd30f0e1

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 10, 2024

This patch series was integrated into seen via git@b83aca1.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 10, 2024

This branch is now known as aa/t7300-modernize.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 10, 2024

This patch series was integrated into seen via git@550c6df.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 10, 2024

There was a status update in the "New Topics" section about the branch aa/t7300-modernize on the Git mailing list:

Test modernization.

Will merge to 'next'.
source: <[email protected]>

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 11, 2024

This patch series was integrated into seen via git@44c4bdb.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 11, 2024

This patch series was integrated into next via git@e3a8d7f.

@gitgitgadget gitgitgadget bot added the next label Oct 11, 2024
@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 12, 2024

There was a status update in the "Cooking" section about the branch aa/t7300-modernize on the Git mailing list:

Test modernization.

Will merge to 'master'.
source: <[email protected]>

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 14, 2024

This patch series was integrated into seen via git@2cf6352.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 14, 2024

This patch series was integrated into seen via git@1068966.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 16, 2024

This patch series was integrated into seen via git@86bbd15.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 18, 2024

This patch series was integrated into seen via git@020c16b.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 18, 2024

This patch series was integrated into master via git@020c16b.

@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 18, 2024

This patch series was integrated into next via git@020c16b.

@gitgitgadget gitgitgadget bot added the master label Oct 18, 2024
@gitgitgadget GitGitGadget
Copy link

gitgitgadget bot commented Oct 18, 2024

Closed via 020c16b.

@gitgitgadget gitgitgadget bot closed this Oct 18, 2024
@devdekunle
Copy link
Author

devdekunle commented Oct 18, 2024 •
edited
Loading

hello @dscho , Please i do not really understand the events happening.
But I can see the patch was first integrated into seen then to master and back to seen then it was closed.But I can see the master tag on it.
I do not really understand if it has been integrated into master or not.
Please can help me understand.
Thanks

Closed via 020c16b.

@dscho
Copy link
Member

dscho commented Oct 20, 2024

I can see the patch was first integrated into seen then to master and back to seen then it was closed.But I can see the master tag on it. I do not really understand if it has been integrated into master or not.

@devdekunle as per this comment, it has been integrated into the master branch.

Congratulations! You did it! This was your first contribution, right?

@devdekunle
Copy link
Author

I can see the patch was first integrated into seen then to master and back to seen then it was closed.But I can see the master tag on it. I do not really understand if it has been integrated into master or not.

@devdekunle as per this comment, it has been integrated into the master branch.

Congratulations! You did it! This was your first contribution, right?

Yessssss 🎉🎉.
It was my first contribution.
I'm so elated. I always check my mails hoping it'll get integrated.
I'm happy it has.

I have another open PR to implement an option for git notes add and append.

It's interesting working with the Git community.
They're friendly, accommodating and explain concepts very easily.

Thanks for your response.
I appreciate

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Reviewers

No reviews

Assignees

No one assigned

Labels

master new user next seen

Projects

None yet

Milestone

No milestone

Development

Successfully merging this pull request may close these issues.

2 participants

@devdekunle @dscho