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

From: Jonathan Nieder <jrnieder@gmail.com>
Date: Oct 28, 2020, 00:19 UTC
Message-ID: <20201028001905.GA1500644@google.com>
In-Reply-To: <bixxjcjnQYoZ6CIUQUXdBTaGE7vawm33kqmy3csw7hsPqGQpQNyEhUqBYkvoDgHb8sb3vjLr1KOR3I71-4a4wDKQqTrFocv-eVtGlyE8S84=@protonmail.com>

Joey Salazar wrote:

Show 7 quoted lines

> 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