Re: Do test-path_is_{file,dir,exists} make sense anymore with -x?

From: Jeff King <peff@peff.net>
Date: Feb 26, 2019, 21:01 UTC
Message-ID: <20190226210101.GA27914@sigill.intra.peff.net>
In-Reply-To: <20190226193912.GD19739@szeder.dev>

On Tue, Feb 26, 2019 at 08:39:12PM +0100, SZEDER Gábor wrote:

Show 12 quoted lines

> > > 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).

Show 10 quoted lines

> 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