[PATCH] Changesets for bzr

[Aaron Bentley]

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

John Arbash Meinel wrote:
> I'm not sure what it is, but Enigmail really doesn't like your message.
> (It has crashed Thunderbird on 2 machines, and is generally just unhappy).

Same here.

> What were the incompatible changes that you wanted to add?

Add the executable bit to StrictTestaments, assume execute bit is false
for new files.

> In the refactoring, I think we would want to change the 'reader'
> parameter to be whatever the final return of Serializer.read() is. Or
> maybe that in itself would do the merging/installing.

I think it should just read.  I still like the idea of using my
ChangesetInstaller api, but ChangesetInfo would also be fine.

> Anyway, I'm wondering if we should make this underscore prefixed, since
> they aren't really going to be permanent.

True, but that's a sort of general problem with merging before the
refactoring is done.

> +class cmd_send_changeset(Command):
> 
> send-changeset is completely borked. 'send_changeset.py' is still in
> bzrlib/changeset/old.py
> 
> I'm thinking we should at least not import it into builtins.py until we
> get it cleaned up.

I don't think we are.

> I'm 75% okay with leaving it here so that we
> eventually fix it up. It might evolve to just plain 'bzr send', I don't
> know for sure. But when we get it working again, we probably should hook
> it up to configs, and the proposal for where to send changes to. (You
> have a personal setting, and a setting where people who branch from you
> should send to)


> 
> +
> +class cmd_changeset(Command):
> 
> Will anyone use this as anything other than 'bzr cset'? Should we just
> name the command 'cmd_cset' and not worry about the alias?

I think it's nice to have a more expressive name in places like the help
list.  Makes it more discoverable.

> It looks like we just need to pass 'b.repository' rather than just plain
> 'b'.
> 
> We should probably at least have a smoke test for 'verify-changeset'.

No, it's known-busted.  It relies on assumptions we can't meet, and you
said you didn't care about it, so I (thought I) disabled it.

> +register_command(cmd_changeset)
> +#register_command(cmd_verify_changeset)
> +#register_command(cmd_send_changeset)
> 
> We probably shouldn't be using register_command here. Instead just
> importing the commands into builtins.py (Otherwise they show up as
> plugin commands).

'kay.

> test_suite should also be removed.

done.

> We should get rid of both 'encode' and 'decode' functions, and just
> switch to s.encode('utf8') if that is what we are using. At one point I
> was escaping certain characters.

Alright.

> +def decode(s):
> 
> 
> ...
> 
> +    >>> t -= 24*3600*365*2 # Start 2 years ago
> +    >>> o = -12*3600
> +    >>> for count in xrange(500):
> +    ...   t += random.random()*24*3600*30
> +    ...   o = ((o/3600 + 13) % 25 - 12)*3600 # Add 1 wrap around from
> [-12, 12]
> +    ...   date = format_highres_date(t, o)
> +    ...   t2, o2 = unpack_highres_date(date)
> +    ...   if t != t2 or o != o2:
> +    ...      print 'Failed on date %r, %s,%s diff:%s' % (date, t, o, t2-t)
> +    ...      break
> +
> 
> This loop creates a bunch of random dates, and is known to fail at
> certain times (around daylight savings time). I think we have tested
> format_highres_date and unpack_highres_date that we don't need a random
> shotgun test anymore.

Okay.

> +def _unescape(name):
> 
> This function is unused, and needs to be removed.

Okay.

> +    def _validate_inventory(self, inv, revision_id):
> +        """At this point we should have generated the ChangesetTree,
> +        so build up an inventory, and make sure the hashes match.
> +        """
> 
> I think _validate_inventory is now bogus, since it expects an exact
> inventory serialization. Which we no longer guarantee.

It's not entirely bogus.  For old revisions, you may get a warning, but
for new revisions you won't.  I was considering it a last line of
defence against corrupt changesets.  Perhaps we should include the real
sha1 and use that instead?

> +    def is_executable(self, file_id):
> +        path = self.id2path(file_id)
> +        if path in self._executable:
> +            return self._executable[path]
> +        else:
> +            return self.base_tree.inventory[file_id].executable
> 
> I still think that if the file is missing in base, then its executable
> bit is False. It doesn't have to do with a particular serialization
> format. Just the idea that a file without a base has executable=False
> rather than executable=None (unknown). 

I think we already have perfectly good ways of expressing the execute
bit, and ChangesetTrees are weird enough without adding that behaviour.
 Leave it for a higher level.  That way, people can see that the execute
bit is being handled, instead of having to find out that this particular
property is different from all other ChangesetTree properties.

Explicit is better than implicit.

> I realize I am wanting to use
> that fact in the current format. But what happens in the future when you
> have a new format, and you have an add. It means a 'note_add' *always*
> has to be accompanied by a 'note_executable', 

Where's the harm in that?

> Am I reading this correctly that you actually do compute the diff, and
> just base-64 encode the result? Rather than sending the whole binary?

That's correct.

> Sounds nice. Though only really useful if we have a good binary diff
> algorithm.

Some binary files are almost-text, and will be handled well.

But yeah, mostly not.

> Shouldn't these exceptions be in errors.py?

Depends whether we want bzr to diverge from the regular version, I
suppose.  I do intend to keep this library separately-available.

> We have the same problem where our test case uses python source
> material. I'm okay with it, but it might be better to try and find other
> material in the future. Also, the test files are rather long. In fact,
> of your total patch, they seem to make up the bulk.

Well, these don't bother me as much.  Seeing it intermixed with real
code was confusing.

> +Ensure changeset command behaves properly w/revisions (e.g. 0)
> +Decide whether to bundle 'send-changeset' into 'changeset'.
> +Decide whether to fix or kill 'verify-changeset'.
> +Figure out whether testament-related bug is my problem
> +Get rid of the 'old' directory
> +Bundle signatures?
> +What to do about ghost ancestors in remote branch that are present locally?
> 
> 
> I know that we still have TODOs in this code. Is it okay to merge them?
> 
> ...
> 
> I thought with the inventory fixes we might either have no executable
> attribute, or it would at least be None for non-files.

Your ChangesetTree patch also sets an execute bit value for non-files.

But this is actually correct.  The execute bit is a member of
InventoryEntry, so it applies to symlinks and directories.  It is set
False by default.  So now I'm asserting that all inventory entries have
an execute bit, and it's False for non-files.

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

iD8DBQFEdwZG0F+nu1YWqI0RAgdWAJ9RbOYvRhBSm7YoUmPcsJLgq9I9XQCfa52o
3GEZukueV+xRTxVOk3S6Qtg=
=BdVn
-----END PGP SIGNATURE-----