Add sesman-follow-symlinks defcustom #13

JAremko · 2018-11-05T02:41:13Z

This works for me with .dir-locals.el:

((nil . ((find-file-visit-truename . nil)
         (vc-follow-symlinks       . nil)
         (sesman-follow-symlinks   . nil))))

JAremko · 2018-11-05T02:42:22Z

sesman.el

+  :group 'sesman
+  :type 'boolean
+  :package-version '(sesman . "0.3.2"))
+(put 'sesman-follow-symlinks 'safe-local-variable (lambda (x) (booleanp x)))


@vspinu I don't see negative repercussions of this.

vspinu · 2018-11-06T13:52:18Z

sesman.el

@@ -72,6 +72,14 @@ sessions running in dependent projects."
  :type 'boolean
  :package-version '(sesman . "0.3.2"))

+(defcustom sesman-follow-symlinks t
+  "If non-nil expand file paths and follow symlinks using `file-truename'.


I am a bit uncomfortable that this goes against the default value of find-file-visit-truename. What are your thoughts regarding this?

@vspinu Agree. But it will be confusing either way. When you use something like ranger it suppresses prompt from vc-follow-symlinks ask so vc fallbacks to vc-follow-symlinks t and since projects are usually under version control the value of find-file-visit-truename doesn't really meter.

I have another idea. I'll update PR.

You are right. I think it's best to keep it simple and following vc here is the right choice. Unless you really have something uniformly better than this simple solution I would rather stick to this boolean setting.

JAremko · 2018-11-06T15:45:13Z

@vspinu How about that ?

JAremko · 2018-11-06T15:55:23Z

Don't know if vc-backend is too slow or not. Mb it also needs caching 🤔

JAremko · 2018-11-06T16:03:45Z

Also it simplifies: .dir-locals.el

((nil . ((find-file-visit-truename . nil)
         (vc-follow-symlinks       . nil))))

I asked Emacs dudes to white-list vc-follow-symlinks https://debbugs.gnu.org/cgi/bugreport.cgi?bug=33264

vspinu · 2018-11-06T16:10:44Z

Not sure. The semantics is considerably more complex. It defaults to nil when not in VC projects and to t within. The users will have to be aware of those two variables and how they interplay for the purpose of sesman. I think the most common use case is to simlink a directory outside of a project and the new setup will fail for that use case. The advantage of the new proposal is that it is consistent with how find-file works and it's not to us to fix that.

Maybe sesman-follow-symlinks could be set to auto in order to achieve this new behavior? I don't like this new -fn. I think if users want to change that -fn they can overwrite or advice sesman-follow-symlink-p directly.

How about the following. Keep both sesman-follow-symlinks and a version of sesman-follow-symlink-p which will be triggered when sesman-follow-symlink is auto? Then users can have the freedom to set all 3 variables as they want and to revert to the old behavior by setting it to t.

I think the vc and file-find variables should be referenced clearly in sesman-follow-symlinks docstring. I have been with emacs for more than 10 years and was not aware of those.

JAremko · 2018-11-06T16:13:05Z

Should auto be default for sesman-follow-symlinks ?

vspinu · 2018-11-06T16:39:10Z

BTW, when set to auto sesman can go a bit further and fallback on the project in the true path if there is no project in symlinked path. But it's getting crazy.

Should auto be default for sesman-follow-symlinks ?

I would think so.

BTW, I think (vc-backend ...) is not representative if vc follows links or not. It follows if the truename is in the VCed dir. For example if you have a link from outside of project into a project file then you will be redirected but vc-backend will return nil on the original path. This behavior would be consistent with what clojure-emacs/cider#2433 was about.

JAremko · 2018-11-06T16:52:09Z

BTW, when set to auto sesman can go a bit further and fallback on the project in the true path if there is no project in symlinked path. But it's getting crazy.

With polylith the both paths have project files. But it's getting kinda too complicated to even thing about it 🤔

JAremko · 2018-11-06T17:02:09Z

BTW, I think (vc-backend ...) is not representative if vc follows links or not. It follows if the truename is in the VCed dir.

Yeah https://github.com/emacs-mirror/emacs/blob/emacs-26/lisp/vc/vc-hooks.el#L824 🤔

And the truename isn't file-truename but expand-file-name 🤔

JAremko · 2018-11-06T17:13:59Z

@vspinu I think this is how they do it 🤔

JAremko · 2018-11-06T18:05:00Z

I'm done

vspinu · 2018-11-06T18:11:25Z

sesman.el

+  :group 'sesman
+  :type '(choice (const :tag "Behave like `find-file'" auto)
+                 (const :tag "Don't fallow symlinks"   nil)
+                 (const :tag "fallow symlinks"         t))


Same here, "follow" and capitalized ;)

vspinu · 2018-11-06T18:14:09Z

sesman.el

+  "This variable determines whether symlinks should be followed.
+nil - Don't fallow symlinks - use `expand-file-name' for expanding file paths.
+t - Fallow symlinks - us `file-truename' for expanding file paths.
+'auto - Don't fallow symlink unless it's under version control and


Fallow -> "follow", "us" -> "use".

vspinu · 2018-11-06T18:21:31Z

sesman.el

@@ -319,6 +332,16 @@ If SORT is non-nil, sort in relevance order."
 (defun sesman--lnk-value (lnk)
  (nth 2 lnk))

+(defun sesman--follow-symlink-p (filename)
+  "FILENAME predicate that tries to predict whether `find-file' will


I am afraid this doesn't do it. With default settings, If am about to open a symlink into a VCed project from outside the VC context I am redirected into the real file, but sesman--follow-symlink-p returns nil on that symlink.

vspinu · 2018-11-06T18:35:20Z

sesman.el

-      (puthash path (file-truename path) sesman--path-cache)))
+  (if (or (eq sesman-follow-symlinks t)
+          (and (eq sesman-follow-symlinks 'auto)
+               (sesman--follow-symlink-p path)))


Here it should be true name instead of path. I think this caching of file-names is not necessary either. There is a buffer local variable buffer-file-truename which could be used instead.

think this caching of file-names is not necessary either.

~~You mean basically remove sesman--expand-path and use something like (file-name-directory buffer-file-truename) instead of (sesman--expand-path default-directory)~~

Got it. Sry multitasking. You mean instead (sesman--expand-path default-directory) do (sesman--expand-path (file-name-directory buffer-file-truename))

Not sure about (sesman--expand-path dir)

Indeed. I meant that that entire caching mechanism of truenames is not needed. I was not aware about buffer-file-truename before.

The sesman-relevant-context-p for project should first sesman--expand-path on buffer true name, then use that as default-directory and call sesman-project and root-dir.

The sesman-relevant-context-p for project should first sesman--expand-path on buffer true name, then use that as default-directory and call sesman-project and root-dir.

@vspinu I don't think that I have sesman expertise required for a refactoring of such scale 😺

Sure. Let me take over from here. This part turned to be much stuffier than expected. Thanks for all the effort!

@vspinu Cool thx 👍

vspinu · 2018-11-06T18:45:21Z

I would really appreciate if you could squeeze a few tests here. Basically 4 cases, VCed -> Non-VCed, VCed -> VCed, Non-VC-ed -> VC-ed, Non-VC-ed-> Non-VCed replicated for two possible values of sesman-follow-symlinks variables.

JAremko · 2018-11-06T20:28:19Z

sesman.el

+                  (vc-backend truename))))))
+
+(defvar sesman--file-truename-cache (make-hash-table :test #'equal))
+(defun sesman--file-truename (filepath)


@vspinu Can we use something like this ?

That dynamic buffer-file-truename will surely screw things up when called with filepath which is not from current buffer.

Didn't know that it can be called in such way... Not a good idea anyway.

JAremko · 2018-11-06T20:29:23Z

sesman.el

+  "Optimizing `file-truename' wrapper."
+  (let ((exp-fp (expand-file-name filepath)))
+   (cond
+    ((string= exp-fp default-directory)


Can default-directory be not expanded?

Yes. Often actually.

@vspinu thx.

JAremko · 2018-11-06T21:29:35Z

sesman.el

+                 (const :tag "Don't follow symlinks"   nil)
+                 (const :tag "Follow symlinks"         t))
+  :package-version '(sesman . "0.3.2"))
+(put 'sesman-follow-symlinks


@vspinu White-listed sesman-follow-symlinks.

Now I'm gone fore real. Good luck and thanks for the package 😄

JAremko · 2018-11-06T22:03:28Z

btw TravisCI reports sesman.el:69: Some lines are over 80 columns wide

Ok. Now I'm gone gone.

vspinu · 2018-11-09T10:52:07Z

I have built on top of this and merged locally. The issues should be fixed. Only vc-follow-symblinks is honored. I think it's simpler and more consistent this way.

JAremko · 2018-11-09T11:22:00Z

That's Great! Thanks @vspinu ❤️

JAremko commented Nov 5, 2018

View reviewed changes

vspinu reviewed Nov 6, 2018

View reviewed changes

vspinu requested changes Nov 6, 2018

View reviewed changes

JAremko commented Nov 6, 2018

View reviewed changes

Add sesman-follow-symlinks defcustom

27ebf84

vspinu closed this Nov 9, 2018

JAremko mentioned this pull request Nov 12, 2018

Add Emacs/CIDER section to readme tengstrand/lein-polylith#19

Merged

Add sesman-follow-symlinks defcustom #13

Add sesman-follow-symlinks defcustom #13

Conversation

JAremko commented Nov 5, 2018

Choose a reason for hiding this comment

vspinu Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAremko commented Nov 6, 2018

JAremko commented Nov 6, 2018 • edited Loading

JAremko commented Nov 6, 2018 • edited Loading

vspinu commented Nov 6, 2018

JAremko commented Nov 6, 2018

vspinu commented Nov 6, 2018

JAremko commented Nov 6, 2018 • edited Loading

JAremko commented Nov 6, 2018

JAremko commented Nov 6, 2018

JAremko commented Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAremko Nov 6, 2018 • edited Loading

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

vspinu commented Nov 6, 2018

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

JAremko commented Nov 6, 2018

vspinu commented Nov 9, 2018

JAremko commented Nov 9, 2018

vspinu Nov 6, 2018 •

edited

Loading

JAremko commented Nov 6, 2018 •

edited

Loading

JAremko commented Nov 6, 2018 •

edited

Loading

JAremko commented Nov 6, 2018 •

edited

Loading

JAremko commented Nov 6, 2018 •

edited

Loading

JAremko Nov 6, 2018 •

edited

Loading