Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
- From
- Joey Salazar <jgsal@protonmail.com>
- Date
- Oct 28, 2020, 02:45 UTC
- Message-ID
- <4q38uC7_84GKAeX0QAsYSjHB0zcWtp3othTmTuALUWus0ENKxQJMaMxf2jbtz9CpqXKgjbybPelAJ1z8PVTZXAcpvKsLroqbksLvLqbTGbw=@protonmail.com>
- In-Reply-To
- <20201028001905.GA1500644@google.com>
Jonathan Nieder wrote:
Show 20 quoted lines
> 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.
Show 5 quoted lines
> 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