m_option: deprecate OPT_PATHLIST #15868
Conversation
|
Download the artifacts for this pull request: |
| @@ -1438,6 +1438,21 @@ static char **separate_input_param(const m_option_t *opt, bstr param, | |||
| str = bstr_cut(str, 1); | |||
| n++; | |||
| } | |||
| // Backwards compatibility for path list options. | |||
There was a problem hiding this comment.
This will not work in practice. There is no sane way to provide backward compatibility.
If you had a list separated with ; you likely didn't have , escaped, so it will already break the list in the main loop above.
By doing this we basically force to escape both separators.
There was a problem hiding this comment.
I know. It's mentioned in the commit message. There is breakage no matter what, but I figured it was worth at least trying if the user doesn't have , in there.
There was a problem hiding this comment.
It's really annoying, change like that will break everything. And even if compatibility is provided, all users are expected to migrate. ¯\_(ツ)_/¯
Honestly we might be stuck with it forever.
There was a problem hiding this comment.
Are there really that many people that use paths with , in them? I know the number isn't zero, but I feel it shouldn't be too many. We can always wait a couple of stable releases or so before yanking it.
There was a problem hiding this comment.
I mean in general switch from :/; to ,. Scripts/configs and so on... Though I like the change, because I always needed to check what is the separator for option X and system Y..
There was a problem hiding this comment.
There might be some pain but I think it's for the greater good.
Dudemanguy
force-pushed
the
deprecate-pathlist
branch
from
71ce0e9 to
8dac659
Compare
mpv has always had a separate type, OPT_PATHLIST, for paths/files. It worked exactly like OPT_STRINGLIST except that it used : (unix) or ; (windows) as a separator. There is no reason to have this distinction. : is allowed in filenames on linux and ; is allowed on windows so it is not even like these characters guarantee no escaping. Instead, we should be consistent and use one separator (,) for everything. In the option parsing, add some old compatibility code for the old path-specific separators with a warning that it is deprecated and , is preferred. It's not perfect. Someone using paths with , in them would now need to escape them, but it's about the best we can do realistically. Remove all references to path lists in the documents and treat it exactly like the normal string list option.
Dudemanguy
force-pushed
the
deprecate-pathlist
branch
from
8dac659 to
0be2f1a
Compare
|
I don't think this compatibility break is worth it. Many of these options are very commonly used. The overhead to support custom separators is minimal so there is really no benefit of removing it. Also technically path separator characters are allowed in files, but because the pervasive use of these separators in the software ecosystem as a whole, there are more file names in the wild which contain A better option would be adding an option to allow custom path separators. Those who prefer consistency with string list options can set it to |
I think it is not a problem of overhead to support those. But fragmentation of mpv string/path options. You always need to actively think what is the correct separator for given option and on top of that it is different on different OSes which makes thing not portable between them. See #10450 (comment) for example, people are confused. And since mpv config is driven by guides and suggestions from others, it's not obvious for the user, why this option works for them, but not the other guy. But I agree, compatibility break here is pita. Also I don't have strong opinion either way. I like the idea of single separator to make the use of it easier and portable. But on the other hand forcing users to change their stuff is annoying outcome. |
|
This is the total count of files on my system excluding root stuff (i.e. my home folder + mounted drives) that have these characters.
Edit: Sorry my bad I forgot I had a bunch of chroots in a subdirectory so that's a bunch of bogus OS files. So comma is indeed the most by this count. |
The closes to mpv example of files with |
mpv has always had a separate type, OPT_PATHLIST, for paths/files. It worked exactly like OPT_STRINGLIST except that it used : (unix) or ; (windows) as a separator. There is no reason to have this distinction. : is allowed in filenames on linux and ; is allowed on windows so it is not even like these characters guarantee no escaping. Instead, we should be consistent and use one separator (,) for everything. In the option parsing, add some old compatibility code for the old path-specific separators with a warning that it is deprecated and , is preferred. It's not perfect. Someone using paths with , in them would now need to escape them, but it's about the best we can do realistically. Remove all references to path lists in the documents and treat it exactly like the normal string list option.