[PATCH] Changesets for bzr

[John Arbash Meinel]

Aaron Bentley wrote:
> 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.

Sorry I was unclear. I was meaning the returned object would be capable
of installing. Not the read() function. I want to be able to read
changesets without acting on them.

> 
>>> 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.

Well, I can run 'bzr send-changeset' and have it fail. So somehow 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.

Sure. It really was just a 'read changeset' command.

> 
>>> +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?

I thought that is what we were using StrictTestament for.

> 
>>> +    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?

I don't really prefer an api where you always have to call a second
function if you call the first. It just feels wrong. If one requires the
other to be called, then it should call it.

> 
>>> 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

I don't know if you followed it, but there was work done on making
InventoryEntry uses __slots__ everywhere, rather than just on the base
class. And at that time they removed a bunch of the members that don't
make sense to all classes.

But we can clean it up when that gets merged. (I've +1'd it, but waiting
on a second review).

John
=:->

-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 254 bytes
Desc: OpenPGP digital signature
Url : https://lists.ubuntu.com/archives/bazaar/attachments/20060526/dfae6f4b/attachment.pgp