Re: svn commit: r20991 - trunk/contrib/hook-scripts

[Bhuvaneswaran Arumugam <bh...@collab.net>]

> -            if(childNode.nodeName == 'author'):
> -                self.feed.insertBefore(item, childNode.nextSibling)
> -            elif(childNode.nodeName == 'entry'):
> +            if childNode.nodeName == 'entry':
> +                if total == 0:
> +                    self.feed.insertBefore(item, childNode)
> +                    total += 1
>                  total += 1

Weird! We increment the "total" twice when the value is 0. Result: We
manage to store max_items-1 items in the feed file. I bet, you intend to
remove the increment statement which resides inside the "if total == 0:"
loop :)

> --        if (total > self.max_items):
> -            self.feed.removeChild(self.feed.lastChild)
> +                if total > self.max_items:
> +                    self.feed.removeChild(childNode)

Here again, why do we wish to include the above "if" condition for every
childNode ? We call this function for every revision, so it's good to
place it outside the "for" loop. Moreover, in the current state it'll
not remove multiple nodes (if you intend to), since we increment the
"total" one at a time.

-- 
Regards,
Bhuvaneswaran

Re: svn commit: r20991 - trunk/contrib/hook-scripts

[Bhuvaneswaran Arumugam <bh...@collab.net>]

> > Nope! Here, the loop is executed separately for the item we insert using
> > "insertBefore" function and it does not bother to iterate over a
> > childNodes collection whilst changing it. In current state, when we run
> > the script as below, it inserts only 9 items instead of 10 items. 
> > 
> > $ ./svn2feed.py -F atom -u .. -U .. -r1:10 /path/to/repos/ -m 10
> > # remove the feed and pickle files before you run the script.
> > 
> > In this case, a temporary print statement for "total" variable explains
> > that the loop is iterated 10 times (while processing last revision) and
> > the "total" variable is set as 11, so, it removes an item
> > unnecessarily.
> 
> Sorry, but *NO*, your analysis is wrong.
> 
> The loop executes zero times for the inserted item, but *twice* for the 
> item after the insert point. We ARE changing the childNodes collection 
> whilst iterating over it. As a result, the iteration is responding in 
> bizarre ways, because the iterator apparently was not designed to be 
> capable of responding sensibly to changes in the collection it is 
> iterating over.

Ok! I'll leave it here. It could be because i'm unable to express what i
found or i'm unable to understand your views!

> > My point is, we call this function for every revision. So, it's fine to
> > remove one item at a time. Moreover, i'm certain the condition is met
> > only when processing the last element, so, why we need to check the
> > condition for all child nodes.
> 
> You need to also consider the case of a user decreasing the --max-items 
> value in a subsequent invocation of the script..

Yep!
-- 
Regards,
Bhuvaneswaran

Re: svn commit: r20991 - trunk/contrib/hook-scripts

[Max Bowsher <ma...@ukf.net>]

Bhuvaneswaran Arumugam wrote:
>>>> -            if(childNode.nodeName == 'author'):
>>>> -                self.feed.insertBefore(item, childNode.nextSibling)
>>>> -            elif(childNode.nodeName == 'entry'):
>>>> +            if childNode.nodeName == 'entry':
>>>> +                if total == 0:
>>>> +                    self.feed.insertBefore(item, childNode)
>>>> +                    total += 1
>>>>                  total += 1
>>> Weird! We increment the "total" twice when the value is 0. Result: We
>>> manage to store max_items-1 items in the feed file. I bet, you intend to
>>> remove the increment statement which resides inside the "if total == 0:"
>>> loop :)
>> No, I meant exactly what I committed.
>> You should count both the <entry> node you have just seen, and the one 
>> you are inserting.  1 + 1 = 2.
>>
>> Or at least, that's the idea. It doesn't work, though, because it seems 
>> that it is not safe to iterate over a childNodes collection whilst 
>> changing it.
> 
> Nope! Here, the loop is executed separately for the item we insert using
> "insertBefore" function and it does not bother to iterate over a
> childNodes collection whilst changing it. In current state, when we run
> the script as below, it inserts only 9 items instead of 10 items. 
> 
> $ ./svn2feed.py -F atom -u .. -U .. -r1:10 /path/to/repos/ -m 10
> # remove the feed and pickle files before you run the script.
> 
> In this case, a temporary print statement for "total" variable explains
> that the loop is iterated 10 times (while processing last revision) and
> the "total" variable is set as 11, so, it removes an item
> unnecessarily.

Sorry, but *NO*, your analysis is wrong.

The loop executes zero times for the inserted item, but *twice* for the 
item after the insert point. We ARE changing the childNodes collection 
whilst iterating over it. As a result, the iteration is responding in 
bizarre ways, because the iterator apparently was not designed to be 
capable of responding sensibly to changes in the collection it is 
iterating over.


>>> Here again, why do we wish to include the above "if" condition for every
>>> childNode ? We call this function for every revision, so it's good to
>>> place it outside the "for" loop. Moreover, in the current state it'll
>>> not remove multiple nodes (if you intend to), since we increment the
>>> "total" one at a time.
>> No, it is not good to place it outside the for loop; it would not then 
>> be able to remove multiple nodes.
>> I believe it *will* remove multiple nodes if needed - I don't understand 
>> your explanation of why you think it won't.
> 
> My point is, we call this function for every revision. So, it's fine to
> remove one item at a time. Moreover, i'm certain the condition is met
> only when processing the last element, so, why we need to check the
> condition for all child nodes.

You need to also consider the case of a user decreasing the --max-items 
value in a subsequent invocation of the script..

Max.

Re: svn commit: r20991 - trunk/contrib/hook-scripts

[Bhuvaneswaran Arumugam <bh...@collab.net>]

> Please try to avoid cc-ing svn@s.t.o - it creates needless duplicate 
> messages.

Yeah, i'll avoid it!

> 
> >> -            if(childNode.nodeName == 'author'):
> >> -                self.feed.insertBefore(item, childNode.nextSibling)
> >> -            elif(childNode.nodeName == 'entry'):
> >> +            if childNode.nodeName == 'entry':
> >> +                if total == 0:
> >> +                    self.feed.insertBefore(item, childNode)
> >> +                    total += 1
> >>                  total += 1
> > 
> > Weird! We increment the "total" twice when the value is 0. Result: We
> > manage to store max_items-1 items in the feed file. I bet, you intend to
> > remove the increment statement which resides inside the "if total == 0:"
> > loop :)
> 
> No, I meant exactly what I committed.
> You should count both the <entry> node you have just seen, and the one 
> you are inserting.  1 + 1 = 2.
> 
> Or at least, that's the idea. It doesn't work, though, because it seems 
> that it is not safe to iterate over a childNodes collection whilst 
> changing it.

Nope! Here, the loop is executed separately for the item we insert using
"insertBefore" function and it does not bother to iterate over a
childNodes collection whilst changing it. In current state, when we run
the script as below, it inserts only 9 items instead of 10 items. 

$ ./svn2feed.py -F atom -u .. -U .. -r1:10 /path/to/repos/ -m 10
# remove the feed and pickle files before you run the script.

In this case, a temporary print statement for "total" variable explains
that the loop is iterated 10 times (while processing last revision) and
the "total" variable is set as 11, so, it removes an item
unnecessarily. 

I'll be happy as long as the script works as expected!

> > Here again, why do we wish to include the above "if" condition for every
> > childNode ? We call this function for every revision, so it's good to
> > place it outside the "for" loop. Moreover, in the current state it'll
> > not remove multiple nodes (if you intend to), since we increment the
> > "total" one at a time.
> 
> No, it is not good to place it outside the for loop; it would not then 
> be able to remove multiple nodes.
> I believe it *will* remove multiple nodes if needed - I don't understand 
> your explanation of why you think it won't.

My point is, we call this function for every revision. So, it's fine to
remove one item at a time. Moreover, i'm certain the condition is met
only when processing the last element, so, why we need to check the
condition for all child nodes.

Thank you very much for looking into it. I really appreciate your
guidance and help.
-- 
Regards,
Bhuvaneswaran

Re: svn commit: r20991 - trunk/contrib/hook-scripts

[Max Bowsher <ma...@ukf.net>]

Bhuvaneswaran Arumugam wrote:
...

Please try to avoid cc-ing svn@s.t.o - it creates needless duplicate 
messages.

>> -            if(childNode.nodeName == 'author'):
>> -                self.feed.insertBefore(item, childNode.nextSibling)
>> -            elif(childNode.nodeName == 'entry'):
>> +            if childNode.nodeName == 'entry':
>> +                if total == 0:
>> +                    self.feed.insertBefore(item, childNode)
>> +                    total += 1
>>                  total += 1
> 
> Weird! We increment the "total" twice when the value is 0. Result: We
> manage to store max_items-1 items in the feed file. I bet, you intend to
> remove the increment statement which resides inside the "if total == 0:"
> loop :)

No, I meant exactly what I committed.
You should count both the <entry> node you have just seen, and the one 
you are inserting.  1 + 1 = 2.

Or at least, that's the idea. It doesn't work, though, because it seems 
that it is not safe to iterate over a childNodes collection whilst 
changing it.

The old code just happened to work, because it suffered from two bugs 
which had precisely opposing effects, cancelling each other out:
   (i): off by one error, fails to count newly inserted node
   (ii): changes childNodes whilst iterating over it, leading to
         unexpected behaviour.

I fixed (i), and now (ii) is no longer accidentally cancelled out.


>> --        if (total > self.max_items):
>> -            self.feed.removeChild(self.feed.lastChild)
>> +                if total > self.max_items:
>> +                    self.feed.removeChild(childNode)
> 
> Here again, why do we wish to include the above "if" condition for every
> childNode ? We call this function for every revision, so it's good to
> place it outside the "for" loop. Moreover, in the current state it'll
> not remove multiple nodes (if you intend to), since we increment the
> "total" one at a time.

No, it is not good to place it outside the for loop; it would not then 
be able to remove multiple nodes.
I believe it *will* remove multiple nodes if needed - I don't understand 
your explanation of why you think it won't.

Max.