Re: [GSoC][PATCH] t2000: modernize path checks to use helper functions
- From
Pablo <pabloosabaterr@gmail.com>
- Date
- Mar 26, 2026, 12:19 UTC
- Message-ID
- <CAN5EUNTSO7KvtO02c-EHJTK95rmcZKRBtKsn8kjNid1qupWZ0w@mail.gmail.com>
- In-Reply-To
- <20260326112149.53050-1-zakariyahali100@gmail.com>
Zakariyah Ali (<zakariyahali100@gmail.com>) escribió:
This should be sent as v2 in reply to your previous v1, not a new thread. read Documentation/SubmittingPatches and Documentation/MyFirstContribution as I mentioned in your v1.
Show 16 quoted lines
> > Replace bare 'test -f/-d' and 'test ! -h ... && test -f/-d' > assertions with their dedicated test_path_is_* helpers. > > These helpers are better than the previous 'test' commands > because they produce clearer diagnostic output on failure. > For example, instead of a bare exit code, the helper outputs > 'File path0 doesn't exist', which makes test failures > significantly easier to debug. > > The replacements are: > - 'test -f' -> 'test_path_is_file' > - 'test -d' -> 'test_path_is_dir' > - 'test ! -h && test -f' -> 'test_path_is_file_not_symlink' > - 'test ! -h && test -d' -> 'test_path_is_dir_not_symlink' >
The commit message is better now, but it's very verbose. The list about what helpers have you changed can be looked at by any reviewer with git log -p and the example only describes one helper, you should be able to make it more general with something like: reports loudly what expectation wasn't met, therefore making debugging easier.
Show 38 quoted lines
> Signed-off-by: Zakariyah Ali <zakariyahali100@gmail.com> > --- > t/t2000-conflict-when-checking-files-out.sh | 12 +++++++----- > 1 file changed, 7 insertions(+), 5 deletions(-) > > diff --git a/t/t2000-conflict-when-checking-files-out.sh b/t/t2000-conflict-when-checking-files-out.sh > index f18616ad2b..96bae6c53d 100755 > --- a/t/t2000-conflict-when-checking-files-out.sh > +++ b/t/t2000-conflict-when-checking-files-out.sh > @@ -58,7 +58,9 @@ test_expect_success \ > > test_expect_success \ > 'git checkout-index conflicting paths.' \ > - 'test -f path0 && test -d path1 && test -f path1/file1' > + 'test_path_is_file path0 && > + test_path_is_dir path1 && > + test_path_is_file path1/file1' > > test_expect_success SYMLINKS 'checkout-index -f twice with --prefix' ' > mkdir -p tar/get && > @@ -127,9 +129,9 @@ test_debug 'show_files $tree2' > > test_expect_success \ > 'checking out conflicting path with -f' \ > - 'test ! -h path2 && test -d path2 && > - test ! -h path3 && test -d path3 && > - test ! -h path2/file0 && test -f path2/file0 && > - test ! -h path3/file1 && test -f path3/file1' > + 'test_path_is_dir_not_symlink path2 && > + test_path_is_dir_not_symlink path3 && > + test_path_is_file_not_symlink path2/file0 && > + test_path_is_file_not_symlink path3/file1' > > test_done > -- > 2.43.0 > >
Code looks OK.