Repository navigation
fix(home): don't shorten sibling dirs that share the home dir prefix - #532
Conversation
|
I had wondered before if there is no simpler, more idiomatic way for home path substitution than using func (p *RealHome) ShortenHome(path string) (string, error) {
home, err := p.os.UserHomeDir()
if err != nil {
return "", err
}
if rel, err := filepath.Rel(home, path); err == nil {
// IsLocal() only accepts non-absolute paths not starting with ../
if filepath.IsLocal(rel) {
return filepath.Join("~", rel), nil
}
}
return path, nil
}It implicitly solves your issue as well... If you want to use it, there is one little test adaptation necessary: @@ -97,7 +98,7 @@ func TestShortenHome(t *testing.T) {
got, err := NewHome(mockOs).ShortenHome("/home/user/projects")
assert.NoError(t, err)
- assert.Equal(t, "~/projects", got)
+ assert.Equal(t, filepath.FromSlash("~/projects"), got)
})
t.Run("shortens the home dir itself to a tilde", func(t *testing.T) { |
|
Thanks @purplezimmermann, that's a neat approach, and I kept the PR as the smallest change that fixes the sibling-prefix bug, and haven't run your version in CI. Two behavior differences I noticed with
Neither looks harmful to me, but it's @joshmedeski's call. If you prefer the |
joshmedeski
left a comment
joshmedeski
left a comment
There was a problem hiding this comment.
Thanks @ChandraChaan, the fix is solid, and thanks for running the before/after CI on your fork. @purplezimmermann, thanks for the filepath.Rel suggestion.
I'd like to go with the Rel version. On Windows, Rel compares paths case-insensitively and accepts both / and \. That means a path like c:/users/me/proj gets shortened, which the HasPrefix check misses. It also fixes the sibling-prefix bug without any boundary logic of our own:
func (p *RealHome) ShortenHome(path string) (string, error) {
home, err := p.os.UserHomeDir()
if err != nil {
return "", err
}
if rel, err := filepath.Rel(home, path); err == nil && filepath.IsLocal(rel) {
return filepath.Join("~", rel), nil
}
return path, nil
}You already covered the behaviour differences, and I'm fine with both:
- Cleaned paths: trailing slashes and
.segments get dropped. That's fine for session names. \on Windows:name_substitutionrunsToSlashbefore applying rules, so user rules written with/still match.
To finish it:
- Update the existing test to expect
filepath.FromSlash("~/projects"), as @purplezimmermann showed. Keep both of your new tests: the home dir becomes~and the sibling dir stays unchanged. They should pass as written. - Leave out code comments, both the one above the current boundary check and the
// IsLocal() ...one in the suggestion. I keep the reasoning for a change in the commit message and PR description rather than in the file, and your PR body already covers it well.
Happy to merge when CI passes.
Switch ShortenHome from a prefix check with a boundary test to
filepath.Rel plus IsLocal. Paths outside the home dir, including
siblings that share its prefix such as /home/username, stay unchanged.
On Windows Rel compares case-insensitively and accepts both separators.
The result is built with filepath.Join, so it uses the OS separator;
the existing test now expects filepath.FromSlash("~/projects").
|
Thanks @joshmedeski. Done in bcb72bf: I ran it on my fork before pushing, and Lint, Build, Benchmarks and Tests on ubuntu, macOS and Windows all passed: https://github.com/ChandraChaan/sesh/actions/runs/42426858629 |
|
Hi @joshmedeski, a gentle ping: the |
|
Thanks for the ping, nice job on this! |
ShortenHomereplaces the home directory with~using onlystrings.HasPrefix(path, home), so it also rewrites sibling directories whose name merely starts with the home path. With home/home/user,ShortenHome("/home/username/projects")returns~name/projectsinstead of leaving the path alone. The result is used for the session names of zoxide directories (lister/zoxide.go) and as the input toname_substitutionrules (namer/substitute.go), so a directory like/home/username/projshows up as~name/proj.This only shortens when the home directory ends at a path boundary: the rest of the path is empty or starts with
/(or the OS path separator). Paths under the home directory and the home directory itself behave as before.I added two cases to
TestShortenHome: the home directory itself becomes~, and/home/username/projectsstays unchanged.I did not run Go locally, so I checked it in CI on my fork:
home_test.go:118,expected: "/home/username/projects",actual : "~name/projects", thenFAIL github.com/joshmedeski/sesh/v2/home. Run: https://github.com/ChandraChaan/sesh/actions/runs/41604695715