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

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

[Drew Adams]

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'

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

[Drew Adams]

> 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.

Also, the doc of `org-store-link-functions' says nothing about the
function being called twice.  It says only:

"Each function should check if it is responsible for creating
 this link (for example by looking at the major mode).
 If not, it must exit and return nil.
 If yes, it should return a non-nil value after a calling
 `org-store-link-props' with a list of properties and values."

(And "a calling" is a typo - it should be "calling".)

This doc is essentially a spec telling you what a function for this
list should do.  From the doc, its purpose is to (a) check whether
it should call `org-store-link-props' and (b) if so, call it and
return non-nil, else return nil (without calling it).

Nothing suggests that the function, if invoked to define a link,
will be invoked twice: once for (a) (even though (b) will also be
done then) and a second time for (b).

---

Also wrt the doc:

The functions on `org-store-link-functions' do not really "create
and store a link", AFAICT.  They merely define a link.  It is
`org-store-link' that uses this definition to create and store a
link.  The most that these functions do is invoke
`org-store-link-props', and that neither creates nor stores a link.

Whether you think of "creating" a link as inserting it somewhere
or just adding it to `org-stored-links' (in which case "creating"
the link is synonymous with "storing" it), the functions on
`org-store-link-functions' do not, themselves, update
`org-stored-links'.  Putting a link on that list is the only thing
that could reasonably be considered "storing" a link, AFAICT.
(Hence the name of function `org-store-link'.)

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

[Nicolas Goaziou]

Hello,

Drew Adams <drew.adams@oracle.com> writes:

>> 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.
>
> Also, the doc of `org-store-link-functions' says nothing about the
> function being called twice.  It says only:
>
> "Each function should check if it is responsible for creating
>  this link (for example by looking at the major mode).
>  If not, it must exit and return nil.
>  If yes, it should return a non-nil value after a calling
>  `org-store-link-props' with a list of properties and values."
>
> (And "a calling" is a typo - it should be "calling".)
>
> This doc is essentially a spec telling you what a function for this
> list should do.  From the doc, its purpose is to (a) check whether
> it should call `org-store-link-props' and (b) if so, call it and
> return non-nil, else return nil (without calling it).
>
> Nothing suggests that the function, if invoked to define a link,
> will be invoked twice: once for (a) (even though (b) will also be
> done then) and a second time for (b).

This function no longer invokes twice the function used to create the
link.

Thank you.

Regrads,

-- 
Nicolas Goaziou                                                0x80A93738

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

[Nicolas Goaziou]

Nicolas Goaziou <mail@nicolasgoaziou.fr> writes:

> Hello,
>
> Drew Adams <drew.adams@oracle.com> writes:
>
>>> 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.
>>
>> Also, the doc of `org-store-link-functions' says nothing about the
>> function being called twice.  It says only:
>>
>> "Each function should check if it is responsible for creating
>>  this link (for example by looking at the major mode).
>>  If not, it must exit and return nil.
>>  If yes, it should return a non-nil value after a calling
>>  `org-store-link-props' with a list of properties and values."
>>
>> (And "a calling" is a typo - it should be "calling".)
>>
>> This doc is essentially a spec telling you what a function for this
>> list should do.  From the doc, its purpose is to (a) check whether
>> it should call `org-store-link-props' and (b) if so, call it and
>> return non-nil, else return nil (without calling it).
>>
>> Nothing suggests that the function, if invoked to define a link,
>> will be invoked twice: once for (a) (even though (b) will also be
>> done then) and a second time for (b).
>
> This function no longer invokes twice the function used to create the
> link.

Closing this bug.