From: Jonathan Nieder <jrnieder@gmail.com>
To: Joey Salazar <jgsal@protonmail.com>
Cc: Junio C Hamano <gitster@pobox.com>,
"git@vger.kernel.org" <git@vger.kernel.org>
Subject: Re: [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script
Date: Tue, 27 Oct 2020 17:19:05 -0700 [thread overview]
Message-ID: <20201028001905.GA1500644@google.com> (raw)
In-Reply-To: <bixxjcjnQYoZ6CIUQUXdBTaGE7vawm33kqmy3csw7hsPqGQpQNyEhUqBYkvoDgHb8sb3vjLr1KOR3I71-4a4wDKQqTrFocv-eVtGlyE8S84=@protonmail.com>
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
next prev parent reply other threads:[~2020-10-28 23:54 UTC|newest]
Thread overview: 16+ messages / expand[flat|nested] mbox.gz Atom feed top
2020-10-20 19:53 [OUTREACHY][PATCH v2] t7006: Use test_path_is_* functions in test script Joey S
2020-10-21 13:25 ` Phillip Wood
2020-10-21 18:42 ` Junio C Hamano
2020-10-21 21:42 ` Joey Salazar
2020-10-21 21:57 ` Junio C Hamano
2020-10-21 22:21 ` Joey Salazar
2020-10-26 20:50 ` Jonathan Nieder
2020-10-26 21:24 ` Junio C Hamano
2020-10-26 21:46 ` Joey Salazar
2020-10-26 22:02 ` Jonathan Nieder
2020-10-26 23:00 ` Joey Salazar
2020-10-27 0:04 ` Joey Salazar
2020-10-27 0:14 ` Jonathan Nieder
2020-10-27 23:45 ` Joey Salazar
2020-10-28 0:19 ` Jonathan Nieder [this message]
2020-10-28 2:45 ` Joey Salazar
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
List information: http://vger.kernel.org/majordomo-info.html
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=20201028001905.GA1500644@google.com \
--to=jrnieder@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=jgsal@protonmail.com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
Code repositories for project(s) associated with this public inbox
https://80x24.org/mirrors/git.git
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for read-only IMAP folder(s) and NNTP newsgroup(s).