dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

On Mon, 2010-06-07 at 15:59 -0500, Augie Fackler wrote:
> On May 30, 2010, at 9:18 AM, Jelmer Vernooij wrote:
> > On Mon, May 24, 2010 at 11:08:56AM -0700, David Borowitz wrote:
> > > This is quite the grab-bag of changes :). Available as usual at my
> > > github
> > > fork:
> > > http://github.com/dborowitz/dulwich
> > > 
> > > I think Augie has requested that we do code reviews in some place
> > > public,
> > > like this mailing list. If others agree I'm happy to oblige by
> > > mailing
> > > patches.
> > That'd be great - mailed patches are a bit easier to comment on for
> > me, and this 
> > mailing list seems like the most appropriate place.
> > 
> > > b9677d0 Add tests for sorted_tree_items and C implementation.
> > > 186fb3a Fix memory leak in C implementation of sorted_tree_items.
> > > 0994d03 Clean up file headers.
> > > 4a42aad Fix copyright in dul-web.
> > I've merged these, thanks.
> > 
> > > e146b26 Move named file initilization to BaseRepo.
> > Thanks, this makes sense. 
> > 
> > We should've really started out by naming BaseRepo Repo and naming
> > Repo
> > DiskRepo or something, but it seems hard to change that now without
> > breaking
> > everybody's existing code. :-(
> > 
> > > 9f08973 Use / as separator for named files on all platforms.
> > I'm not sure about this change - other than the use of info/excludes
> > we don't 
> > actually appear to use / in calls to {get,put}_named_file so it
> > seems like a 
> > silly thing to break the API for. 
> Is it an API break then? It also means that you can always request
> 'info/excludes' without respect to the platform you're on.
It changes the behaviour of this particular call on some platforms
(specifying info\excludes on Windows will break), so users will have to
change their callsites. 

> > Alternatively, perhaps we can make get_named_file private/protected?
> > I'd be
> > less hesitant about making this sort of change if we do that. 

> -1, it feels like clients of Repo will need to load files within .git
> and get_named_file feels like the correct call for this.
Do they need to do that directly though, and for what sort of
functionality? I'd rather see higher-level function calls on Repo, in
particular because that makes it easier to e.g. implement the Repo API
on top of other databases with a different on-disk format.   

Cheers,

Jelmer

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