[PATCH] Fixed bug in org-entry-get-with-inheritance

[PATCH] Fixed bug in org-entry-get-with-inheritance

[Ilya Shlyakhter]

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



[-- Attachment #2: 0001-Properties-Fix-property-getting-with-inheritance.patch --]
[-- Type: text/plain, Size: 2755 bytes --]

From bea0daf422e9ab8f27addb412aa03456c89d5844 Mon Sep 17 00:00:00 2001
From: Ilya Shlyakhter <ilya_shl@alum.mit.edu>
Date: Fri, 7 Mar 2014 01:09:13 -0500
Subject: [PATCH] Properties: Fix property-getting with inheritance

* lisp/org.el (org-entry-get-with-inheritance): Temporarily clear
org-file-properties, org-global-properties and org-global-properties-fixed
before calling org-entry-get on entries up the hierarchy from the queried entry.

Problem was that when org-entry-get-with-inheritance went up the hierarchy of
entries from a given entry, checking whether the property has been set in any of
the entries, it was calling org-entry-get, which always looks at file-scope and
global-scope properties.  So if our property was set file-wide or system-wide,
and somewhere up the hierarchy there was an entry which set some properties _other_
than the one we're looking up but did not set ours, org-entry-get would fill in
the global property value and report that our property was in fact set in that
entry.  The search would stop, and if the property was actually set further up
the hierarchy (which should override file-wide or system-wide settings), we
would never get to that up-the-hierarchy setting.

Illustration of fixed problem:

#+PROPERTY: myprop aaa
* headline A
	:PROPERTIES:
	:myprop: bbb
	:END:
*** headline B
		:PROPERTIES:
		:otherprop:       ccc
		:END:

    #+BEGIN_SRC emacs-lisp
    (message (org-entry-get-with-inheritance "myprop"))
    #+END_SRC

    #+RESULTS:
    : aaa

		Result should be bbb, which it is after the fix.
---
 lisp/org.el | 17 +++++++++--------
 1 file changed, 9 insertions(+), 8 deletions(-)

diff --git a/lisp/org.el b/lisp/org.el
index 6f4b853..cd2a1c6 100644
--- a/lisp/org.el
+++ b/lisp/org.el
@@ -15548,14 +15548,15 @@ However, if LITERAL-NIL is set, return the string value \"nil\" instead."
       (save-restriction
 	(widen)
 	(catch 'ex
-	  (while t
-	    (when (setq tmp (org-entry-get nil property nil 'literal-nil))
-	      (or (ignore-errors (org-back-to-heading t))
-		  (goto-char (point-min)))
-	      (move-marker org-entry-property-inherited-from (point))
-	      (throw 'ex tmp))
-	    (or (ignore-errors (org-up-heading-safe))
-		(throw 'ex nil))))))
+	  (let (org-file-properties org-global-properties org-global-properties-fixed)
+	    (while t
+	      (when (setq tmp (org-entry-get nil property nil 'literal-nil))
+		(or (ignore-errors (org-back-to-heading t))
+		    (goto-char (point-min)))
+		(move-marker org-entry-property-inherited-from (point))
+		(throw 'ex tmp))
+	      (or (ignore-errors (org-up-heading-safe))
+		  (throw 'ex nil)))))))
     (setq tmp (or tmp
 		  (cdr (assoc property org-file-properties))
 		  (cdr (assoc property org-global-properties))
-- 
1.8.4

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Applied, thanks!

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Bastien writes:
> Applied, thanks!

That badly breaks the following tests:

   FAILED  test-ob-header-arg-defaults/tree/accumulate/call
   FAILED  test-ob-header-arg-defaults/tree/accumulate/noweb
   FAILED  test-ob-header-arg-defaults/tree/complex/call
   FAILED  test-ob-header-arg-defaults/tree/complex/noweb
   FAILED  test-org-property-accumulation-append-use
   FAILED  test-org-property-accumulation-append-val

Please revert.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Samples for the Waldorf Blofeld:
http://Synth.Stromeko.net/Downloads.html#BlofeldSamplesExtra

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Achim Gratz <Stromeko@nexgo.de> writes:

> Bastien writes:
>> Applied, thanks!
>
> That badly breaks the following tests:
>
>    FAILED  test-ob-header-arg-defaults/tree/accumulate/call
>    FAILED  test-ob-header-arg-defaults/tree/accumulate/noweb
>    FAILED  test-ob-header-arg-defaults/tree/complex/call
>    FAILED  test-ob-header-arg-defaults/tree/complex/noweb
>    FAILED  test-org-property-accumulation-append-use
>    FAILED  test-org-property-accumulation-append-val

Can you tell a bit more about what's wrong with the test?

If the patch is good and the tests are outdated, I'd rather
fix the tests than revert the patch to re-revert it again.

Thanks,

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Bastien writes:
> Can you tell a bit more about what's wrong with the test?

There is nothing wrong with those tests.

> If the patch is good and the tests are outdated, I'd rather
> fix the tests than revert the patch to re-revert it again.

No, the patch is bad, otherwise it wouldn't break the tests.  It may
well be that there'd be more tests needed to catch that bug the patch
supposedly fixes.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf rackAttack V1.04R1:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Achim Gratz <Stromeko@nexgo.de> writes:

>> Can you tell a bit more about what's wrong with the test?
>
> There is nothing wrong with those tests.

I meant: can you tell me how the tests fail?

I'm interested in the answer.

>> If the patch is good and the tests are outdated, I'd rather
>> fix the tests than revert the patch to re-revert it again.
>
> No, the patch is bad, otherwise it wouldn't break the tests.

Sorry, I don't buy this.

Or maybe explain me why the patch is wrong.

Thanks,

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Bastien writes:
> Achim Gratz <Stromeko@nexgo.de> writes:
>
>>> Can you tell a bit more about what's wrong with the test?
>>
>> There is nothing wrong with those tests.
>
> I meant: can you tell me how the tests fail?

They don't produce the result they are supposed to produce.

> I'm interested in the answer.

make BTEST_RE='\\(header-arg-defaults\\|property-accumulation\\)' test-dirty

>>> If the patch is good and the tests are outdated, I'd rather
>>> fix the tests than revert the patch to re-revert it again.
>>
>> No, the patch is bad, otherwise it wouldn't break the tests.
>
> Sorry, I don't buy this.

I'm not selling anything.

> Or maybe explain me why the patch is wrong.

It breaks property inheritance and accumulation.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptations for Waldorf Q V3.00R3 and Q+ V3.54R2:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Achim Gratz <Stromeko@nexgo.de> writes:

>> I meant: can you tell me how the tests fail?
>
> They don't produce the result they are supposed to produce.

Thanks for this explanation.

>> I'm interested in the answer.
>
> make BTEST_RE='\\(header-arg-defaults\\|property-accumulation\\)'
> test-dirty

Thanks!

>>>> If the patch is good and the tests are outdated, I'd rather
>>>> fix the tests than revert the patch to re-revert it again.
>>>
>>> No, the patch is bad, otherwise it wouldn't break the tests.
>>
>> Sorry, I don't buy this.
>
> I'm not selling anything.

What I meant is this: broken tests are not a sufficient reason to
revert a commit.  You need to show the commit is wrong and the tests
are not outdated.

In this case, I made the error of reproduce Ilya's solution,
not Ilya's problem, so I wrong assumed his patch was the problem
to his problem.

Ilya: from the maint and master branch, I get "bbb" as a result
for the example you placed in your commit message.  Do you have
"aaa" as a result with Org from maint or master?  If so, can you
provide a recipe?

Thanks,

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Bastien writes:
> What I meant is this: broken tests are not a sufficient reason to
> revert a commit.  You need to show the commit is wrong and the tests
> are not outdated.

No code breaking a test should have been committed in the first place,
then we wouldn't need to have this discussion.  If the "tests are
outdated" (in this case they ensure that the documentation and
implementation are matching up which is difficult to ascertain by just
playing with a single example, so that shouldn't ever happen), the onus
is on the committer to explain why and change the tests and
documentation accordingly (either as a preparation for that commit or in
the same commit).


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

SD adaptation for Waldorf Blofeld V1.15B11:
http://Synth.Stromeko.net/Downloads.html#WaldorfSDada

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Achim Gratz <Stromeko@nexgo.de> writes:

> Bastien writes:
>> What I meant is this: broken tests are not a sufficient reason to
>> revert a commit.  You need to show the commit is wrong and the tests
>> are not outdated.
>
> No code breaking a test should have been committed in the first place,
> then we wouldn't need to have this discussion.  If the "tests are
> outdated" (in this case they ensure that the documentation and
> implementation are matching up which is difficult to ascertain by just
> playing with a single example, so that shouldn't ever happen), the onus
> is on the committer to explain why and change the tests and
> documentation accordingly (either as a preparation for that commit or in
> the same commit).

My assumption is that every committer does its best and that we
collectively try to help each other.

That's another way not to have this discussion.

I'm fine fixing bugs and tests introduced by someone else.

After all, I've answered hundreds of emails by people who are
confused by the new way of setting Org's version (which I will
revisit soon.)

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Ilya Shlyakhter]

In the current master branch, doing the example from the patch
(reproduced below again) gives "aaa", because the line
          (let (org-file-properties org-global-properties
org-global-properties-fixed)
has been removed from org-entry-get-with-inheritance .
I agree that patching a function as core as this should be done with
care; however, I'm pretty positive that there was a bug, as explained
in the patch message.
org-entry-get-with-inheritance calls org-entry-get for each entry
going up the tree, to read the property at that entry _without_
inheritance; however, unless
the let line above is included, this reading actually happens _with_
inheritance -- of global properties.  So a property can appear to be
set at a node during
up-the-tree traversal, when in fact it is only set as a global
property.   If org-entry-get-with-inheritance see a property set at a
node during up-the-tree traversal,
it stops the traversal right there, ignoring any settings of this
property further up the tree -- which should override any global
settings of the same property.

Here is the test case again:

#+PROPERTY: myprop aaa
* headline A
  :PROPERTIES:
  :myprop: bbb
  :END:
*** headline B
    :PROPERTIES:
    :otherprop:       ccc
    :END:

    #+BEGIN_SRC emacs-lisp
    (message (org-entry-get-with-inheritance "myprop"))
    #}

    #+RESULTS:
    : aaa



On Mon, Mar 17, 2014 at 4:35 PM, Bastien <bzg@gnu.org> wrote:
> Achim Gratz <Stromeko@nexgo.de> writes:
>
>>> I meant: can you tell me how the tests fail?
>>
>> They don't produce the result they are supposed to produce.
>
> Thanks for this explanation.
>
>>> I'm interested in the answer.
>>
>> make BTEST_RE='\\(header-arg-defaults\\|property-accumulation\\)'
>> test-dirty
>
> Thanks!
>
>>>>> If the patch is good and the tests are outdated, I'd rather
>>>>> fix the tests than revert the patch to re-revert it again.
>>>>
>>>> No, the patch is bad, otherwise it wouldn't break the tests.
>>>
>>> Sorry, I don't buy this.
>>
>> I'm not selling anything.
>
> What I meant is this: broken tests are not a sufficient reason to
> revert a commit.  You need to show the commit is wrong and the tests
> are not outdated.
>
> In this case, I made the error of reproduce Ilya's solution,
> not Ilya's problem, so I wrong assumed his patch was the problem
> to his problem.
>
> Ilya: from the maint and master branch, I get "bbb" as a result
> for the example you placed in your commit message.  Do you have
> "aaa" as a result with Org from maint or master?  If so, can you
> provide a recipe?
>
> Thanks,
>
> --
>  Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Ilya Shlyakhter writes:
> Here is the test case again:

The test case doesn't work as posted.  A working test case produces the
result shown below (with and without your patch reverted) on current
master (tested again via make vanilla just to be sure).

--8<---------------cut here---------------start------------->8---
#+PROPERTY: myprop aaa
* headline A
  :PROPERTIES:
  :myprop: bbb
  :END:
*** headline B
    :PROPERTIES:
    :otherprop:       ccc
    :END:

#+BEGIN_SRC emacs-lisp
    (message (org-entry-get-with-inheritance "myprop"))
#+END_SRC

#+RESULTS:
: bbb
--8<---------------cut here---------------end--------------->8---

So what problem is your patch supposed to fix?


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Factory and User Sound Singles for Waldorf rackAttack:
http://Synth.Stromeko.net/Downloads.html#WaldorfSounds

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Ilya Shlyakhter]

On 3/17/14 5:43 PM, Achim Gratz wrote:
> The test case doesn't work as posted.  A working test case produces the

Try http://ilya.cc/testcase.org

When I open emacs with this file, move to the emacs-lisp block, and 
evaluate it, I get "aaa".

I could easily be wrong re: the logic of the code, but can you help me 
see where I'm wrong in the explanation of the patch?

Thanks,

ilya

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Ilya Shlyakhter <ilya_shl@alum.mit.edu> writes:

> When I open emacs with this file, move to the emacs-lisp block, and
> evaluate it, I get "aaa".

I can reproduce your problem now, I'm on it, and the problem is real,
but I need to make sure all tests pass fine before fixing this.

Thanks,

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Hi Ilya,

Bastien <bzg@gnu.org> writes:

> I can reproduce your problem now, I'm on it, and the problem is real,
> but I need to make sure all tests pass fine before fixing this.

Don't expect a fix anytime soon -- things are complex here and I need
a few hours in a raw to fix this.

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Bastien <bzg@gnu.org> writes:

> Ilya Shlyakhter <ilya_shl@alum.mit.edu> writes:
>
>> When I open emacs with this file, move to the emacs-lisp block, and
>> evaluate it, I get "aaa".
>
> I can reproduce your problem now, I'm on it, and the problem is real,
> but I need to make sure all tests pass fine before fixing this.

Okay, so I committed a different fix in maint and master.

Please test it and let me know.

Thanks,

PS: You may want to read the commit message:
http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=42ee862d

-- 
 Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Ilya Shlyakhter]

Thanks Bastien.

Property API documentation could be made more precise in some places.
From the documentation of org-entry-get (both the docstring and the
Org manual), it would seem that unless the inherit argument is
non-nil, file-wide and system-wide property settings should not be
checked at all?   The Org manual seems especially clear: "By default,
this only looks at properties defined locally in the entry."
Logically, it makes sense to think of file-wide properties as being on
an implicit "level 0" headline under which all headlines in the file
are grouped, and system-wide properties as being on an implicit "level
-1" headline under which all level-0 headlines are grouped.
But, existing code and Org files might rely on org-entry-get without
inheritance considering file-wide and system-wide properties?

Documentation of the variable org-use-property-inheritance also can be
read to mean that it controls the inheritance of file-wide and
system-wide properties, though currently it does not ("When nil, only
the properties directly given in the current entry count.")

 For org-entry-properties, it says "Get all properties of the entry"
-- but it returns only properties explicitly defined at the entry, not
anything inherited from up the hierarchy or file-wide or system-wide,
right?   It also says, "Keys may occur multiple times if the property
key was used several times." -- but the manual also says that "a
property can only have one entry per Drawer".

Also, #+PROPERTY: lines have an effect no matter where they are in the
file, but what happens when several of them set the same property
isn't fully specified; I assume they're processed top-to-bottom, and
later settings overwrite earlier ones, just as later prop+ settings
append to earlier ones?  (So in this sense it does matter where in the
file a #+PROPERTY line is).  It might also be useful to clarify that
even if a subtree is archived, global property settings in that
subtree continue to have an effect (because they aren't really in a
subtree).

Manual says that #+PROPERTY lines specify "properties that can be
inherited by any entry in a file"; more precise would be that they
specify "property settings inherited by every entry"?  Likewise for
the variable org-global-properties.

On Tue, Mar 18, 2014 at 11:14 AM, Bastien <bzg@gnu.org> wrote:
> Bastien <bzg@gnu.org> writes:
>
>> Ilya Shlyakhter <ilya_shl@alum.mit.edu> writes:
>>
>>> When I open emacs with this file, move to the emacs-lisp block, and
>>> evaluate it, I get "aaa".
>>
>> I can reproduce your problem now, I'm on it, and the problem is real,
>> but I need to make sure all tests pass fine before fixing this.
>
> Okay, so I committed a different fix in maint and master.
>
> Please test it and let me know.
>
> Thanks,
>
> PS: You may want to read the commit message:
> http://orgmode.org/cgit.cgi/org-mode.git/commit/?id=42ee862d
>
> --
>  Bastien

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Achim Gratz]

Bastien writes:
> Okay, so I committed a different fix in maint and master.

That fix (and the explanation) makes much more sense… I'll have to see
that this also gets tested, but I won't get to it for some time,
unfortunately.


Regards,
Achim.
-- 
+<[Q+ Matrix-12 WAVE#46+305 Neuron microQkb Andromeda XTk Blofeld]>+

Wavetables for the Terratec KOMPLEXER:
http://Synth.Stromeko.net/Downloads.html#KomplexerWaves

Re: [PATCH] Fixed bug in org-entry-get-with-inheritance

[Bastien]

Hi Achim,

Achim Gratz <Stromeko@nexgo.de> writes:

> Bastien writes:
>> Okay, so I committed a different fix in maint and master.
>
> That fix (and the explanation) makes much more sense… I'll have to see
> that this also gets tested, but I won't get to it for some time,
> unfortunately.

I'll first fix the documentation, as Ilya suggested, then it will be
easier to design the tests.

-- 
 Bastien