Make check-test-install-error-stop.sh order-sensitive for ON_ERROR_STOP - #122
jnasbyupgrade wants to merge 1 commit into
Conversation
The bare substring grep for ON_ERROR_STOP passed any file mentioning the variable at all, including one that only ever turns it off. Read \set ON_ERROR_STOP statements (and psql.sql inclusion) in file order and require an explicit on-value to have been seen; a later explicit off no longer masks the absence of an on. Addresses maintainer review feedback on Postgres-Extensions#109: Postgres-Extensions#109 (comment) Co-Authored-By: Claude Sonnet 5 <noreply@anthropic.com>
|
Important Review skippedAuto reviews are disabled on this repository. Please check the settings in the CodeRabbit UI or the ⚙️ Run configurationConfiguration used: Organization UI Review profile: ASSERTIVE Plan: Advanced Run ID: You can disable this status message by setting the Use the checkbox below for a quick retry:
Thanks for using CodeRabbit! It's free for OSS, and your support helps us grow. If you like it, consider giving us a shout-out. Comment |
| # A file passes if it explicitly turns ON_ERROR_STOP on at some point -- | ||
| # directly (`\set ON_ERROR_STOP on`/true/1/yes) or by sourcing | ||
| # test/pgxntool/psql.sql (which already turns it on, among other things). A | ||
| # later explicit turn-off doesn't undo an earlier turn-on. A file that only |
There was a problem hiding this comment.
This is confusingly worded. The real idea here is: If and only if a user explicitly enables ON_ERROR_STOP directly (not just via psql.sql), then it's OK if at some point they also disable it. The assumption here is that if a user has explicitly enabled ON_ERROR_STOP then they know what they're doing if they also disable it at some point. We don't treat including psql.sql the same way, because there's no reason to think the user knows psql.sql enables ON_ERROR_STOP.
| # turned on. Succeeds (exit 0) once an on-value is seen; a later off-value | ||
| # doesn't reset that. Sourcing psql.sql counts as turning it on, wherever it | ||
| # occurs in the file. | ||
| file_turns_error_stop_on() { |
There was a problem hiding this comment.
Yeah, see comment above. This code isn't doing what we want.
1 participant
Summary
Follow-up to #109's review:
check-test-install-error-stop.shdid a baregrep -q 'ON_ERROR_STOP'substring match, which passes any file thatmentions the variable at all — including one that only ever turns it
off. Per maintainer feedback
(#109 (comment)),
the check now reads each file's
\set ON_ERROR_STOPstatements (and\i/\ir ... psql.sqlinclusion) in file order and requires an expliciton-value to have been seen somewhere in the file. A later explicit
turn-off no longer masks the absence of an earlier turn-on.
Paired with Postgres-Extensions/pgxntool-test#86, which adds the test
coverage for this.
Test plan
test/install/directory before pushing.🤖 Generated with Claude Code