On Sat, Feb 27, 2021, at 11:04 AM, Adnan Maolood wrote:
> I've added the ServerName helper method to Request to make it easier to
> access the TLS server name.

That seems like a good compromise. Thanks!

> I had considered setting Request.Host before, but I was not sure whether
> the overhead of tls.Conn.ConnectionState could affect the performance.
> Request used to have a TLS field like in net/http, but it was moved to a
> method so that it would not be allocated if it was not needed.
> 
> Currently those who need to access SNI information can use
> TLS().ServerName. For the majority of use cases I don't think that
> accessing the ServerName is necessary, as the certificate.Store struct
> handles automatic creation of certificates and will not create a
> certificate for a host that has not been registered. Do you have an
> example where this could be necessary?

The reason for adding this isn't for limiting what hosts that can be connected to,
but routing to different locations for different hosts on in a single service. You are

---
Setting Request.Host will allow Handlers to properly handle different virtual
hosts in the same process.

I actually wanted to clean up server.go a bit, but I'll leave that for another
patch. I think it's cleaner to use a plain net.Listener and create a tls.Conn
farther down the line, because that makes it easier to pass the tls.Conn through
the call chain. Unfortunately, there's an exposed method, ServeConn, which would
mean breaking backwards compatibility to do this so I left it alone for now.

 request.go | 4 +++-
 server.go  | 4 ++++
 2 files changed, 7 insertions(+), 1 deletion(-)
[message trimmed]

On Wed, Feb 24, 2021, at 12:24 PM, Adnan Maolood wrote:
> I have removed the unexported methods before releasing v0.1.15. So it is
> now possible to implement ResponseWriter without embedding.

I missed that. Awesome!

> The Server still closes the connection after returning, Close just allows the
> Handler to close the connection before returning. Close is not really
> necessary since you can set the read and write deadline on the
> Request.Conn, so I am fine with removing it.

Ah, that makes way more sense.
 
> Since you can set the read and write deadline on the Conn, I am fine

On Wed, Feb 24, 2021, at 10:45 AM, Adnan Maolood wrote:
> I haven't seen this behavior in the wild, but I did bring it up on the
> Gemini mailing list. Someone then commented that they thought it was a
> common way to create multi-line input prompts.
>
> Relevant discussion:
> https://lists.orbitalfox.eu/archives/gemini/2020/003065.html
> https://lists.orbitalfox.eu/archives/gemini/2020/003067.html
> 
> If it were up to me I would make all requests and response headers end
> with '\n' only, and so this behavior would be invalid.

Yeah, strictly conforming to the spec means your implementation is probably
better. I was basing mine off of how I've handled IRC in the past which doesn't

Oof, that's unfortunate. I wonder if godocs.io is running on Go 1.15. The
mime registration could always be moved to another file if you want to
keep the bulk of this patch.

In any sense, this isn't a huge deal for me (it's pretty easy to install Go
1.16 directly) , but I thought it would be nice for people on Go 1.15 and below.

On Wed, Feb 24, 2021, at 8:01 AM, Adnan Maolood wrote:
> I decided to undo this change because the documentation for ServeFile
> and FileSystem no longer shows up on godocs.io, and because with this
> change the Gemini mime types will not be registered on Go 1.15.

Thanks for the thought-out response!

On Wed, Feb 24, 2021, at 4:43 AM, Adnan Maolood wrote:

> I named it Write because the net/http Request has a Write method. I'm
> willing to change this.

Ah, I didn't realize that. The current design probably makes sense then.
 
> > * I found it easier in parsing to read until \n and check that the string ended
> > in \r\n. It would let you avoid a separate call to Read for the next byte.
> 
> This would break the practice of using multiple \n before a final \r\n.
> I'm fine with breaking this practice, as in my opinion Meta should only

Sorry, my client missed the autowrap - here's the same text with autowrap re-enabled and some extra spacing.

I actually started work on a separate go-gemini package (at
https://github.com/go-gemini/gemini so gopkg.in/gemini could be used - I'm happy
to give this up if you want it) and ran across a number of things that might be
worth considering here. My package isn't anywhere near as complete as this, so
it would be nice to drop it in favor of this implementation.

* It would probably make more sense for Request to implement io.WriterTo rather
  than overloading the Write method.

* I found it easier in parsing to read until \n and check that the string ended
  in \r\n. It would let you avoid a separate call to Read for the next byte.

---
This patch isn't strictly necessary, but it addresses some of the feedback in
the thread about v0.1.15. It makes the tests run again on Go 1.15 and only
enables ServeFile (and FileServer) when fs.FS is available (Go 1.16 and above).
This will allow people who want to keep using the core of the library with Go
1.15 and below to do that while using the cleaner fs.FS for the actual
FileSystem implementation.

 fs.go            | 2 ++
 go.mod           | 2 +-
 response_test.go | 5 +++--
 3 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/fs.go b/fs.go
[message trimmed]