On Tue, Sep 19, 2023 at 01:23:54PM +0300, Aleksander Alekseev wrote:
> You are right, I missed it. Your patch is correct while the original
> one is not quite so.
Actually there was a bit more to it in the presence of \e, that could
also get some unpredictible behaviors if some errors happen while
editing a query, which is something unlikely, still leads to strange
behaviors on failure injections. I was considering first to move the
reset in do_edit(), but also we have the case of \e[v|f] where the
buffer has no edits so it felt a bit more natural to do that in the
upper layer like in this patch.
Another aspect of all these code paths is the line number that can be
optionally number after an object name for \e[v|f] or a file name for
\e (in the latter case it is possible to have a line number without a
file name, as well). Anyway, we only fill the query buffer after
validating all the options at hand. So, while the status is set to
PSQL_CMD_ERROR, we'd still do a reset of the query buffer but nothing
got added to it yet.
I've also considered a backpatch for this change, but at the end
discarded this option, at least for now. I don't think that someone
is relying on the existing behavior of having the query buffer still
around on failure if \ev or \ef fail their object lookup as the
contents are undefined, because that's not unintuitive, but this
change is not critical enough to make it backpatchable if somebody's
been actually relying on the previous behavior. I'm OK to revisit
this choice later on depending on the feedback, though.
--
Michael
