Discussion:
Adding gitweb.owner, last shot
(too old to reply)
Bruno Ribas
2008-02-08 04:41:52 UTC
Permalink
After some study about the insertion of a new repository configuration,
which sets repository owner, it was decided by the group that creating
another small file inside .git/ is not a good idea. So I started to bench
the viability to add just the gitweb.owner configuration, using gitweb.owner
together with gitweb.description there is no major performance downgrade
compared to $projects_list , as seen below:

8<-------
These times i got with a 1000projects running 2 dd to generate disk IO.
Here comes the resultm
NO projects_list projects_list
16m30s69 15m10s74 default gitweb, using FS's owner
16m07s40 15m24s34 patched to get gitweb.owner
16m37s76 15m59s32 same above, but without gitweb.owner

Now results for a 1000projects on an idle machine.
NO projects_list projects_list
1m19s08 1m09s55 default gitweb, using FS's owner
1m17s58 1m09s55 patched to get gitweb.owner
1m18s49 1m08s96 same above, but without gitweb.owner
8<-------

The idea of creating only the gitweb.owner can be a case of study to
centralize all gitweb repository configuration in one file. Maybe even
change the way $projects_list is formated, as gitweb.cgi needs to check
repository configuration to get repository description to generate
projecT_list page, we could just list repository's directories.
Bruno Ribas
2008-02-08 04:41:53 UTC
Permalink
From: Git Managment for C3SL <***@git.c3sl.ufpr.br>

Now gitweb checks if gitweb.owner exists before trying to get filesystem's
owner.

Allow to use configuration variable gitweb.owner set the repository owner,
it checks the gitweb.owner, if not set it uses filesystem directory's owner.

Useful when we don't want to maintain project list file, and all
repository directories have to have the same owner (for example when the
same SSH account is shared for all projects, using ssh_acl to control
access instead).

Signed-off-by: Git Managment for C3SL <***@git.c3sl.ufpr.br>
---
gitweb/gitweb.perl | 7 ++++++-
1 files changed, 6 insertions(+), 1 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ef2735..e8a43b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1767,7 +1767,12 @@ sub git_get_project_owner {
if (exists $gitweb_project_owner->{$project}) {
$owner = $gitweb_project_owner->{$project};
}
- if (!defined $owner) {
+
+ if (!defined $owner){
+ $owner = git_get_project_config('owner');
+ }
+
+ if (!$owner) {
$owner = get_file_owner("$projectroot/$project");
}
--
1.5.4.34.g053d9-dirty
Bruno Ribas
2008-02-08 04:41:54 UTC
Permalink
From: Git Managment for C3SL <***@git.c3sl.ufpr.br>


Signed-off-by: Git Managment for C3SL <***@git.c3sl.ufpr.br>
---
gitweb/README | 4 ++++
1 files changed, 4 insertions(+), 0 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index 4c8bedf..2163071 100644
--- a/gitweb/README
+++ b/gitweb/README
@@ -233,6 +233,10 @@ You can use the following files in repository:
Displayed in the project summary page. You can use multiple-valued
gitweb.url repository configuration variable for that, but the file
takes precendence.
+ * gitweb.owner
+ You can use the gitweb.owner repository configuration variable to set
+ repository's owner. It is displayed in the project list and summary
+ page. If it's not set, filesystem directory's owner is used.
* various gitweb.* config variables (in config)
Read description of %feature hash for detailed list, and some
descriptions.
--
1.5.4.34.g053d9-dirty
Jakub Narebski
2008-02-08 10:55:33 UTC
Permalink
Post by Bruno Ribas
Allow to use configuration variable gitweb.owner set the repository owner,
it checks the gitweb.owner, if not set it uses filesystem directory's owner.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ef2735..e8a43b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1767,7 +1767,12 @@ sub git_get_project_owner {
if (exists $gitweb_project_owner->{$project}) {
$owner = $gitweb_project_owner->{$project};
}
- if (!defined $owner) {
+
+ if (!defined $owner){
+ $owner = git_get_project_config('owner');
+ }
+
+ if (!$owner) {
$owner = get_file_owner("$projectroot/$project");
}
First, I think the empty lines added are not needed.

Second, git_get_project_config() subroutine _REQUIRES_ for $git_dir to
be set. So you have to set $git_dir before checking repo config; then
you can reuse $git_dir in checking file owner.
--
Jakub Narebski
Poland
ShadeHawk on #git
Bruno Cesar Ribas
2008-02-08 13:53:27 UTC
Permalink
Post by Junio C Hamano
<snip>
First, I think the empty lines added are not needed.
I made those empty lines because original code had same empty lines above, I
just let it to have same pattern, but I can remove. Should I remove?! I'll
resend without it, and with $git_dir set.
Post by Junio C Hamano
Second, git_get_project_config() subroutine _REQUIRES_ for $git_dir to
be set. So you have to set $git_dir before checking repo config; then
you can reuse $git_dir in checking file owner.
--
Jakub Narebski
Poland
ShadeHawk on #git
--
Bruno Ribas - ***@c3sl.ufpr.br
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
Bruno Cesar Ribas
2008-02-08 14:30:27 UTC
Permalink
Post by Bruno Ribas
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ef2735..e8a43b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1767,7 +1767,12 @@ sub git_get_project_owner {
if (exists $gitweb_project_owner->{$project}) {
$owner = $gitweb_project_owner->{$project};
}
- if (!defined $owner) {
+
+ if (!defined $owner){
+ $owner = git_get_project_config('owner');
+ }
+
+ if (!$owner) {
$owner = get_file_owner("$projectroot/$project");
}
I that last 3lines should be inside the block that we call
git_get_project_config, don't you think?
Post by Bruno Ribas
Post by Jakub Narebski
First, I think the empty lines added are not needed.
I made those empty lines because original code had same empty lines above, I
just let it to have same pattern, but I can remove. Should I remove?! I'll
resend without it, and with $git_dir set.
Post by Jakub Narebski
Second, git_get_project_config() subroutine _REQUIRES_ for $git_dir to
be set. So you have to set $git_dir before checking repo config; then
you can reuse $git_dir in checking file owner.
--
Jakub Narebski
Poland
ShadeHawk on #git
--
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
-
To unsubscribe from this list: send the line "unsubscribe git" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Bruno Ribas - ***@c3sl.ufpr.br
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
Jakub Narebski
2008-02-08 15:33:54 UTC
Permalink
I have joined the two emails to reply only once.
Post by Bruno Cesar Ribas
Post by Jakub Narebski
Post by Bruno Ribas
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ef2735..e8a43b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1767,7 +1767,12 @@ sub git_get_project_owner {
if (exists $gitweb_project_owner->{$project}) {
$owner = $gitweb_project_owner->{$project};
}
- if (!defined $owner) {
+
+ if (!defined $owner){
+ $owner = git_get_project_config('owner');
+ }
+
+ if (!$owner) {
$owner = get_file_owner("$projectroot/$project");
}
Another comment: why did you change from checking of "!defined $owner"
to checking "!$owner"? git_get_project_config('owner') returns undef
if gitweb.owner is not defined. With checking for defined we can avoid
false positives of owner being "0" (in practice I think this does not
matter) or "" (this could happen if somebody doesn't want for project
to have owner shown).
Post by Bruno Cesar Ribas
Post by Jakub Narebski
First, I think the empty lines added are not needed.
I made those empty lines because original code had same empty lines
above, I just let it to have same pattern, but I can remove. Should I
remove?!
The idea was for empty lines to separate blocks of code: variables
declaration, initialization, finding an owner, and return value.
So I think that empty lines are not needed here. There were no empty
lines between check for owner in the structure populated by
git_get_project_list_from_file() and checking filesystem stat for
project directory owner.

By the way, the git_get_project_list_from_file() interface is a bit
strange...
Post by Bruno Cesar Ribas
I that last 3lines should be inside the block that we call
git_get_project_config, don't you think?
No. I think using "if (!defined $foo) { maybe define foo }..."
sequence is a good flow.
Post by Bruno Cesar Ribas
I'll resend [...] with $git_dir set.
And with signoff corrected, I assume?

Please try to check if the code works with and without gitweb.owner set
before sending new version of the patch...
--
Jakub Narebski
Poland
Bruno Cesar Ribas
2008-02-08 16:07:57 UTC
Permalink
Post by Jakub Narebski
I have joined the two emails to reply only once.
Post by Bruno Cesar Ribas
Post by Bruno Ribas
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 8ef2735..e8a43b7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -1767,7 +1767,12 @@ sub git_get_project_owner {
if (exists $gitweb_project_owner->{$project}) {
$owner = $gitweb_project_owner->{$project};
}
- if (!defined $owner) {
+
+ if (!defined $owner){
+ $owner = git_get_project_config('owner');
+ }
+
+ if (!$owner) {
$owner = get_file_owner("$projectroot/$project");
}
Another comment: why did you change from checking of "!defined $owner"
to checking "!$owner"? git_get_project_config('owner') returns undef
if gitweb.owner is not defined. With checking for defined we can avoid
false positives of owner being "0" (in practice I think this does not
matter) or "" (this could happen if somebody doesn't want for project
to have owner shown).
When I tested it returned empty, but i found out it was lack of NAME for the
user i was running tests.
Post by Jakub Narebski
Post by Bruno Cesar Ribas
I'll resend [...] with $git_dir set.
And with signoff corrected, I assume?
of course.
Post by Jakub Narebski
Please try to check if the code works with and without gitweb.owner set
before sending new version of the patch...
I always do that.
Post by Jakub Narebski
--
Jakub Narebski
Poland
--
Bruno Ribas - ***@c3sl.ufpr.br
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
Junio C Hamano
2008-02-08 07:38:44 UTC
Permalink
... there is no major performance downgrade
8<-------
These times i got with a 1000projects running 2 dd to generate disk IO.
Here comes the resultm
NO projects_list projects_list
16m30s69 15m10s74 default gitweb, using FS's owner
16m07s40 15m24s34 patched to get gitweb.owner
16m37s76 15m59s32 same above, but without gitweb.owner
Now results for a 1000projects on an idle machine.
NO projects_list projects_list
1m19s08 1m09s55 default gitweb, using FS's owner
1m17s58 1m09s55 patched to get gitweb.owner
1m18s49 1m08s96 same above, but without gitweb.owner
8<-------
Large installations would maintain the project_list in the flat
file format for performance reasons anyway. Benchmarking under
a condition that yields unreasonably long response time is
somewhat meaningless, I am afraid. Who sane would wait for 15
minutes for project list to come up?

So I think your patch makes sense. It would not help nor hurt
large installations, and would help smaller installations that
do not care much about performance but are more interested in
the convenience of not having to worry about maintaining the
project_list.

As the act of signing off patches is a legal statement, I'd
prefer real person's name, not "Git Managment for C3SL", in the
messages to be applied. The change that adds the feature, and
the documentation update to describe that new feature, should be
in the same single patch for a small change like this.
Bruno Cesar Ribas
2008-02-08 13:49:21 UTC
Permalink
Post by Junio C Hamano
... there is no major performance downgrade
8<-------
These times i got with a 1000projects running 2 dd to generate disk IO.
Here comes the resultm
NO projects_list projects_list
16m30s69 15m10s74 default gitweb, using FS's owner
16m07s40 15m24s34 patched to get gitweb.owner
16m37s76 15m59s32 same above, but without gitweb.owner
Now results for a 1000projects on an idle machine.
NO projects_list projects_list
1m19s08 1m09s55 default gitweb, using FS's owner
1m17s58 1m09s55 patched to get gitweb.owner
1m18s49 1m08s96 same above, but without gitweb.owner
8<-------
Large installations would maintain the project_list in the flat
file format for performance reasons anyway. Benchmarking under
a condition that yields unreasonably long response time is
somewhat meaningless, I am afraid. Who sane would wait for 15
minutes for project list to come up?
But it got 15minutes with project_list to! The real problem is geting last
change on repository under high IO.
Post by Junio C Hamano
<snip>
As the act of signing off patches is a legal statement, I'd
prefer real person's name, not "Git Managment for C3SL", in the
messages to be applied. The change that adds the feature, and
the documentation update to describe that new feature, should be
in the same single patch for a small change like this.
I'll resend it !! my mistake of names =(
Post by Junio C Hamano
-
To unsubscribe from this list: send the line "unsubscribe git" in
More majordomo info at http://vger.kernel.org/majordomo-info.html
--
Bruno Ribas - ***@c3sl.ufpr.br
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
Jakub Narebski
2008-02-08 10:34:55 UTC
Permalink
Post by Bruno Ribas
After some study about the insertion of a new repository configuration,
which sets repository owner, it was decided by the group that creating
another small file inside .git/ is not a good idea. So I started to bench
the viability to add just the gitweb.owner configuration, using gitweb.owner
together with gitweb.description there is no major performance downgrade
8<-------
These times i got with a 1000projects running 2 dd to generate disk IO.
Here comes the resultm
NO projects_list projects_list
16m30s69 15m10s74 default gitweb, using FS's owner
16m07s40 15m24s34 patched to get gitweb.owner
16m37s76 15m59s32 same above, but without gitweb.owner
Now results for a 1000projects on an idle machine.
NO projects_list projects_list
1m19s08 1m09s55 default gitweb, using FS's owner
1m17s58 1m09s55 patched to get gitweb.owner
1m18s49 1m08s96 same above, but without gitweb.owner
8<-------
It looks like there is almost no difference between using only FS
owner, and reading also repository config using "git config -z -l"...
Could anyone using gitweb on MS Windows or MacOS X, where fork is mich
slower, check those figures?

Pasky, could you please try to benchmark this (well, at least without
gitweb.owner set) on a _real_ large set of repositories?
Post by Bruno Ribas
The idea of creating only the gitweb.owner can be a case of study to
centralize all gitweb repository configuration in one file. Maybe even
change the way $projects_list is formated, as gitweb.cgi needs to check
repository configuration to get repository description to generate
project_list page, we could just list repository's directories.
I was thinking about git-config-like format (but simplified to make it
easy to parse it in Perl, just like git-cvsserver configuration), in
the form of

[gitweb "<repository path, relative to $projectroot>"]
description = <project description>
url = <first URL>
url = <seconf URL>
owner = <repository owner>

The problem with parsing lies (among others) in the in-line comments,
novalue keys, and key after section, I think...
--
Jakub Narebski
Poland
ShadeHawk on #git
Bruno Cesar Ribas
2008-02-08 13:51:50 UTC
Permalink
Post by Junio C Hamano
<snip>
Pasky, could you please try to benchmark this (well, at least without
gitweb.owner set) on a _real_ large set of repositories?
And I'd like to know what this machine runs! If it is git dedicated, or
shares with something else.

If it is git dedicated, it makes time better as disk IO will not be severe.
Post by Junio C Hamano
Post by Bruno Ribas
The idea of creating only the gitweb.owner can be a case of study to
centralize all gitweb repository configuration in one file. Maybe even
change the way $projects_list is formated, as gitweb.cgi needs to check
repository configuration to get repository description to generate
project_list page, we could just list repository's directories.
I was thinking about git-config-like format (but simplified to make it
easy to parse it in Perl, just like git-cvsserver configuration), in
the form of
[gitweb "<repository path, relative to $projectroot>"]
description = <project description>
url = <first URL>
url = <seconf URL>
owner = <repository owner>
The problem with parsing lies (among others) in the in-line comments,
novalue keys, and key after section, I think...
--
Jakub Narebski
Poland
ShadeHawk on #git
--
Bruno Ribas - ***@c3sl.ufpr.br
http://web.inf.ufpr.br/ribas
C3SL: http://www.c3sl.ufpr.br
Continue reading on narkive:
Loading...