[vim/vim] fix: correct sh detection (PR #13172)

Shane-XB-Qian

unread,

Sep 24, 2023, 1:25:05 PM9/24/23

to vim/vim, Subscribed

// #13171 (comment)

You can view, comment on, or merge this pull request online at:

https://github.com/vim/vim/pull/13172

Commit Summary

f77048a fix: correct sh detection

File Changes

(1 file)

M runtime/syntax/sh.vim (30)

Patch Links:

—
Reply to this email directly, view it on GitHub.
You are receiving this because you are subscribed to this thread.

codecov[bot]

unread,

Sep 24, 2023, 1:35:31 PM9/24/23

to vim/vim, Subscribed

Codecov Report

Merging #13172 (f77048a) into master (9fcde94) will increase coverage by 0.65%.
Report is 387 commits behind head on master.
The diff coverage is n/a.

@@            Coverage Diff             @@
##           master   #13172      +/-   ##
==========================================
+ Coverage   82.08%   82.74%   +0.65%     
==========================================
  Files         160      150      -10     
  Lines      193690   182112   -11578     
  Branches    43492    40923    -2569     
==========================================
- Hits       158997   150692    -8305     
+ Misses      21840    18473    -3367     
- Partials    12853    12947      +94

Flag	Coverage Δ
huge-clang-Array	`82.74% <ø> (?)`
huge-clang-none	`?`
linux	`82.74% <ø> (+0.02%)`	⬆️
mingw-x64-HUGE	`?`
mingw-x86-HUGE	`?`
windows	`?`

Flags with carried forward coverage won't be shown. Click here to find out more.

see 148 files with indirect coverage changes

—
Reply to this email directly, view it on GitHub, or unsubscribe.
You are receiving this because you are subscribed to this thread.

cecamp

unread,

Sep 24, 2023, 2:18:29 PM9/24/23

to vim/vim, Subscribed

Well, this "fix" would definitely be problematic. You've taken out the section where syntax/sh.vim is trying to figure out which shell it should use when the user hasn't specified one.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 24, 2023, 2:34:43 PM9/24/23

to vim/vim, Subscribed

You've taken out the section where syntax/sh.vim is trying to figure out which shell it should use when the user hasn't specified one.

i can not get you, what you mean? or anyway this is to make foo.sh kept a default b: var value, which not relay on bin link,
e.g ubuntu is default link sh to dash, but user like to keep it as sh at that, or he moved to another machine which no dash.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Sep 24, 2023, 5:00:47 PM9/24/23

to vim/vim, Subscribed

I don't understand what problem you are trying to solve here

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 24, 2023, 6:25:43 PM9/24/23

to vim/vim, Subscribed

please refer that reference comment in ticket description.

is it ok to rely on bin link to detect sh type?
specially how to make your one no shenbang `sh` file behavior and syntax same in different machines?
I supposed it should default to the one it used-to, unless user g: var setting overrode it by need.

--
shane.xb.qian

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 24, 2023, 6:45:20 PM9/24/23

to vim/vim, Subscribed

anyway let @cecamp make decision, I am ok to pull back this PR, to this one I am just trying to help clarify but it's not my goal to sell it :smile:

--
shane-xb-qian

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

fritzophrenic

unread,

Sep 24, 2023, 11:48:07 PM9/24/23

to vim/vim, Subscribed

please refer that reference comment in ticket description. is it ok to rely on bin link to detect sh type? specially how to make your one no shenbang sh file behavior and syntax same in different machines? I supposed it should default to the one it used-to, unless user g: var setting overrode it by need.

Sorry this is still very unclear. I am sure we are dealing with a language barrier here. Can you maybe illustrate the problem with an example? What specifically is Vim doing that you don't think it should do, and why?

By "that reference comment in ticket description" I assume you are talking about this? #13171 (comment)

Reading that comment I still have no idea what you are trying to accomplish by removing shell detection.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 12:36:35 AM9/25/23

to vim/vim, Subscribed

still have no idea what you are trying to accomplish by removing shell detection.

it did Not remove shell detection, please note it just to make b:is_xxsh not rely on symblink, and did not change ft.
please image you are writing a foo.sh in your ubuntn and macos and debian/suse, you may find it highlight different.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

dkearns

unread,

Sep 25, 2023, 12:40:29 AM9/25/23

to vim/vim, Subscribed

@Shane-XB-Qian is, I think, arguing that the installed binary isn't relevant for the purposes of filetype detection. Which is certainly a reasonable perspective.

There's not necessarily any relationship between the file that's being edited and the system it's being edited on. If the dialect can't be determined from the file content or a specific user configuration override then it should just default to plain old sh.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 12:44:08 AM9/25/23

to vim/vim, Subscribed

@dkearns cool, you are my man, oh no, my bro.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

fritzophrenic

unread,

Sep 25, 2023, 1:05:16 AM9/25/23

to vim/vim, Subscribed

@Shane-XB-Qian is, I think, arguing that the installed binary isn't relevant for the purposes of filetype detection. Which is certainly a reasonable perspective. There's not necessarily any relationship between the file that's being edited and the system it's being edited on.

From the comments in the code, and from what the code is actually doing, it looks like the removed code is intended to account for when the script file is specifying /bin/sh or /usr/bin/sh. It seems a little strange to just assume "sh" in the case that the script doesn't specify any shell but maybe there is a good reason for that. Regardless, per the comments in the code if the script had a "#!/bin/sh" then figuring out which shell that actually points to on the installed system actually makes a lot of sense. Presumably this script is intended to actually run on this system at some point in most cases.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Sep 25, 2023, 4:13:40 AM9/25/23

to vim/vim, Subscribed

please image you are writing a foo.sh in your ubuntn and macos and debian/suse, you may find it highlight different.

But this makes sense to highlight depending on what your actual shell does support, no?

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 4:25:49 AM9/25/23

to vim/vim, Subscribed

please image you are writing a foo.sh in your ubuntn and macos and debian/suse, you may find it highlight different.

But this makes sense to highlight depending on what your actual shell does support, no?

i think no: there's not necessarily any relationship between the file that's being edited and the system it's being edited on, e.g you are editting a java code but didnot meant you had to install jdk first.
// you can control it via g:is_xxsh anyway.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Sep 25, 2023, 4:53:09 AM9/25/23

to vim/vim, Subscribed

I would think differently. We are changing the current default behaviour when it's not possible to know what shell is being used for /bin/sh. I assume this would mean, the syntax highlighting will use default POSIX highlighting only and not consider bash constructs, even if /bin/sh is a symlink to /usr/bin/bash and so is totally fine with such special constructs.

Especially for shell scripts, I think it would be okay to detect the current shell, because unless you are writing a larger or more complex shell script, I would think you do it on the system where you will run it (so you can also test it etc). At least that's how I do it usually. And for complex or larger shell scripts, I would think you carefully decide what shell to use in your shebang line anyhow (or you at least set one of those configuration variables).

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 5:08:10 AM9/25/23

to vim/vim, Subscribed

I would think differently. We are changing the current default behaviour when it's not possible to know what shell is being used for /bin/sh. I assume this would mean, the syntax highlighting will use default POSIX highlighting only and not consider bash constructs, even if /bin/sh is a symlink to /usr/bin/bash and so is totally fine with such special constructs.

i think you are thinking it just opposite to it, foo.sh used to be treated it as sh (if no shenbang), meant sh is sh,
game changed just recently i guess it is ubuntu changed sh to link it to dash, that's the one changed default behavior,
i am just trying to recover the tradition or history, that's it.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Sep 25, 2023, 6:19:27 AM9/25/23

to vim/vim, Subscribed

yeah exactly, a lot of people use bashism even if using /bin/sh in the shebang line (which is okay, if your /bin/sh is a symlink to bash, because it will happily accept it, even though this is not considered good style).

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 6:34:32 AM9/25/23

to vim/vim, Subscribed

problem is e.g ubuntu changed it link to dash recently (note: it is a 'd'), and please check the case which reported in #13171 , now it is beginning to break the game. so I or we better to rescue the game. :smile:

--
shane.xb.qian

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Christian Brabandt

unread,

Sep 25, 2023, 7:52:23 AM9/25/23

to vim/vim, Subscribed

problem is e.g ubuntu changed it link to dash recently (note: it is a 'd'), and please check the case which reported in #13171

I remember that, but that was like 10 years ago, so it is not recently. And I checked #13171 and I think it's wrong over there and one should rather use exectuable("bash") instead as mentioned over there.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 25, 2023, 8:14:59 AM9/25/23

to vim/vim, Subscribed

I would say you probably remember not correctly and besides it normally would be a bit delayed from dev version to LTS version to end users, and I recalled that #13171 was for or tried to fix another ticket which there is a difference between dash and sh/bash, so your mentioned exectuable('bash') was Not the problem or solution.
anyway, I wish sh back to sh (and indeed generally it was a subset of bash).
// BTW: in Linux world, my opinion, vim & bash & firefox is his/her faith. :smile:

--
shane.xb.qian

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 26, 2023, 12:08:07 AM9/26/23

to vim/vim, Subscribed

seems clarify done.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Shane-XB-Qian

unread,

Sep 26, 2023, 12:08:08 AM9/26/23

to vim/vim, Subscribed

Closed #13172.

—
Reply to this email directly, view it on GitHub.

You are receiving this because you are subscribed to this thread.

Reply all

Reply to author

Forward