Re: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Bastien b...@gnu.org writes: Ilya Shlyakhter ilya_...@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: [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 b...@gnu.org wrote: Bastien b...@gnu.org writes: Ilya Shlyakhter ilya_...@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: [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
Hi Achim, Achim Gratz strom...@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
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
Achim Gratz strom...@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: [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 strom...@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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Bastien writes: Achim Gratz strom...@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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Achim Gratz strom...@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: [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 strom...@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: [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
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 b...@gnu.org wrote: Achim Gratz strom...@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: [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
Ilya Shlyakhter ilya_...@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: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Applied, thanks! -- Bastien
