Re: [Emacs-diffs] Changes to emacs/lisp/bookmark.el,v

[Karl Fogel <kfogel@red-bean.com>]

Regarding this change:

Chong Yidong <cyd@stupidchicken.com> writes:
> [[[
> (bookmark-get-bookmark-record): Signal error for invalid bookmark.
> ]]]
>
> Index: bookmark.el
> ===================================================================
> RCS file: /sources/emacs/emacs/lisp/bookmark.el,v
> retrieving revision 1.117
> retrieving revision 1.118
> diff -u -b -r1.117 -r1.118
> --- bookmark.el	29 Oct 2008 17:42:49 -0000	1.117
> +++ bookmark.el	29 Oct 2008 18:22:12 -0000	1.118
> @@ -330,7 +330,8 @@
>  (defun bookmark-get-bookmark-record (bookmark)
>    "Return the guts of the entry for BOOKMARK in `bookmark-alist'.
>  That is, all information but the name."
> -  (let ((alist (cdr (bookmark-get-bookmark bookmark))))
> +  (let ((alist (cdr (or (bookmark-get-bookmark bookmark)
> +			(error "Invalid bookmark %s" bookmark)))))
>      ;; The bookmark objects can either look like (NAME ALIST) or
>      ;; (NAME . ALIST), so we have to distinguish the two here.
>      (if (and (null (cdr alist)) (consp (caar alist)))

Seeing this change made me ask: "Why doesn't `bookmark-get-bookmark'
just return error itself, if no such bookmark?"

Answer: "Because some callers might expect it to return nil in that
case, rather than error."  (Even though it is not documented to do
either -- its doc string is silent on this question!)  I think there are
only two callers who would care, though.  In `bookmark-store', we have
this:

    (if (and (bookmark-get-bookmark stripped-name) (not no-overwrite))
        ;; already existing bookmark under that name and
        ;; no prefix arg means just overwrite old bookmark
        ;; Use the new (NAME . ALIST) format.
        (setcdr (bookmark-get-bookmark stripped-name) alist)
      ;; otherwise just cons it onto the front (either the bookmark
      ;; doesn't exist already, or there is no prefix arg.  In either
      ;; case, we want the new bookmark consed onto the alist...)
      (push (cons stripped-name alist) bookmark-alist))

And `bookmark-delete', this:

    (or (bookmark-get-bookmark bookmark-current-bookmark)
        (setq bookmark-current-bookmark nil)))

I'm tempted to add an optional NOERROR parameter to
`bookmark-get-bookmark'.  That way we'd many more error checks for free,
and could remove the special-case error check added in Chong Yidong's
change above.

Any objections to that?

As an aside: it's very difficult to review & respond to commits to
Emacs, because the ChangeLog entry arrives in a separate email from the
diff.  If the change package were kept intact (i.e., arrived as one
email, the way every other project in the universe does it), we'd
probably get more review here.

-Karl