bug#19914: 25.0.50; `org-store-link' invokes function to add to `org-store-link-functions' twice

[Drew Adams <drew.adams@oracle.com>]

I'm no expert on this, so PLEASE CORRECT ME if I'm mistaken.

See this code in `org-store-link':

((and (not (equal arg '(16))) ; Store a link using an external link type
      (setq sfuns
            (delq
             nil (mapcar (lambda (f)
                           (let (fs) (if (funcall f) (push f fs)))) ; #1
                         org-store-link-functions))
            sfunsn (mapcar (lambda (fu) (symbol-name (car fu))) sfuns))
      (or (and (cdr sfuns)
               (funcall (intern
                         (completing-read
                          "Which function for creating the link? "
                          sfunsn nil t (car sfunsn)))))
          (funcall (caar sfuns)))                                   ; #2
      (setq link (plist-get org-store-link-plist :link)
            desc (or (plist-get org-store-link-plist
                                :description) link))))

Consider a function `foo' in `org-store-link-functions'.  It should
return non-nil if it actually does its link-defining thing in the given
context, i.e., if it is applicable there.

In #1 above, it is apparently called merely to test whether it applies.
If so then it is made a member of SFUNS.

Assume that it applies and it is the only such function that applies
(i.e., SFUNS is a singleton), for simplicity.  Then its name is made a
member of SFUNSN.  So `foo' then gets invoked again (#2).

This means that it will have done its thing TWICE: once just to check
whether it should/could do its thing and another time so that it does
its thing.

That is, it seems that the point of the first AND clause is only to
determine which functions in `org-store-link-functions' are to be
candidates for invocation (have their names added to SFUNSN).  And it
seems that the point of the second AND clause is to invoke one of them.

That means that the one that is chosen to be invoked gets invoked twice.
(This is so even if SFUNS is not a singleton.)

Why invoke the chosen function twice?  This could be costly.  Or it
might be annoying, if the function interacts with a user.  Or it might
be incorrect, if the function has side effects.

(And why the dance between the function and its name?  Is that really
needed?  Seems like the name is used only for `completing-read', which
might not even be invoked.)

This logic seems a bit convoluted to me.  That might well be a sign that
I'm missing something - please let me know.

And what is the point of this function, which is mapped over SFUNS:
(lambda (f) (let (fs) (if (funcall f) (push f fs))))

That seems the same as (lambda (f) (and (funcall f) (list f))).

IOW, that seems only to return (list f) if invoking f returns non-nil
(and nil otherwise).  Why bother to `push' to a local variable that is
otherwise unused?  What am I missing?  (This is not important to the
bug, unless I misunderstand completely.)

If I have a function `foo' on `org-store-link-functions', and if that
function does something non-trivial or non-idempotent when its test for
applicability succeeds, then I need to find some way to separate the
part of it that simply tests whether it is applicable (returning non-nil
if yes) from the part that actually defines the link.

How to do that?  I could try to record some state and do one or the
other part alternately, each time `foo' is invoked.  But then I would
need to handle possible interruptions (e.g. C-g) etc. that might throw
things off - getting the recorded state out of sync with the actual
state.  That's fragile and ugly.

This logic seems quite weird to me.  Please let me know what I'm
missing, or whether this is indeed something that needs to be fixed in
`org-store-link'.

And in the latter case, please suggest a reasonable workaround for use
with the pre-bugfix (i.e., the current) logic: how to code a function
`foo' so that the first invocation just returns non-nil when `foo' is
applicable and the second invocation does the actual link definition.
I will need that to let my code to work with pre-bugfix versions of
org.el.

In GNU Emacs 25.0.50.1 (i686-pc-mingw32)
 of 2014-10-20 on LEG570
Bzr revision: 118168 rgm@gnu.org-20141020195941-icp42t8ttcnud09g
Windowing system distributor `Microsoft Corp.', version 6.1.7601
Configured using:
 `configure --enable-checking=yes,glyphs CPPFLAGS=-DGLYPH_DEBUG=1'