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

From: Joey Salazar <jgsal@protonmail.com>
Date: Oct 27, 2020, 23:45 UTC
Message-ID: <bixxjcjnQYoZ6CIUQUXdBTaGE7vawm33kqmy3csw7hsPqGQpQNyEhUqBYkvoDgHb8sb3vjLr1KOR3I71-4a4wDKQqTrFocv-eVtGlyE8S84=@protonmail.com>
In-Reply-To: <20201027001427.GG2645313@google.com>

Jonathan Nieder wrote:

Show 11 quoted lines

> > 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.
> 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.

I see, thank you. I'm now thinking of a paragraph like this (thank you Emily Shaffer for your guidance in the IRC channel); Messages from checks to `${if_local_config}` are also printed when the result is false, which can be confusing. Improve readability by removing `${if_local_config}` checks and print messages only when a pager is wanted.

> Thanks,
> Jonathan

Thank you for your patient input and feedback.