[MERGE] Refactor commit to prepare for population by tree walking

[Aaron Bentley]

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

Ian Clatworthy wrote:
> +        # Build the new inventory
> +        self._populate_from_inventory(specific_files, old_commit_builder)
> +
> +        # If specific files/directories were nominated, it is possible
> +        # that some data from outside those needs to be preserved from
> +        # the basis tree. For example, if a file x is moved from out of
> +        # directory foo into directory bar and the user requests
> +        # ``commit foo``, then information about bar/x must also be
> +        # recorded.
> +        # ABENTLEY says:
> +        # This implies that bar knows what its children are, which isn't
> +        # really accurate.  Strictly speaking, bar only needs to be committed
> +        # if it wasn't a directory in the basis tree - the current
> +        # implementation is overkill.

I don't think it's helpful to put it this way.  It looks like an
argument.  I was hoping you would use my comments to adjust yours.
Something like this:

- --
If specific files or directories were nominated, it is possible that
some data from outside those needs to be included in the commit.
For example, if a file foo is moved out of directory bar into directory
baz, and then the user commits 'bar', there is no need to commit the
move of 'foo'.  But if bar was removed or changed into a different
filetype, then the move of 'foo' must be committed.  Similarly, if 'foo'
was committed, it would not normally be necessary to commit 'baz'.  But
if 'baz' did not exist previously or was not a directory, then it would
be necessary to commit 'baz' as well.  However, only filetype changes to
'baz' need be committed.  If it had been renamed, the rename would not
need to be committed.  The actual code ignores these intricacies, and
commits all changes to all paths that could be affected.
- --

However, I realize that you're really talking about something else.
Your comment could be much simpler:

- --
If specific files are selected, then all un-selected files must be
recorded in their previous state.
- --

> +    def _populate_from_inventory(self, specific_files, skip_first_entry):
> +        """Populate the CommitBuilder by walking the working tree inventory."""


^^^ I still don't see what the point of skip_first_entry is.  It will
always be the opposite of self.builder.record_root_entry, so why make it
a parameter?

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

iD8DBQFGo4vW0F+nu1YWqI0RAtIcAJ0Tmo4anBPnfGAxFBoUpKrG07taNgCfZEJB
g7Cm8gUzhVMuA24QH4sYtBk=
=m+0G
-----END PGP SIGNATURE-----