[Top][All Lists]

[Date Prev][Date Next][Thread Prev][Thread Next][Date Index][Thread Index]

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

From: Ilya Shlyakhter
Subject: Re: [O] [PATCH] Fixed bug in org-entry-get-with-inheritance
Date: Mon, 17 Mar 2014 17:30:31 -0400

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
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
  :myprop: bbb
*** headline B
    :otherprop:       ccc

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

    : 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

reply via email to

[Prev in Thread] Current Thread [Next in Thread]