[MERGE] Give commit a progress bar
Martin Pool
mbp at sourcefrog.net
Fri Apr 21 07:49:42 BST 2006
I think this is useful but I'd really like at this point to freeze out
any non-critical bugs so that we can get 0.8 out. So please add this to
a wishlist bug and keep the branch until next week when we can reopen
for general development.
So +1 with comments, but not right now.
On 21 Apr 2006, Robert Collins <robertc at robertcollins.net> wrote:
>
> --
> GPG key available at: <http://www.robertcollins.net/keys.txt>.
> === added file 'b/bzrlib/tests/workingtree_implementations/test_commit.py'
> +class CapturingUIFactory(ui.UIFactory):
> + """A UI Factory for testing - capture the updates made through it."""
Perhaps this should move (along with its selftests) to a more generic
location, to encourage other test code to use it?
> === modified file 'a/bzrlib/commit.py'
> --- a/bzrlib/commit.py
> +++ b/bzrlib/commit.py
> @@ -245,7 +245,12 @@
> self.reporter = reporter
>
> self.work_tree.lock_write()
> + self.pb = bzrlib.ui.ui_factory.nested_progress_bar()
> try:
> + # Cannot commit with conflicts present.
> + if len(self.work_tree.conflicts())>0:
> + raise ConflictsInTree
> +
> # setup the bound branch variables as needed.
> self._check_bound_branch()
I'm curious - why did you move the conflict detection from below?
> @@ -293,6 +298,13 @@
> self.work_inv = self.work_tree.inventory
> self.basis_tree = self.work_tree.basis_tree()
> self.basis_inv = self.basis_tree.inventory
> + # one to finish, one for rev and inventory, and one for each
> + # inventory entry, and the same for the new inventory.
> + # note that this estimate is too long when we do a partial tree
> + # commit which excludes some new files from being considered.
> + # The estimate is corrected when we populate the new inv.
> + self.pb_total = len(self.basis_inv) + len(self.work_inv) + 3 - 1
> + self.pb_count = 0
>
> self._gather_parents()
> if len(self.parents) > 1 and self.specific_files:
> @@ -309,14 +321,13 @@
> or self.new_inv != self.basis_inv):
> raise PointlessCommit()
>
> - if len(self.work_tree.conflicts())>0:
> - raise ConflictsInTree
> -
> + self._update()
> self.inv_sha1 = self.branch.repository.add_inventory(
> self.rev_id,
> self.new_inv,
> self.present_parents
> )
> + self._update()
> self._make_revision()
> # revision data is in the local branch now.
>
> @@ -346,9 +357,9 @@
> {'branch':self.branch,
> 'bzrlib':bzrlib,
> 'rev_id':self.rev_id})
> + self._update()
> finally:
> - self._cleanup_bound_branch()
> - self.work_tree.unlock()
> + self._cleanup()
>
> def _check_bound_branch(self):
> """Check to see if the local branch is bound.
> @@ -399,6 +410,29 @@
> #### for revision_id in pending_merges:
> #### self.master_branch.repository.fetch(self.bound_branch.repository,
> #### revision_id=revision_id)
> +
> + def _cleanup(self):
> + """Cleanup any open locks, progress bars etc."""
> + cleanups = [self._cleanup_bound_branch,
> + self.work_tree.unlock,
> + self.pb.finished]
> + found_exception = None
> + for cleanup in cleanups:
> + try:
> + cleanup()
> + # we want every cleanup to run no matter what.
> + # so we have a catchall here, but we will raise the
> + # last encountered exception up the stack: and
> + # typically this will be useful enough.
> + except Exception, e:
> + found_exception = e
> + if found_exception is not None:
> + # dont do a plan raise, because the last exception may have been
> + # trashed, e is our sure-to-work exception even though it loses the
> + # full traceback. XXX: RBC 20060421 perhaps we could check the
> + # exc_info and if its the same one do a plain raise otherwise
> + # 'raise e' as we do now.
> + raise e
>
I see this just factors out the existing code but it feels quite
strange. Shouldn't we just have multiple try/finally blocks, and split
out the code if that makes it too deeply nested?
> def _cleanup_bound_branch(self):
> """Executed at the end of a try/finally to cleanup a bound branch.
> @@ -503,7 +537,12 @@
> # XXX: Need to think more here about when the user has
> # made a specific decision on a particular value -- c.f.
> # mark-merge.
> +
> + # iter_entries does not visit the ROOT_ID node so we need to call
> + # self._update once by hand.
> + self._update()
> for path, ie in self.new_inv.iter_entries():
> + self._update()
> previous_entries = ie.find_previous_heads(
> self.parent_invs,
> self.weave_store,
> @@ -554,6 +597,11 @@
> mutter('%s selected for commit', path)
> self._select_entry(new_ie)
>
> + def _update(self):
> + """Emit an update to the progress bar."""
> + self.pb.update("Committing", self.pb_count, self.pb_total)
> + self.pb_count += 1
> +
> def _select_entry(self, new_ie):
> """Make new_ie be considered for committing."""
> ie = new_ie.copy()
I'd much prefer _update_pb or some such, _update is a bit generic.
> @@ -565,6 +613,9 @@
> """Carry the file unchanged from the basis revision."""
> if self.basis_inv.has_id(file_id):
> self.new_inv.add(self.basis_inv[file_id].copy())
> + else:
> + # this entry is new and not being committed
> + self.pb_total -= 1
>
> def _report_deletes(self):
> for file_id in self.basis_inv:
>
> === modified file 'a/bzrlib/tests/blackbox/__init__.py'
> --- a/bzrlib/tests/blackbox/__init__.py
> +++ b/bzrlib/tests/blackbox/__init__.py
> @@ -97,6 +97,9 @@
> def clear(self):
> """See progress.ProgressBar.clear()."""
>
> + def clear_term(self):
> + """See progress.ProgressBar.clear_term()."""
> +
> def finished(self):
> """See progress.ProgressBar.finished()."""
I (still) dislike these docstrings; they use up memory and hide the real
docstring without adding anything useful. (I know we talked about it
before, no action is required for this commit.) I would prefer if they
could just be comments.
--
Martin
More information about the bazaar
mailing list