On Tue, Oct 8, 2024 at 2:48 AM Junio C Hamano <gitster@xxxxxxxxx> wrote: > > "Samuel Adekunle Abraham via GitGitGadget" <gitgitgadget@xxxxxxxxx> > writes: > > > From: Abraham Samuel Adekunle <abrahamadekunle50@xxxxxxxxx> > > > > 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.
