code review 127380043: mime: Make filename extensions case-insensitive

mime: Make filename extensions case-insensitive

Fixes issue 8350.

[jra, 2014-08-14 21:25:37]

Hello golang-codereviews@googlegroups.com,

I'd like you to review this change to
https://go.googlecode.com/hg/

[bradfitz, 2014-08-15 01:19:21]

I think this needs more tests too.

https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go
File src/pkg/mime/type.go (right):

https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:48: typename := mimeTypes[strings.ToLower(ext)]
I'd malloc (well: it might malloc) outside of the lock to reduce contention.

https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:74: mimeTypes[extension] = mimeType
why isn't this lower too?

don't you want two maps? an original-case-keyed one and a lower-keyed one, and
then your lookup is first as given, and then as a fallback mapped to the lower
one?

[dave_cheney.net, 2014-08-15 01:33:21]

What if we just change the documentation on the method to say it
expects an exact match ?



On Fri, Aug 15, 2014 at 11:19 AM,  <bradfitz@golang.org> wrote:
> I think this needs more tests too.
>
>
>
> https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go
> File src/pkg/mime/type.go (right):
>
>
https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go#newc...
> src/pkg/mime/type.go:48: typename := mimeTypes[strings.ToLower(ext)]
> I'd malloc (well: it might malloc) outside of the lock to reduce
> contention.
>
>
https://codereview.appspot.com/127380043/diff/40001/src/pkg/mime/type.go#newc...
> src/pkg/mime/type.go:74: mimeTypes[extension] = mimeType
> why isn't this lower too?
>
> don't you want two maps? an original-case-keyed one and a lower-keyed
> one, and then your lookup is first as given, and then as a fallback
> mapped to the lower one?
>
> https://codereview.appspot.com/127380043/
>
>
> --
> You received this message because you are subscribed to the Google Groups
> "golang-codereviews" group.
> To unsubscribe from this group and stop receiving emails from it, send an
> email to golang-codereviews+unsubscribe@googlegroups.com.
> For more options, visit https://groups.google.com/d/optout.

[jra, 2014-08-17 19:53:40]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org, dave@cheney.net
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-08-18 17:07:05]

https://codereview.appspot.com/127380043/diff/60001/src/pkg/mime/type.go
File src/pkg/mime/type.go (right):

https://codereview.appspot.com/127380043/diff/60001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:25: var mimeTypesLc = mimeTypes
what is this? this is the same map.

This doesn't make a copy.

It will probably be faster if I just do this CL.

[jra, 2014-08-19 06:56:57]

https://codereview.appspot.com/127380043/diff/60001/src/pkg/mime/type.go
File src/pkg/mime/type.go (right):

https://codereview.appspot.com/127380043/diff/60001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:25: var mimeTypesLc = mimeTypes
This is too-fast hacking with wife pestering to start the movie. Sorry about
that. 
Please go ahead and take it over, I'm off chasing more performance in bzip. :)

[jra, 2014-08-25 20:48:29]

Hello golang-codereviews@googlegroups.com, bradfitz@golang.org, dave@cheney.net
(cc: golang-codereviews@googlegroups.com),

Please take another look.

[bradfitz, 2014-08-25 21:08:00]

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go
File src/pkg/mime/type.go (right):

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:25: var mimeTypesLc = map[string]string{}
Let's go with Lower instead of Lc which isn't obvious or common

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:33: func initMimeBuiltin() {
let's drop this func and just do:

   var (
       mimeLock sync.RWMutex // guards following maps
       mimeTypes = map[string]string{} // case-sensitive keys
       mimeTypesLower = map[string]string{} // case-insensitive (lowercase keys)
    )

    func init() {
      AddExtension(".css", "text/css; charset=utf-8")
      AddExtension(".gif", "image/gif")
      ...
    }

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:34: for k,v := range mimeTypes {
gofmt

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:43: // Extensions are looked up case-sensitively. If that
fails to find a
Or maybe just:

// Extension lookup is done first case-sensitively, else insensitively.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:54: // Windows system mime types are extracted from
registry.
MIME all caps here. It's referring to the spec acronym ("MIME"), not the package
("mime")

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:75: if ext == "" || ext[0] != '.' {
if !strings.HasPrefix(ext, ".")

... we're already using the strings package in this code.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:76: return fmt.Errorf(`mime: extension "%s" misses dot`,
ext)
%q instead of "%s"

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:91: extensionLc := strings.ToLower(extension)
extLower to avoid awkward "Lc"

[jra, 2014-08-26 21:48:25]

Ready for a final look.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go
File src/pkg/mime/type.go (right):

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:25: var mimeTypesLc = map[string]string{}
On 2014/08/25 21:08:00, bradfitz wrote:
> Let's go with Lower instead of Lc which isn't obvious or common

Done.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:33: func initMimeBuiltin() {
On 2014/08/25 21:07:59, bradfitz wrote:
> let's drop this func and just do:
> 
>    var (
>        mimeLock sync.RWMutex // guards following maps
>        mimeTypes = map[string]string{} // case-sensitive keys
>        mimeTypesLower = map[string]string{} // case-insensitive (lowercase
keys)
>     )
> 
>     func init() {
>       AddExtension(".css", "text/css; charset=utf-8")
>       AddExtension(".gif", "image/gif")
>       ...
>     }
> 
>       

Done.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:34: for k,v := range mimeTypes {
On 2014/08/25 21:07:59, bradfitz wrote:
> gofmt

Acknowledged.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:43: // Extensions are looked up case-sensitively. If that
fails to find a
On 2014/08/25 21:07:59, bradfitz wrote:
> Or maybe just:
> 
> // Extension lookup is done first case-sensitively, else insensitively.

Done.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go#newc...
src/pkg/mime/type.go:54: // Windows system mime types are extracted from
registry.
On 2014/08/25 21:07:59, bradfitz wrote:
> MIME all caps here. It's referring to the spec acronym ("MIME"), not the
package
> ("mime")

Done.

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type_test.go
File src/pkg/mime/type_test.go (right):

https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type_test.go...
src/pkg/mime/type_test.go:14: func TestMimeTypesLcIsLower(t *testing.T) {
This is no longer needed, since we rely on AddExtensionType and it is tested
below.

[bradfitz, 2014-08-28 15:22:44]

LGTM



On Tue, Aug 26, 2014 at 2:48 PM, <jra@nella.org> wrote:

> Ready for a final look.
>
>
>
> https://codereview.appspot.com/127380043/diff/80001/src/pkg/mime/type.go
> File src/pkg/mime/type.go (right):
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type.go#newcode25
> src/pkg/mime/type.go:25: var mimeTypesLc = map[string]string{}
> On 2014/08/25 21:08:00, bradfitz wrote:
>
>> Let's go with Lower instead of Lc which isn't obvious or common
>>
>
> Done.
>
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type.go#newcode33
> src/pkg/mime/type.go:33: func initMimeBuiltin() {
> On 2014/08/25 21:07:59, bradfitz wrote:
>
>> let's drop this func and just do:
>>
>
>      var (
>>         mimeLock sync.RWMutex // guards following maps
>>         mimeTypes = map[string]string{} // case-sensitive keys
>>         mimeTypesLower = map[string]string{} // case-insensitive
>>
> (lowercase keys)
>
>>      )
>>
>
>       func init() {
>>        AddExtension(".css", "text/css; charset=utf-8")
>>        AddExtension(".gif", "image/gif")
>>        ...
>>      }
>>
>
>
>
> Done.
>
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type.go#newcode34
> src/pkg/mime/type.go:34: for k,v := range mimeTypes {
> On 2014/08/25 21:07:59, bradfitz wrote:
>
>> gofmt
>>
>
> Acknowledged.
>
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type.go#newcode43
> src/pkg/mime/type.go:43: // Extensions are looked up case-sensitively.
> If that fails to find a
> On 2014/08/25 21:07:59, bradfitz wrote:
>
>> Or maybe just:
>>
>
>  // Extension lookup is done first case-sensitively, else
>>
> insensitively.
>
> Done.
>
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type.go#newcode54
> src/pkg/mime/type.go:54: // Windows system mime types are extracted from
> registry.
> On 2014/08/25 21:07:59, bradfitz wrote:
>
>> MIME all caps here. It's referring to the spec acronym ("MIME"), not
>>
> the package
>
>> ("mime")
>>
>
> Done.
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type_test.go
> File src/pkg/mime/type_test.go (right):
>
> https://codereview.appspot.com/127380043/diff/80001/src/
> pkg/mime/type_test.go#newcode14
> src/pkg/mime/type_test.go:14: func TestMimeTypesLcIsLower(t *testing.T)
> {
> This is no longer needed, since we rely on AddExtensionType and it is
> tested below.
>
> https://codereview.appspot.com/127380043/
>

[bradfitz, 2014-08-28 15:22:57]

*** Submitted as https://code.google.com/p/go/source/detail?r=3b0890b3de1d ***

mime: Make filename extensions case-insensitive

Fixes issue 8350.

LGTM=bradfitz
R=golang-codereviews, bradfitz, dave
CC=golang-codereviews
https://codereview.appspot.com/127380043

Committer: Brad Fitzpatrick <bradfitz@golang.org>