From: Jeff King <peff@peff.net>
To: "SZEDER Gábor" <szeder.dev@gmail.com>
Cc: "Ævar Arnfjörð Bjarmason" <avarab@gmail.com>,
"Duy Nguyen" <pclouds@gmail.com>,
"Rohit Ashiwal via GitGitGadget" <gitgitgadget@gmail.com>,
"Git Mailing List" <git@vger.kernel.org>,
"Junio C Hamano" <gitster@pobox.com>,
"Rohit Ashiwal" <rohit.ashiwal265@gmail.com>,
"Matthieu Moy" <git@matthieu-moy.fr>
Subject: Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?
Date: Tue, 26 Feb 2019 16:01:01 -0500 [thread overview]
Message-ID: <20190226210101.GA27914@sigill.intra.peff.net> (raw)
In-Reply-To: <20190226193912.GD19739@szeder.dev>
On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:
> > > I didn't find this to be an issue, but because of functions like
> > > 'test_seq' and 'test_must_fail' I've thought about suppressing '-x'
> > > output for test helpers (haven't actually done anything about it,
> > > though).
> >
> > I'd be curious how you'd do that.
>
> Well, I started replying with "Dunno" and explaining why I don't think
> that it can be done with 'test_must_fail'... but then got a bit of a
> lightbulb moment. Now look at this:
> [...]
> + { set +x ; } 2>/dev/null 4>/dev/null
Ah, this is the magic. Doing:
set +x 2>/dev/null
will still show it, but doing the redirection in a wrapping block means
that it is applied before the command inside the block is run. Clever.
I think this braces trick could be used in general to fix all of the
remaining "you can't run this under -x" cases, though it might be ugly.
It might also be possible to make test_eval_ a bit less subtle with it,
though I think it is relying on the braces already (which makes me
wonder if I just totally forgot about its existence today, or if I
earlier somehow stumbled onto a working recipe because I wanted to run
multiple redirected commands).
> There are a couple of tricky cases:
>
> - Some test helper functions call other test helper functions, and
> in those cases tracing would be enabled upon returning from the
> inner helper function. This is not an issue with e.g.
> 'test_might_fail' or 'test_cmp_config', because the inner helper
> function is the last command anyway. However, there is
> 'test_must_be_empty', 'test_dir_is_empty', 'test_config',
> 'test_commit', etc. which call the other test helper functions
> right at the start or in the middle.
Yeah, this is inherently a global flag that we're playing games with. It
does seem like it would be easy to get it wrong. I guess the right model
is considering it like a stack, like:
-- >8 --
#!/bin/sh
x_counter=0
pop_x() {
ret=$?
case "$x_counter" in
0)
echo >&2 "BUG: too many pops"
exit 1
;;
1)
x_counter=0
set -x
;;
*)
x_counter=$((x_counter - 1))
;;
esac
{ return $ret; } 2>/dev/null
}
# you _must_ call this as "{ push_x; } 2>/dev/null" to avoid polluting
# trace output with the push call
push_x() {
set +x 2>/dev/null
x_counter=$((x_counter + 1))
}
bar() {
{ push_x; } 2>/dev/null
echo in bar
pop_x
}
foo() {
{ push_x; } 2>/dev/null
echo in foo, before bar
bar
echo in foo, after bar
false
pop_x
}
set -x
foo
echo \$? is $?
-- 8< --
I wish there was a way to avoid having to do the block-and-redirect in
the push_x calls in each function, though.
I dunno. I do like the output, but this is rapidly getting complex.
> - && chains in test helper functions; we must make sure that the
> tracing is restored even in case of a failure.
Yeah, there is no "goto out" to help give a common exit point from the
function. You could probably do it with a wrapper, like:
foo() {
{ push_x; } 2>/dev/null
real_foo "$@"
pop_x
}
and then real_foo() is free to return however it likes. I wonder if you
could even wrap that up in a helper:
disable_function_tracing () {
# rename foo() to orig_foo(); this works in bash, but I'm not
# sure if there's a portable way to do it (and ideally one that
# wouldn't involve an extra process).
eval "real_$1 () $(declare -f $1 | tail -n +2)"
# and then install a wrapper which pushes/pops tracing
eval "$1 () { { push_x; } 2>/dev/null; real_$1 \"\$@\"; pop_x; }"
}
foo () { .... }
disable_function_tracing foo
It would be easier if you could just declare the function body as an
argument (and then it would be "declare_untraceable_function", where you
do it all in one step). But then the function body has to be in single
quotes, which is a pain. I think this is definitely pushing the limits
of portable shell (and quite possibly the limits of good taste).
-Peff
next prev parent reply other threads:[~2019-02-26 21:01 UTC|newest]
Thread overview: 42+ messages / expand[flat|nested] mbox.gz Atom feed top
2019-02-26 13:42 [PATCH 0/1] [GSoC][PATCH] tests: replace test -(d|f) with test_path_is_(dir|file) Rohit Ashiwal via GitGitGadget
2019-02-26 13:42 ` [PATCH 1/1] tests: replace `test -(d|f)` " Rohit Ashiwal via GitGitGadget
2019-02-26 14:04 ` Duy Nguyen
2019-02-26 16:10 ` Do test-path_is_{file,dir,exists} make sense anymore with -x? Ævar Arnfjörð Bjarmason
2019-02-26 17:04 ` SZEDER Gábor
2019-02-26 17:43 ` Jeff King
2019-02-26 19:39 ` SZEDER Gábor
2019-02-26 21:01 ` Jeff King [this message]
2019-03-03 16:04 ` SZEDER Gábor
2019-03-05 4:55 ` Jeff King
2019-03-04 14:36 ` SZEDER Gábor
2019-03-05 4:58 ` Jeff King
2019-02-26 17:48 ` Matthieu Moy
2019-02-26 18:24 ` Jeff King
2019-02-26 17:35 ` Jeff King
2019-02-26 19:58 ` Johannes Schindelin
2019-02-26 21:02 ` Jeff King
2019-02-27 10:01 ` Duy Nguyen
2019-03-01 2:52 ` Junio C Hamano
2019-02-26 16:01 ` [PATCH 1/1] tests: replace `test -(d|f)` with test_path_is_(dir|file) Johannes Schindelin
2019-02-26 16:30 ` Martin Ågren
2019-02-26 18:29 ` Rohit Ashiwal
2019-02-26 19:52 ` Johannes Schindelin
2019-02-26 20:01 ` Rohit Ashiwal
2019-02-27 5:49 ` Martin Ågren
2019-02-26 14:26 ` [PATCH v2 0/1] [GSoC][PATCH] t3600: use test_path_is_dir and test_path_is_file Rohit Ashiwal via GitGitGadget
2019-02-26 14:26 ` [PATCH v2 1/1] " Rohit Ashiwal via GitGitGadget
2019-02-26 16:37 ` SZEDER Gábor
2019-02-26 18:40 ` Rohit Ashiwal
2019-02-26 20:02 ` Johannes Schindelin
2019-02-26 20:05 ` Rohit Ashiwal
2019-02-26 22:48 ` [PATCH v3 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget
2019-02-26 22:48 ` [PATCH v3 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget
2019-02-27 10:12 ` Duy Nguyen
2019-02-28 10:26 ` [PATCH v4 0/1] [GSoC][PATCH] t3600: use test_path_is_* helper functions Rohit Ashiwal via GitGitGadget
2019-02-28 10:26 ` [PATCH v4 1/1] t3600: use test_path_is_* functions Rohit Ashiwal via GitGitGadget
2019-02-28 19:02 ` [GSoC] acknowledging mistakes Rohit Ashiwal
2019-03-01 2:51 ` Junio C Hamano
2019-03-01 13:13 ` Feeling confused a little bit Rohit Ashiwal
2019-03-02 4:24 ` Rafael Ascensão
2019-03-02 14:46 ` Thomas Gummerer
2019-03-02 16:21 ` [GSoC] Thanking Rohit Ashiwal
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=20190226210101.GA27914@sigill.intra.peff.net \
--to=peff@peff.net \
--cc=avarab@gmail.com \
--cc=git@matthieu-moy.fr \
--cc=git@vger.kernel.org \
--cc=gitgitgadget@gmail.com \
--cc=gitster@pobox.com \
--cc=pclouds@gmail.com \
--cc=rohit.ashiwal265@gmail.com \
--cc=szeder.dev@gmail.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).