bug#4229: 23.1; wrong-type arg: char or string-p, nil

bug#4229: 23.1; wrong-type arg: char or string-p, nil

[Drew Adams]

emacs -Q
M-x set-variable debug-on-error t
M-x load-file /......./bookmark.el
C-x r l
 
raises error:
 
Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  insert(nil)
  (prog1 (insert (bookmark-location bookmark)) (if (and ... ...)
(add-text-properties start ... ...)))
  (let ((start ...)) (prog1 (insert ...) (if ... ...)))
  bookmark-insert-location("*Messages* buffer list" t)
  (let ((bmrk ...)) (setq bookmark-bmenu-hidden-bookmarks (cons bmrk
bookmark-bmenu-hidden-bookmarks)) (let (...) (move-to-column
bookmark-bmenu-file-column t) (if ... ...)) (delete-region (point) (progn ...
...)) (insert "  ") (bookmark-insert-location bmrk t) (forward-line 1))
  (while (< (point) (point-max)) (let (...) (setq
bookmark-bmenu-hidden-bookmarks ...) (let ... ... ...) (delete-region ... ...)
(insert "  ") (bookmark-insert-location bmrk t) (forward-line 1)))
  (let ((inhibit-read-only t)) (while (< ... ...) (let ... ... ... ... ... ...
...)))
  (save-window-excursion (goto-char (point-min)) (forward-line 2) (setq
bookmark-bmenu-hidden-bookmarks nil) (let (...) (while ... ...)))
  (save-excursion (save-window-excursion (goto-char ...) (forward-line 2) (setq
bookmark-bmenu-hidden-bookmarks nil) (let ... ...)))
  (if (and (not force) bookmark-bmenu-toggle-filenames) nil (save-excursion
(save-window-excursion ... ... ... ...)))
  bookmark-bmenu-show-filenames()
  (cond (show (setq bookmark-bmenu-toggle-filenames nil)
(bookmark-bmenu-show-filenames) (setq bookmark-bmenu-toggle-filenames t))
(bookmark-bmenu-toggle-filenames (bookmark-bmenu-hide-filenames) (setq
bookmark-bmenu-toggle-filenames nil)) (t (bookmark-bmenu-show-filenames) (setq
bookmark-bmenu-toggle-filenames t)))
  bookmark-bmenu-toggle-filenames(t)
  (if bookmark-bmenu-toggle-filenames (bookmark-bmenu-toggle-filenames t))
  bookmark-bmenu-list()
  call-interactively(bookmark-bmenu-list nil nil)
 
 
 
In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'

bug#4250: 23.1; bookmark.el should raise reasonable error for empty filename entry

[Drew Adams]

I have code that allows for bookmarks that have no filename, that is,
where the `filename' entry in `bookmark-alist' is `(filename)'.
 
It would be ideal and most reasonable if vanilla Emacs simply ignored
such a bookmark - tolerance. More generally, it would be great if it would
tolerate any bookmark in `bookmark-alist' that it doesn't recognize.
 
If that is not something you agree about, then at least it should
raise a reasonable error - not just "wrong-type-arg stringp, nil",
whenever a "malformed" bookmark is encountered.
 
The code in `bookmark-default-handler' is an example of the problem.
The local variable `file' is bound to (bookmark-get-filename bmk) -
which returns nil if there is no `filename' entry or the `filename'
entry is `(filename)'.
 
The code then simply calls (file-exists-p nil), via
(bookmark-file-or-variation-thereof nil), raising the wrong-type-arg
error.
 
In the alternative `if' branch, the code calls (find-file-noselect
nil), which has the same problem.
 
In general, it would be great if places where `bookmark-get-filename'
is called tested the result and treated nil by raising a
bookmark-level error.
 
As I said, it would be even better if the bookmark.el code could be
made tolerant of any bookmark whose format it does not
recognize.

bookmark.el already allows for applications and users to add new,
addtional entries. The problem is that it doesn't allow for
unexpected forms of the entries it predefines. It would be nice if it
were tolerant, and simply ignored any such bookmark (no error).
 

In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'
 

bug#4250: 23.1; bookmark.el should raise reasonable error for empty filename entry

[Chong Yidong]

Hi Karl,

Could you take a look at bug#4250 for the bookmark package?  Thanks.

(Drew Adams has filed a few other bookmark.el bugs in the bug tracker,
and if you could look at them as well, that would be good.  Thanks.)

bug#4250: 23.1; bookmark.el should raise reasonable error for empty filename entry

[Karl Fogel]

Chong Yidong <cyd@stupidchicken.com> writes:
> Hi Karl,
>
> Could you take a look at bug#4250 for the bookmark package?  Thanks.
>
> (Drew Adams has filed a few other bookmark.el bugs in the bug tracker,
> and if you could look at them as well, that would be good.  Thanks.)

Yes, I will take a look.  Thank you for bringing it to my attention.

-Karl

bug#4250: 23.1; bookmark.el should raise reasonable error for empty filename entry

[Drew Adams]

> > Could you take a look at bug#4250 for the bookmark package?  Thanks.
> >
> > (Drew Adams has filed a few other bookmark.el bugs in the 
> > bug tracker, and if you could look at them as well, that
> > would be good.  Thanks.)
> 
> Yes, I will take a look.  Thank you for bringing it to my attention.

Thanks. When you can, please take a look also at the bookmark.el bugs filed by
Thierry Volpiatto.

Thx - Drew

bug#4229: marked as done (23.1; wrong-type arg: char or string-p, nil)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 896 bytes --]

Your message dated Mon, 05 Oct 2009 00:26:53 -0400
with message-id <87d452mpo2.fsf@red-bean.com>
and subject line Fix "bookmark.el should raise reasonable error for empty filename entry"
has caused the Emacs bug report #4250,
regarding 23.1; wrong-type arg: char or string-p, nil
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4250: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4250
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 4827 bytes --]

From: "Drew Adams" <drew.adams@oracle.com>
To: <bug-gnu-emacs@gnu.org>
Subject: 23.1; wrong-type arg: char or string-p, nil
Date: Fri, 21 Aug 2009 15:38:22 -0700
Message-ID: <C662C109DC1F4B0F95892D63ADC7A0E3@us.oracle.com>

emacs -Q
M-x set-variable debug-on-error t
M-x load-file /......./bookmark.el
C-x r l
 
raises error:
 
Debugger entered--Lisp error: (wrong-type-argument char-or-string-p nil)
  insert(nil)
  (prog1 (insert (bookmark-location bookmark)) (if (and ... ...)
(add-text-properties start ... ...)))
  (let ((start ...)) (prog1 (insert ...) (if ... ...)))
  bookmark-insert-location("*Messages* buffer list" t)
  (let ((bmrk ...)) (setq bookmark-bmenu-hidden-bookmarks (cons bmrk
bookmark-bmenu-hidden-bookmarks)) (let (...) (move-to-column
bookmark-bmenu-file-column t) (if ... ...)) (delete-region (point) (progn ...
...)) (insert "  ") (bookmark-insert-location bmrk t) (forward-line 1))
  (while (< (point) (point-max)) (let (...) (setq
bookmark-bmenu-hidden-bookmarks ...) (let ... ... ...) (delete-region ... ...)
(insert "  ") (bookmark-insert-location bmrk t) (forward-line 1)))
  (let ((inhibit-read-only t)) (while (< ... ...) (let ... ... ... ... ... ...
...)))
  (save-window-excursion (goto-char (point-min)) (forward-line 2) (setq
bookmark-bmenu-hidden-bookmarks nil) (let (...) (while ... ...)))
  (save-excursion (save-window-excursion (goto-char ...) (forward-line 2) (setq
bookmark-bmenu-hidden-bookmarks nil) (let ... ...)))
  (if (and (not force) bookmark-bmenu-toggle-filenames) nil (save-excursion
(save-window-excursion ... ... ... ...)))
  bookmark-bmenu-show-filenames()
  (cond (show (setq bookmark-bmenu-toggle-filenames nil)
(bookmark-bmenu-show-filenames) (setq bookmark-bmenu-toggle-filenames t))
(bookmark-bmenu-toggle-filenames (bookmark-bmenu-hide-filenames) (setq
bookmark-bmenu-toggle-filenames nil)) (t (bookmark-bmenu-show-filenames) (setq
bookmark-bmenu-toggle-filenames t)))
  bookmark-bmenu-toggle-filenames(t)
  (if bookmark-bmenu-toggle-filenames (bookmark-bmenu-toggle-filenames t))
  bookmark-bmenu-list()
  call-interactively(bookmark-bmenu-list nil nil)
 
 
 
In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'





[-- Attachment #3: Type: message/rfc822, Size: 2889 bytes --]

From: Karl Fogel <kfogel@red-bean.com>
To: 4250-close@emacsbugs.donarmstrong.com
Subject: Fix "bookmark.el should raise reasonable error for empty filename entry"
Date: Mon, 05 Oct 2009 00:26:53 -0400
Message-ID: <87d452mpo2.fsf@red-bean.com>

fixed
--

I think this is fixed now, though I'm not positive it was broken in the
first place.  I've made `bookmark-default-handler' raise a catchable
error on entries that have no filename.

But in general, if you're defining your own types of bookmarks, you
should be defining custom handlers for them.  So when you say "I have
code that allows for bookmarks that have no filename", I'm curious to
see that code.  It's impossible, in the general case, for bookmark.el to
tolerate all entries it does not recognize -- a sufficiently bizarre
entry might not even have discernable boundaries, for example.  We must
depend on custom handlers to handle custom data.

Anyway, here's the commit:

  $ cvs ci -F msg lisp/bookmark.el lisp/ChangeLog
  /sources/emacs/emacs/lisp/bookmark.el,v  <--  lisp/bookmark.el
  new revision: 1.131; previous revision: 1.130
  /sources/emacs/emacs/lisp/ChangeLog,v  <--  lisp/ChangeLog
  new revision: 1.16357; previous revision: 1.16356
  Mailing notification to emacs-diffs@gnu.org... sent.
  $ 

And this is the log message, so you can get an idea of what I did:

  (bookmark-handle-bookmark): If bookmark has no file,
    don't do anything related to relocating, just return nil. 
  (bookmark-error-no-filename): New error.
  (bookmark-default-handler): Signal `bookmark-error-no-filename' if
    bookmark has no file.  Don't even attempt to handle things that
    are not files; the whole point of custom handlers is to keep that
    knowledge elsewhere anyway.  Tighten some comments.
  (bookmark-file-or-variation-thereof): Remove now-unused function.
  (bookmark-location): Doc string fix.

bug#4250: marked as done (23.1; bookmark.el should raise reasonable error for empty filename entry)

[Emacs bug Tracking System]

[-- Attachment #1: Type: text/plain, Size: 925 bytes --]

Your message dated Mon, 05 Oct 2009 00:26:53 -0400
with message-id <87d452mpo2.fsf@red-bean.com>
and subject line Fix "bookmark.el should raise reasonable error for empty filename entry"
has caused the Emacs bug report #4250,
regarding 23.1; bookmark.el should raise reasonable error for empty filename entry
to be marked as done.

This means that you claim that the problem has been dealt with.
If this is not the case it is now your responsibility to reopen the
bug report if necessary, and/or fix the problem forthwith.

(NB: If you are a system administrator and have no idea what this
message is talking about, this may indicate a serious mail system
misconfiguration somewhere. Please contact owner@emacsbugs.donarmstrong.com
immediately.)


-- 
4250: http://emacsbugs.donarmstrong.com/cgi-bin/bugreport.cgi?bug=4250
Emacs Bug Tracking System
Contact owner@emacsbugs.donarmstrong.com with problems

[-- Attachment #2: Type: message/rfc822, Size: 4479 bytes --]

From: "Drew Adams" <drew.adams@oracle.com>
To: <bug-gnu-emacs@gnu.org>
Subject: 23.1; bookmark.el should raise reasonable error for empty filename entry
Date: Mon, 24 Aug 2009 15:14:50 -0700
Message-ID: <2AB5A2C17C4C41FA8ED68F391075B631@us.oracle.com>

I have code that allows for bookmarks that have no filename, that is,
where the `filename' entry in `bookmark-alist' is `(filename)'.
 
It would be ideal and most reasonable if vanilla Emacs simply ignored
such a bookmark - tolerance. More generally, it would be great if it would
tolerate any bookmark in `bookmark-alist' that it doesn't recognize.
 
If that is not something you agree about, then at least it should
raise a reasonable error - not just "wrong-type-arg stringp, nil",
whenever a "malformed" bookmark is encountered.
 
The code in `bookmark-default-handler' is an example of the problem.
The local variable `file' is bound to (bookmark-get-filename bmk) -
which returns nil if there is no `filename' entry or the `filename'
entry is `(filename)'.
 
The code then simply calls (file-exists-p nil), via
(bookmark-file-or-variation-thereof nil), raising the wrong-type-arg
error.
 
In the alternative `if' branch, the code calls (find-file-noselect
nil), which has the same problem.
 
In general, it would be great if places where `bookmark-get-filename'
is called tested the result and treated nil by raising a
bookmark-level error.
 
As I said, it would be even better if the bookmark.el code could be
made tolerant of any bookmark whose format it does not
recognize.

bookmark.el already allows for applications and users to add new,
addtional entries. The problem is that it doesn't allow for
unexpected forms of the entries it predefines. It would be nice if it
were tolerant, and simply ignored any such bookmark (no error).
 

In GNU Emacs 23.1.1 (i386-mingw-nt5.1.2600)
 of 2009-07-29 on SOFT-MJASON
Windowing system distributor `Microsoft Corp.', version 5.1.2600
configured using `configure --with-gcc (4.4)'
 




[-- Attachment #3: Type: message/rfc822, Size: 2889 bytes --]

From: Karl Fogel <kfogel@red-bean.com>
To: 4250-close@emacsbugs.donarmstrong.com
Subject: Fix "bookmark.el should raise reasonable error for empty filename entry"
Date: Mon, 05 Oct 2009 00:26:53 -0400
Message-ID: <87d452mpo2.fsf@red-bean.com>

fixed
--

I think this is fixed now, though I'm not positive it was broken in the
first place.  I've made `bookmark-default-handler' raise a catchable
error on entries that have no filename.

But in general, if you're defining your own types of bookmarks, you
should be defining custom handlers for them.  So when you say "I have
code that allows for bookmarks that have no filename", I'm curious to
see that code.  It's impossible, in the general case, for bookmark.el to
tolerate all entries it does not recognize -- a sufficiently bizarre
entry might not even have discernable boundaries, for example.  We must
depend on custom handlers to handle custom data.

Anyway, here's the commit:

  $ cvs ci -F msg lisp/bookmark.el lisp/ChangeLog
  /sources/emacs/emacs/lisp/bookmark.el,v  <--  lisp/bookmark.el
  new revision: 1.131; previous revision: 1.130
  /sources/emacs/emacs/lisp/ChangeLog,v  <--  lisp/ChangeLog
  new revision: 1.16357; previous revision: 1.16356
  Mailing notification to emacs-diffs@gnu.org... sent.
  $ 

And this is the log message, so you can get an idea of what I did:

  (bookmark-handle-bookmark): If bookmark has no file,
    don't do anything related to relocating, just return nil. 
  (bookmark-error-no-filename): New error.
  (bookmark-default-handler): Signal `bookmark-error-no-filename' if
    bookmark has no file.  Don't even attempt to handle things that
    are not files; the whole point of custom handlers is to keep that
    knowledge elsewhere anyway.  Tighten some comments.
  (bookmark-file-or-variation-thereof): Remove now-unused function.
  (bookmark-location): Doc string fix.