Re: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Hi Achim, Achim Gratz 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
Re: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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 wrote:
> Bastien writes:
>
>> Ilya Shlyakhter 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Bastien writes: > Ilya Shlyakhter 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Hi Ilya, Bastien 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Ilya Shlyakhter 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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 wrote: > Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Bastien writes: > Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Achim Gratz 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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Applied, thanks! -- Bastien
[O] [PATCH] Fixed bug in org-entry-get-with-inheritance
>From bea0daf422e9ab8f27addb412aa03456c89d5844 Mon Sep 17 00:00:00 2001 From: Ilya Shlyakhter 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
