{"thread":{"id":"35297","subject":"[PATCH 0/5] fix up 'jk/pack-bitmap' branch","startedAt":"2013-11-07T21:58:02Z","lastAt":"2013-11-09T21:21:07Z","messageCount":10,"participants":["Ramsay Jones","Jeff King","Vicent Martí","Torsten Bögershausen","Johannes Sixt","Thomas Rast"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"230218","messageId":"527C0CEA.4020705@ramsay1.demon.co.uk","threadId":"35297","inReplyTo":null,"subject":"[PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-11-07T21:58:02Z","receivedAt":"2013-11-07T21:58:02Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"Hi Jeff,\n\nThese patches fix various errors/warnings on the cygwin, MinGW and\nmsvc builds, provoked by the jk/pack-bitmap branch.\n\nNote that this does not fix all problems on the msvc build; I have\na solution, but I don't like it. :-D So, I'm going to try a different\nfix. I had hoped to have done this by now, but ... (hopefully, sometime\nthis weekend).\n\nNote that I have only tested the cygwin and MinGW builds by running\nthe t5310-pack-bitmaps.sh test, and only on a little-endian machine.\n(Torsten has tested the first patch on a big-endian)\n\nNote also, that these patches are build on top of the 'pu' branch\nas of yesterday (pu @ 2b65d9ebc).\n\nSo, could you please squash these patches into the relevant commits\non your branch.\n\nThanks!\n\nATB,\nRamsay Jones\n\nRamsay Jones (5):\n compat/bswap.h: Fix build on cygwin, MinGW and msvc\n Makefile: Add object files in ewah/ to clean target\n khash.h: Spell the null pointer as NULL\n pack-objects: Limit visibility of 'indexed_commits' symbols\n ewah_bitmap.c: Fix printf format warnings on MinGW\n\n Makefile | 5 +--\n builtin/pack-objects.c | 6 ++--\n compat/bswap.h | 97 +++++++++++++++++++++++++++++++++++---------------\n ewah/ewah_bitmap.c | 6 ++--\n khash.h | 2 +-\n 5 files changed, 79 insertions(+), 37 deletions(-)\n\n-- \n1.8.4\n"},{"id":"230224","messageId":"20131107221944.GA19238@sigill.intra.peff.net","threadId":"35297","inReplyTo":"527C0CEA.4020705@ramsay1.demon.co.uk","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-07T22:19:44Z","receivedAt":"2013-11-07T22:19:44Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote:\n\n> These patches fix various errors/warnings on the cygwin, MinGW and\n> msvc builds, provoked by the jk/pack-bitmap branch.\n\nThanks. Your timing is impeccable, as I was just sitting down to\nfinalize the next re-roll. I'll add these in.\n\n-Peff\n"},{"id":"230230","messageId":"CAFFjANR-4x4qDoRQtDLyd8VnpeQBDsg3mNh+tV87E-ZHDqnxSA@mail.gmail.com","threadId":"35297","inReplyTo":"20131107221944.GA19238@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Vicent Martí","fromEmail":"tanoku@gmail.com","sentAt":"2013-11-08T01:39:12Z","receivedAt":"2013-11-08T01:39:12Z","isPatch":true,"sender":{"key":"tanoku@gmail.com","avatar":"https://gravatar.com/avatar/271386991cb4c2b8f1e1ed1d059f3422cc3485de7a598f65043f70be021d095b?d=mp&s=160"},"body":"Thank you Ramsay, all the patches look OK to me.\n\nOn Thu, Nov 7, 2013 at 11:19 PM, Jeff King wrote:\n> On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote:\n>\n>> These patches fix various errors/warnings on the cygwin, MinGW and\n>> msvc builds, provoked by the jk/pack-bitmap branch.\n>\n> Thanks. Your timing is impeccable, as I was just sitting down to\n> finalize the next re-roll. I'll add these in.\n>\n> -Peff\n"},{"id":"230239","messageId":"527D1B06.3090807@web.de","threadId":"35297","inReplyTo":"20131107221944.GA19238@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-08T17:10:30Z","receivedAt":"2013-11-08T17:10:30Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-11-07 23.19, Jeff King wrote:\n> On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote:\n> \n>> These patches fix various errors/warnings on the cygwin, MinGW and\n>> msvc builds, provoked by the jk/pack-bitmap branch.\n> \n> Thanks. Your timing is impeccable, as I was just sitting down to\n> finalize the next re-roll. I'll add these in.\n> \n> -Peff\n\nSide question:\nDo we have enough test coverage for htonll()/ntohll(),\nor do we want do the \"module test\" which I send a couple of days before ?\n/Torsten\n"},{"id":"230243","messageId":"527D2C0B.6040604@ramsay1.demon.co.uk","threadId":"35297","inReplyTo":"527D1B06.3090807@web.de","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Ramsay Jones","fromEmail":"ramsay@ramsay1.demon.co.uk","sentAt":"2013-11-08T18:23:07Z","receivedAt":"2013-11-08T18:23:07Z","isPatch":true,"sender":{"key":"ramsay@ramsayjones.plus.com","avatar":"https://avatars.githubusercontent.com/u/33702710?v=4"},"body":"On 08/11/13 17:10, Torsten Bögershausen wrote:\n> On 2013-11-07 23.19, Jeff King wrote:\n>> On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote:\n>>\n>>> These patches fix various errors/warnings on the cygwin, MinGW and\n>>> msvc builds, provoked by the jk/pack-bitmap branch.\n>>\n>> Thanks. Your timing is impeccable, as I was just sitting down to\n>> finalize the next re-roll. I'll add these in.\n>>\n>> -Peff\n> \n> Side question:\n> Do we have enough test coverage for htonll()/ntohll(),\n> or do we want do the \"module test\" which I send a couple of days before ?\n\nYes, sorry for not bringing that up; I didn't get to try (or even look at)\nyour test patch, because I couldn't 'git am' it. :(\n\nI've been meaning to look into why that is, but just haven't had time yet.\n(I think it may have something to do with your name - I've noticed in the\npast that any mail with utf8 characters causes problems.)\n\nHowever, I should have alerted Jeff to your patch; sorry about that!\n\nATB,\nRamsay Jones\n"},{"id":"230251","messageId":"20131108222903.GB19912@sigill.intra.peff.net","threadId":"35297","inReplyTo":"527D1B06.3090807@web.de","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-08T22:29:03Z","receivedAt":"2013-11-08T22:29:03Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote:\n\n> Side question:\n> Do we have enough test coverage for htonll()/ntohll(),\n> or do we want do the \"module test\" which I send a couple of days before ?\n\nThe series adds tests for building and using the ewah bitmaps, which in\nturn rely on the htonll code. So they are being tested in the existing\nseries.\n\n-Peff\n"},{"id":"230356","messageId":"527E1DED.9030800@web.de","threadId":"35297","inReplyTo":"20131108222903.GB19912@sigill.intra.peff.net","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Torsten Bögershausen","fromEmail":"tboegi@web.de","sentAt":"2013-11-09T11:35:09Z","receivedAt":"2013-11-09T11:35:09Z","isPatch":true,"sender":{"key":"tboegi@web.de","avatar":"https://avatars.githubusercontent.com/u/7138363?v=4"},"body":"On 2013-11-08 23.29, Jeff King wrote:\n> On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote:\n> \n>> Side question:\n>> Do we have enough test coverage for htonll()/ntohll(),\n>> or do we want do the \"module test\" which I send a couple of days before ?\n> \n> The series adds tests for building and using the ewah bitmaps, which in\n> turn rely on the htonll code. So they are being tested in the existing\n> series.\n> \n> -Peff\nYou are thinking about t5310-pack-bitmaps.sh ?\nIf I do like this in compat/bswap.h\n\n# define ntohll(n) (n)\n# define htonll(n) (n)\n(on an Intel processor, little endian)\n\nthen t5310 passes, even if the uint64_t words are written\nin little endian to disc instead of big endian.\n\nWhat do I miss?\n/torsten\n"},{"id":"230361","messageId":"527E7021.8060204@kdbg.org","threadId":"35297","inReplyTo":"527E1DED.9030800@web.de","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Johannes Sixt","fromEmail":"j6t@kdbg.org","sentAt":"2013-11-09T17:25:53Z","receivedAt":"2013-11-09T17:25:53Z","isPatch":true,"sender":{"key":"j6t@kdbg.org","avatar":"https://avatars.githubusercontent.com/u/14810926?v=4"},"body":"Am 09.11.2013 12:35, schrieb Torsten Bögershausen:\n> On 2013-11-08 23.29, Jeff King wrote:\n>> On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote:\n>>\n>>> Side question:\n>>> Do we have enough test coverage for htonll()/ntohll(),\n>>> or do we want do the \"module test\" which I send a couple of days before ?\n>>\n>> The series adds tests for building and using the ewah bitmaps, which in\n>> turn rely on the htonll code. So they are being tested in the existing\n>> series.\n>>\n>> -Peff\n> You are thinking about t5310-pack-bitmaps.sh ?\n> If I do like this in compat/bswap.h\n> \n> # define ntohll(n) (n)\n> # define htonll(n) (n)\n> (on an Intel processor, little endian)\n> \n> then t5310 passes, even if the uint64_t words are written\n> in little endian to disc instead of big endian.\n\nOf course. You write little endian and also read back little endian;\nthat should work just fine, no?\n\nOTOH, if you write with Intel and read with PPC, then you should observe\nmisbehavior with the above patch.\n\n-- Hannes\n"},{"id":"230363","messageId":"87txflnv7n.fsf@linux-k42r.v.cablecom.net","threadId":"35297","inReplyTo":"527E7021.8060204@kdbg.org","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Thomas Rast","fromEmail":"tr@thomasrast.ch","sentAt":"2013-11-09T21:03:56Z","receivedAt":"2013-11-09T21:03:56Z","isPatch":true,"sender":{"key":"tr@thomasrast.ch","avatar":"https://avatars.githubusercontent.com/u/153510?v=4"},"body":"Johannes Sixt writes:\n\n> Am 09.11.2013 12:35, schrieb Torsten Bögershausen:\n>>\n>> If I do like this in compat/bswap.h\n>> \n>> # define ntohll(n) (n)\n>> # define htonll(n) (n)\n>> (on an Intel processor, little endian)\n>> \n>> then t5310 passes, even if the uint64_t words are written\n>> in little endian to disc instead of big endian.\n>\n> Of course. You write little endian and also read back little endian;\n> that should work just fine, no?\n>\n> OTOH, if you write with Intel and read with PPC, then you should observe\n> misbehavior with the above patch.\n\nMaybe you could check in a simple test that the bitmap for a very\npredictable pack (e.g. only one commit) has a certain content, by\nchecking its hash. That would guard against accidental format changes.\n\n-- \nThomas Rast\ntr@thomasrast.ch\n"},{"id":"230364","messageId":"20131109212106.GA2712@sigill.intra.peff.net","threadId":"35297","inReplyTo":"87txflnv7n.fsf@linux-k42r.v.cablecom.net","subject":"Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2013-11-09T21:21:07Z","receivedAt":"2013-11-09T21:21:07Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Nov 09, 2013 at 10:03:56PM +0100, Thomas Rast wrote:\n\n> > Of course. You write little endian and also read back little endian;\n> > that should work just fine, no?\n> >\n> > OTOH, if you write with Intel and read with PPC, then you should observe\n> > misbehavior with the above patch.\n> \n> Maybe you could check in a simple test that the bitmap for a very\n> predictable pack (e.g. only one commit) has a certain content, by\n> checking its hash. That would guard against accidental format changes.\n\nAnother option would be to cross-check reading and writing against JGit,\nwhich is valuable for other reasons besides checking endianness. I do\nnot think most people will have JGit installed, but it's better than\nnothing (and if even one person runs it, we have a chance of finding a\nbug).\n\n-Peff\n"}]}