On Fri, Jun 11, 2010 at 07:26:31PM -0400, David Borowitz wrote: > I'm a bit torn on this issue. On the one hand, I agree that there are some > things like excludes that we can provide well-defined APIs for rather than > depending on specific filenames. On the other hand, there are two cases that > I think merit keeping get_named_file around: > 1. Allow users to read and parse files on their own that Dulwich does not > (yet) have support for. (Presumably we would encourage such users to > contribute their parsing code back to Dulwich and/or transition to the > official API once one exists.) I don't have any objections against making it possible for people to do this but I'd rather keep it optional, not a requirement for the correct or full functioning.
> 2. It's necessary for the implementation of dumb HTTP serving. Is there any reason for this to go through the Repo API rather than through the standard os module ? It doesn't seem right to make up these files on the fly for repository implementations that do not use the standard git disk representation. > I'm still slightly for using '/' to be consistent with refnames and URL > representations in the HTTP server. Note that both of these points are > orthogonal to the choice of path separator. I'm not opposed to making that change but I would prefer to make that change at the same time as any other changes we make to get_named_file. Cheers, Jelmer > > On Wed, Jun 9, 2010 at 17:46, Jelmer Vernooij <[email protected]> wrote: > > > On Wed, Jun 09, 2010 at 02:48:18PM -0500, Augie Fackler wrote: > > > On Mon, Jun 7, 2010 at 5:32 PM, Jelmer Vernooij <[email protected]> > > wrote: > > > > 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. > > > > > > It seems reasonable that we'll need access to (for example) > > > info/excludes so we can construct the appropriate matcher for > > > Mercurial, Bazaar or whatever else. It seems kind of odd to my mind to > > > have a function per special file on the repo object. > > I think it's reasonable to have a separate function, the fact that the > > exclude info can be found in info/excludes is a representation issue. > > > > I'd rather have a function that returns a list of exclude patterns for > > which we can put in a stub that e.g. returns [] or uses some other magic. > > The alternative is to have a get_named_file() implementation that returns > > custom file contents (the disk format rather than the content) based on the > > filename that gets passed in. The implementation for a bzr repo would > > look something like this: > > > > def get_named_file(self, name): > > if name == "info/excludes": > > return StringIO() > > elif name == "description": > > return "A Bazaar repository" > > elif name =="config": > > # Construct a ConfigObj, serialize it and return it as a > > # file-like object > > elif name.startswith("refs/heads/"): > > # Some magic involving a RefsContainer and reserializing > > else: > > ... > > > > Making the disk format part of the API is something I'd like to avoid as > > much > > as possible as it makes it hard (and slow) to implement the API on top of > > different disk representations (not just bzr or mercurial but also on top > > of > > say a highly scalable database system). I don't think having a function for > > getting at the excludes would make the API that much more complicated, > > there > > aren't much similar files like this. > > > > Cheers, > > > > Jelmer > > -- _______________________________________________ Mailing list: https://launchpad.net/~dulwich-users Post to : [email protected] Unsubscribe : https://launchpad.net/~dulwich-users More help : https://help.launchpad.net/ListHelp
