dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

On Wed, 2010-10-06 at 14:19 -0700, David Borowitz wrote:
> In the stuff I'm doing on rename detection, I've been bitten a few
> times by the weird ordering of (name, mode, sha) tuples coming out of
> various tree iteration methods.

> Currently, we have:
> Tree.entries() yields (mode, name, sha)
> Tree.iteritems() and BaseObjectStore.iter_tree_contents() yield (name,
> mode, sha)
> index.commit_tree() takes (name, sha, mode)
This is all for hysterical raisins, unfortunately. I'm all for fixing
it :-) 

> We should really standardize this, preferably in one of three ways:
> 1. Use (name, mode, sha) everywhere.
> 2. Use a namedtuple of (name, mode, sha).
> 2. Use a new data object.
> I would prefer using a namedtuple would make some of the code I'm
> about to write cleaner. It would also be backwards-compatible with the
> appropriate type. The downside is that it's another feature that's new
> in 2.6.
It doesn't really help that you have two options 2 ! :-)

This is a really performance-sensitive area (at least for me), so if we
go for anything else than a tuple we should make sure it doesn't have a
(significant) impact on performance. 

I don't think we should drop support for pre-2.6 Python yet; we could of
course provide a custom implementation for those who run 2.4 or 2.5, but
that seems like more trouble than (3).

3 seems like the best solution if it doesn't make things too much
slower. Of course, we could give it semantics similar to what namedtuple
would give us.

> In my ideal world we would get rid of Tree.entries() and change
> index.commit_tree() to use the standard format, but I don't have a
> sense of how widely either of those are used. Thoughts on how to
> proceed?
Changing commit_tree() is probably possible as it's not very heavily
used. Removing Tree.entries() is not possible I think, at least not
without deprecating it first.

Cheers,

Jelmer

Attachment: signature.asc
Description: This is a digitally signed message part