threads / patch / 19900

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

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

## tl;dr

14 messages between Jun 22, 2009 and Jun 25, 2009. Diffs are folded; open one to read it.

replies: 13people: 7as markdown or json

Ingo Molnar· Jun 22, 2009, 15:34 UTC · lore
* Roel Kluin <roel.kluin@gmail.com> wrote:
Show 18 quoted lines
> 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
roel kluin· Jun 22, 2009, 15:47 UTC · re: Ingo Molnar · lore
On Mon, Jun 22, 2009 at 5:34 PM, Ingo Molnar<mingo@elte.hu> wrote:
Show 29 quoted lines
>
> * 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
René Scharfe· Jun 22, 2009, 16:42 UTC · re: roel kluin · lore

[PATCH] fread does not return negative on error

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(-)
Show changes to strbuf.c +1 −1
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;
 }
Junio C Hamano· Jun 23, 2009, 23:56 UTC · re: René Scharfe · lore

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

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.
Show 23 quoted lines
> 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;
>  }
Ingo Molnar· Jun 24, 2009, 08:18 UTC · re: Junio C Hamano · lore

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

* Junio C Hamano <gitster@pobox.com> wrote:
Show 12 quoted lines
> 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
Johannes Schindelin· Jun 24, 2009, 10:03 UTC · re: Ingo Molnar · lore

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

Hi,
On Wed, 24 Jun 2009, Ingo Molnar wrote:
Show 38 quoted lines
> 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

Junio C Hamano· Jun 24, 2009, 16:15 UTC · re: Johannes Schindelin · lore

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

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
Show 13 quoted lines
> 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:

Show 5 quoted lines
> 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).

Johannes Schindelin· Jun 24, 2009, 16:40 UTC · re: Junio C Hamano · lore

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

Hi,
On Wed, 24 Jun 2009, Junio C Hamano wrote:
Show 52 quoted lines
> 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

Ingo Molnar· Jun 24, 2009, 17:59 UTC · re: Johannes Schindelin · lore

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

* 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(-)
Show changes to tools/perf/CREDITS +30 −0
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.
Alex Riesen· Jun 24, 2009, 21:19 UTC · re: Ingo Molnar · lore

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

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.

Junio C Hamano· Jun 24, 2009, 21:55 UTC · re: Alex Riesen · lore

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

Alex Riesen <raa.lkml@gmail.com> writes:
Show 11 quoted lines
> 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.
Christian Couder· Jun 24, 2009, 10:53 UTC · re: Ingo Molnar · lore

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

Hi,
On Wed, Jun 24, 2009 at 10:18 AM, Ingo Molnar<mingo@elte.hu> wrote:
Show 45 quoted lines
>
> * 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.

Ingo Molnar· Jun 24, 2009, 12:12 UTC · re: Christian Couder · lore

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

* Christian Couder <christian.couder@gmail.com> wrote:
Show 54 quoted lines
> 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
Junio C Hamano· Jun 25, 2009, 18:31 UTC · re: Ingo Molnar · lore

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

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.

← back to recent threads