[MERGE] show_log with get_revisions

[Aaron Bentley]

-----BEGIN PGP SIGNED MESSAGE-----
Hash: SHA1

John Arbash Meinel wrote:
> === modified file bzrlib/log.py //
> last-changed:aaron.bentley at utoronto.ca-20060
> ... 610142239-7c75c4e0dc2814b3
> --- bzrlib/log.py
> +++ bzrlib/log.py
> @@ -157,8 +157,12 @@
>      """
>      branch.lock_read()
>      try:
> -        _show_log(branch, lf, specific_fileid, verbose, direction,
> -                  start_revision, end_revision, search)
> +        branch.repository.lock_read()
> +        try:
> +            _show_log(branch, lf, specific_fileid, verbose, direction,
> +                      start_revision, end_revision, search)
> +        finally:
> +            branch.repository.unlock()
>      finally:
>          branch.unlock()
> 
> 
> I assume the extra 'repository.lock_read()' is because we are trying to
> move away from 'branch.lock_read() auto locks the repository'?
> Is it worth adding these changes now?

I thought we were moving in that direction.  It seems Robert wants to
address per-item locking by having a different method, so I can revert
it.  There are lots of places where we're locking both branch and
repository-- switching to branch.lock_ will make things cleaner.

> === modified file bzrlib/store/revision/__init__.py
> --- bzrlib/store/revision/__init__.py
> +++ bzrlib/store/revision/__init__.py
> @@ -108,6 +108,10 @@
> 
>      def get_revision(self, revision_id, transaction):
>          """Return the Revision object for a named revision."""
> +        return self.get_revisions([revision_id], transaction)[0]
> +
> +    def get_revisions(self, revision_ids, transaction):
> +        """Return the Revision objects for a list of named revisions."""
>          raise NotImplementedError(self.get_revision)
> 
>      def get_signature_text(self, revision_id, transaction):
> 
> 
> Shouldn't we add a default implementation which just iterates over the
> list and gets them one at a time? That way all Store objects would have
> implemented this codepath.

I have implemented get_revisions for all RevisionStore objects.  I think
it's simpler to have one codepath where possible, and get_revision is
easily implemented in terms of get_revisions.

> I also thought that part of the goal was to be able to get the first 5
> revisions quickly, so that 'bzr log' is instantaneous to start up, and
> then be able to get the rest of them later.

I'm not sure exactly where the sweet spot will be.  Right now, I'm
focusing on general optimization of multi-revision retrieval.  Once
there's no performance left to squeeze from that, I'll move on to
optimizing log itself.

The next thing I expect to do is is implement vf.get_texts(), so that we
can use readv effectively.  Each invocation of LocalTransport.readv
opens a file, so changing the request order doesn't help.  Once all
revisions are generating a combined request, we can sort them by offset,
so that we don't seek backwards.  Doing that transparently in readv
means that we don't need to predict the optimal request order higher up.

> I think we need to change this into a generator, rather than returning
> the whole list.

I don't think so; I think show_logs should grab the first n revisions
explicitly, because it knows which revisions it wants first, and how
many it needs.  For example, your proposal would pessimize log --forward
and other operations that either:

a) need all revisions in order to start work
b) need the revisions in forward order.

If we choose to implement a generator also, we can implement it on top
of get_revisions.  So we might define the generator in show_log.  That
would be pretty nice.

> Also, what we really want is for this stuff to also be available as a
> per-file, and for inventories. So that we can do the 'bzr log -v' fast
> as well.

Well, small steps first, I think.  Are we using inventories for log -v
now?  There was talk of using the file indices for that.

Aaron
-----BEGIN PGP SIGNATURE-----
Version: GnuPG v1.4.1 (GNU/Linux)
Comment: Using GnuPG with Thunderbird - http://enigmail.mozdev.org

iD8DBQFEjZWQ0F+nu1YWqI0RAsnRAJ9SZqZ/jP2q2vXLaTn9aXnaq0vv/gCdFOMg
YOHua1iMDaupsri4OxUAHGg=
=m+79
-----END PGP SIGNATURE-----