bug#4819: file-truename's undocumented behavior

[MON KEY]

On Tue, Oct 27, 2009 at 9:52 PM, Stefan Monnier
<monnier@iro.umontreal.ca> wrote:
>> This form returns a value.
>> (file-truename "")
>
>> WHY?
>
> Why not?  What value did you expect?
I assumed nil.
I'm not sure what I would expect esp. having since noted that:

(file-relative-name "")     ;=> "."
(file-exists-p "")          ;=> t
(file-readable-p "")        ;=> t
(file-directory-p "")       ;=> t
(file-name-as-directory "") ;=> "./"

However, discarding the state of my "least surprisedness", I would _not_ expect
that file-truename would step all over the match-data, and where it does so not
before first:

a) Using string-match-p where applicable;

b) Noting that it does so in the documention. i.e. as per `split-string'.

>
>> I just spent 2 1/2 hours in a break-loop three functions away trying
>> to debug _undocumented_ behavior.
>
> Which part of the documentation do you think this behavior contradicts?
>

This part:
 (file-name-absolute-p "") ;=> nil
 (file-symlink-p "")       ;=> nil

On which systems/platforms does "" denote an absolute filename?
On which systems/platforms does "" denote a symbolic link for a filename?

,---- (documentation 'file-truename)
| Return the truename of filename, which should be absolute.
| The truename of a file name is found by chasing symbolic links
| both at the level of the file and at the level of the directories
| containing it, until no links are left at any level.
`----

And, this other part where documentation:

a) Neglects to mention that this function invokes repeatedly invokes
   `string-match' twice per invokation.

{...} (unless (string-match "[[*?]" filename)
      (string-match "~[^/]*/?" filename)) {...}

{...} ((and (string= (substring filename 0 1) "~")
       (string-match "~[^/]*/?" filename)) {...}

b) Neglects to mention that the remaining args COUNTER and PREV-DIRS are
   iterative counters for operations on recursive calls.  Which means that where
   file-truename recurses `string-match' may be invoked more than twice times.

   ,---- Comments in the definition of `file-truename' in files.el
   |;; Don't document the optional arguments.
   |;; COUNTER and PREV-DIRS are only used in recursive calls.
   |;; COUNTER can be a cons cell whose car is the count of how many
   |;; more links to chase before getting an error.
   |;; PREV-DIRS can be a cons cell whose car is an alist
   |;; of truenames we've just recently computed.
   `----
It was only as a late afterthought that I realized that it wasn't _MY-CODE+
clobbering the match-data - as is usual : ) but maybe Emacs'. The realization
was elusive because the recursion only happens when the w32 conditional which
drops into a recursion predicated on the return value of `w32-long-file-name'.

{...} (setq filename (concat (file-truename rest) missing)) {...}

Why not mention in the docs that on w32 `w32-long-file-name' may be a more
suitable alternative esp. as it is a primitive and as it will expand "8.3 DOS"
short name aliases in the process. (Again, per _existing_ comments in body of
`file-truename's definition).

I understand _why_ the optional args have been left undocumented - they are
essentially hacks which the user shouldn't rely on. However, I don't understand
_why_ the w32 hack isn't made known esp. where the hack is applicable for user
code and is indicated as the preferred solution...

W/re the string-match vs. string-matchp doesn't the following accomplish the
same:
;;; ==============================

*** /files.el-p 2009-10-28 15:49:38.843750000 -0400
--- /emacs/lisp/files.el        2009-06-30 15:51:22.000000000 -0400
***************
*** 893,904 ****
         (setq filename (expand-file-name filename))
         (if (string= filename "")
             (setq filename "/")))
!         ((and (string= (substring filename 0 1) "~")
!               (string-match-p "~[^/]*/?" filename))
!          (string-match "~[^/]*/?" filename)
!          (let ((first-part
!                 (substring filename 0 (match-end 0)))
!                (rest (substring filename (match-end 0))))     
           (setq filename (concat (expand-file-name first-part) rest)))))

    (or counter (setq counter (list 100)))
--- 893,903 ----
         (setq filename (expand-file-name filename))
         (if (string= filename "")
             (setq filename "/")))
!       ((and (string= (substring filename 0 1) "~")
!             (string-match "~[^/]*/?" filename))
!        (let ((first-part
!               (substring filename 0 (match-end 0)))
!              (rest (substring filename (match-end 0))))
           (setq filename (concat (expand-file-name first-part) rest)))))

    (or counter (setq counter (list 100)))
***************
*** 930,936 ****
        (if handler
            (setq filename (funcall handler 'file-truename filename))
          ;; If filename contains a wildcard, newname will be the old name.
!         (unless (string-match-p "[[*?]" filename)
            ;; If filename exists, use the long name.  If it doesn't exist,
              ;; drill down until we find a directory that exists, and use
              ;; the long name of that, with the extra non-existent path
--- 929,935 ----
        (if handler
            (setq filename (funcall handler 'file-truename filename))
          ;; If filename contains a wildcard, newname will be the old name.
!         (unless (string-match "[[*?]" filename)
            ;; If filename exists, use the long name.  If it doesn't exist,
              ;; drill down until we find a directory that exists, and use
              ;; the long name of that, with the extra non-existent path

diff.files.el
Description: Binary data