skywind3000 / z.lua

:zap: A new cd command that helps you navigate faster by learning your habits.
MIT License
2.94k stars 137 forks source link

Support '|' in path #189

Closed Arkaeriit closed 3 months ago

Arkaeriit commented 10 months ago

Using '|' as the data separator caused crashes if a path with a '|' char in it was in the data file. Changing that separator to a null byte ensures that this kind of issue can never happen again as the null byte can't be in a path.

Arkaeriit commented 4 months ago

I rebased the branch onto master.

skywind3000 commented 4 months ago

The original format is compatible with z.sh, I believe a lot of users may benefit from this PR, before it get merged there are still some problems to deal with:

1) after the upgrading, will the old history data corrupt ? 2) if users have no idea about this upgrading (just run a clink update *), will they find all of they history data have been lost ?

Some possible options:

1) make the separator configurable ? (via a environment variable), when people are changing it they know what they are doing. 2) make some header in the new format so z.lua can figure out (maybe a "\0\0" marker in the first line) if it is a new format, if not it will convert the old format into the new one ? 3) what about a binary format ?

Arkaeriit commented 3 months ago

Thanks for the feedback. I didn't thought about migration so I definitively need to fix that. Option 2 seems the most sensible to me so I will go with it.

Arkaeriit commented 3 months ago

Turns out changing the separator is not needed. Improving the reading function is enough to fix the bugs and crashes with pipes in filenames.

skywind3000 commented 3 months ago

I like this solution, no need to convert the old data, the second argument of string:split() function can specify maximum split items:

https://github.com/skywind3000/z.lua/blob/30220996cae675f9ec12fc3aeb2b5c3d957af8c8/z.lua#L138-L152

is it enough to just use

local part = string.split(line, '|', 3)

??

Arkaeriit commented 3 months ago

Using the second argument of the split function merges the not split part at the end of the input and not at the start like we want.

line = "path|with|a|lot|of|pipes|1212|9999"
line:split("|", 3)) --> {"path", "with", "a", "lot|of|pipes|1212|9999"}
skywind3000 commented 3 months ago

got it, thank you for this PR