dulwich-users team mailing list archive

[Jelmer Vernooij <jelmer@xxxxxxxxx>]

On Wed, 2011-07-27 at 14:51 -0700, Dave Borowitz wrote:
> I'll mail another copy of these with the right email adresses and NEWS
> entries. Hopefully tonight (depending on OSCON :)
Great. Also, have fun at OSCON. :)

> On Wed, Jul 27, 2011 at 14:29, Jelmer Vernooij <jelmer@xxxxxxxxx>
> wrote:

>         Perhaps we should also document our de-facto policy on API
>         changes
>         somewhere. So far it has been something along the lines of:
>         
>          - Everything that has a name starting with an underscore is
>           considered private and fair game for backwards incompatible
>         changes
>          - For other code we try to document the API changes in NEWS 
>          - For commonly used APIs we deprecate first (and add
>         deprecation
>           warnings), then remove the functionality a couple of
>         releases
>           later. What is considered commonly used is a bit vague and
>         up to
>           our judgement.

> Agreed, and you're certainly in a better place to give an estimate
> than I am. I'm happy to deprecate and add "new" versions of some of
> the functions in pack.py I was messing with, if you have any in mind.
IMHO removing them and documenting that should be sufficient. Backwards
compatibility is useful, but I'm not sure if it's worth the development
effort for the particular functions you've changed. 
 
>         Do we care about breakages in the testsuite utility functions,
>         etc?
> Good question. I think public functions in test utils should fall in
> the same category as any other public functions.
Me too. I don't use any of them in bzr-git yet, but some might be
useful, in particular the new build_pack.
 
>         Do you agree that's what we've been doing, and does it seem
>         reasonable
>         as a policy?

> I agree that it's what you've been doing (though I've been remiss),
> and it certainly seems reasonable as a general policy. I'd love to see
> it in HACKING so I can refer to it in the future.
Great, I'll have a look at updating HACKING.

> As part of this, we may want to scrub through a few modules and decide
> to move some functions to private.
Yeah, that's a good point.

>         Are you both dborowitz@xxxxxxxxxx and ddborowitz@xxxxxxxxxx,
>         or is the
>         second a typo? Your emails seem to use the first but your
>         patches use
>         the second.

> dborowitz@xxxxxxxxxx and ddborowitz@xxxxxxxxx are both me. If you ever
> see ddborowitz@xxxxxxxxxx (and I didn't see any in this series), that
> is nonexistent, but I don't think I have that set in any of
> my .gitconfigs.
Sorry, my mistake - I didn't see the domains were different. The patches
indeed have ddborowitz@xxxxxxxxx - not ddborowitz@xxxxxxxxxx.

>         > 9b3fc71 pack: Compute CRC32 during object unpacking.
>         
>         ok.  This changes the behaviour of take_msb_bytes,
>         read_zlib_chunks,
>         unpack_object and PackStreamReader.read_objects as they now
>         return a tuple or one
>         more element in the tuple they already returned.
>         
>         I'm also a bit surprised this didn't require more tests to be
>         updated.
>         Your patch doesn't make the situation worse, it'd be nice to
>         add more
>         tests at some point.

> Yes, more tests are good, sorry about that. Do you mind if those come
> in some followup changes to cut down on conflicts within the series?
Sorry, I mostly meant that more tests in general would be good - as a
general observation.

Your patches don't make the situation worse, which I think should be the
main requirement for landing them, and improve the coverage in some
cases. 

>         > 7caefca pack: Expose which refs were external in
>         DeltaChainIterator.
>         
>         It seems a bit odd that the tests look at the private variable
>         for the
>         ext refs here rather than using the public accessor method. Is
>         there a
>         particular reason for that?

> Almost certainly not, but I'll be honest and say I don't remember.
Ok, at least that means I haven't missed something obvious. :)

>         > 3f7b7dc pack: Nuke ThinPackData.
>         This will need to be documented in NEWS. IIRC we use it in
>         bzr-git

> This is a fine candidate for deprecation, I don't mind leaving it in.
I'm happy to update git to avoid it.
 
>         > a2a6078 _compat: Use namedtuple recipe rather than
>         hard-coding.
>         
>         We should note the license under which the code fragment is
>         available.
>         That also appears to be missing for permutations().

> Hm, I thought the PSF copyright was sufficient for PSL software, but I
> see no harm in mentioning it. permutations() is also PSL since it's
> straight from the itertools docs.
It is compatible, but the "All rights reserved" bit by itself is
worrying without the license. 

>         This also isn't backwards compatible, so would need update to
>         NEWS.

> I'll leave it up to your call on this, but IMO it would be pretty ugly
> to go the deprecation route here; we would end up with two
> basically-identical, moderately complex code paths. I also think
> UnpackedObjects are enough cleaner that people would hopefully be
> motivated to switch :)
Yeah, they're definitely cleaner. I'll have a closer look at this, also
what implications it would have for bzr-git.

Cheers (from DebConf),

Jelmer

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