[PATCH] Cast things properly to handle >2G files.

[PATCH] Cast things properly to handle >2G files.

[Alfred M. Szmidt]

This small patch fixes things so that repack, fsck, and other things
work on >2GiB files.  There are still some other problems (cloning
over ssh being one), but this makes it atleast possible to handle such
files.

(not subscribed, please CC)

---
 delta.h       |    2 +-
 patch-delta.c |    2 +-
 sha1_file.c   |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/delta.h b/delta.h
index 40ccf5a..b3acc72 100644
--- a/delta.h
+++ b/delta.h
@@ -95,7 +95,7 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,
 	int i = 0;
 	do {
 		cmd = *data++;
-		size |= (cmd & ~0x80) << i;
+		size |= (cmd & ~0x80UL) << i;
 		i += 7;
 	} while (cmd & 0x80 && data < top);
 	*datap = data;
diff --git a/patch-delta.c b/patch-delta.c
index ed9db81..a9ad2bc 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -44,7 +44,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			if (cmd & 0x01) cp_off = *data++;
 			if (cmd & 0x02) cp_off |= (*data++ << 8);
 			if (cmd & 0x04) cp_off |= (*data++ << 16);
-			if (cmd & 0x08) cp_off |= (*data++ << 24);
+			if (cmd & 0x08) cp_off |= ((unsigned long) *data++ << 24);
 			if (cmd & 0x10) cp_size = *data++;
 			if (cmd & 0x20) cp_size |= (*data++ << 8);
 			if (cmd & 0x40) cp_size |= (*data++ << 16);
diff --git a/sha1_file.c b/sha1_file.c
index e73cd4f..4566ea1 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1176,7 +1176,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
 			return 0;
 		}
 		c = buf[used++];
-		size += (c & 0x7f) << shift;
+		size += (c & 0x7fUL) << shift;
 		shift += 7;
 	}
 	*sizep = size;
-- 
1.6.3.2.225.gb8364.dirty

Re: [PATCH] Cast things properly to handle >2G files.

[Johannes Schindelin]

Hi,

Disclaimer: if you are offended by constructive criticism, or likely to
answer with insults to the comments I offer, please stop reading this mail
now (and please do not answer my mail, either). :-)

Still with me?  Good.  Nice to meet you.

Just for the record: responding to a patch is my strongest way of saying
that I appreciate your work.

On Sun, 14 Jun 2009, Alfred M. Szmidt wrote:

> This small patch fixes things so that repack, fsck, and other things
> work on >2GiB files.  There are still some other problems (cloning
> over ssh being one), but this makes it atleast possible to handle such
> files.
> 
> (not subscribed, please CC)

This is almost perfect, but please add a Signed-off-by: line.

Oh, and maybe add a test like t/t5705-clone-2gb.sh?

Ciao,
Dscho

Re: [PATCH] Cast things properly to handle >2G files.

[Nicolas Pitre]

On Sun, 14 Jun 2009, Johannes Schindelin wrote:

> Hi,
> 
> Disclaimer: if you are offended by constructive criticism, or likely to
> answer with insults to the comments I offer, please stop reading this mail
> now (and please do not answer my mail, either). :-)
> 
> Still with me?  Good.  Nice to meet you.
> 
> Just for the record: responding to a patch is my strongest way of saying
> that I appreciate your work.
> 
> On Sun, 14 Jun 2009, Alfred M. Szmidt wrote:
> 
> > This small patch fixes things so that repack, fsck, and other things
> > work on >2GiB files.  There are still some other problems (cloning
> > over ssh being one), but this makes it atleast possible to handle such
> > files.
> > 
> > (not subscribed, please CC)
> 
> This is almost perfect, but please add a Signed-off-by: line.
> 
> Oh, and maybe add a test like t/t5705-clone-2gb.sh?

Creating test for files >= 2GB is kinda hard.  It'll be really slow and 
take a lot of disk space, and I doubt people will be happy about that.

Been there.  That's the only reason why I created the --index-version 
argument to pack-objects: to fake behavior for huge files by the test 
suite.


Nicolas

Re: [PATCH] Cast things properly to handle >2G files.

[Alfred M. Szmidt]

   > This small patch fixes things so that repack, fsck, and other
   > things work on >2GiB files.  There are still some other problems
   > (cloning over ssh being one), but this makes it atleast possible
   > to handle such files.

   This is almost perfect, but please add a Signed-off-by: line.

I cannot agree to the D-C-O in good faith, since it speaks of open
source licenses, a group of licenses that include non-free software
licenses, something which I cannot support.

   Oh, and maybe add a test like t/t5705-clone-2gb.sh?

Thank you, that is a very good idea.

(still not subscribed, please CC)

Re: [PATCH] Cast things properly to handle >2G files.

[Junio C Hamano]

"Alfred M. Szmidt" <ams@gnu.org> writes:

> I cannot agree to the D-C-O in good faith, since it speaks of open
> source licenses, a group of licenses that include non-free software
> licenses, something which I cannot support.

DCO is not something you "agree to".

Are you the original author of the patch, and have the right to submit it
under the license "indicated in the file"?

The overall license of git is GPLv2, and that is what applies to unless
there is an explicit license term indicated in the file otherwise. We do
have some code under different licenses in some parts of the system, but
the files that you are touching are all GPLv2.

Can you certify that your patch is yours and you have rights to make it
part of git under the same terms as the original?  Or can you not?  There
is nothing for you to "agree to"; just a simple "yes or no", and I hope
the answer is yes in this case, judging from your gnu.org address.

>    Oh, and maybe add a test like t/t5705-clone-2gb.sh?
>
> Thank you, that is a very good idea.

Nah, one blob that is over 2gb, deltified against something else?  That's
a bit too much for a test script.

Re: [PATCH] Cast things properly to handle >2G files.

[Linus Torvalds]

On Sun, 14 Jun 2009, Alfred M. Szmidt wrote:
> 
> I cannot agree to the D-C-O in good faith, since it speaks of open
> source licenses, a group of licenses that include non-free software
> licenses, something which I cannot support.

If you can't sign off on it, then Junio shouldn't take it, since you're 
basically saying that you cannot say that you own the copyrights or cannot 
license it under the appropriate copyright.

Yes, it speaks of "open source licenses", but it says: "under the open 
source license indicated in the file", and "appropriate open source
license".

For git, that's GPLv2 (or GPLv2-compatible, ie something like the 
simplified BSD license that can just be converted to GPLv2).

The DCO is phrased that way so that other projects that use things like 
the BSD or Apache license can still use the DCO as-is.

Side note: for somebody with a "gnu.org" address, you're showing some 
really bad taste. Do you know that the FSF ends up asking for a hell of a 
lot of paperwork and other crazy things to take peoples submissions. And 
they actually want the copyrights signed over, so that they can change it 
to _any_ license.

The DCO, in contrast, is a paragon of simplicity and clarity, and doesn't 
ask you to sign away any rights.

			Linus

Re: [PATCH] Cast things properly to handle >2G files.

[Johannes Schindelin]

Hi,

On Sun, 14 Jun 2009, Junio C Hamano wrote:

> "Alfred M. Szmidt" <ams@gnu.org> writes:
> 
> >    Oh, and maybe add a test like t/t5705-clone-2gb.sh?
> >
> > Thank you, that is a very good idea.
> 
> Nah, one blob that is over 2gb, deltified against something else?  
> That's a bit too much for a test script.

Why not?  It's not a difficult thing to come up with such a test.  And 
just like t5705, it can be hidden behind an "EXPENSIVE_TEST=YesPlease" 
environment variable.  Maybe I should have spelt that out loud instead of 
relying on interested parties to know t5705.

And just like t5705 it should help people to verify that the patch solves 
the problems indeed.

Ciao,
Dscho

Re: [PATCH] Cast things properly to handle >2G files.

[Alfred M. Szmidt]

   Side note: for somebody with a "gnu.org" address, you're showing
   some really bad taste. Do you know that the FSF ends up asking for
   a hell of a lot of paperwork and other crazy things to take peoples
   submissions. And they actually want the copyrights signed over, so
   that they can change it to _any_ license.

   The DCO, in contrast, is a paragon of simplicity and clarity, and
   doesn't ask you to sign away any rights.

You are misinformed.  The copyright assignments from the FSF explictly
state that the code will always be free software, and they grant back
all rights to the code you assigned to them.

Re: [PATCH] Cast things properly to handle >2G files.

[Alfred M. Szmidt]

   > I cannot agree to the D-C-O in good faith, since it speaks of
   > open source licenses, a group of licenses that include non-free
   > software licenses, something which I cannot support.

   If you can't sign off on it, then Junio shouldn't take it, since
   you're basically saying that you cannot say that you own the
   copyrights or cannot license it under the appropriate copyright.

That is not what I am saying.  In either case, I already explained why
I cannot agree to the DCO above.  And you are right, Junio shouldn't
apply the patch if it goes against commit policies.

   Yes, it speaks of "open source licenses", but it says: "under the
   open source license indicated in the file", and "appropriate open
   source license".

That is a good point, almost none of the files in git specify the
license so it is impossible to know what one is actually agreeing too.

I would suggest the following update to the DCO, it makes it more
general both to free software hackers, and open source hackers alike.

  Developer's Certificate of Origin 1.2-draft

  By making a contribution to this project, I certify that:

  (a) The contribution was created in whole or in part by me and I
      have the right to submit it under the license indicated in the
      file; or

  (b) The contribution is based upon previous work that, to the best
      of my knowledge, is covered under an compatible license and I
      have the right under that license to submit that work with
      modifications, whether created in whole or in part by me, under
      the same license (unless I am permitted to submit under a
      different license), as indicated in the file; or

  (c) The contribution was provided directly to me by some other
      person who certified (a), (b) or (c) and I have not modified it.

  (d) I understand and agree that this project and the contribution
      are public and that a record of the contribution (including all
      personal information I submit with it, including my sign-off) is
      maintained indefinitely and may be redistributed consistent with
      this project or the license(s) involved.

Re: [PATCH] Cast things properly to handle >2G files.

[Linus Torvalds]

On Wed, 17 Jun 2009, Alfred M. Szmidt wrote:
> 
> I would suggest the following update to the DCO, it makes it more
> general both to free software hackers, and open source hackers alike.

And I would suggest that Junio just not take patches from people who 
aren't able to read the existing DCO. It's not worth the pain. 

		Linus

Re: [PATCH] Cast things properly to handle >2G files.

[Linus Torvalds]

On Wed, 17 Jun 2009, Alfred M. Szmidt wrote:
>    The DCO, in contrast, is a paragon of simplicity and clarity, and
>    doesn't ask you to sign away any rights.
> 
> You are misinformed.  The copyright assignments from the FSF explictly
> state that the code will always be free software, and they grant back
> all rights to the code you assigned to them.

Blah. They define the term "free software" too, so that doesn't make any 
difference. They can relicense it any crazy way they want, as shown by the 
whole annoying GPLv3 idiocy. It doesn't matter one whit whether you agree 
with them or not.

And isn't it "nice" of them to not require exclusive ownership? Gag. And 
what a bunch of hypocritical people they are too - because they'll happily 
take code from other people without any copyright assignment what-so-ever 
when they feel like it (case in point: Hurd took drivers and filesystems 
from Linux, and I can pretty much guarantee that they didn't have 
copyright assignments for any of it - but hey, rules are rules only when 
they apply to _other_ people, right?)

And you didn't face the actual issue: the papers the FSF makes you sign 
are _way_ less obvious than the DCO is.

		Linus

Re: [PATCH] Cast things properly to handle >2G files.

[Linus Torvalds]

On Wed, 17 Jun 2009, Alfred M. Szmidt wrote:
> 
> That is a good point, almost none of the files in git specify the
> license so it is impossible to know what one is actually agreeing too.

Right. Because that "COPYING" file is so hard to find.

You're making excuses. Just admit it.

		Linus

Re: [PATCH] Cast things properly to handle >2G files.

[Junio C Hamano]

Linus Torvalds <torvalds@linux-foundation.org> writes:

> On Wed, 17 Jun 2009, Alfred M. Szmidt wrote:
>> 
>> I would suggest the following update to the DCO, it makes it more
>> general both to free software hackers, and open source hackers alike.
>
> And I would suggest that Junio just not take patches from people who 
> aren't able to read the existing DCO. It's not worth the pain. 
>
> 		Linus

Thanks for trying to reduce my load.  Very much appreciated.

Even though I am _not_ a nice person, I _am_ a practical one.  Before
stopping to pay attention to this thread, I'll quote from my response once
more to ask a simple yes-or-no questions to Alfred.

  Are you the original author of the patch, and have the right to submit
  it under the license "indicated in the file"?

  The overall license of git is GPLv2, and that is what applies to unless
  there is an explicit license term indicated in the file otherwise. We do
  have some code under different licenses in some parts of the system, but
  the files that you are touching are all GPLv2.

  Can you certify that your patch is yours and you have rights to make it
  part of git under the same terms as the original?  Or can you not?

So Alfred, "yes"?  or "no"?  If "yes", you can send a sign-off.

Of course you can choose not to even if the answer is "yes", but then we
can choose to ignore your patch, too; and in fact we probably have to.

Fix big left-shifts of unsigned char

[Linus Torvalds]

Shifting 'unsigned char' or 'unsigned short' left can result in sign 
extension errors, since the C integer promotion rules means that the 
unsigned char/short will get implicitly promoted to a signed 'int' due to 
the shift (or due to other operations).

This normally doesn't matter, but if you shift things up sufficiently, it 
will now set the sign bit in 'int', and a subsequent cast to a bigger type 
(eg 'long' or 'unsigned long') will now sign-extend the value despite the 
original expression being unsigned.

One example of this would be something like

	unsigned long size;
	unsigned char c;

	size += c << 24;

where despite all the variables being unsigned, 'c << 24' ends up being a 
signed entity, and will get sign-extended when then doing the addition in 
an 'unsigned long' type.

Since git uses 'unsigned char' pointers extensively, we actually have this 
bug in a couple of places. 

I may have missed some, but this is the result of looking at

	git grep '[^0-9 	][ 	]*<<[ 	][a-z]' -- '*.c' '*.h'
	git grep '<<[   ]*24'

which catches at least the common byte cases (shifting variables by a 
variable amount, and shifting by 24 bits).

I also grepped for just 'unsigned char' variables in general, and 
converted the ones that most obviously ended up getting implicitly cast 
immediately anyway (eg hash_name(), encode_85()).

In addition to just avoiding 'unsigned char', this patch also tries to use 
a common idiom for the delta header size thing. We had three different 
variations on it: "& 0x7fUL" in one place (getting the sign extension 
right), and "& ~0x80" and "& 0x7f" in two other places (not getting it 
right). Apart from making them all just avoid using "unsigned char" at 
all, I also unified them to then use a simple "& 0x7f".

I considered making a sparse extension which warns about doing implicit 
casts from unsigned types to signed types, but it gets rather complex very 
quickly, so this is just a hack. 

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

This is _not_ tested in any way. And I got bored with getting rid of 
'unsigned char' variables, so I by no means did all of them, just the 
first few that caugth my grepping eye.

On Wed, 17 Jun 2009, Linus Torvalds wrote:
> 
> And I would suggest that Junio just not take patches from people who 
> aren't able to read the existing DCO. It's not worth the pain. 
> 
> 		Linus
> 

 attr.c                   |    3 +--
 base85.c                 |    2 +-
 builtin-pack-objects.c   |    3 +--
 builtin-unpack-objects.c |    4 ++--
 delta.h                  |    5 ++---
 index-pack.c             |    6 +++---
 patch-delta.c            |    2 +-
 sha1_file.c              |    3 +--
 8 files changed, 12 insertions(+), 16 deletions(-)

diff --git a/attr.c b/attr.c
index 98eb636..f8f6faa 100644
--- a/attr.c
+++ b/attr.c
@@ -35,8 +35,7 @@ static struct git_attr *(git_attr_hash[HASHSIZE]);
 
 static unsigned hash_name(const char *name, int namelen)
 {
-	unsigned val = 0;
-	unsigned char c;
+	unsigned val = 0, c;
 
 	while (namelen--) {
 		c = *name++;
diff --git a/base85.c b/base85.c
index b88270f..b417a15 100644
--- a/base85.c
+++ b/base85.c
@@ -91,7 +91,7 @@ void encode_85(char *buf, const unsigned char *data, int bytes)
 		unsigned acc = 0;
 		int cnt;
 		for (cnt = 24; cnt >= 0; cnt -= 8) {
-			int ch = *data++;
+			unsigned ch = *data++;
 			acc |= ch << cnt;
 			if (--bytes == 0)
 				break;
diff --git a/builtin-pack-objects.c b/builtin-pack-objects.c
index 9742b45..941cc2d 100644
--- a/builtin-pack-objects.c
+++ b/builtin-pack-objects.c
@@ -653,8 +653,7 @@ static void rehash_objects(void)
 
 static unsigned name_hash(const char *name)
 {
-	unsigned char c;
-	unsigned hash = 0;
+	unsigned c, hash = 0;
 
 	if (!name)
 		return 0;
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 9a77323..8e831be 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -422,8 +422,8 @@ static void unpack_delta_entry(enum object_type type, unsigned long delta_size,
 static void unpack_one(unsigned nr)
 {
 	unsigned shift;
-	unsigned char *pack, c;
-	unsigned long size;
+	unsigned char *pack;
+	unsigned long size, c;
 	enum object_type type;
 
 	obj_list[nr].offset = consumed_bytes;
diff --git a/delta.h b/delta.h
index 40ccf5a..b9d333d 100644
--- a/delta.h
+++ b/delta.h
@@ -90,12 +90,11 @@ static inline unsigned long get_delta_hdr_size(const unsigned char **datap,
 					       const unsigned char *top)
 {
 	const unsigned char *data = *datap;
-	unsigned char cmd;
-	unsigned long size = 0;
+	unsigned long cmd, size = 0;
 	int i = 0;
 	do {
 		cmd = *data++;
-		size |= (cmd & ~0x80) << i;
+		size |= (cmd & 0x7f) << i;
 		i += 7;
 	} while (cmd & 0x80 && data < top);
 	*datap = data;
diff --git a/index-pack.c b/index-pack.c
index 6e93ee6..0c92baf 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -293,8 +293,8 @@ static void *unpack_entry_data(unsigned long offset, unsigned long size)
 
 static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_base)
 {
-	unsigned char *p, c;
-	unsigned long size;
+	unsigned char *p;
+	unsigned long size, c;
 	off_t base_offset;
 	unsigned shift;
 	void *data;
@@ -312,7 +312,7 @@ static void *unpack_raw_entry(struct object_entry *obj, union delta_base *delta_
 		p = fill(1);
 		c = *p;
 		use(1);
-		size += (c & 0x7fUL) << shift;
+		size += (c & 0x7f) << shift;
 		shift += 7;
 	}
 	obj->size = size;
diff --git a/patch-delta.c b/patch-delta.c
index ed9db81..ef748ce 100644
--- a/patch-delta.c
+++ b/patch-delta.c
@@ -44,7 +44,7 @@ void *patch_delta(const void *src_buf, unsigned long src_size,
 			if (cmd & 0x01) cp_off = *data++;
 			if (cmd & 0x02) cp_off |= (*data++ << 8);
 			if (cmd & 0x04) cp_off |= (*data++ << 16);
-			if (cmd & 0x08) cp_off |= (*data++ << 24);
+			if (cmd & 0x08) cp_off |= ((unsigned) *data++ << 24);
 			if (cmd & 0x10) cp_size = *data++;
 			if (cmd & 0x20) cp_size |= (*data++ << 8);
 			if (cmd & 0x40) cp_size |= (*data++ << 16);
diff --git a/sha1_file.c b/sha1_file.c
index e73cd4f..8f5fe62 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -1162,8 +1162,7 @@ unsigned long unpack_object_header_buffer(const unsigned char *buf,
 		unsigned long len, enum object_type *type, unsigned long *sizep)
 {
 	unsigned shift;
-	unsigned char c;
-	unsigned long size;
+	unsigned long size, c;
 	unsigned long used = 0;
 
 	c = buf[used++];

Re: Fix big left-shifts of unsigned char

[Johannes Schindelin]

Hi,

On Wed, 17 Jun 2009, Linus Torvalds wrote:

> This is _not_ tested in any way. And I got bored with getting rid of 
> 'unsigned char' variables, so I by no means did all of them, just the 
> first few that caugth my grepping eye.

I wonder if there is a mode of 'sparse' which could spot these buggers.

Ciao,
Dscho

Re: Fix big left-shifts of unsigned char

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> On Wed, 17 Jun 2009, Linus Torvalds wrote:
>
>> This is _not_ tested in any way. And I got bored with getting rid of 
>> 'unsigned char' variables, so I by no means did all of them, just the 
>> first few that caugth my grepping eye.
>
> I wonder if there is a mode of 'sparse' which could spot these buggers.

That's a very good meta question ;-)

Re: Fix big left-shifts of unsigned char

[Linus Torvalds]

On Thu, 18 Jun 2009, Johannes Schindelin wrote:
> 
> I wonder if there is a mode of 'sparse' which could spot these buggers.

Hmm. A very quick hack gets me (this is _after_ my patch):

	diffcore-delta.c:53:24: warning: Signed left-shift
	diffcore-delta.c:58:21: warning: Signed left-shift
	diffcore-delta.c:88:21: warning: Signed left-shift

which are all of the type "<< alloc_log", and not very interesting.

	read-cache.c:1155:17: warning: Signed left-shift

This one is "real", and I noticed it in my earlier greps, but we don't 
care (it's the CACHE_EXT_TREE/CACHE_EXT() matching, and it's all done in 
"int", and the only case we care about is a non-signed case anyway)

	imap-send.c:785:52: warning: Signed left-shift
	imap-send.c:1190:35: warning: Signed left-shift

These are both "real", but we're only working on "unsigned" so we don't 
really care.

	builtin-rev-list.c:201:21: warning: Signed left-shift

this is exp2i(), and it returns 'int', and wouldn't work for big left 
shifts anyway.

And just to see that my sparse logic actually worked, _without_ the patch 
I sent to fix left-shifts, I got these:

	base85.c:95:38: warning: Signed left-shift		(fixed)
	diffcore-delta.c:53:24: warning: Signed left-shift
	diffcore-delta.c:58:21: warning: Signed left-shift
	diffcore-delta.c:88:21: warning: Signed left-shift
	delta.h:98:42: warning: Signed left-shift		(fixed)
	delta.h:98:42: warning: Signed left-shift		(dup - inline)
	patch-delta.c:47:63: warning: Signed left-shift		(fixed)
	read-cache.c:1155:17: warning: Signed left-shift
	sha1_file.c:1179:39: warning: Signed left-shift		(fixed)
	delta.h:98:42: warning: Signed left-shift		(dup - inline)
	delta.h:98:42: warning: Signed left-shift		(dup - inline)
	imap-send.c:785:52: warning: Signed left-shift
	imap-send.c:1190:35: warning: Signed left-shift
	builtin-rev-list.c:201:21: warning: Signed left-shift
	builtin-unpack-objects.c:441:39: warning: Signed left-shift (fixed)

just to verify that my sparse checker actually found the ones the patch 
modified.

			Linus

Re: Fix big left-shifts of unsigned char

[Johannes Schindelin]

Hi,

On Thu, 18 Jun 2009, Linus Torvalds wrote:

> On Thu, 18 Jun 2009, Johannes Schindelin wrote:
> 
> > I wonder if there is a mode of 'sparse' which could spot these 
> > buggers.
> 
> Hmm. A very quick hack gets me (this is _after_ my patch):
>
> [snip thorough analysis]

Nice.

The quick hack is not going to be part of sparse.git, I take it?

Ciao,
Dscho

Re: Fix big left-shifts of unsigned char

[Linus Torvalds]

On Thu, 18 Jun 2009, Johannes Schindelin wrote:
> 
> The quick hack is not going to be part of sparse.git, I take it?

Not clear yet. I sent a couple of patches to the sparse list already to 
make things go better in general with running sparse on git. I can now do

	make "CC=cgcc -m64"

on my git tree, and get reasonable warnings. I'll play around with it 
that whole left-shift thing a bit more, but before I do that I'll post a 
patch for all the _other_ things sparse found in git.

		Linus

Fix various sparse warnings in the git source code

[Linus Torvalds]

There are a few remaining ones, but this fixes the trivial ones. It boils 
down to two main issues that sparse complains about:

 - warning: Using plain integer as NULL pointer

   Sparse doesn't like you using '0' instead of 'NULL'. For various good 
   reasons, not the least of which is just the visual confusion. A NULL 
   pointer is not an integer, and that whole "0 works as NULL" is a 
   historical accident and not very pretty.

   A few of these remain: zlib is a total mess, and Z_NULL is just a 0. 
   I didn't touch those.

 - warning: symbol 'xyz' was not declared. Should it be static?

   Sparse wwants to see declarations for any functions you export. A lack 
   fo a declaration tends to mean that you should either add one, or you 
   should mark the function 'static' to show that it's in file scope. 

   A few of these remain: I only did the ones that should obviously just 
   be made static.

Signed-off-by: Linus Torvalds <torvalds@linux-foundation.org>
---

With this, and a few sparse fixes, git is _mostly_ sparse-clean.

On Thu, 18 Jun 2009, Linus Torvalds wrote:
> 
> 
> On Thu, 18 Jun 2009, Johannes Schindelin wrote:
> > 
> > The quick hack is not going to be part of sparse.git, I take it?
> 
> Not clear yet. I sent a couple of patches to the sparse list already to 
> make things go better in general with running sparse on git. I can now do
> 
> 	make "CC=cgcc -m64"
> 
> on my git tree, and get reasonable warnings. I'll play around with it 
> that whole left-shift thing a bit more, but before I do that I'll post a 
> patch for all the _other_ things sparse found in git.
> 
> 		Linus
> 

 bisect.c                 |    4 ++--
 builtin-add.c            |    2 +-
 builtin-apply.c          |    2 +-
 builtin-clone.c          |    2 +-
 builtin-fsck.c           |    4 ++--
 builtin-help.c           |    2 +-
 builtin-log.c            |    4 ++--
 builtin-merge.c          |    4 ++--
 builtin-remote.c         |   16 ++++++++--------
 builtin-unpack-objects.c |    6 +++---
 connect.c                |    2 +-
 daemon.c                 |    2 +-
 imap-send.c              |    2 +-
 index-pack.c             |    2 +-
 mailmap.c                |    6 +++---
 merge-recursive.c        |    2 +-
 parse-options.c          |    2 +-
 quote.c                  |    4 ++--
 remote.c                 |    6 +++---
 test-parse-options.c     |    4 ++--
 20 files changed, 39 insertions(+), 39 deletions(-)

diff --git a/bisect.c b/bisect.c
index 6fdff05..dbeb287 100644
--- a/bisect.c
+++ b/bisect.c
@@ -454,7 +454,7 @@ static int read_bisect_refs(void)
 	return for_each_ref_in("refs/bisect/", register_ref, NULL);
 }
 
-void read_bisect_paths(struct argv_array *array)
+static void read_bisect_paths(struct argv_array *array)
 {
 	struct strbuf str = STRBUF_INIT;
 	const char *filename = git_path("BISECT_NAMES");
@@ -780,7 +780,7 @@ static void handle_bad_merge_base(void)
 	exit(1);
 }
 
-void handle_skipped_merge_base(const unsigned char *mb)
+static void handle_skipped_merge_base(const unsigned char *mb)
 {
 	char *mb_hex = sha1_to_hex(mb);
 	char *bad_hex = sha1_to_hex(current_bad_sha1);
diff --git a/builtin-add.c b/builtin-add.c
index 566c313..ad8e562 100644
--- a/builtin-add.c
+++ b/builtin-add.c
@@ -160,7 +160,7 @@ int interactive_add(int argc, const char **argv, const char *prefix)
 	return status;
 }
 
-int edit_patch(int argc, const char **argv, const char *prefix)
+static int edit_patch(int argc, const char **argv, const char *prefix)
 {
 	char *file = xstrdup(git_path("ADD_EDIT.patch"));
 	const char *apply_argv[] = { "apply", "--recount", "--cached",
diff --git a/builtin-apply.c b/builtin-apply.c
index 94ba2bd..bb59567 100644
--- a/builtin-apply.c
+++ b/builtin-apply.c
@@ -2614,7 +2614,7 @@ static int get_current_sha1(const char *path, unsigned char *sha1)
 static void build_fake_ancestor(struct patch *list, const char *filename)
 {
 	struct patch *patch;
-	struct index_state result = { 0 };
+	struct index_state result = { NULL };
 	int fd;
 
 	/* Once we start supporting the reverse patch, it may be
diff --git a/builtin-clone.c b/builtin-clone.c
index 5c46496..2ceacb7 100644
--- a/builtin-clone.c
+++ b/builtin-clone.c
@@ -141,7 +141,7 @@ static char *guess_dir_name(const char *repo, int is_bundle, int is_bare)
 	if (is_bare) {
 		struct strbuf result = STRBUF_INIT;
 		strbuf_addf(&result, "%.*s.git", (int)(end - start), start);
-		dir = strbuf_detach(&result, 0);
+		dir = strbuf_detach(&result, NULL);
 	} else
 		dir = xstrndup(start, end - start);
 	/*
diff --git a/builtin-fsck.c b/builtin-fsck.c
index 7da706c..e077e72 100644
--- a/builtin-fsck.c
+++ b/builtin-fsck.c
@@ -104,7 +104,7 @@ static int mark_object(struct object *obj, int type, void *data)
 
 static void mark_object_reachable(struct object *obj)
 {
-	mark_object(obj, OBJ_ANY, 0);
+	mark_object(obj, OBJ_ANY, NULL);
 }
 
 static int traverse_one_object(struct object *obj, struct object *parent)
@@ -292,7 +292,7 @@ static int fsck_sha1(const unsigned char *sha1)
 		fprintf(stderr, "Checking %s %s\n",
 			typename(obj->type), sha1_to_hex(obj->sha1));
 
-	if (fsck_walk(obj, mark_used, 0))
+	if (fsck_walk(obj, mark_used, NULL))
 		objerror(obj, "broken links");
 	if (fsck_object(obj, check_strict, fsck_error_func))
 		return -1;
diff --git a/builtin-help.c b/builtin-help.c
index 6e53b23..e1eba77 100644
--- a/builtin-help.c
+++ b/builtin-help.c
@@ -394,7 +394,7 @@ static void get_html_page_path(struct strbuf *page_path, const char *page)
  * HTML.
  */
 #ifndef open_html
-void open_html(const char *path)
+static void open_html(const char *path)
 {
 	execl_git_cmd("web--browse", "-c", "help.browser", path, NULL);
 }
diff --git a/builtin-log.c b/builtin-log.c
index 0d34050..44f9a27 100644
--- a/builtin-log.c
+++ b/builtin-log.c
@@ -94,7 +94,7 @@ static void show_early_header(struct rev_info *rev, const char *stage, int nr)
 	printf("Final output: %d %s\n", nr, stage);
 }
 
-struct itimerval early_output_timer;
+static struct itimerval early_output_timer;
 
 static void log_show_early(struct rev_info *revs, struct commit_list *list)
 {
@@ -977,7 +977,7 @@ int cmd_format_patch(int argc, const char **argv, const char *prefix)
 		strbuf_addch(&buf, '\n');
 	}
 
-	rev.extra_headers = strbuf_detach(&buf, 0);
+	rev.extra_headers = strbuf_detach(&buf, NULL);
 
 	if (start_number < 0)
 		start_number = 1;
diff --git a/builtin-merge.c b/builtin-merge.c
index 793f2f4..af9adab 100644
--- a/builtin-merge.c
+++ b/builtin-merge.c
@@ -370,7 +370,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 
 	strbuf_addstr(&buf, "refs/heads/");
 	strbuf_addstr(&buf, remote);
-	resolve_ref(buf.buf, branch_head, 0, 0);
+	resolve_ref(buf.buf, branch_head, 0, NULL);
 
 	if (!hashcmp(remote_head->sha1, branch_head)) {
 		strbuf_addf(msg, "%s\t\tbranch '%s' of .\n",
@@ -409,7 +409,7 @@ static void merge_name(const char *remote, struct strbuf *msg)
 		strbuf_addstr(&truname, "refs/heads/");
 		strbuf_addstr(&truname, remote);
 		strbuf_setlen(&truname, truname.len - len);
-		if (resolve_ref(truname.buf, buf_sha, 0, 0)) {
+		if (resolve_ref(truname.buf, buf_sha, 0, NULL)) {
 			strbuf_addf(msg,
 				    "%s\t\tbranch '%s'%s of .\n",
 				    sha1_to_hex(remote_head->sha1),
diff --git a/builtin-remote.c b/builtin-remote.c
index dfc0b9e..f73c657 100644
--- a/builtin-remote.c
+++ b/builtin-remote.c
@@ -740,7 +740,7 @@ static int rm(int argc, const char **argv)
 	return result;
 }
 
-void clear_push_info(void *util, const char *string)
+static void clear_push_info(void *util, const char *string)
 {
 	struct push_info *info = util;
 	free(info->dest);
@@ -815,7 +815,7 @@ struct show_info {
 	int any_rebase;
 };
 
-int add_remote_to_show_info(struct string_list_item *item, void *cb_data)
+static int add_remote_to_show_info(struct string_list_item *item, void *cb_data)
 {
 	struct show_info *info = cb_data;
 	int n = strlen(item->string);
@@ -825,7 +825,7 @@ int add_remote_to_show_info(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
-int show_remote_info_item(struct string_list_item *item, void *cb_data)
+static int show_remote_info_item(struct string_list_item *item, void *cb_data)
 {
 	struct show_info *info = cb_data;
 	struct ref_states *states = info->states;
@@ -852,7 +852,7 @@ int show_remote_info_item(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
-int add_local_to_show_info(struct string_list_item *branch_item, void *cb_data)
+static int add_local_to_show_info(struct string_list_item *branch_item, void *cb_data)
 {
 	struct show_info *show_info = cb_data;
 	struct ref_states *states = show_info->states;
@@ -874,7 +874,7 @@ int add_local_to_show_info(struct string_list_item *branch_item, void *cb_data)
 	return 0;
 }
 
-int show_local_info_item(struct string_list_item *item, void *cb_data)
+static int show_local_info_item(struct string_list_item *item, void *cb_data)
 {
 	struct show_info *show_info = cb_data;
 	struct branch_info *branch_info = item->util;
@@ -906,7 +906,7 @@ int show_local_info_item(struct string_list_item *item, void *cb_data)
 	return 0;
 }
 
-int add_push_to_show_info(struct string_list_item *push_item, void *cb_data)
+static int add_push_to_show_info(struct string_list_item *push_item, void *cb_data)
 {
 	struct show_info *show_info = cb_data;
 	struct push_info *push_info = push_item->util;
@@ -935,7 +935,7 @@ static int cmp_string_with_push(const void *va, const void *vb)
 	return cmp ? cmp : strcmp(a_push->dest, b_push->dest);
 }
 
-int show_push_info_item(struct string_list_item *item, void *cb_data)
+static int show_push_info_item(struct string_list_item *item, void *cb_data)
 {
 	struct show_info *show_info = cb_data;
 	struct push_info *push_info = item->util;
@@ -1187,7 +1187,7 @@ static int get_one_remote_for_update(struct remote *remote, void *priv)
 	return 0;
 }
 
-struct remote_group {
+static struct remote_group {
 	const char *name;
 	struct string_list *list;
 } remote_group;
diff --git a/builtin-unpack-objects.c b/builtin-unpack-objects.c
index 9a77323..f8d597d 100644
--- a/builtin-unpack-objects.c
+++ b/builtin-unpack-objects.c
@@ -158,7 +158,7 @@ struct obj_info {
 #define FLAG_WRITTEN (1u<<21)
 
 static struct obj_info *obj_list;
-unsigned nr_objects;
+static unsigned nr_objects;
 
 /*
  * Called only from check_object() after it verified this object
@@ -200,7 +200,7 @@ static int check_object(struct object *obj, int type, void *data)
 
 	if (fsck_object(obj, 1, fsck_error_function))
 		die("Error in object");
-	if (!fsck_walk(obj, check_object, 0))
+	if (!fsck_walk(obj, check_object, NULL))
 		die("Error on reachable objects of %s", sha1_to_hex(obj->sha1));
 	write_cached_object(obj);
 	return 1;
@@ -210,7 +210,7 @@ static void write_rest(void)
 {
 	unsigned i;
 	for (i = 0; i < nr_objects; i++)
-		check_object(obj_list[i].obj, OBJ_ANY, 0);
+		check_object(obj_list[i].obj, OBJ_ANY, NULL);
 }
 
 static void added_object(unsigned nr, enum object_type type,
diff --git a/connect.c b/connect.c
index 0ce941e..76e5427 100644
--- a/connect.c
+++ b/connect.c
@@ -464,7 +464,7 @@ static void git_proxy_connect(int fd[2], char *host)
 
 #define MAX_CMD_LEN 1024
 
-char *get_port(char *host)
+static char *get_port(char *host)
 {
 	char *end;
 	char *p = strchr(host, ':');
diff --git a/daemon.c b/daemon.c
index b2babcc..366db37 100644
--- a/daemon.c
+++ b/daemon.c
@@ -453,7 +453,7 @@ static void parse_host_arg(char *extra_args, int buflen)
 		memset(&hints, 0, sizeof(hints));
 		hints.ai_flags = AI_CANONNAME;
 
-		gai = getaddrinfo(hostname, 0, &hints, &ai);
+		gai = getaddrinfo(hostname, NULL, &hints, &ai);
 		if (!gai) {
 			struct sockaddr_in *sin_addr = (void *)ai->ai_addr;
 
diff --git a/imap-send.c b/imap-send.c
index e4c83b9..3847fd1 100644
--- a/imap-send.c
+++ b/imap-send.c
@@ -238,7 +238,7 @@ static const char *Flags[] = {
 #ifndef NO_OPENSSL
 static void ssl_socket_perror(const char *func)
 {
-	fprintf(stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), 0));
+	fprintf(stderr, "%s: %s\n", func, ERR_error_string(ERR_get_error(), NULL));
 }
 #endif
 
diff --git a/index-pack.c b/index-pack.c
index 6e93ee6..4d85aeb 100644
--- a/index-pack.c
+++ b/index-pack.c
@@ -469,7 +469,7 @@ static void sha1_object(const void *data, unsigned long size,
 				die("invalid %s", typename(type));
 			if (fsck_object(obj, 1, fsck_error_function))
 				die("Error in object");
-			if (fsck_walk(obj, mark_link, 0))
+			if (fsck_walk(obj, mark_link, NULL))
 				die("Not all child objects of %s are reachable", sha1_to_hex(obj->sha1));
 
 			if (obj->type == OBJ_TREE) {
diff --git a/mailmap.c b/mailmap.c
index bb1f2fb..f167c00 100644
--- a/mailmap.c
+++ b/mailmap.c
@@ -103,7 +103,7 @@ static char *parse_name_and_email(char *buffer, char **name,
 		char **email, int allow_empty_email)
 {
 	char *left, *right, *nstart, *nend;
-	*name = *email = 0;
+	*name = *email = NULL;
 
 	if ((left = strchr(buffer, '<')) == NULL)
 		return NULL;
@@ -136,7 +136,7 @@ static int read_single_mailmap(struct string_list *map, const char *filename, ch
 	if (f == NULL)
 		return 1;
 	while (fgets(buffer, sizeof(buffer), f) != NULL) {
-		char *name1 = 0, *email1 = 0, *name2 = 0, *email2 = 0;
+		char *name1 = NULL, *email1 = NULL, *name2 = NULL, *email2 = NULL;
 		if (buffer[0] == '#') {
 			static const char abbrev[] = "# repo-abbrev:";
 			int abblen = sizeof(abbrev) - 1;
@@ -200,7 +200,7 @@ int map_user(struct string_list *map,
 	if (!p) {
 		/* email passed in might not be wrapped in <>, but end with a \0 */
 		p = memchr(email, '\0', maxlen_email);
-		if (p == 0)
+		if (!p)
 			return 0;
 	}
 	if (p - email + 1 < sizeof(buf))
diff --git a/merge-recursive.c b/merge-recursive.c
index f5df9b9..c703445 100644
--- a/merge-recursive.c
+++ b/merge-recursive.c
@@ -38,7 +38,7 @@ static struct tree *shift_tree_object(struct tree *one, struct tree *two)
  * A virtual commit has (const char *)commit->util set to the name.
  */
 
-struct commit *make_virtual_commit(struct tree *tree, const char *comment)
+static struct commit *make_virtual_commit(struct tree *tree, const char *comment)
 {
 	struct commit *commit = xcalloc(1, sizeof(struct commit));
 	commit->tree = tree;
diff --git a/parse-options.c b/parse-options.c
index 79de18b..4eefdb1 100644
--- a/parse-options.c
+++ b/parse-options.c
@@ -478,7 +478,7 @@ static int usage_argh(const struct option *opts)
 #define USAGE_OPTS_WIDTH 24
 #define USAGE_GAP         2
 
-int usage_with_options_internal(const char * const *usagestr,
+static int usage_with_options_internal(const char * const *usagestr,
 				const struct option *opts, int full)
 {
 	if (!usagestr)
diff --git a/quote.c b/quote.c
index 7a49fcf..48bce2e 100644
--- a/quote.c
+++ b/quote.c
@@ -272,8 +272,8 @@ void write_name_quoted(const char *name, FILE *fp, int terminator)
 	fputc(terminator, fp);
 }
 
-extern void write_name_quotedpfx(const char *pfx, size_t pfxlen,
-                                 const char *name, FILE *fp, int terminator)
+void write_name_quotedpfx(const char *pfx, size_t pfxlen,
+                          const char *name, FILE *fp, int terminator)
 {
 	int needquote = 0;
 
diff --git a/remote.c b/remote.c
index 08a5964..c2a2846 100644
--- a/remote.c
+++ b/remote.c
@@ -301,7 +301,7 @@ static void read_branches_file(struct remote *remote)
 		strbuf_addstr(&branch, "HEAD:");
 	}
 	add_url_alias(remote, p);
-	add_fetch_refspec(remote, strbuf_detach(&branch, 0));
+	add_fetch_refspec(remote, strbuf_detach(&branch, NULL));
 	/*
 	 * Cogito compatible push: push current HEAD to remote #branch
 	 * (master if missing)
@@ -312,7 +312,7 @@ static void read_branches_file(struct remote *remote)
 		strbuf_addf(&branch, ":refs/heads/%s", frag);
 	else
 		strbuf_addstr(&branch, ":refs/heads/master");
-	add_push_refspec(remote, strbuf_detach(&branch, 0));
+	add_push_refspec(remote, strbuf_detach(&branch, NULL));
 	remote->fetch_tags = 1; /* always auto-follow */
 }
 
@@ -1105,7 +1105,7 @@ int match_refs(struct ref *src, struct ref **dst,
 	int send_all = flags & MATCH_REFS_ALL;
 	int send_mirror = flags & MATCH_REFS_MIRROR;
 	int errs;
-	static const char *default_refspec[] = { ":", 0 };
+	static const char *default_refspec[] = { ":", NULL };
 	struct ref **dst_tail = tail_ref(dst);
 
 	if (!nr_refspec) {
diff --git a/test-parse-options.c b/test-parse-options.c
index a90bc30..efa734b 100644
--- a/test-parse-options.c
+++ b/test-parse-options.c
@@ -9,7 +9,7 @@ static int verbose = 0, dry_run = 0, quiet = 0;
 static char *string = NULL;
 static char *file = NULL;
 
-int length_callback(const struct option *opt, const char *arg, int unset)
+static int length_callback(const struct option *opt, const char *arg, int unset)
 {
 	printf("Callback: \"%s\", %d\n",
 		(arg ? arg : "not set"), unset);
@@ -20,7 +20,7 @@ int length_callback(const struct option *opt, const char *arg, int unset)
 	return 0;
 }
 
-int number_callback(const struct option *opt, const char *arg, int unset)
+static int number_callback(const struct option *opt, const char *arg, int unset)
 {
 	*(int *)opt->value = strtol(arg, NULL, 10);
 	return 0;

Re: Fix various sparse warnings in the git source code

[Matthieu Moy]

Linus Torvalds <torvalds@linux-foundation.org> writes:

>    Sparse wwants to see declarations for any functions you export. A lack 
>    fo a declaration tends to mean that you should either add one, or

Patch content is good, but in case it matters, you can s/wwants/wants/
and s/fo/for/ in the commit message.

-- 
Matthieu

Re: Fix various sparse warnings in the git source code

[Linus Torvalds]

On Thu, 18 Jun 2009, Matthieu Moy wrote:

> Linus Torvalds <torvalds@linux-foundation.org> writes:
> 
> >    Sparse wwants to see declarations for any functions you export. A lack 
> >    fo a declaration tends to mean that you should either add one, or
> 
> Patch content is good, but in case it matters, you can s/wwants/wants/
> and s/fo/for/ in the commit message.

My speling is gud, thak you vety much. I just miss a fwe keys.

That "fo" should not be "for", it should be "of".

Of course, the "wwants" may not be a typo. Maybe it's like "iff": "iff" 
means "if and only if", and "wants" means "wants and really wants"?

You be the judge.

		Linus

Re: Fix various sparse warnings in the git source code

[Linus Torvalds]

On Thu, 18 Jun 2009, Linus Torvalds wrote:
> 
>  - warning: symbol 'xyz' was not declared. Should it be static?
> 
>    Sparse wants to see declarations for any functions you export. A lack 
>    of a declaration tends to mean that you should either add one, or you 
>    should mark the function 'static' to show that it's in file scope. 
> 
>    A few of these remain: I only did the ones that should obviously just 
>    be made static.

I don't know why I missed a couple.

That 'wt_status_submodule_summary' one is debatable. It has a few related 
flags (like 'wt_status_use_color') which _are_ declared, and are used by 
builtin-commit.c. So maybe we'd like to export it at some point, but it's 
not declared now, and not used outside of that file, so 'static' it is in 
this patch.

		Linus

---
 http-push.c    |    2 +-
 unpack-trees.c |    2 +-
 wt-status.c    |    2 +-
 3 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/http-push.c b/http-push.c
index e4ea395..8cc8ee0 100644
--- a/http-push.c
+++ b/http-push.c
@@ -1750,7 +1750,7 @@ static int delete_remote_branch(char *pattern, int force)
 	return 0;
 }
 
-void run_request_queue(void)
+static void run_request_queue(void)
 {
 #ifdef USE_CURL_MULTI
 	is_running_queue = 1;
diff --git a/unpack-trees.c b/unpack-trees.c
index ac9927f..1958319 100644
--- a/unpack-trees.c
+++ b/unpack-trees.c
@@ -140,7 +140,7 @@ static int unpack_index_entry(struct cache_entry *ce, struct unpack_trees_option
 	return call_unpack_fn(src, o);
 }
 
-int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long df_conflicts, struct name_entry *names, struct traverse_info *info)
+static int traverse_trees_recursive(int n, unsigned long dirmask, unsigned long df_conflicts, struct name_entry *names, struct traverse_info *info)
 {
 	int i;
 	struct tree_desc t[MAX_UNPACK_TREES];
diff --git a/wt-status.c b/wt-status.c
index 24a6abf..47735d8 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -13,7 +13,7 @@
 
 int wt_status_relative_paths = 1;
 int wt_status_use_color = -1;
-int wt_status_submodule_summary;
+static int wt_status_submodule_summary;
 static char wt_status_colors[][COLOR_MAXLEN] = {
 	GIT_COLOR_NORMAL, /* WT_STATUS_HEADER */
 	GIT_COLOR_GREEN,  /* WT_STATUS_UPDATED */