Re: [PATCH] t7300-clean.sh: use test_path_* helper functions

[Junio C Hamano <gitster@pobox.com>]

"Samuel Adekunle Abraham via GitGitGadget" <gitgitgadget@gmail.com>
writes:

> From: Abraham Samuel Adekunle <abrahamadekunle50@gmail.com>
>
> 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.