[PATCH 0/5] fix up 'jk/pack-bitmap' branch

[PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Ramsay Jones]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Jeff King]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Vicent Martí]

Thank you Ramsay, all the patches look OK to me.

On Thu, Nov 7, 2013 at 11:19 PM, Jeff King <peff@peff.net> 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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Torsten Bögershausen]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Ramsay Jones]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Jeff King]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[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.

What do I miss?
/torsten

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Johannes Sixt]

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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Thomas Rast]

Johannes Sixt <j6t@kdbg.org> 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

Re: [PATCH 0/5] fix up 'jk/pack-bitmap' branch

[Jeff King]

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