Re: svn commit: r20300 - trunk/subversion/tests/cmdline/svntest

On Fri, 30 Jun 2006, Madan S. wrote:

Hi Madan, thanks for the review! In your response, you snipped off
part of the diff:

   def add_child(self, newchild):
+ child_already_exists = 0
     if self.children is None: # if you're a file,
       self.children = [] # become an empty dir.
- child_already_exists = 0
- for a in self.children:
- if a.name == newchild.name:
- child_already_exists = 1
- break
- if child_already_exists == 0:
- self.children.append(newchild)
- newchild.path = os.path.join (self.path, newchild.name)
+ else:
+ for a in self.children:
+ if a.name == newchild.name:
+ child_already_exists = 1
+ break
 
> On Fri, 30 Jun 2006 04:55:50 +0530, <dlr@tigris.org> wrote:
...
> >- # If you already have the node,
> > else:
> >+ for a in self.children:
> >+ if a.name == newchild.name:
> >+ child_already_exists = 1
> >+ break
> >+
> >+ if child_already_exists:
> > if newchild.children is None:
> > # this is the 'end' of the chain, so copy any content here.
> > a.contents = newchild.contents
> >@@ -180,6 +177,9 @@
> > # try to add dangling children to your matching node
> > for i in newchild.children:
> > a.add_child(i)
> >+ else:
> >+ self.children.append(newchild)
> >+ newchild.path = os.path.join(self.path, newchild.name)
>
> This has changed the logic of the existing code. Now the function
> doesnt do add_child() of the newchild's children. Am wondering why you
> made this change.

I'm not seeing any place where the logic changed here, other than at
the beginning of the method, where we now avoid iterating over a newly
created, known-to-be-empty list of children for "newchild".

As I mentioned in the change log message, I removed the negative logic
from the conditional -- which I find makes for much more
comprehensible code -- in the second section of the method, which
necessitated that I swap the blocks for its "if" and the "else"
statements. The children of "newchild" are still added, but now in
the "else" block instead of the "if" block.

Thanks, Dan