fix(filetype): fix filetype patterns

[smjonas]

Correct some incorrect filetype patterns and add missing ones to avoid test failures that became evident in #19216.

[clason]

All the starset functions should be in the pattern table, not the filename table -- that should remove a sizeable chunk of errors.

[smjonas]

What do you think about refactoring some patterns by omitting leading and trailing .* in Lua patterns?

Another issue seems to be that Lua .* does not match nothing, so, e.g. , .%.git/modules/./config fails to match file.git/modules//config.

This should be solved by using .- instead of .* (.- matches non-greedily).

[clason]

What do you think about refactoring some patterns by omitting leading and trailing .* in Lua patterns?

Hmmm... I think that would make it harder to check, for relatively little technical advantage. But if there's an actual benefit (and it's documented), why not? Especially if it's a matter of consistency (i.e., all patterns have an implicit .*<foo>.*, so that the explicit ones are redundant and used inconsistently).

This should be solved by using .- instead of .* (.- matches non-greedily).

I don't think that's it, actually -- a "naked" string.find works, so this must be related to the match_pattern function (full path vs. tail)?

[clason]

But in any case, the first order of business is to pass the test suite -- that will give us much more confidence when refactoring ;)

[smjonas]

Now all tests are passing except the Git check (@gpanders):

1 FAILED:
Found errors in Test_filetype_detection():
function RunTheTest[37]..Test_filetype_detection[2]..CheckItems line 13: with file name: file.git/modules//config: Expected 'gitconfig' but got ''
make[1]: *** [Makefile:112: test_filetype_lua] Error 1

[clason]

For me also the etc/hostname.file check (expects config) fails?

(EDIT: see following collapsed comment chain for explanation; TL;DR on macOS, the actual expanded name is /private/etc/hostname.file; this doesn't crop up for filetype.vim since it uses the unexpanded name from <amatch>)

[clason]

Now all tests are passing except the Git check (@gpanders):

1 FAILED:
Found errors in Test_filetype_detection():
function RunTheTest[37]..Test_filetype_detection[2]..CheckItems line 13: with file name: file.git/modules//config: Expected 'gitconfig' but got ''
make[1]: *** [Makefile:112: test_filetype_lua] Error 1

This seems to be something to do with actual filename normalization, since vim.filetype.match({filename='file.git/modules//config'}) does correctly return 'gitconfig'.

After a lot of experimentation, this specific issue seems to be due to fnamemodify(name, ":p") stripping duplicate slashes.

I still have no idea why exactly filetype.lua gets the target /private/etc/hostname.file, while filetype.vim gets the link itself /etc/hostname.file. (On macOS, /etc is a symlink to /private/etc.)

For this PR, I'd therefore suggest

• adding a separate .*/etc/hostname%..* pattern
• adding a separate file.git/modules/config pattern

and putting a TODO comment behind each mentioning this issue (e.g., -- needed since filetype.lua uses fully expanded file names). We should also mention in vim-differences.txt that filetype.lua does follow (some) symlinks, while filetype.vim doesn't.

@gpanders ?

[smjonas]

@clason I created a PR on the Vim repo that updates the etc/hostname.* pattern: vim/vim#10662

[gpanders]

I still have no idea why exactly filetype.lua gets the target /private/etc/hostname.file, while filetype.vim gets the link itself /etc/hostname.file. (On macOS, /etc is a symlink to /private/etc.)

Because we call vim.fn.resolve() in filetype.lua:

  local path = vim.fn.resolve(vim.fn.fnamemodify(name, ':p'))

Maybe we ought to remove that.

this doesn't crop up for filetype.vim since it uses the unexpanded name from

We could pass args.match to vim.filetype.match to replicate this:

diff --git a/runtime/filetype.lua b/runtime/filetype.lua
index 35bb31edc..a9afbed27 100644
--- a/runtime/filetype.lua
+++ b/runtime/filetype.lua
@@ -12,7 +12,7 @@ vim.api.nvim_create_augroup('filetypedetect', { clear = false })
 vim.api.nvim_create_autocmd({ 'BufRead', 'BufNewFile' }, {
   group = 'filetypedetect',
   callback = function(args)
-    local ft, on_detect = vim.filetype.match({ buf = args.buf })
+    local ft, on_detect = vim.filetype.match({ buf = args.buf, filename = args.match })
     if ft then
       vim.api.nvim_buf_set_option(args.buf, 'filetype', ft)
       if on_detect then

[smjonas]

but do we support filename and bufnr?

Yes we do since recently.
(The original reason was to support checks like this:

bak = function(path, bufnr)
    local root = vim.fn.fnamemodify(path, ':r')
    return M.match({ buf = bufnr, filename = root })
end,

)

[clason]

Yes, I think that sounds like a good idea.

(Sorry, I was on mobile and misremembered; now I checked the actual code :))

[clason]

Because we call vim.fn.resolve() in filetype.lua:
Maybe we ought to remove that.

We could pass args.match to vim.filetype.match to replicate this:

I just had a chance to test this; either change is sufficient to fix the symlink issue (which is also fixed by @smjonas' port ;)). It does not fix the double slash issue, though, which is due to the (necessary) fnamemodify.

I think these two changes make sense in themselves (align more closely to upstream behavior) -- can you think of any downside to passing args.match?

For the double slash issue, I don't see any alternative to either adding a new pattern or removing that (frankly a bit silly) test.

[smjonas]

can you think of any downside to passing args.match?

Isn't the buffer name retrieved anyway if not passed? So it should be working exactly the same, right (plus saving a function call ;))?

For the double slash issue, I don't see any alternative to either adding a new pattern or removing that (frankly a bit silly) test.

I would suggest to simply add the additional pattern '.*%.git/modules/config'. Not worth replicating Vim's exact behavior here imo (the tests are still passing). I did both things in the new commit. Should be good to merge then :D

[clason]

Thanks! If I can request one last iteration: Could you put the autocommand changes and the removal of the vim.fn.resolve in a separate commit (which I wouldn't squash when merging)? Would make it easier to pick out later on in case it becomes important.

[smjonas]

Sure, do you want me to also include the "generic fallback" change in the autocommand in this separate commit or in the old one because it's more related to filetype changes? And btw, is that a fix or refactor?

[clason]

Sure, do you want me to also include the "generic fallback" change in the autocommand in this separate commit or in the old one because it's more related to filetype changes? And btw, is that a fix or refactor?

Good idea; the point is to separate the "infrastructure" changes that (may) have global effect from the changes to individual patterns.

I'd say it's a fix, not a refactor, as it has observable changes.

[clason]

Thank you, @smjonas, for seeing this through!

[smjonas]

Thank you as well for helping me with this! Seems like Lua filetype detection is almost ready, awesome! 🎉