rejeep / f.el

Modern API for working with files and directories in Emacs
GNU General Public License v3.0
680 stars 68 forks source link

[Bug]: `(f-join "/" "foo" "/")` => "/" #128

Closed xbc5 closed 1 year ago

xbc5 commented 1 year ago

Expected behavior

It should return "/foo/".

Actual behavior

It returns "/".

f.el version

0.20.0

Specifically I am using this commit: af7d37c.

Emacs version

27.2

Relevant code or log output

No response

xbc5 commented 1 year ago

In addition: (f-join "/foo" "/bar") => "/bar", which should be "/foo/bar"; (f-join "/foo" "/" "bar") => "/bar", which should be "/foo/bar";

Phundrak commented 1 year ago

Thanks for submitting an issue!

From what I can see in the source code of f-join, this is intended behaviour. There is an explicit check to see whether any of the arguments of f-join is absolute, and if this is the case it then sets the returned path to this expanded absolute path. You can see it clearly here:

(setq path (cond ((not path) arg)
                 ((f-absolute-p arg)
                  (progn
                    (setq relative nil)
                    arg))
                 (t (f-expand arg path))))

Changing this behaviour could break existing code. I’ll mark this issue as wontfix but leave it open in case @rejeep disagrees with me.

xbc5 commented 1 year ago

Yes it would break things, I see why you are reluctant to change it. However, I don't think it should work like that -- the function should be to join paths, and join paths only (it shouldn't make decisions for you). Sometimes directories end with a /, I could see some situations where this behaviour would lead to a bug (as it did for me).

Phundrak commented 1 year ago

Directories ending with a separator are not an issue with f.el:

(f-join "foo" "bar/" "baz") ;; => "foo/bar/baz"

The real issue is when it begins with one, in which case it will always be considered as an absolute path.

xbc5 commented 1 year ago

Yes, I guess it's subjective to say that (f-join "bar" "/") is a relative (bar/) or absolute path (/).

Phundrak commented 1 year ago

Out of curiosity, I had a look at how other libraries outside of Emacs handle this. C++, Python, Rust, and EmacsLisp agree with an absolute path discarding everything in front of it:

namespace fs = std::filesystem;

int main() { fs::path foo = "foo"; foo /= "/"; std::cout << foo << std::endl; return 0; }


``` shell
$ clang++ -std=c++17 main.cpp && ./a.out
"/"

fn main() { let path = Path::new("foo").join("/"); println!("{}", path.display()); }


``` shell
$ rustc main.rs && ./main.rs
/

Java seems to disagree on whether the previous path elements should be discarded, but it doesn’t append any final / anyway:

import java.nio.file.Path;
import java.nio.file.Paths;

class main
{
    public static void main(String[] args)
    {
        Path path = Paths.get("foo", "/");
        System.out.println(path);
    }
}
$ javac main.java && java main
foo

As the current behaviour of f.el seems to be the norm rather than the exception, I’ll close this issue, I doubt it will ever change.

(note that, apparently, Python’s behaviour is different on Windows than on Unix. Thank you, Windows, for simplifying stuff for everyone…)

xbc5 commented 1 year ago

No problem. This issue rang some bells for me, and now that you mention it, I remember having this issue on Python too. I'm just waiting for the day that a trailing slash causes chaos on the stock market and brings down the world economy. :)

rejeep commented 1 year ago

I’ll mark this issue as wontfix but leave it open in case @rejeep disagrees with me.

Not really, I'll trust your jugdement!