reconsider FAIL INVALID_TARGET if you're not joined to the channel

[slingamn]

Gamja 43f1329fb07 against Ergo 2.8.0 on the testnet just now:

Dec 15 04:07:05 oragono-testnet ergo[623037]: 2021-12-15T04:07:05.063Z : debug : userinput  : slingamn : <-  : PART #asdf
Dec 15 04:07:05 oragono-testnet ergo[623037]: 2021-12-15T04:07:05.063Z : debug : useroutput : slingamn :  -> : @msgid=qu2jpejsyjxeyc8pf4jrrzdy8w;time=2021-12-15T04:07:05.063Z :slingamn!~u@vhost.example PART #asdf
Dec 15 04:07:05 oragono-testnet ergo[623037]: 2021-12-15T04:07:05.155Z : debug : userinput  : slingamn : <-  : @label=13 CHATHISTORY BEFORE #asdf timestamp=2021-12-15T04:07:05.063Z 0
Dec 15 04:07:05 oragono-testnet ergo[623037]: 2021-12-15T04:07:05.155Z : debug : useroutput : slingamn :  -> : @time=2021-12-15T04:07:05.155Z;label=13 :testnet.ergo.chat FAIL CHATHISTORY INVALID_TARGET BEFORE #asdf :Messages could not be retrieved

This displays a user-visible error message. Unclear what we want to do here, but probably Ergo should be returning an empty batch instead of a FAIL?

[slingamn]

There is currently an irctest asserting the old behavior:

https://github.com/progval/irctest/blob/627f0b6415cd34c4e123acffe7490632e5ae4bf5/irctest/server_tests/chathistory.py#L99-L106

I don't want to revisit this in the current development cycle.

[slingamn]

I don't remember all the back-and-forth on this issue, but the current behavior is in line with the current specification:

If the target does not exist or the client does not have permissions to query it, the INVALID_TARGET error code SHOULD be returned

So I think this is probably a wontfix? @emersion, any thoughts?

[emersion]

I don't remember the scenario in which this happens sadly.

From a client dev PoV: I can't guess when it's inappropriate to load history since this depends on server-specific rules. I don't want to silence errors since they could be due to a client bug (which I want users to open bug reports about) or real server error (which users want to know about).

tl;dr If you want my client to display an error to the user, send FAIL. If you want my client to no show any history, send an empty batch.

[slingamn]

That makes sense, but the current spec language implies that you should treat INVALID_TARGET in particular as meaning that the server rejected the history request (the equivalent of either an HTTP 403 or 404, essentially). It seems to me that as specified, INVALID_TARGET should not become a user-visible error. But we could also just amend the spec.

[slingamn]

For what it's worth, the original reproduction case was from /part in Gamja, but I can no longer reproduce this against Gamja 3f059567c5. So I'm mostly interested in resolving the theoretical question here, since it might be a spec bug.

[emersion]

I am not sure. My main concern is that I want to properly bubble back errors to the user when it makes sense to, and not receive bug reports about broken chat history from confused users when displaying an error would disambiguate the situation.

[slingamn]

That makes sense. Since there is no clear evidence of a current compatibility problem, I'm going to keep the status quo behavior for the next Ergo release, then revisit the question in 2023.