[PATCH] tools: fread does not return negative on error

[PATCH] tools: fread does not return negative on error

[Roel Kluin]

size_t res cannot be less than 0. fread returns 0 on error.

Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
Is this correct? please review.

diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
index eaba093..376a337 100644
--- a/tools/perf/util/strbuf.c
+++ b/tools/perf/util/strbuf.c
@@ -259,7 +259,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 	res = fread(sb->buf + sb->len, 1, size, f);
 	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	else if (res < 0 && oldalloc == 0)
+	else if (res == 0 && oldalloc == 0)
 		strbuf_release(sb);
 	return res;
 }

Re: [PATCH] tools: fread does not return negative on error

[Ingo Molnar]

* Roel Kluin <roel.kluin@gmail.com> wrote:

> size_t res cannot be less than 0. fread returns 0 on error.
> 
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
> Is this correct? please review.
> 
> diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
> index eaba093..376a337 100644
> --- a/tools/perf/util/strbuf.c
> +++ b/tools/perf/util/strbuf.c
> @@ -259,7 +259,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
>  	res = fread(sb->buf + sb->len, 1, size, f);
>  	if (res > 0)
>  		strbuf_setlen(sb, sb->len + res);
> -	else if (res < 0 && oldalloc == 0)
> +	else if (res == 0 && oldalloc == 0)
>  		strbuf_release(sb);
>  	return res;

This comes straight from Git's strbuf.c so i've Cc:-ed the Git list.

Roel, did you get some compiler warning that made you look at this 
code?

	Ingo

Re: [PATCH] tools: fread does not return negative on error

[roel kluin]

On Mon, Jun 22, 2009 at 5:34 PM, Ingo Molnar<mingo@elte.hu> wrote:
>
> * Roel Kluin <roel.kluin@gmail.com> wrote:
>
>> size_t res cannot be less than 0. fread returns 0 on error.
>>
>> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
>> ---
>> Is this correct? please review.
>>
>> diff --git a/tools/perf/util/strbuf.c b/tools/perf/util/strbuf.c
>> index eaba093..376a337 100644
>> --- a/tools/perf/util/strbuf.c
>> +++ b/tools/perf/util/strbuf.c
>> @@ -259,7 +259,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
>>       res = fread(sb->buf + sb->len, 1, size, f);
>>       if (res > 0)
>>               strbuf_setlen(sb, sb->len + res);
>> -     else if (res < 0 && oldalloc == 0)
>> +     else if (res == 0 && oldalloc == 0)
>>               strbuf_release(sb);
>>       return res;
>
> This comes straight from Git's strbuf.c so i've Cc:-ed the Git list.
>
> Roel, did you get some compiler warning that made you look at this
> code?
>
>        Ingo
>

No, I use sed to catch these bugs.

Roel

[PATCH] fread does not return negative on error

[René Scharfe]

Hi,

the following patch is for git.  I just removed the unneeded check for
res == 0 from your version.  Does it look OK?

Thanks,
René

--- snip! ---
From: Roel Kluin <roel.kluin@gmail.com>

size_t res cannot be less than 0. fread returns 0 on error.

Reported-by: Ingo Molnar <mingo@elte.hu>
Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
---
 strbuf.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/strbuf.c b/strbuf.c
index a884960..f03d117 100644
--- a/strbuf.c
+++ b/strbuf.c
@@ -260,7 +260,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
 	res = fread(sb->buf + sb->len, 1, size, f);
 	if (res > 0)
 		strbuf_setlen(sb, sb->len + res);
-	else if (res < 0 && oldalloc == 0)
+	else if (oldalloc == 0)
 		strbuf_release(sb);
 	return res;
 }

Re: [PATCH] fread does not return negative on error

[Junio C Hamano]

René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:

> the following patch is for git.  I just removed the unneeded check for
> res == 0 from your version.  Does it look OK?

The patch looks good, and both of our in-tree users do error out when the
returned value is 0 (imap-send.c checks with "<= 0" which looks a tad
amateurish, though) correctly.

Funny, there is no caller of this function in the original context this
bug originally found, which I think is linux-2.6/tools/perf ;-).

Thanks.

> From: Roel Kluin <roel.kluin@gmail.com>
>
> size_t res cannot be less than 0. fread returns 0 on error.
>
> Reported-by: Ingo Molnar <mingo@elte.hu>
> Signed-off-by: Roel Kluin <roel.kluin@gmail.com>
> ---
>  strbuf.c |    2 +-
>  1 files changed, 1 insertions(+), 1 deletions(-)
>
> diff --git a/strbuf.c b/strbuf.c
> index a884960..f03d117 100644
> --- a/strbuf.c
> +++ b/strbuf.c
> @@ -260,7 +260,7 @@ size_t strbuf_fread(struct strbuf *sb, size_t size, FILE *f)
>  	res = fread(sb->buf + sb->len, 1, size, f);
>  	if (res > 0)
>  		strbuf_setlen(sb, sb->len + res);
> -	else if (res < 0 && oldalloc == 0)
> +	else if (oldalloc == 0)
>  		strbuf_release(sb);
>  	return res;
>  }

Re: [PATCH] fread does not return negative on error

[Ingo Molnar]

* Junio C Hamano <gitster@pobox.com> wrote:

> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> 
> > the following patch is for git.  I just removed the unneeded check for
> > res == 0 from your version.  Does it look OK?
> 
> The patch looks good, and both of our in-tree users do error out 
> when the returned value is 0 (imap-send.c checks with "<= 0" which 
> looks a tad amateurish, though) correctly.
> 
> Funny, there is no caller of this function in the original context 
> this bug originally found, which I think is linux-2.6/tools/perf 
> ;-).

Hehe, yes :-)

Background: when creating tools/perf/ i cherry-picked all the nice 
Git libraries into tools/perf/util/, to give a standard environment 
for all tooling things that might come up in the future.

Some of those are not used yet but it looked more logical to pick up 
whole pieces - some already gained uses. For example config.c is not 
truly used yet, but very much expected to have a role in the future.

( The only invasive thing i had to do was the s/git_/perf_/ mass 
  rename across all the files - having 'git_' in perf looked
  quite confusing. )

And our general experience with the Git libraries in 
tools/perf/util/* is: we love them!

For example parse-options.c is a striking improvement compared to 
getopt.h we used before, and all the other facilities are sane and 
straight to the point as well. So in this sense 'perf' is an ... 
interesting cross-discipline 'fork' of Git's generic libraries.

The auto-generation of everything out of Documentation/*.txt is 
another thing we picked up, and that's very nice too.

One bookeeping issue: i found few explicit credits in those files - 
so i noted in the changelog that i took them from Git and i noted 
the specific upstream Git sha1 when i copied them. Would be nice to 
update each file with names to make credit more explicit:

-rw-rw-r-- 1 mingo mingo  2808 2009-06-23 10:49 abspath.c
-rw-rw-r-- 1 mingo mingo  1447 2009-06-23 10:49 alias.c
-rw-rw-r-- 1 mingo mingo  4660 2009-06-23 10:49 cache.h
-rw-rw-r-- 1 mingo mingo  4817 2009-06-23 10:49 color.c
-rw-rw-r-- 1 mingo mingo  1187 2009-06-23 10:49 color.h
-rw-rw-r-- 1 mingo mingo 19149 2009-06-23 10:49 config.c
-rw-rw-r-- 1 mingo mingo  1041 2009-06-23 10:52 ctype.c
-rw-rw-r-- 1 mingo mingo   256 2009-06-23 10:49 environment.c
-rw-rw-r-- 1 mingo mingo  3262 2009-06-23 10:49 exec_cmd.c
-rw-rw-r-- 1 mingo mingo   496 2009-06-23 10:49 exec_cmd.h
-rw-rw-r-- 1 mingo mingo  8515 2009-06-23 10:49 help.c
-rw-rw-r-- 1 mingo mingo   751 2009-06-23 10:49 help.h
-rw-rw-r-- 1 mingo mingo  2592 2009-06-23 10:49 levenshtein.c
-rw-rw-r-- 1 mingo mingo   201 2009-06-23 10:49 levenshtein.h
-rw-rw-r-- 1 mingo mingo  1909 2009-06-23 10:49 pager.c
-rw-rw-r-- 1 mingo mingo 12454 2009-06-23 10:49 parse-options.c
-rw-rw-r-- 1 mingo mingo  5693 2009-06-23 10:49 parse-options.h
-rw-rw-r-- 1 mingo mingo  7986 2009-06-23 10:49 path.c
-rw-rw-r-- 1 mingo mingo 10442 2009-06-23 10:49 quote.c
-rw-rw-r-- 1 mingo mingo  2667 2009-06-23 10:49 quote.h
-rw-rw-r-- 1 mingo mingo  7966 2009-06-23 10:49 run-command.c
-rw-rw-r-- 1 mingo mingo  2838 2009-06-23 10:49 run-command.h
-rw-rw-r-- 1 mingo mingo   969 2009-06-23 10:49 sigchain.c
-rw-rw-r-- 1 mingo mingo   215 2009-06-23 10:49 sigchain.h
-rw-rw-r-- 1 mingo mingo  7270 2009-06-23 10:49 strbuf.c
-rw-rw-r-- 1 mingo mingo  4995 2009-06-23 10:49 strbuf.h
-rw-rw-r-- 1 mingo mingo   556 2009-06-23 10:52 string.c
-rw-rw-r-- 1 mingo mingo   120 2009-06-23 10:52 string.h
-rw-rw-r-- 1 mingo mingo 13859 2009-06-24 10:01 symbol.c
-rw-rw-r-- 1 mingo mingo  1112 2009-06-23 10:52 symbol.h
-rw-rw-r-- 1 mingo mingo  1690 2009-06-23 10:49 usage.c
-rw-rw-r-- 1 mingo mingo  9878 2009-06-23 10:52 util.h
-rw-rw-r-- 1 mingo mingo  4249 2009-06-23 10:49 wrapper.c

	Ingo

Re: [PATCH] fread does not return negative on error

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 4341 bytes --]

Hi,

On Wed, 24 Jun 2009, Ingo Molnar wrote:

> One bookeeping issue: i found few explicit credits in those files - 
> so i noted in the changelog that i took them from Git and i noted 
> the specific upstream Git sha1 when i copied them. Would be nice to 
> update each file with names to make credit more explicit:
> 
> -rw-rw-r-- 1 mingo mingo  2808 2009-06-23 10:49 abspath.c
> -rw-rw-r-- 1 mingo mingo  1447 2009-06-23 10:49 alias.c
> -rw-rw-r-- 1 mingo mingo  4660 2009-06-23 10:49 cache.h
> -rw-rw-r-- 1 mingo mingo  4817 2009-06-23 10:49 color.c
> -rw-rw-r-- 1 mingo mingo  1187 2009-06-23 10:49 color.h
> -rw-rw-r-- 1 mingo mingo 19149 2009-06-23 10:49 config.c
> -rw-rw-r-- 1 mingo mingo  1041 2009-06-23 10:52 ctype.c
> -rw-rw-r-- 1 mingo mingo   256 2009-06-23 10:49 environment.c
> -rw-rw-r-- 1 mingo mingo  3262 2009-06-23 10:49 exec_cmd.c
> -rw-rw-r-- 1 mingo mingo   496 2009-06-23 10:49 exec_cmd.h
> -rw-rw-r-- 1 mingo mingo  8515 2009-06-23 10:49 help.c
> -rw-rw-r-- 1 mingo mingo   751 2009-06-23 10:49 help.h
> -rw-rw-r-- 1 mingo mingo  2592 2009-06-23 10:49 levenshtein.c
> -rw-rw-r-- 1 mingo mingo   201 2009-06-23 10:49 levenshtein.h
> -rw-rw-r-- 1 mingo mingo  1909 2009-06-23 10:49 pager.c
> -rw-rw-r-- 1 mingo mingo 12454 2009-06-23 10:49 parse-options.c
> -rw-rw-r-- 1 mingo mingo  5693 2009-06-23 10:49 parse-options.h
> -rw-rw-r-- 1 mingo mingo  7986 2009-06-23 10:49 path.c
> -rw-rw-r-- 1 mingo mingo 10442 2009-06-23 10:49 quote.c
> -rw-rw-r-- 1 mingo mingo  2667 2009-06-23 10:49 quote.h
> -rw-rw-r-- 1 mingo mingo  7966 2009-06-23 10:49 run-command.c
> -rw-rw-r-- 1 mingo mingo  2838 2009-06-23 10:49 run-command.h
> -rw-rw-r-- 1 mingo mingo   969 2009-06-23 10:49 sigchain.c
> -rw-rw-r-- 1 mingo mingo   215 2009-06-23 10:49 sigchain.h
> -rw-rw-r-- 1 mingo mingo  7270 2009-06-23 10:49 strbuf.c
> -rw-rw-r-- 1 mingo mingo  4995 2009-06-23 10:49 strbuf.h
> -rw-rw-r-- 1 mingo mingo   556 2009-06-23 10:52 string.c
> -rw-rw-r-- 1 mingo mingo   120 2009-06-23 10:52 string.h
> -rw-rw-r-- 1 mingo mingo 13859 2009-06-24 10:01 symbol.c
> -rw-rw-r-- 1 mingo mingo  1112 2009-06-23 10:52 symbol.h
> -rw-rw-r-- 1 mingo mingo  1690 2009-06-23 10:49 usage.c
> -rw-rw-r-- 1 mingo mingo  9878 2009-06-23 10:52 util.h
> -rw-rw-r-- 1 mingo mingo  4249 2009-06-23 10:49 wrapper.c

This here script:

-- snip --
for file in abspath.c alias.c cache.h color.c color.h config.c ctype.c \
	environment.c exec_cmd.c exec_cmd.h help.c help.h levenshtein.c \
	levenshtein.h pager.c parse-options.c parse-options.h path.c \
	quote.c quote.h run-command.c run-command.h sigchain.c sigchain.h \
	strbuf.c strbuf.h string.c string.h symbol.c symbol.h usage.c \
	util.h wrapper.c
do
	echo $file
	git shortlog -n -s $file | head -n 2
done
-- snap --

outputs this (note that a few files you mentioned are not in git.git):

abspath.c
     2	Junio C Hamano
     1	Dmitry Potapov
alias.c
     2	Jeff King
     1	Felipe Contreras
cache.h
   295	Junio C Hamano
    98	Linus Torvalds
color.c
     4	Junio C Hamano
     3	Jeff King
color.h
     2	Jeff King
     2	Johannes Schindelin
config.c
    65	Junio C Hamano
    20	Johannes Schindelin
ctype.c
     4	René Scharfe
     2	Junio C Hamano
environment.c
    29	Junio C Hamano
    11	Johannes Schindelin
exec_cmd.c
     8	Johannes Sixt
     7	Junio C Hamano
exec_cmd.h
     2	Junio C Hamano
     2	Scott R Parish
help.c
    18	Junio C Hamano
    14	Christian Couder
help.h
     2	Miklos Vajna
     1	Alex Riesen
levenshtein.c
     2	Johannes Schindelin
     1	Mike Ralphson
levenshtein.h
     1	Johannes Schindelin
pager.c
     9	Junio C Hamano
     4	Johannes Schindelin
parse-options.c
    18	Pierre Habouzit
     8	Junio C Hamano
parse-options.h
    15	Pierre Habouzit
     6	Stephen Boyd
path.c
    20	Junio C Hamano
     5	Johannes Sixt
quote.c
    12	Junio C Hamano
     5	Christian Couder
quote.h
     6	Junio C Hamano
     5	Christian Couder
run-command.c
    11	Shawn O. Pearce
     7	Johannes Sixt
run-command.h
    10	Shawn O. Pearce
     5	Johannes Sixt
sigchain.c
     2	Jeff King
sigchain.h
     2	Jeff King
strbuf.c
     9	Pierre Habouzit
     8	Junio C Hamano
strbuf.h
     9	Pierre Habouzit
     4	Junio C Hamano
string.c
string.h
symbol.c
symbol.h
usage.c
     3	Linus Torvalds
     2	Junio C Hamano
util.h
wrapper.c
     4	Junio C Hamano
     2	Linus Torvalds

Ciao,
Dscho

Re: [PATCH] fread does not return negative on error

[Junio C Hamano]

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

> This here script:
>
> -- snip --
> for file in abspath.c alias.c cache.h color.c color.h config.c ctype.c \
> 	environment.c exec_cmd.c exec_cmd.h help.c help.h levenshtein.c \
> 	levenshtein.h pager.c parse-options.c parse-options.h path.c \
> 	quote.c quote.h run-command.c run-command.h sigchain.c sigchain.h \
> 	strbuf.c strbuf.h string.c string.h symbol.c symbol.h usage.c \
> 	util.h wrapper.c
> do
> 	echo $file
> 	git shortlog -n -s $file | head -n 2
> done

I have thought about suggesting this myself, and your output for many of
the files matched my intuition, but some were grossly off, so I checked.

The above procedure counts commits, and a one liner "s/char \*/const &/"
weighs as heavily as the patch that implemented the whole thing, for a
file that was done in one commit almost perfectly except that it needed a
small constness fix.  Summarizing output from "blame" for each file may
give you a more meaningful results:

    # timestamp
    ts='[12][0-9][0-9][0-9]-[0-9][0-9]-[0-3][0-9] ..:..:.. [-+]....'
    # linenum
    lno='[1-9][0-9]*'
    git blame "$file" |
    sed -e 's/^[^ ]*  *(\([^)]*[^ ]\)  *'"$ts  *$lno"').*/\1/' |
    sort |
    uniq -c |
    sort -r -n

For example, I do not think it is fair to credit me for abspath.c more
than Dmitry like this:

> outputs this (note that a few files you mentioned are not in git.git):
>
> abspath.c
>      2	Junio C Hamano
>      1	Dmitry Potapov

Initially Dmitry introduced this file with 5b8e6f8 (shrink git-shell by
avoiding redundant dependencies, 2008-06-28) at 68 lines.  J6t added 36
lines for add_path() with 10c4c88 (Allow add_path() to add non-existent
directories to the path, 2008-07-21), I added 12 lines to add a new
function with 90b4a71 (is_directory(): a generic helper function,
2008-09-09) and then added a two-liner out-of-bounds-then-die check in
737e31a (make_absolute_path(): check bounds when seeing an overlong
symlink, 2008-12-17).

Re: [PATCH] fread does not return negative on error

[Johannes Schindelin]

[-- Attachment #1: Type: TEXT/PLAIN, Size: 3999 bytes --]

Hi,

On Wed, 24 Jun 2009, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > This here script:
> >
> > -- snip --
> > for file in abspath.c alias.c cache.h color.c color.h config.c ctype.c \
> > 	environment.c exec_cmd.c exec_cmd.h help.c help.h levenshtein.c \
> > 	levenshtein.h pager.c parse-options.c parse-options.h path.c \
> > 	quote.c quote.h run-command.c run-command.h sigchain.c sigchain.h \
> > 	strbuf.c strbuf.h string.c string.h symbol.c symbol.h usage.c \
> > 	util.h wrapper.c
> > do
> > 	echo $file
> > 	git shortlog -n -s $file | head -n 2
> > done
> 
> I have thought about suggesting this myself, and your output for many of
> the files matched my intuition, but some were grossly off, so I checked.
> 
> The above procedure counts commits, and a one liner "s/char \*/const &/"
> weighs as heavily as the patch that implemented the whole thing, for a
> file that was done in one commit almost perfectly except that it needed a
> small constness fix.  Summarizing output from "blame" for each file may
> give you a more meaningful results:
> 
>     # timestamp
>     ts='[12][0-9][0-9][0-9]-[0-9][0-9]-[0-3][0-9] ..:..:.. [-+]....'
>     # linenum
>     lno='[1-9][0-9]*'
>     git blame "$file" |
>     sed -e 's/^[^ ]*  *(\([^)]*[^ ]\)  *'"$ts  *$lno"').*/\1/' |
>     sort |
>     uniq -c |
>     sort -r -n
> 
> For example, I do not think it is fair to credit me for abspath.c more
> than Dmitry like this:
> 
> > outputs this (note that a few files you mentioned are not in git.git):
> >
> > abspath.c
> >      2	Junio C Hamano
> >      1	Dmitry Potapov
> 
> Initially Dmitry introduced this file with 5b8e6f8 (shrink git-shell by
> avoiding redundant dependencies, 2008-06-28) at 68 lines.  J6t added 36
> lines for add_path() with 10c4c88 (Allow add_path() to add non-existent
> directories to the path, 2008-07-21), I added 12 lines to add a new
> function with 90b4a71 (is_directory(): a generic helper function,
> 2008-09-09) and then added a two-liner out-of-bounds-then-die check in
> 737e31a (make_absolute_path(): check bounds when seeing an overlong
> symlink, 2008-12-17).

Okay, a script similar to what you propose shows this:

abspath.c
     67 Dmitry Potapov	
     36 Johannes Sixt	
alias.c
     49 Miklos Vajna	
     24  Jeff King	
cache.h
    305 Junio C Hamano	
    263 Linus Torvalds	
color.c
    136  Jeff King	
     29 Johannes Schindelin	
color.h
     10 Matthias Kestenholz	
     10  Jeff King	
config.c
    352 Linus Torvalds	
    284 Johannes Schindelin	
ctype.c
     15 René Scharfe	
     11 Linus Torvalds	
environment.c
     68 Linus Torvalds	
     34 Johannes Schindelin	
exec_cmd.c
     47 Michal Ostrowski	
     40 Steffen Prohaska	
exec_cmd.h
      5 Junio C Hamano	
      2 Steve Haslam	
help.c
     79 Linus Torvalds	
     73 Johannes Schindelin	
help.h
     25 Miklos Vajna	
      3 Alex Riesen	
levenshtein.c
     82 Johannes Schindelin	
      1 Samuel Tardieu	
levenshtein.h
      8 Johannes Schindelin	
pager.c
     34  Jeff King	
     25 Johannes Sixt	
parse-options.c
    386 Pierre Habouzit	
     81 René Scharfe	
parse-options.h
    151 Pierre Habouzit	
     14 René Scharfe	
path.c
    201 Junio C Hamano	
     83 Linus Torvalds	
quote.c
    189 Pierre Habouzit	
    106 Junio C Hamano	
quote.h
     31 Junio C Hamano	
     11 Christian Couder	
run-command.c
    173 Johannes Sixt	
     87 Shawn O. Pearce	
run-command.h
     46 Johannes Sixt	
     20 Shawn O. Pearce	
sigchain.c
     52  Jeff King	
sigchain.h
     11  Jeff King	
strbuf.c
    178 Johannes Schindelin	
    146 Pierre Habouzit	
strbuf.h
     93 Pierre Habouzit	
     16 Junio C Hamano	
string.c
string.h
symbol.c
symbol.h
usage.c
     31 Linus Torvalds	
     28 Petr Baudis	
util.h
wrapper.c
    220 Linus Torvalds	
     69 Junio C Hamano	

Obviously I don't like these results as much, as I do not show up as often 
anymore.

Besides, I think it is not fair to put me on top of the list of authors of 
strbuf.c.

Ciao,
Dscho

Re: [PATCH] fread does not return negative on error

[Ingo Molnar]

* Johannes Schindelin <Johannes.Schindelin@gmx.de> wrote:

> Okay, a script similar to what you propose shows this:

thanks Johannes! I've queued up the commit below, with the names 
added to a new CREDITS file (sorted alphabetically).

	Ingo

-------------->
>From 1b173f77dd0d5fd4f0ff18034aaa79e30da068b9 Mon Sep 17 00:00:00 2001
From: Ingo Molnar <mingo@elte.hu>
Date: Wed, 24 Jun 2009 19:54:29 +0200
Subject: [PATCH] perf_counter tools: Add CREDITS file for Git contributors

Much of perf's libraries comes from the Git project. I noticed
that the files (in tools/perf/util/*.[ch] and elsewhere) are
quite spartan wrt. credits, so lets add a CREDITS file that
includes an (incomplete!) list of main contributors.

Thanks guys, these libraries are really useful. Special thanks
go to Johannes Schindelin and Junio C Hamano for coming up with
this list.

List-Composed-By: Johannes Schindelin <Johannes.Schindelin@gmx.de>
Cc: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Ingo Molnar <mingo@elte.hu>
---
 tools/perf/CREDITS |   30 ++++++++++++++++++++++++++++++
 1 files changed, 30 insertions(+), 0 deletions(-)

diff --git a/tools/perf/CREDITS b/tools/perf/CREDITS
new file mode 100644
index 0000000..c2ddcb3
--- /dev/null
+++ b/tools/perf/CREDITS
@@ -0,0 +1,30 @@
+Most of the infrastructure that 'perf' uses here has been reused
+from the Git project, as of version:
+
+    66996ec: Sync with 1.6.2.4
+
+Here is an (incomplete!) list of main contributors to those files
+in util/* and elsewhere:
+
+ Alex Riesen
+ Christian Couder
+ Dmitry Potapov
+ Jeff King
+ Johannes Schindelin
+ Johannes Sixt
+ Junio C Hamano
+ Linus Torvalds
+ Matthias Kestenholz
+ Michal Ostrowski
+ Miklos Vajna
+ Petr Baudis
+ Pierre Habouzit
+ René Scharfe
+ Samuel Tardieu
+ Shawn O. Pearce
+ Steffen Prohaska
+ Steve Haslam
+
+Thanks guys!
+
+The full history of the files can be found in the upstream Git commits.

Re: [PATCH] fread does not return negative on error

[Alex Riesen]

2009/6/24 Ingo Molnar <mingo@elte.hu>:
> +Here is an (incomplete!) list of main contributors to those files
> +in util/* and elsewhere:
> +
> + Alex Riesen

Wow. But I actually think it may be more fair to sort the list after
any kind of rating (even something as dumb as commit count).

As everyone one the list knows, I am nowhere near a "major contributor"
to the project, as the first place may imply to a casual reader.

Re: [PATCH] fread does not return negative on error

[Junio C Hamano]

Alex Riesen <raa.lkml@gmail.com> writes:

> 2009/6/24 Ingo Molnar <mingo@elte.hu>:
>> +Here is an (incomplete!) list of main contributors to those files
>> +in util/* and elsewhere:
>> +
>> + Alex Riesen
>
> Wow. But I actually think it may be more fair to sort the list after
> any kind of rating (even something as dumb as commit count).
>
> As everyone one the list knows, I am nowhere near a "major contributor"
> to the project, as the first place may imply to a casual reader.

Heh, it was kind of obvious to a casual reader that the list was sorted
alphabetically by the first name (unusual, eh?).

And no, you are indeed a valuable contributor to the git project.

Thanks.

Re: [PATCH] fread does not return negative on error

[Christian Couder]

Hi,

On Wed, Jun 24, 2009 at 10:18 AM, Ingo Molnar<mingo@elte.hu> wrote:
>
> * Junio C Hamano <gitster@pobox.com> wrote:
>
>> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
>>
>> > the following patch is for git.  I just removed the unneeded check for
>> > res == 0 from your version.  Does it look OK?
>>
>> The patch looks good, and both of our in-tree users do error out
>> when the returned value is 0 (imap-send.c checks with "<= 0" which
>> looks a tad amateurish, though) correctly.
>>
>> Funny, there is no caller of this function in the original context
>> this bug originally found, which I think is linux-2.6/tools/perf
>> ;-).
>
> Hehe, yes :-)
>
> Background: when creating tools/perf/ i cherry-picked all the nice
> Git libraries into tools/perf/util/, to give a standard environment
> for all tooling things that might come up in the future.
>
> Some of those are not used yet but it looked more logical to pick up
> whole pieces - some already gained uses. For example config.c is not
> truly used yet, but very much expected to have a role in the future.
>
> ( The only invasive thing i had to do was the s/git_/perf_/ mass
>  rename across all the files - having 'git_' in perf looked
>  quite confusing. )
>
> And our general experience with the Git libraries in
> tools/perf/util/* is: we love them!
>
> For example parse-options.c is a striking improvement compared to
> getopt.h we used before, and all the other facilities are sane and
> straight to the point as well. So in this sense 'perf' is an ...
> interesting cross-discipline 'fork' of Git's generic libraries.
>
> The auto-generation of everything out of Documentation/*.txt is
> another thing we picked up, and that's very nice too.
>
> One bookeeping issue: i found few explicit credits in those files -
> so i noted in the changelog that i took them from Git and i noted
> the specific upstream Git sha1 when i copied them. Would be nice to
> update each file with names to make credit more explicit:

>From http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=tree;f=tools/perf;hb=HEAD
it looks like there may be some other files like builtin-help.c (and
perhaps some files in perf/Documentation/ too though there should be
some AUTHOR information already in them).

Thanks,
Christian.

Re: [PATCH] fread does not return negative on error

[Ingo Molnar]

* Christian Couder <christian.couder@gmail.com> wrote:

> Hi,
> 
> On Wed, Jun 24, 2009 at 10:18 AM, Ingo Molnar<mingo@elte.hu> wrote:
> >
> > * Junio C Hamano <gitster@pobox.com> wrote:
> >
> >> René Scharfe <rene.scharfe@lsrfire.ath.cx> writes:
> >>
> >> > the following patch is for git.  I just removed the unneeded check for
> >> > res == 0 from your version.  Does it look OK?
> >>
> >> The patch looks good, and both of our in-tree users do error out
> >> when the returned value is 0 (imap-send.c checks with "<= 0" which
> >> looks a tad amateurish, though) correctly.
> >>
> >> Funny, there is no caller of this function in the original context
> >> this bug originally found, which I think is linux-2.6/tools/perf
> >> ;-).
> >
> > Hehe, yes :-)
> >
> > Background: when creating tools/perf/ i cherry-picked all the nice
> > Git libraries into tools/perf/util/, to give a standard environment
> > for all tooling things that might come up in the future.
> >
> > Some of those are not used yet but it looked more logical to pick up
> > whole pieces - some already gained uses. For example config.c is not
> > truly used yet, but very much expected to have a role in the future.
> >
> > ( The only invasive thing i had to do was the s/git_/perf_/ mass
> >  rename across all the files - having 'git_' in perf looked
> >  quite confusing. )
> >
> > And our general experience with the Git libraries in
> > tools/perf/util/* is: we love them!
> >
> > For example parse-options.c is a striking improvement compared to
> > getopt.h we used before, and all the other facilities are sane and
> > straight to the point as well. So in this sense 'perf' is an ...
> > interesting cross-discipline 'fork' of Git's generic libraries.
> >
> > The auto-generation of everything out of Documentation/*.txt is
> > another thing we picked up, and that's very nice too.
> >
> > One bookeeping issue: i found few explicit credits in those files -
> > so i noted in the changelog that i took them from Git and i noted
> > the specific upstream Git sha1 when i copied them. Would be nice to
> > update each file with names to make credit more explicit:
> 
> >From http://git.kernel.org/?p=linux/kernel/git/x86/linux-2.6-tip.git;a=tree;f=tools/perf;hb=HEAD
>
> it looks like there may be some other files like builtin-help.c 
> (and perhaps some files in perf/Documentation/ too though there 
> should be some AUTHOR information already in them).

Correct - the makefile and the whole glue code (and much else!) 
comes from Git - see commits:

 0780060: perf_counter tools: add in basic glue from Git
 d24e473: perf_counter: copy in Git's top Makefile

Any suggestions about how best credit everyone there? One central 
linux/tools/perf/CREDITS file?

	Ingo

Re: [PATCH] fread does not return negative on error

[Junio C Hamano]

Ingo Molnar <mingo@elte.hu> writes:

> And our general experience with the Git libraries in 
> tools/perf/util/* is: we love them!

> For example parse-options.c is a striking improvement compared to 
> getopt.h we used before, and all the other facilities are sane and 
> straight to the point as well. So in this sense 'perf' is an ... 
> interesting cross-discipline 'fork' of Git's generic libraries.

Kudos to Pierre, then.