Review internal tree traversal of symlinks

[lread]

While @borkdude was having a peek at #163 he noted that the internal file-seq (which PR replaces with internal path-seq) does not take symlinks into account.

This internal fn is used by modified-since and zip.

Option 0: Do nothing

Affected fns will navigate symbolic links in dir/file trees, potentially getting into circular behaviour.

Options 1: Never follow symbolic links

Never follow symbolic links for affected fns.
I'm guessing that this is typically what users might want?

Option 2: Optionally follow symbolic links

Like other fs fns, add a :follow-links option, default false.

Proposal

Option 2 seems to be the most flexible. Is there any issue with option 2?

Next Steps

Happy to do up a PR after path is chosen.

[lread]

Was playing in the REPL to confirm file-seq behaviour with respect to circular symbolic links:

(fs/delete-tree "foo")

(fs/create-dirs "foo/bar/baz")

(spit "foo/bar/baz/boop.txt" "boop.txt")

(fs/create-sym-link "foo/bar/baz/mylink" "../..")

(take 20 (file-seq (fs/file "foo")))
;; => (#object[java.io.File 0x5532f14a "foo"]
;;     #object[java.io.File 0x5e863815 "foo/bar"]
;;     #object[java.io.File 0x6b1cfc12 "foo/bar/baz"]
;;     #object[java.io.File 0x56c84218 "foo/bar/baz/mylink"]
;;     #object[java.io.File 0x79e4b39e "foo/bar/baz/mylink/bar"]
;;     #object[java.io.File 0x77693ab5 "foo/bar/baz/mylink/bar/baz"]
;;     #object[java.io.File 0x7b19a426 "foo/bar/baz/mylink/bar/baz/mylink"]
;;     #object[java.io.File 0x6f55e1df "foo/bar/baz/mylink/bar/baz/mylink/bar"]
;;     #object[java.io.File 0x33f1d7e8 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz"]
;;     #object[java.io.File 0xd73efb1 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink"]
;;     #object[java.io.File 0x3b7d52ad "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar"]
;;     #object[java.io.File 0x7be60704 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz"]
;;     #object[java.io.File 0x4695d107 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink"]
;;     #object[java.io.File 0x5489b003 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar"]
;;     #object[java.io.File 0x5306805 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz"]
;;     #object[java.io.File 0x4ea59180 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink"]
;;     #object[java.io.File 0x26506cba "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar"]
;;     #object[java.io.File 0x3f40d9db "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz"]
;;     #object[java.io.File 0x39b6fe76 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink"]
;;     #object[java.io.File 0x5bfd4151 "foo/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar/baz/mylink/bar"])

[borkdude]

Option 2 please.

[lread]

Oh: current behaviour is to follow links.
So if :follow-links is false by default, wouldn't this be a breaking change?

Do we want to go with :nofollow-links default false instead?

Reminder: affected fns: modified-since and zip.

Precedent: the fs API fns have a mix of :nofollow-links and :follow-links.

[borkdude]

Maybe nofollow-links and follow-links are inspired by the underlying Java API? Or do the Java APIs always have one of both?

[lread]

no-follow-links

Yeah, I think :no-followlinks is definitely inspired by underlying Java APIs, which seem to follow links by default unless asked not to...

follow-links

...except for walkFileTree. It does not follow links unless asked to.

How does this apply?:

• walk-file-tree used by:
  • copy-tree (without setting walk-file-tree :follow-links) and recognizes :no-follow-links
  • delete-tree (without setting walk-file-tree :follow-links) used by:
    • with-temp-dir
  • match (recognizes and passes along :follow-links to walk-file-tree) used by:
    • glob (recognizes and passes along :follow-links to match.

To summarize my understanding:

• walk-file-tree will not follow links by default, which matches underlying API
• copy-tree implies it will follow links by default
• delete-tree never follows links, must be for safety
• match will not follow links by default, matching the spirit of walk-file-tree
• glob will not follow links by default, matching the spirit of walk-file-tree

So maybe copy-tree is a bit inconsistent?

Ok, back to modified-since and zip

So, ideally, I think modified-since would not have followed links by default to match behaviour of walk-file-tree,glob, and match, but would it be a big deail it does follow links by default? To avoid a breaking change?

And, it would probably be fine for zip to follow behaviour of copy-tree and follow links by default.

Soo...

To avoid breaking changes, while remaining consistent enough with the current API, a default of following links for modified-since and zip should be fine.

But...

I really don't see tests around copy-tree and symbolic links.
I'm not sure what it does wrt to symbolic links.
Since I am justifying new behavior on behavior of copy-tree, do we need a separate issue to explore copy-tree and symbolic links before moving forward with this issue?

[borkdude]

• copy-tree implies it will follow links by default

why does it imply that?

[borkdude]

Just a useful comment maybe in the context of this issue: #144 (comment)

[borkdude]

but would it be a big deail it does follow links by default? To avoid a breaking change?

So in the above comment, @gaverhae says that defaulting to not following symlinks is the least surprising and that following symlinks can bite you. One example of this is circular behavior. So I think, if we can preserve not following symlinks, we should do that.

[lread]

• copy-tree implies it will follow links by default

why does it imply that?

Ah. Yes. copy-tree opts look at :nofollow-links which, to me, implies, that it will follow links by default. But I think this really needs some tests around symbolic links.
For example, initial question: Without understanding more, I found it a bit odd that copy-tree src and dest must be directories (and not symbolic links that point to directories).

I think we need I need to better understand how copy-tree deals with symbolic links. I'll create a separate issue to flesh out tests.

So I think, if we can preserve not following symlinks, we should do that.

When you say "preserve" that tells me you think we are keeping an existing behaviour.
But are we? Isn't bb fs, with the current file-seq (not path-seq), following symbolic links?
Won't it be a breaking change to alter the default behaviour?