[attn aaron] differences between RevisionTree and WorkingTree

[Robert Collins]

On Fri, 2006-07-28 at 09:01 -0400, Aaron Bentley wrote:
> -----BEGIN PGP SIGNED MESSAGE-----
> Hash: SHA1
> 
> Robert Collins wrote:
> > On Wed, 2006-07-26 at 15:03 -0500, John Arbash Meinel wrote:
> > 
> >>-----BEGIN PGP SIGNED MESSAGE-----
> >>Hash: SHA1
> >>
> >>Robert Collins wrote:
> >>
> >>
> >>>So heres a patch that does this: changes iter_entries to yield the root.
> >>>
> >>>I hope this will be uncontentious - its a small change, it hasn't broken
> >>>any plugins AFAICT. 
> >>>
> >>>The tree implementation tests I have planned at this point are primarily
> >>>for ensuring test trees are available, but there are a number of tests
> >>>we can move into there should someone be interested.
> >>>
> >>>Cheers,
> >>>Rob
> >>>
> >>>
> > 
> > ...
> > 
> > Aaron, what do you think of this patch?
> 
> I would really like to understand the test adaption system better.
> 
> I would do this differently:
> 
> - --- bzrlib/commit.py
> +++ bzrlib/commit.py
> @@ -502,10 +502,11 @@
>          # in bugs like #46635.  Any reason not to use/enhance
> compare_trees?
>          # ADHB 11-07-2006
>          mutter("Selecting files for commit with filter %s",
> self.specific_files)
> - -        # iter_entries does not visit the ROOT_ID node so we need to call
> - -        # self._emit_progress_update once by hand.
> +        # at this point we dont copy the root entry:
> +        entries = self.work_inv.iter_entries()
> +        entries.next()
>          self._emit_progress_update()
> - -        for path, new_ie in self.work_inv.iter_entries():
> +        for path, new_ie in entries:
>              self._emit_progress_update()
>              file_id = new_ie.file_id
>              mutter('check %s {%s}', path, file_id)
> 
> Instead of calling entries.next() once, I'd do
>         for path, new_ie in self.work_inv.iter_entries():
>             self._emit_progress_update()
>             if new_id is self.work_inv.root:
>                 continue
>             file_id = new_ie.file_id
>             mutter('check %s {%s}', path, file_id)
> 
> Which seems to telegraph the reasoning better.  Mostly a personal style
> thing, but I think it removes the need for a comment, and therefore
> removes the need to keep a comment up to date.  This pattern applies to
> a lot of the code.

On the other hand if we are committing 20K entries, doing such an if
test once and only once is highly desirable - which is why I followed
the pattern of grabbing the iterator early and advancing it once.

Thanks for the review,
Rob
-- 
GPG key available at: <http://www.robertcollins.net/keys.txt>.
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 191 bytes
Desc: This is a digitally signed message part
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060729/fbc03eac/attachment.pgp