Skip to content

Conversation

@sergiodegioia
Copy link

GetFilenames is changed to manage paths with a trailing semantically-neutral '/.', thus avoiding to check l:path=='/' at each iteration.

…etFilenames is changed to manage paths with a trailing semantically-neutral '/.', thus avoiding to check l:path=='/' at each iteration.
@xuhdev
Copy link
Member

xuhdev commented Aug 5, 2020

Could you explain a bit more?

@sergiodegioia
Copy link
Author

sergiodegioia commented Aug 5, 2020

Since I installed editorconfig-vim, buffer loading started to be dramatically slow, making Vim unusable. I launched the vim profiler: this is the excerpt

FUNCTION  32_UseConfigFiles()
    Defined: ~/.vim/pack/local/start/editorconfig-vim/plugin/editorconfig.vim line 199
Called 1 time
Total time:   4.670006
 Self time:   4.520006

count  total (s)   self (s)
    1              0.000000     let l:buffer_name = expand('%:p')
                                " ignore buffers without a name
    1              0.000000     if empty(l:buffer_name)
                                    return
    1              0.000000     endif
                            
                                " Check if any .editorconfig does exist
    1              0.000000     let l:conf_files = s:GetFilenames(expand('%:p:h'), '.editorconfig')
    1              0.000000     let l:conf_found = 0
    6              0.000000     for conf_file in conf_files
    6              4.510006         if filereadable(conf_file)
    1              0.000000             let l:conf_found = 1
    1              0.000000             break
    5              0.000000         endif
    6              0.000000     endfor

Then I spotted a problem with the execution of filereadable(conf_file), as you can see.
Then I echoed conf_file in the loop whose values come from s:GetFilenames(expand('%:p:h'), '.editorconfig').
Among its values there is always '//.editorconfig' because s:GetFilenames walks the way up to the root starting from the current directory of the current buffer expand('%:p:h'), and appends '/.editorconfig'.
Then I tried

:call filereadable('//.editorconfig')
and it took more than 4 seconds to return.
I'm using vim under mingw64 and it seems to me that spurious slashes are badly handled at the start of a path under mingw64.
Eliminating the double slash at the start of the path, buffers are loaded instantly instead of after 4.5 seconds.
In order to eliminate the double slash, s:GetFilenames must be changed such that, when l:path=='/',

let l:path_list += [l:path . '/' . a:filename]

is substituted for by

let l:path_list += [l:path . a:filename]

Or conveniently, in order to avoid checking for l:path=='/' at each iteration,

let l:path_list += [l:path . '/' . a:filename]

must be substituted for by

let l:path_list += ['/.' . l:path . '/' . a:filename]

as I did.
In this case if a new buffer is loaded for a file under /home/foo
filereadable will be executed for these file paths:

/./home/foo/.editorconfig
/./home/.editorconfig
/.//.editorconfig

that are semantically equivalent to these file paths (which are returned by the original s:GetFilenames):

/home/foo/.editorconfig
/home/.editorconfig
//.editorconfig

with the advantages that no double slash appears at the start of a path and no additional check is added.

@cxw42
Copy link
Member

cxw42 commented Aug 13, 2020

Good find! Makes sense to me. Have you been able to test on any other platforms? Does this code also work correctly in native Windows vim?

@sergiodegioia
Copy link
Author

sergiodegioia commented Aug 13, 2020

Thanks for your insight. It did not work under Windows, so I reverted back to the former idea of substituting

        if l:path == '/'
            let l:path_list += [ '/' . a:filename]
        else
            let l:path_list += [ l:path . '/' . a:filename]
        endif

for

let l:path_list += [l:path . '/' . a:filename]

Moreover I also checked for its proper functioning under Centos 7 and Cygwin.
(I found that Cygwin suffered from the same problem as Mingw64: dramatically slow buffer loading. This commit will then be useful for Cygwin user as well).

@divVerent
Copy link

This is actually very related to this:

#238

Note that here //.editorconfig was generated too. It seems the path name handling generally has some issues.

@ZenithMDC
Copy link

I encountered this same issue, using MSYS2, and came up with essentially the same solution. From what I read, // is a Cygwin virtual directory for Windows UNC (local network resource) paths, and in my testing, any file under that directory returns true using filereadable(), even if said file doesn't exist. E.g. filereadable('//.editorconfig'). And accessing nonexistent files under // is subject to varying amounts of latency, for reasons I don't understand. The same latency occurs when attempting to stat nonexistent files under //.

Printing out the list of possible .editorconfig locations, from my home directory, returns:

/home/Zenith/.editorconfig
/home/.editorconfig
//.editorconfig

As it currently stands, /.editorconfig is not considered at all. And because of how filereadable() behaves with the // directory, the plugin will always think it found a config, even when it doesn't exist. This was actually what caused me to discover the bug: I wanted to have different behavior for my vimrc, depending on whether .editorconfig was found or not. Basically, I was checking for the existence of editorconfig_applied in an autocmd. But, because of the aforementioned bug, editorconfig_applied was defined no matter what.

With the fix applied, the list of possible .editorconfig locations becomes:

/home/Zenith/.editorconfig
/home/.editorconfig
/.editorconfig

And editorconfig_applied will only be defined if .editorconfig was found.

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment

Labels

None yet

Projects

None yet

Development

Successfully merging this pull request may close these issues.

5 participants