martanne / vis

A vi-like editor based on Plan 9's structural regular expressions
Other
4.2k stars 258 forks source link

Detect filetype via hashbang #997

Closed Nomarian closed 2 years ago

Nomarian commented 2 years ago

I did not, no...

There is still merit in just having a hashbang table, at least for readability I guess (no functions, just a table of strings) I added only 2, but a lot of languages have a pretty well known and unique interpreter in PATH, adding more is rather trivial with a table. and the problem is a bit more complicated than just a simple match, IMO you should also ignore /usr/bin/env.

but its up to yall.

deepcube commented 2 years ago

I do like the idea of checking the shebang, and I'm leaning towards accepting this. A few questions/comments:

Nomarian commented 2 years ago

Does this offer significantly more functionality than calling file for the mime type? Is it just to support shebangs that aren't already included in the file command?

Well, that's entirely dependent on the /bin/file database/version. It doesn't have to be either this or that. it can try the mime and if it fails it can go for the internal hashbang check. executing file to me is an unnecessary fork. (But loops and shellscript has left me slightly paranoid about them.)

What happens if there's a flag? e.g. #!/bin/sh -xe or #!/usr/bin/awk -f It looks like the provided string would have to omit the $ that you used in your example to signify end of string, is that correct? I wonder if it would be better to only match against the first word, or to leave that open to the user to specify whether or not to care about it.

The flag gets ignored, it only cares about the last FILENAME of #!/FILEPATH/FILENAME or the first argument to /usr/bin/env (yes, -S is valid because that would require parsing CLI args and identifying where the command arg for env is, which is slightly complex but can be added in) there is a caveat or a bug, which is a false positive because it just wants the last filename (or whatever looks like a filepath)

Can you think of any use case where someone would care about that flag when matching?

Yes, bash has a --posix flag which mimics sh behavior, zsh also has one. The fix is probably to use the detect function instead, since CLI args can be incredibly complicated.

Can you update it to accept a space between the ! and / as that's fairly common e.g. #! /bin/sh (if it does already support this and I missed it let me know)

Sure, I haven't programmed in months and I've forgotten everything. Now that I look at it, line 520 and 522 should be changed as well.

Further Notes:

Its also a bad idea to keep the full hashbang in anyway, you want the utility only, suppose you have a hashbang for #!/bin/sh, ok, now the matcher has to match sh, if its android, sh is in another place, but now the pattern should be ^sh%s?, suppose there's another interpreter/utility called shark, now you'll get a false positive for shark. so it should be utility only.

The entire thing is quite buggy, and gnu just adds a wrench to an already hacky system. will need a full rewrite

Nomarian commented 2 years ago

Updated to use a space. will also not open the file and use the data variable that ft.detect uses. Also added a utility table which matches only the utility directly and the hashbang table matches the entire hashbang (but not the #!).

Please test.

deepcube commented 2 years ago

Thanks for the update! I'll review now. In the meantime can you squash the commits?

deepcube commented 2 years ago

Testing now. Can you remove the extra white space on blank lines you added?

deepcube commented 2 years ago

Applied. Thanks for your patience.