2009-06-22 14:25:39

by Roel Kluin

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

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

Signed-off-by: Roel Kluin <[email protected]>
---
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;
}


2009-06-22 15:34:49

by Ingo Molnar

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


* Roel Kluin <[email protected]> wrote:

> size_t res cannot be less than 0. fread returns 0 on error.
>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> 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

2009-06-22 15:47:22

by Roel Kluin

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

On Mon, Jun 22, 2009 at 5:34 PM, Ingo Molnar<[email protected]> wrote:
>
> * Roel Kluin <[email protected]> wrote:
>
>> size_t res cannot be less than 0. fread returns 0 on error.
>>
>> Signed-off-by: Roel Kluin <[email protected]>
>> ---
>> 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

2009-06-22 16:53:28

by René Scharfe

[permalink] [raw]
Subject: [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 <[email protected]>

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

Reported-by: Ingo Molnar <[email protected]>
Signed-off-by: Roel Kluin <[email protected]>
---
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;
}

2009-06-23 23:56:46

by Junio C Hamano

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

René Scharfe <[email protected]> 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 <[email protected]>
>
> size_t res cannot be less than 0. fread returns 0 on error.
>
> Reported-by: Ingo Molnar <[email protected]>
> Signed-off-by: Roel Kluin <[email protected]>
> ---
> 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;
> }

2009-06-24 08:18:50

by Ingo Molnar

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


* Junio C Hamano <[email protected]> wrote:

> Ren? Scharfe <[email protected]> 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

2009-06-24 10:02:21

by Johannes Schindelin

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

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

2009-06-24 10:54:07

by Christian Couder

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

Hi,

On Wed, Jun 24, 2009 at 10:18 AM, Ingo Molnar<[email protected]> wrote:
>
> * Junio C Hamano <[email protected]> wrote:
>
>> Ren? Scharfe <[email protected]> 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.

2009-06-24 12:12:59

by Ingo Molnar

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


* Christian Couder <[email protected]> wrote:

> Hi,
>
> On Wed, Jun 24, 2009 at 10:18 AM, Ingo Molnar<[email protected]> wrote:
> >
> > * Junio C Hamano <[email protected]> wrote:
> >
> >> Ren? Scharfe <[email protected]> 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

2009-06-24 16:15:31

by Junio C Hamano

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

Johannes Schindelin <[email protected]> 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).

2009-06-24 16:40:33

by Johannes Schindelin

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

Hi,

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

> Johannes Schindelin <[email protected]> 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

2009-06-24 17:59:58

by Ingo Molnar

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


* Johannes Schindelin <[email protected]> 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 <[email protected]>
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 <[email protected]>
Cc: Junio C Hamano <[email protected]>
Signed-off-by: Ingo Molnar <[email protected]>
---
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.

2009-06-24 21:20:09

by Alex Riesen

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

2009/6/24 Ingo Molnar <[email protected]>:
> +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.

2009-06-24 21:56:17

by Junio C Hamano

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

Alex Riesen <[email protected]> writes:

> 2009/6/24 Ingo Molnar <[email protected]>:
>> +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.

2009-06-25 18:31:21

by Junio C Hamano

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

Ingo Molnar <[email protected]> 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.