[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