Re: proposition about bookmark-buffer-file-name in bookmark.el

[Karl Fogel]

"Toru TSUNEYOSHI" <address@hidden> writes:
> Thanks. I have understood clearly.
>
> The effect is as same as description about `bookmark-buffer-file-name'
> in bookmark.el on Emacs 22.3.1. In short, "shorter and portable".
>
> comment about `bookmark-buffer-file-name' in bookmark.el on Emacs 22.3.1:
> ========================================================================
>    (buffer-file-name
>     ;; Abbreviate the path, both so it's shorter and so it's more
>     ;; portable.  E.g., the user's home dir might be a different
>     ;; path on different machines, but "~/" will still reach it.
>     (abbreviate-file-name buffer-file-name))
> ========================================================================

Thanks, I've looked at the patch now.  A few comments:

   1. It applies to some earlier version of bookmark.el, but not to the
      current version.  You can get the current version here:

      
http://cvs.savannah.gnu.org/viewvc/emacs/lisp/bookmark.el?root=emacs&view=markup

   2. In the current version, I think there is no need to abbreviate the
      file name in `bookmark-buffer-file-name', as
      `bookmark-buffer-name' uses `file-name-nondirectory' anyway.  See
      the code path starting from `bookmark-set'.  So, there seems to be
      no behavior difference with the patch applied (at least as I tested).

Regarding your patch itself, a couple of comments:

   --- bookmark.el      2008-01-07 11:45:03.000000000 +0900
   +++ bookmark.el      2008-11-19 20:45:26.331974400 +0900
   @@ -1007,18 +1007,20 @@
    (defun bookmark-buffer-file-name ()
      "Return the current buffer's file in a way useful for bookmarks.
    For example, if this is a Info buffer, return the Info file's name."
   -  (cond
   -   ((eq major-mode 'Info-mode)
   -    Info-current-file)
   -   (buffer-file-name
   -    ;; Abbreviate the path, both so it's shorter and so it's more
   -    ;; portable.  E.g., the user's home dir might be a different
   -    ;; path on different machines, but "~/" will still reach it.
   -    (abbreviate-file-name buffer-file-name))
   -   ((and (boundp 'dired-directory) dired-directory)
   -    (if (stringp dired-directory)
   -        dired-directory
   -      (car dired-directory)))))
   +  (let (file)
   +    (setq file (cond
   +            ((eq major-mode 'Info-mode)
   +             Info-current-file)
   +            (buffer-file-name
   +             ;; Abbreviate the path, both so it's shorter and so it's more
   +             ;; portable.  E.g., the user's home dir might be a different
   +             ;; path on different machines, but "~/" will still reach it.
   +             buffer-file-name)
   +            ((and (boundp 'dired-directory) dired-directory)
   +             (if (stringp dired-directory)
   +                 dired-directory
   +               (car dired-directory)))))
   +    (and file (abbreviate-file-name file))))

First, instead of

   (let (file)
     (setq file ...))

it's more direct to just do this:

   (let ((file ...))

Second, when moving the `abbreviate-file-name' call, we should move the
comment that goes with it too.

However, in the new code, the `cond' structure falls through to an error
now.  That means there is no need for a `let' at all.  Here is a revised
version of the patch, for the current bookmark.el:

   2008-11-21  Karl Fogel  <address@hidden>
   
        * bookmark.el (bookmark-buffer-file-name): Abbreviate the result
        unconditionally.
        Suggested by Toru TSUNEYOSHI <t_tuneyosi {_AT_} hotmail.com>.
   
   Index: bookmark.el
   ===================================================================
   RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
   retrieving revision 1.120
   diff -u -r1.120 bookmark.el
   --- bookmark.el      21 Nov 2008 10:32:41 -0000      1.120
   +++ bookmark.el      21 Nov 2008 16:37:30 -0000
   @@ -888,18 +888,17 @@
    
    (defun bookmark-buffer-file-name ()
      "Return the current buffer's file in a way useful for bookmarks."
   -  (cond
   -   (buffer-file-name
   -    ;; Abbreviate the path, both so it's shorter and so it's more
   -    ;; portable.  E.g., the user's home dir might be a different
   -    ;; path on different machines, but "~/" will still reach it.
   -    (abbreviate-file-name buffer-file-name))
   -   ((and (boundp 'dired-directory) dired-directory)
   -    (if (stringp dired-directory)
   -        dired-directory
   -      (car dired-directory)))
   -   (t (error "Buffer not visiting a file or directory"))))
   -
   +  ;; Abbreviate the path, both so it's shorter and so it's more
   +  ;; portable.  E.g., the user's home dir might be a different
   +  ;; path on different machines, but "~/" will still reach it.
   +  (abbreviate-file-name 
   +   (cond
   +    (buffer-file-name buffer-file-name)
   +    ((and (boundp 'dired-directory) dired-directory)
   +     (if (stringp dired-directory)
   +         dired-directory
   +       (car dired-directory)))
   +    (t (error "Buffer not visiting a file or directory")))))
    
    (defun bookmark-maybe-load-default-file ()
      (and (not bookmarks-already-loaded)

The above is just for demonstration.  Again, I don't believe the patch
is necessary, but maybe you will point out some case I didn't think of.

Thank you,
-Karl