[merge] change commit to use update_to_one_parent_via_delta; other fixes
Ian Clatworthy
ian.clatworthy at internode.on.net
Mon Oct 15 04:41:55 BST 2007
Martin Pool wrote:
> The main change that had to be made was that some commit builders were
> changing the root entry's revision property, but not including that in
> the delta they pass back.
bb:tweak
Please double check my comments below. If they are wrong, then my vote
lacks confidence and ought to be 'comments' instead.
> self._update_builder_with_changes()
> + self._report_and_accumulate_deletes()
> self._check_pointless()
Now that we're calculating a delta, we can use it in _check_pointless().
That can be a separate patch of course but adding a TODO to this one
mentioning that would be good.
> # Check and warn about old CommitBuilders
> if not self.builder.record_root_entry:
> - symbol_versioning.warn('CommitBuilders should support recording'
> - ' the root entry as of bzr 0.10.', DeprecationWarning,
> - stacklevel=1)
> - self.builder.new_inventory.add(self.basis_inv.root.copy())
> + raise AssertionError('CommitBuilders should support recording'
> + ' the root entry as of bzr 0.10.')
As best I can tell, this test at this level is actually completely
redundant as it's done anyway (inside finish_inventory) at the level below.
I recall taking it out of one of my branches and all tests still passed.
There may be good reasons for leaving it there?
> + # XXX: It looks like this is only hit when _check_root decided
> + # to set a new revision on the root. We seem to be overriding
> + # ie.revision being set at this point to mean either its an
> + # unversioned root, or that it's an unchanged file.
> + #
I actually find the code easier to follow without this comment.
> # repositories that do not version the root set the root's
> # revision to the new commit even when no change occurs, and
> # this masks when a change may have occurred against the basis,
> # so calculate if one happened.
> - if ie.file_id not in basis_inv:
> + if ie.file_id in basis_inv:
> + delta = (basis_inv.id2path(ie.file_id), path,
> + ie.file_id, ie)
Hmm. This *feels* wrong to me: always returning a delta even when the
old path and the new path are the same. In Robert's old code, he
explicitly tested for that case and didn't bother with a delta when
encountered. Given this change is the essence of this fix, I think
we need a clear comment immediately before we set the delta,
explaining that we still need a delta because ie has an updated
entry (including an updated rev-no).
Old code was ...
> - basis_id = basis_inv[ie.file_id]
> - if basis_id.name != '':
> - # not the root
> - delta = (basis_inv.id2path(ie.file_id), path,
> - ie.file_id, ie)
> + # we don't need to commit this, because the caller already
> + # determined that an existing revision of this file is
> + # appropriate.
> + delta = None
> + return delta, ie.revision == self._new_revision_id
I think "return None, ..." is clear enough here. (The separate setting
of delta to None looks like a hangover from refactoring the earlier code.)
> + # XXX: This assumes its being run against a repository that does
> + # update the root revision on every commit. Newer ones that use
> + # RootCommitBuilder won't update it on each commit.
Minor spelling grammar notes:
* its => it's
* does update => updates
Ian C.
More information about the bazaar
mailing list