Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 27, 2020, 00:14 UTC
Message-ID: <20201027001427.GG2645313@google.com>
In-Reply-To: <XDwhHkxbkjL46BTp1WCJ5gJ3UbrMxRGwR2VgeRxehZh0-G07JlxCHrBGCpruqk-OegVspO6LJ0Y3yW9izl97pufYDwOhGaS885_xaVK63vs=@protonmail.com>

Joey Salazar wrote:

> On Monday, October 26, 2020 5:00 PM, Joey Salazar <jgsal@protonmail.com> wrote:

Show 14 quoted lines

>> Modernize the test by replacing `test -e` instances with
>> `test_path_is_file` helper functions, and `! test -e` with
>> `test_path_is_missing`, for better readability and diagnostic messages.
>> For instances when `${if_local_config}` is either '' or '! ' then
>> `test_path_is_file` will diagnose the directory and print a message if
>> and only if the result `is false` goes away.
>
> If `if test -n '$pager_wanted'` is checking if pager_wanted=true
> before diagnosing core.pager_used, then would;
>
> For other instances when '$pager_wanted' is not empty then `test_path_is_file`
> will diagnose the directory and print a message.
>
> be more accurate?

Yes, but because it restates what the patch says instead of describing the "why", it's at the wrong level of abstraction.

Starting from

    t7006: Use test_path_is_* functions in test script

    Modernize the test by replacing `test -e` instances with
    `test_path_is_file` helper functions, and `! test -e` with
    `test_path_is_missing`, for better readability and diagnostic messages.

I think what would make sense is to add a second paragraph describing why the existing code uses ${if_local_config} and why what the new code is doing is better.

Thanks, Jonathan