Joey Salazar wrote:
> 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.
I think that's on the right track, though I'm having trouble
understanding the details of it. If this goes as part of the same
patch, we'd want to focus on why we're making this change in the
context of this commit. Using an example can make the idea clearer:
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.
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.
Thanks,
Jonathan
