[svn.haxx.se] · SVN Dev · SVN Users · SVN Org · TSVN Dev · TSVN Users · Subclipse Dev · Subclipse Users · this month's index

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

From: Max Bowsher <maxb1_at_ukf.net>
Date: 2006-08-05 21:07:16 CEST

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.

Received on Sat Aug 5 21:08:02 2006

This is an archived mail posted to the Subversion Dev mailing list.

This site is subject to the Apache Privacy Policy and the Apache Public Forum Archive Policy.