# [PATCH 0/5] fix up 'jk/pack-bitmap' branch 10 messages from 2013-11-07 to 2013-11-09. Participants: Ramsay Jones, Jeff King, Vicent Martí, Torsten Bögershausen, Johannes Sixt, Thomas Rast. Thread: https://gitlist.dev/t/35297 ## Ramsay Jones, 2013-11-07 21:58 Subject: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <527C0CEA.4020705@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/527C0CEA.4020705%40ramsay1.demon.co.uk ``` Hi Jeff, These patches fix various errors/warnings on the cygwin, MinGW and msvc builds, provoked by the jk/pack-bitmap branch. Note that this does not fix all problems on the msvc build; I have a solution, but I don't like it. :-D So, I'm going to try a different fix. I had hoped to have done this by now, but ... (hopefully, sometime this weekend). Note that I have only tested the cygwin and MinGW builds by running the t5310-pack-bitmaps.sh test, and only on a little-endian machine. (Torsten has tested the first patch on a big-endian) Note also, that these patches are build on top of the 'pu' branch as of yesterday (pu @ 2b65d9ebc). So, could you please squash these patches into the relevant commits on your branch. Thanks! ATB, Ramsay Jones Ramsay Jones (5): compat/bswap.h: Fix build on cygwin, MinGW and msvc Makefile: Add object files in ewah/ to clean target khash.h: Spell the null pointer as NULL pack-objects: Limit visibility of 'indexed_commits' symbols ewah_bitmap.c: Fix printf format warnings on MinGW Makefile | 5 +-- builtin/pack-objects.c | 6 ++-- compat/bswap.h | 97 +++++++++++++++++++++++++++++++++++--------------- ewah/ewah_bitmap.c | 6 ++-- khash.h | 2 +- 5 files changed, 79 insertions(+), 37 deletions(-) -- 1.8.4 ``` ## Jeff King, 2013-11-07 22:19 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <20131107221944.GA19238@sigill.intra.peff.net> URL: https://gitlist.dev/e/20131107221944.GA19238%40sigill.intra.peff.net In-Reply-To: <527C0CEA.4020705@ramsay1.demon.co.uk> ``` On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote: > These patches fix various errors/warnings on the cygwin, MinGW and > msvc builds, provoked by the jk/pack-bitmap branch. Thanks. Your timing is impeccable, as I was just sitting down to finalize the next re-roll. I'll add these in. -Peff ``` ## Vicent Martí, 2013-11-08 01:39 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: URL: https://gitlist.dev/e/CAFFjANR-4x4qDoRQtDLyd8VnpeQBDsg3mNh%2BtV87E-ZHDqnxSA%40mail.gmail.com In-Reply-To: <20131107221944.GA19238@sigill.intra.peff.net> ``` Thank you Ramsay, all the patches look OK to me. On Thu, Nov 7, 2013 at 11:19 PM, Jeff King wrote: > On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote: > >> These patches fix various errors/warnings on the cygwin, MinGW and >> msvc builds, provoked by the jk/pack-bitmap branch. > > Thanks. Your timing is impeccable, as I was just sitting down to > finalize the next re-roll. I'll add these in. > > -Peff ``` ## Torsten Bögershausen, 2013-11-08 17:10 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <527D1B06.3090807@web.de> URL: https://gitlist.dev/e/527D1B06.3090807%40web.de In-Reply-To: <20131107221944.GA19238@sigill.intra.peff.net> ``` On 2013-11-07 23.19, Jeff King wrote: > On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote: > >> These patches fix various errors/warnings on the cygwin, MinGW and >> msvc builds, provoked by the jk/pack-bitmap branch. > > Thanks. Your timing is impeccable, as I was just sitting down to > finalize the next re-roll. I'll add these in. > > -Peff Side question: Do we have enough test coverage for htonll()/ntohll(), or do we want do the "module test" which I send a couple of days before ? /Torsten ``` ## Ramsay Jones, 2013-11-08 18:23 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <527D2C0B.6040604@ramsay1.demon.co.uk> URL: https://gitlist.dev/e/527D2C0B.6040604%40ramsay1.demon.co.uk In-Reply-To: <527D1B06.3090807@web.de> ``` On 08/11/13 17:10, Torsten Bögershausen wrote: > On 2013-11-07 23.19, Jeff King wrote: >> On Thu, Nov 07, 2013 at 09:58:02PM +0000, Ramsay Jones wrote: >> >>> These patches fix various errors/warnings on the cygwin, MinGW and >>> msvc builds, provoked by the jk/pack-bitmap branch. >> >> Thanks. Your timing is impeccable, as I was just sitting down to >> finalize the next re-roll. I'll add these in. >> >> -Peff > > Side question: > Do we have enough test coverage for htonll()/ntohll(), > or do we want do the "module test" which I send a couple of days before ? Yes, sorry for not bringing that up; I didn't get to try (or even look at) your test patch, because I couldn't 'git am' it. :( I've been meaning to look into why that is, but just haven't had time yet. (I think it may have something to do with your name - I've noticed in the past that any mail with utf8 characters causes problems.) However, I should have alerted Jeff to your patch; sorry about that! ATB, Ramsay Jones ``` ## Jeff King, 2013-11-08 22:29 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <20131108222903.GB19912@sigill.intra.peff.net> URL: https://gitlist.dev/e/20131108222903.GB19912%40sigill.intra.peff.net In-Reply-To: <527D1B06.3090807@web.de> ``` On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote: > Side question: > Do we have enough test coverage for htonll()/ntohll(), > or do we want do the "module test" which I send a couple of days before ? The series adds tests for building and using the ewah bitmaps, which in turn rely on the htonll code. So they are being tested in the existing series. -Peff ``` ## Torsten Bögershausen, 2013-11-09 11:35 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <527E1DED.9030800@web.de> URL: https://gitlist.dev/e/527E1DED.9030800%40web.de In-Reply-To: <20131108222903.GB19912@sigill.intra.peff.net> ``` On 2013-11-08 23.29, Jeff King wrote: > On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote: > >> Side question: >> Do we have enough test coverage for htonll()/ntohll(), >> or do we want do the "module test" which I send a couple of days before ? > > The series adds tests for building and using the ewah bitmaps, which in > turn rely on the htonll code. So they are being tested in the existing > series. > > -Peff You are thinking about t5310-pack-bitmaps.sh ? If I do like this in compat/bswap.h # define ntohll(n) (n) # define htonll(n) (n) (on an Intel processor, little endian) then t5310 passes, even if the uint64_t words are written in little endian to disc instead of big endian. What do I miss? /torsten ``` ## Johannes Sixt, 2013-11-09 17:25 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <527E7021.8060204@kdbg.org> URL: https://gitlist.dev/e/527E7021.8060204%40kdbg.org In-Reply-To: <527E1DED.9030800@web.de> ``` Am 09.11.2013 12:35, schrieb Torsten Bögershausen: > On 2013-11-08 23.29, Jeff King wrote: >> On Fri, Nov 08, 2013 at 06:10:30PM +0100, Torsten Bögershausen wrote: >> >>> Side question: >>> Do we have enough test coverage for htonll()/ntohll(), >>> or do we want do the "module test" which I send a couple of days before ? >> >> The series adds tests for building and using the ewah bitmaps, which in >> turn rely on the htonll code. So they are being tested in the existing >> series. >> >> -Peff > You are thinking about t5310-pack-bitmaps.sh ? > If I do like this in compat/bswap.h > > # define ntohll(n) (n) > # define htonll(n) (n) > (on an Intel processor, little endian) > > then t5310 passes, even if the uint64_t words are written > in little endian to disc instead of big endian. Of course. You write little endian and also read back little endian; that should work just fine, no? OTOH, if you write with Intel and read with PPC, then you should observe misbehavior with the above patch. -- Hannes ``` ## Thomas Rast, 2013-11-09 21:03 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <87txflnv7n.fsf@linux-k42r.v.cablecom.net> URL: https://gitlist.dev/e/87txflnv7n.fsf%40linux-k42r.v.cablecom.net In-Reply-To: <527E7021.8060204@kdbg.org> ``` Johannes Sixt writes: > Am 09.11.2013 12:35, schrieb Torsten Bögershausen: >> >> If I do like this in compat/bswap.h >> >> # define ntohll(n) (n) >> # define htonll(n) (n) >> (on an Intel processor, little endian) >> >> then t5310 passes, even if the uint64_t words are written >> in little endian to disc instead of big endian. > > Of course. You write little endian and also read back little endian; > that should work just fine, no? > > OTOH, if you write with Intel and read with PPC, then you should observe > misbehavior with the above patch. Maybe you could check in a simple test that the bitmap for a very predictable pack (e.g. only one commit) has a certain content, by checking its hash. That would guard against accidental format changes. -- Thomas Rast tr@thomasrast.ch ``` ## Jeff King, 2013-11-09 21:21 Subject: Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch Message-ID: <20131109212106.GA2712@sigill.intra.peff.net> URL: https://gitlist.dev/e/20131109212106.GA2712%40sigill.intra.peff.net In-Reply-To: <87txflnv7n.fsf@linux-k42r.v.cablecom.net> ``` On Sat, Nov 09, 2013 at 10:03:56PM +0100, Thomas Rast wrote: > > Of course. You write little endian and also read back little endian; > > that should work just fine, no? > > > > OTOH, if you write with Intel and read with PPC, then you should observe > > misbehavior with the above patch. > > Maybe you could check in a simple test that the bitmap for a very > predictable pack (e.g. only one commit) has a certain content, by > checking its hash. That would guard against accidental format changes. Another option would be to cross-check reading and writing against JGit, which is valuable for other reasons besides checking endianness. I do not think most people will have JGit installed, but it's better than nothing (and if even one person runs it, we have a chance of finding a bug). -Peff ```