On Nov 16, 2010, at 9:34 AM, Jelmer Vernooij wrote: > Hi Augie, > > On Mon, 2010-11-15 at 22:31 -0600, Augie Fackler wrote: >> On Nov 15, 2010, at 8:53 AM, Jelmer Vernooij wrote: >>> On Sun, 2010-11-14 at 19:19 -0600, Augie Fackler wrote: >>>> First patch is trailing whitespace cleanup courtesy of my .emacs, I >>>> don't feel strongly about it. >>> I'll merge this one. >>> >>>> Second patch removes an assertion which only gets hit on >>>> non-c-extension builds. I noticed it when I was using a broken install >>>> and ended up using the Python parsing code instead of the native code, >>>> which worked fine on the repo in question. It's been lurking around so >>>> long I've forgotten which repository this broke on, but I can dig it >>>> up again if you want. >>> This assertion is actually correct. It's come up a couple of times >>> recently because GitHub sometimes writes invalid data. Apparently this >>> makes jgit fall over too and cgit's fsck warns about it. >> Huh? I've seen *real* repositories that I can't use with the >> pure-python Dulwich, but that work fine when C speedups are enabled >> because *this exact assertion* is absent in the C code? git fsck >> warning is very different than actually blowing up. > The inconsistency between the C extensions and the Python implementation > is wrong and definitely something we should fix. As I said earlier I'm > not necessarily opposed to removing the assertion (rather than making > the C extensions raise an AssertionError as well), but we need some way > for API users to find that an object is not formatted properly. > > The reason I'm worried about this sort of thing is that it breaks > roundtripping of objects: > > a = my_repo[broken_tree_sha1] > b = Tree.from_string(str(Tree())) > assert a.id == b.id # This will fail
Per discussion here and in IRC, sent a version that requires .check() to detect such brokenness. > > This is the relevant bug on github: > > http://support.github.com/discussions/repos/4341-githubs-inline-edit-breaks-git-repositories > >>> I am hesitant to remove this assertion without adding some code >>> to .check() to point it out and a test to demonstrate we now handle it >>> correctly. >> Happy to add that, is that what you'd like? > Yes, please. That would address my concerns. > >>>> If you'd rather have a pull request on github, that's fine, happy to >>>> resend there. Also, please let me know if this patchbomb gives you any >>>> problems, as I sent it with Mercurial. >>> This seems to work correctly at the moment. I also wouldn't mind if you >>> sent Mercurial bundles though, I still have to implement those in bzr-hg >>> and I could use some test cases. ;-) >> Eh, mailing bundles makes patch review hard. > Sorry, I hadn't realized hg bundles were binary. > > 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
