Jonathan Nieder wrote:
> A subtlety: one advantage of helpers such as test_path_is_missing is
> that they print a diagnostic if and only if the condition fails, which
> can make the output from a failing test easier to read. Unfortunately,
> some helpers in this test communicate whether a configured pager is
> expected to run using a shell constract that doesn't have that property:
>
> my_generic_helper () {
> ...
> ${if_local_config}test -e core.pager_used
> }
>
> ...
> if_local_config='! '
> my_generic_helper
>
> Rewriting this to "${if_local_config}test_path_is_file core.pager_used"
> would print a diagnostic when the file is absent, which is the opposite
> of what we want. Make the logic more explicit instead, using "test" to
> check a variable core_pager_wanted that is nonempty when core.pager is
> expected to be used.
Thank you for this explanation, much appreciated.
> That said, it looks like js/t7006-cleanup is in "next", indicating
> that it has finished being reviewed and is now safe to build on (see
> https://git-scm.com/docs/SubmittingPatches for more on this subject),
> so it would be even better to make this a patch on top of the existing
> v2 patch after all.
A proposed commit msg for patch v1 in the branch js/t7006-cleanup that explains "what was wrong in the previous patches and how the problem was corrected" would be;
The previous patch introduces `test_path_is_file` and helper functions
for instances that include checking if a configured pager is expected
to run using a shell construct. This prints a diagnostic message even
when the file is absent, as opposed to only when the file exists and
has been checked.
Use "test" to first check if a pager is wanted when `core.pager` is
expected to be used, then diagnose `core.pager_used` and print a
diagnostic message as appropriate.
Should I include an explicit mention to the removal of `${if_local_config}`?
Should I start a new email thread with "[PATCH v1]" on further discussion?
Thank you,
Joey
