Re: [O] [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 <address@hidden> wrote:
> Achim Gratz <address@hidden> 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