[PATCH] gitweb: Option to omit column with time of the last change

[PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

Generating information about last change for a large number of git
repositories can be time consuming. This commit adds an option to
omit 'Last Change' column when presenting the list of repositories.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Documentation/gitweb.conf.txt |    3 +++
 gitweb/gitweb.perl            |   16 +++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index 7aba497..bfeef21 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -403,6 +403,9 @@ $default_projects_order::
 	i.e. path to repository relative to `$projectroot`), "descr"
 	(project description), "owner", and "age" (by date of most current
 	commit).
+
+$no_list_age::
+	Omit column with date of the most curren commit
 +
 Default value is "project".  Unknown value means unsorted.
 
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..f42468c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -133,6 +133,9 @@ our $default_projects_order = "project";
 # (only effective if this variable evaluates to true)
 our $export_ok = "++GITWEB_EXPORT_OK++";
 
+# don't generate age column
+our $no_list_age = 0;
+
 # show repository only if this subroutine returns true
 # when given the path to the project, for example:
 #    sub { return -e "$_[0]/git-daemon-export-ok"; }
@@ -5462,9 +5465,11 @@ sub git_project_list_rows {
 		                        : esc_html($pr->{'descr'})) .
 		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
-		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
-		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
-		      "<td class=\"link\">" .
+		unless ($no_list_age) {
+		        print "<td class=\"". age_class($pr->{'age'}) . "\">" .
+		            (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n";
+		}
+		print"<td class=\"link\">" .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary")}, "summary")   . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"shortlog")}, "shortlog") . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
@@ -5495,7 +5500,8 @@ sub git_project_list_body {
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $search_regexp);
 	# fill the rest
-	@projects = fill_project_list_info(\@projects);
+	my @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
+	@projects = fill_project_list_info(\@projects, @all_fields);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
@@ -5527,7 +5533,7 @@ sub git_project_list_body {
 		print_sort_th('project', $order, 'Project');
 		print_sort_th('descr', $order, 'Description');
 		print_sort_th('owner', $order, 'Owner');
-		print_sort_th('age', $order, 'Last Change');
+		print_sort_th('age', $order, 'Last Change') unless $no_list_age;
 		print "<th></th>\n" . # for links
 		      "</tr>\n";
 	}
-- 
1.7.10.rc3

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Jakub Narebski]

On Tue, 3 Apr 2012, Kacper Kornet wrote:

> Generating information about last change for a large number of git
> repositories can be time consuming. This commit adds an option to
> omit 'Last Change' column when presenting the list of repositories.
>
This is quite a good idea, I think.

Even a better solution would be to add caching support to gitweb, but
first it is a long way from being ready, and second not in all cases
you are able to / wants to have a cache.
 
> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
> ---
>  Documentation/gitweb.conf.txt |    3 +++
>  gitweb/gitweb.perl            |   16 +++++++++++-----
>  2 files changed, 14 insertions(+), 5 deletions(-)
> 
> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> index 7aba497..bfeef21 100644
> --- a/Documentation/gitweb.conf.txt
> +++ b/Documentation/gitweb.conf.txt
> @@ -403,6 +403,9 @@ $default_projects_order::
>  	i.e. path to repository relative to `$projectroot`), "descr"
>  	(project description), "owner", and "age" (by date of most current
>  	commit).
> +
> +$no_list_age::
> +	Omit column with date of the most curren commit

s/curren/current/

Thanks for adding documentation, though I would prefer if you expanded
this description (for example including the information that it touches
projects list page).

>  +

This '+' here means "continuation".  You by accident inserted
description of new $no_list_age in the middle of description for
$default_projects_order variable...

>  Default value is "project".  Unknown value means unsorted.

...as you can see here.

>  
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..f42468c 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -133,6 +133,9 @@ our $default_projects_order = "project";
>  # (only effective if this variable evaluates to true)
>  our $export_ok = "++GITWEB_EXPORT_OK++";
>  
> +# don't generate age column
> +our $no_list_age = 0;

"age" column where?

Hmmm... can't we come with a better name than $no_list_age?

> +
>  # show repository only if this subroutine returns true
>  # when given the path to the project, for example:
>  #    sub { return -e "$_[0]/git-daemon-export-ok"; }
> @@ -5462,9 +5465,11 @@ sub git_project_list_rows {
>  		                        : esc_html($pr->{'descr'})) .
>  		      "</td>\n" .
>  		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
> -		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
> -		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
> -		      "<td class=\"link\">" .
> +		unless ($no_list_age) {
> +		        print "<td class=\"". age_class($pr->{'age'}) . "\">" .
> +		            (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n";
> +		}
> +		print"<td class=\"link\">" .

O.K.  I guess that it ismore readable than

  +		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
  +		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
  +		      "<td class=\"link\">"
  +			unless ($no_list_age);

>  		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary")}, "summary")   . " | " .
>  		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"shortlog")}, "shortlog") . " | " .
>  		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
> @@ -5495,7 +5500,8 @@ sub git_project_list_body {
>  	                                 'tagfilter'  => $tagfilter)
>  		if ($tagfilter || $search_regexp);
>  	# fill the rest
> -	@projects = fill_project_list_info(\@projects);
> +	my @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
> +	@projects = fill_project_list_info(\@projects, @all_fields);

That looks quite strange on first glance.  I know that empty list means
filling all fields, but the casual reader migh wonder about this
conditional expression.

Perhaps it would be better to write it this way:

  -	@projects = fill_project_list_info(\@projects);
  +	my @fields = qw(descr descr_long owner ctags category);
  +	push @fields, 'age' unless ($no_list_age);
  +	@projects = fill_project_list_info(\@projects, @fields);

or something like that.

Well, at least until we come up with a better way to specify "all fields
except those specified".

>  
>  	$order ||= $default_projects_order;
>  	$from = 0 unless defined $from;
> @@ -5527,7 +5533,7 @@ sub git_project_list_body {
>  		print_sort_th('project', $order, 'Project');
>  		print_sort_th('descr', $order, 'Description');
>  		print_sort_th('owner', $order, 'Owner');
> -		print_sort_th('age', $order, 'Last Change');
> +		print_sort_th('age', $order, 'Last Change') unless $no_list_age;

  +		print_sort_th('age', $order, 'Last Change')
  +			unless $no_list_age;

might be more readable.

>  		print "<th></th>\n" . # for links
>  		      "</tr>\n";
>  	}
> -- 
> 1.7.10.rc3
> 

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
> On Tue, 3 Apr 2012, Kacper Kornet wrote:
> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
> > index 7aba497..bfeef21 100644
> > --- a/Documentation/gitweb.conf.txt
> > +++ b/Documentation/gitweb.conf.txt
> > @@ -403,6 +403,9 @@ $default_projects_order::
> >  	i.e. path to repository relative to `$projectroot`), "descr"
> >  	(project description), "owner", and "age" (by date of most current
> >  	commit).
> > +
> > +$no_list_age::
> > +	Omit column with date of the most curren commit

> s/curren/current/

> Thanks for adding documentation, though I would prefer if you expanded
> this description (for example including the information that it touches
> projects list page).

What about:

$no_list_age::
	Whether to show the column with date of the most current commit on the
	projects list page. It can save a bit of I/O.

> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index a8b5fad..f42468c 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -133,6 +133,9 @@ our $default_projects_order = "project";
> >  # (only effective if this variable evaluates to true)
> >  our $export_ok = "++GITWEB_EXPORT_OK++";

> > +# don't generate age column
> > +our $no_list_age = 0;

> "age" column where?

> Hmmm... can't we come with a better name than $no_list_age?

Any of $no_age_column, $omit_age_column, $no_last_commit would be better?


> > @@ -5495,7 +5500,8 @@ sub git_project_list_body {
> >  	                                 'tagfilter'  => $tagfilter)
> >  		if ($tagfilter || $search_regexp);
> >  	# fill the rest
> > -	@projects = fill_project_list_info(\@projects);
> > +	my @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
> > +	@projects = fill_project_list_info(\@projects, @all_fields);

> That looks quite strange on first glance.  I know that empty list means
> filling all fields, but the casual reader migh wonder about this
> conditional expression.

> Perhaps it would be better to write it this way:

>   -	@projects = fill_project_list_info(\@projects);
>   +	my @fields = qw(descr descr_long owner ctags category);
>   +	push @fields, 'age' unless ($no_list_age);
>   +	@projects = fill_project_list_info(\@projects, @fields);

> or something like that.

> Well, at least until we come up with a better way to specify "all fields
> except those specified".

Yes, that's better. Especially that I would like also to introduce
option to prevent printing repository owner everywhere.

-- 
  Kacper Kornet

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Jakub Narebski]

On Wed, 4 April 2012, Kacper Kornet wrote:
> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
>> On Tue, 3 Apr 2012, Kacper Kornet wrote:

>>> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
>>> index 7aba497..bfeef21 100644
>>> --- a/Documentation/gitweb.conf.txt
>>> +++ b/Documentation/gitweb.conf.txt
>>> @@ -403,6 +403,9 @@ $default_projects_order::
>>>  	i.e. path to repository relative to `$projectroot`), "descr"
>>>  	(project description), "owner", and "age" (by date of most current
>>>  	commit).
>>> +
>>> +$no_list_age::
>>> +	Omit column with date of the most curren commit
> 
>> s/curren/current/
> 
>> Thanks for adding documentation, though I would prefer if you expanded
>> this description (for example including the information that it touches
>> projects list page).
> 
> What about:
> 
> $no_list_age::
> 	Whether to show the column with date of the most current commit on the
> 	projects list page. It can save a bit of I/O.

Perhaps it would be better to say it like this:

  $no_list_age::
  	If true, omit the column with date of the most current commit on the
  	projects list page. [...]

It is true that it can save a bit of I/O: the git_get_last_activity()
examines all branches (some of which are usually loose), and must hit
the object database, unpacking/getting commit objects to get at commit
date.

But the fact that it also saves a fork (a git command call) per repository
reminds me of something which I missed in first round of review, namely
that generating 'age' and 'age_string' fields serve also as a check if
repository really exist.

So either we document this fact, or use some other way to verify that
git repository is valid.

>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
>>> index a8b5fad..f42468c 100755
>>> --- a/gitweb/gitweb.perl
>>> +++ b/gitweb/gitweb.perl
>>> @@ -133,6 +133,9 @@ our $default_projects_order = "project";
>>>  # (only effective if this variable evaluates to true)
>>>  our $export_ok = "++GITWEB_EXPORT_OK++";
> 
>>> +# don't generate age column
>>> +our $no_list_age = 0;
> 
>> "age" column where?
> 
>> Hmmm... can't we come with a better name than $no_list_age?
> 
> Any of $no_age_column, $omit_age_column, $no_last_commit would be better?

$no_age_column seems better than $no_list_age... but see below. 
 
>>> @@ -5495,7 +5500,8 @@ sub git_project_list_body {
>>>  	                                 'tagfilter'  => $tagfilter)
>>>  		if ($tagfilter || $search_regexp);
>>>  	# fill the rest
>>> -	@projects = fill_project_list_info(\@projects);
>>> +	my @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
>>> +	@projects = fill_project_list_info(\@projects, @all_fields);
> 
>> That looks quite strange on first glance.  I know that empty list means
>> filling all fields, but the casual reader migh wonder about this
>> conditional expression.
> 
>> Perhaps it would be better to write it this way:
> 
>>   -	@projects = fill_project_list_info(\@projects);
>>   +	my @fields = qw(descr descr_long owner ctags category);
>>   +	push @fields, 'age' unless ($no_list_age);
>>   +	@projects = fill_project_list_info(\@projects, @fields);
> 
>> or something like that.
> 
>> Well, at least until we come up with a better way to specify "all fields
>> except those specified".
> 
> Yes, that's better. Especially that I would like also to introduce
> option to prevent printing repository owner everywhere.

Well, because this patch affects gitweb configuration, and because we
need to preserve (as far as possible) the backward compatibility with
existing gitweb configuration files we need to be careful with changes.

Perhaps instead of $no_age_column that can be single configuration
variable like @excluded_project_list_fields instead of one variable
per column.  Somebody might want to omit project description as well
(though then project search must be limited to project names only).
Though this approach will have problem that some of columns simply
have to be present... maybe one variable per column (perhaps hidden
in a hash) is a better solution.

-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:
> On Wed, 4 April 2012, Kacper Kornet wrote:
> > On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
> >> On Tue, 3 Apr 2012, Kacper Kornet wrote:
> > What about:

> > $no_list_age::
> > 	Whether to show the column with date of the most current commit on the
> > 	projects list page. It can save a bit of I/O.

> Perhaps it would be better to say it like this:

>   $no_list_age::
>   	If true, omit the column with date of the most current commit on the
>   	projects list page. [...]

> It is true that it can save a bit of I/O: the git_get_last_activity()
> examines all branches (some of which are usually loose), and must hit
> the object database, unpacking/getting commit objects to get at commit
> date.

> But the fact that it also saves a fork (a git command call) per repository
> reminds me of something which I missed in first round of review, namely
> that generating 'age' and 'age_string' fields serve also as a check if
> repository really exist.

> So either we document this fact, or use some other way to verify that
> git repository is valid.

I think that git_project_list_body always works with the list returned
by git_get_projects_list. And git_get_projects_list validates if the
path is a git repository. So it should not be a problem. Please correct
me, if I am wrong.



> >>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> >>> index a8b5fad..f42468c 100755
> >>> --- a/gitweb/gitweb.perl
> >>> +++ b/gitweb/gitweb.perl
> >>> @@ -133,6 +133,9 @@ our $default_projects_order = "project";
> >>>  # (only effective if this variable evaluates to true)
> >>>  our $export_ok = "++GITWEB_EXPORT_OK++";

> >>> +# don't generate age column
> >>> +our $no_list_age = 0;

> >> "age" column where?

> >> Hmmm... can't we come with a better name than $no_list_age?

> > Any of $no_age_column, $omit_age_column, $no_last_commit would be better?

> $no_age_column seems better than $no_list_age... but see below. 

> >>> @@ -5495,7 +5500,8 @@ sub git_project_list_body {
> >>>  	                                 'tagfilter'  => $tagfilter)
> >>>  		if ($tagfilter || $search_regexp);
> >>>  	# fill the rest
> >>> -	@projects = fill_project_list_info(\@projects);
> >>> +	my @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
> >>> +	@projects = fill_project_list_info(\@projects, @all_fields);

> >> That looks quite strange on first glance.  I know that empty list means
> >> filling all fields, but the casual reader migh wonder about this
> >> conditional expression.

> >> Perhaps it would be better to write it this way:

> >>   -	@projects = fill_project_list_info(\@projects);
> >>   +	my @fields = qw(descr descr_long owner ctags category);
> >>   +	push @fields, 'age' unless ($no_list_age);
> >>   +	@projects = fill_project_list_info(\@projects, @fields);

> >> or something like that.

> >> Well, at least until we come up with a better way to specify "all fields
> >> except those specified".

> > Yes, that's better. Especially that I would like also to introduce
> > option to prevent printing repository owner everywhere.

> Well, because this patch affects gitweb configuration, and because we
> need to preserve (as far as possible) the backward compatibility with
> existing gitweb configuration files we need to be careful with changes.

> Perhaps instead of $no_age_column that can be single configuration
> variable like @excluded_project_list_fields instead of one variable
> per column.  Somebody might want to omit project description as well
> (though then project search must be limited to project names only).
> Though this approach will have problem that some of columns simply
> have to be present... maybe one variable per column (perhaps hidden
> in a hash) is a better solution.

I thought about two different variables as that would have a slightly
different functionality. While I want to get rid off Last Change from
the projects list page, I still want to get this information on pages of
single repositories. On the other hand I don't want repository owner to
be shown anywhere.

-- 
  Kacper Kornet

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Perhaps instead of $no_age_column that can be single configuration
> variable like @excluded_project_list_fields instead of one variable
> per column.

True.

In general we avoid no_anything because it tends to introduce hard-to-read
double negation in the code and the documentation (e.g. "if (!$no_frotz)"),
but "next if (exists $exclude{$it})" is probably not so bad.

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Jakub Narebski]

I'm sorry for the delay answering.

On Wed, 4 Apr 2012, Kacper Kornet wrote:
> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:
>> On Wed, 4 April 2012, Kacper Kornet wrote:
>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
>>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:
 
>> Perhaps it would be better to say it like this:
>> 
>>   $no_list_age::
>>   	If true, omit the column with date of the most current commit on the
>>   	projects list page. [...]
>> 
>> It is true that it can save a bit of I/O: the git_get_last_activity()
>> examines all branches (some of which are usually loose), and must hit
>> the object database, unpacking/getting commit objects to get at commit
>> date.
>> 
>> But the fact that it also saves a fork (a git command call) per repository
>> reminds me of something which I missed in first round of review, namely
>> that generating 'age' and 'age_string' fields serve also as a check if
>> repository really exist.
>> 
>> So either we document this fact, or use some other way to verify that
>> git repository is valid.
> 
> I think that git_project_list_body always works with the list returned
> by git_get_projects_list. And git_get_projects_list validates if the
> path is a git repository. So it should not be a problem. Please correct
> me, if I am wrong.
 
If $projects_list points to plain file, git_get_projects_list() just
gets list of projects (and project owners) from $projects_list file
by reading and parsing this file.  No verification that repository
exists is done.

If $projects_list points to directory (which is the default), then
git_get_projects_list() scans starting from $projects_list directory
for somtehing that _looks like_ git repository with check_head_link()
via check_export_ok().  But you still can encounter something that
looks like git repository (has "HEAD" file in it), but isn't.


So we would probably want to have said variable or set of variables
describe three states:

* find date of last change in repository with git-for-each-ref called
  by git_get_last_activity(), which as a side effect verifies that
  repository actually exist.  

  git_get_last_activity() returns empty list in list context if repo
  does not exist, hence

  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}

* verify that repository exists with "git rev-parse --git-dir" or
  "git rev-parse --verify HEAD", or "git symbolic-ref HEAD", redirecting
  stderr to /dev/null (we would probably want to save output of the
  latter two somewhere to use it later).
  
  That saves I/O, but not fork.

* don't verify that repository exists.


Though perhaps the last possibility isn't a good idea, so it would be
left two-state, as in your patch. 

>>> [...]. Especially that I would like also to introduce
>>> option to prevent printing repository owner everywhere.
>>> 
>> Well, because this patch affects gitweb configuration, and because we
>> need to preserve (as far as possible) the backward compatibility with
>> existing gitweb configuration files we need to be careful with changes.
>> 
>> Perhaps instead of $no_age_column that can be single configuration
>> variable like @excluded_project_list_fields instead of one variable
>> per column.  Somebody might want to omit project description as well
>> (though then project search must be limited to project names only).
>> Though this approach will have problem that some of columns simply
>> have to be present... maybe one variable per column (perhaps hidden
>> in a hash) is a better solution.
> 
> I thought about two different variables as that would have a slightly
> different functionality. While I want to get rid off Last Change from
> the projects list page, I still want to get this information on pages of
> single repositories. On the other hand I don't want repository owner to
> be shown anywhere.

Ah, in this case we want to keep $dont_show_repo_age / $no_age_column
(or something like that) separate from $no_owner... and in this case
these features can be controlled by separate scalar configuration
variables.


-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:
> On Wed, 4 Apr 2012, Kacper Kornet wrote:
> > On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:
> >> On Wed, 4 April 2012, Kacper Kornet wrote:
> >>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
> >>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:

> >> Perhaps it would be better to say it like this:

> >>   $no_list_age::
> >>   	If true, omit the column with date of the most current commit on the
> >>   	projects list page. [...]

> >> It is true that it can save a bit of I/O: the git_get_last_activity()
> >> examines all branches (some of which are usually loose), and must hit
> >> the object database, unpacking/getting commit objects to get at commit
> >> date.

> >> But the fact that it also saves a fork (a git command call) per repository
> >> reminds me of something which I missed in first round of review, namely
> >> that generating 'age' and 'age_string' fields serve also as a check if
> >> repository really exist.

> >> So either we document this fact, or use some other way to verify that
> >> git repository is valid.

> > I think that git_project_list_body always works with the list returned
> > by git_get_projects_list. And git_get_projects_list validates if the
> > path is a git repository. So it should not be a problem. Please correct
> > me, if I am wrong.

> If $projects_list points to plain file, git_get_projects_list() just
> gets list of projects (and project owners) from $projects_list file
> by reading and parsing this file.  No verification that repository
> exists is done.

I think that even in this case check_export_ok is called. But there is
still the problem you have mentioned below.

> If $projects_list points to directory (which is the default), then
> git_get_projects_list() scans starting from $projects_list directory
> for somtehing that _looks like_ git repository with check_head_link()
> via check_export_ok().  But you still can encounter something that
> looks like git repository (has "HEAD" file in it), but isn't.


> So we would probably want to have said variable or set of variables
> describe three states:

> * find date of last change in repository with git-for-each-ref called
>   by git_get_last_activity(), which as a side effect verifies that
>   repository actually exist.  

>   git_get_last_activity() returns empty list in list context if repo
>   does not exist, hence

>   	my (@activity) = git_get_last_activity($pr->{'path'});
>   	unless (@activity) {
>   		next PROJECT;
>   	}

> * verify that repository exists with "git rev-parse --git-dir" or
>   "git rev-parse --verify HEAD", or "git symbolic-ref HEAD", redirecting
>   stderr to /dev/null (we would probably want to save output of the
>   latter two somewhere to use it later).

>   That saves I/O, but not fork.

> * don't verify that repository exists.

> Though perhaps the last possibility isn't a good idea, so it would be
> left two-state, as in your patch. 

My tests show that forks are also a bottleneck in my setup. On the other
hand I think that I can trust that my projects.list contains only valid
repositories. So I would prefer to have a don't verify option. Or there
is a possibility to write perl function with the same functionality as
is_git_directory() from setup.c and use it to verify if the directory is a
valid git repo.

-- 
  Kacper Kornet

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Jakub Narebski]

On Mon, 16 Apr 2012, Kacper Kornet wrote:
> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:
>> On Wed, 4 Apr 2012, Kacper Kornet wrote:
>>> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:
>>>> On Wed, 4 April 2012, Kacper Kornet wrote:
>>>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
>>>>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:
[...] 
>>>> But the fact that it also saves a fork (a git command call) per repository
>>>> reminds me of something which I missed in first round of review, namely
>>>> that generating 'age' and 'age_string' fields serve also as a check if
>>>> repository really exist.
>>>> 
>>>> So either we document this fact, or use some other way to verify that
>>>> git repository is valid.
>>> 
>>> I think that git_project_list_body always works with the list returned
>>> by git_get_projects_list. And git_get_projects_list validates if the
>>> path is a git repository. So it should not be a problem. Please correct
>>> me, if I am wrong.
> 
>> If $projects_list points to plain file, git_get_projects_list() just
>> gets list of projects (and project owners) from $projects_list file
>> by reading and parsing this file.  No verification that repository
>> exists is done.
> 
> I think that even in this case check_export_ok is called. But there is
> still the problem you have mentioned below.

True, somehow I missed the fact that check_export_ok() is called to
check if it looks like repository and if it is to be exported in both
cases ($projects_list a directory or a file).

>> If $projects_list points to directory (which is the default), then
>> git_get_projects_list() scans starting from $projects_list directory
>> for somtehing that _looks like_ git repository with check_head_link()
>> via check_export_ok().  But you still can encounter something that
>> looks like git repository (has "HEAD" file in it), but isn't.
>> 
>> So we would probably want to have said variable or set of variables
>> describe three states:
> 
>> * find date of last change in repository with git-for-each-ref called
>>   by git_get_last_activity(), which as a side effect verifies that
>>   repository actually exist.  
>> 
>>   git_get_last_activity() returns empty list in list context if repo
>>   does not exist, hence
>> 
>>   	my (@activity) = git_get_last_activity($pr->{'path'});
>>   	unless (@activity) {
>>   		next PROJECT;
>>   	}
>> 
>> * verify that repository exists with "git rev-parse --git-dir" or
>>   "git rev-parse --verify HEAD", or "git symbolic-ref HEAD", redirecting
>>   stderr to /dev/null (we would probably want to save output of the
>>   latter two somewhere to use it later).
>> 
>>   That saves I/O, but not fork.

Actually if you look at the footer of projects list page with 'timed'
feature enable you see that for N projects on list, gitweb uses 2*N+1
git commands.  The "+1" part is from "git version", the "2*N" are from
git-for-each-ref to get last activity (and verify repository as a
side-effect)...

...and from call to "git config" to get owner (unconditional check for
`gitweb.owner` override), description (if '.git/description' file got
deleted), if applicable category (file then config), if applicable ctags
(file(s) then config).

So we can rely on "git config" being called, no need for separate
verification.  My mistake.  (Though it might be hard to use this fact.)


Well, with proposed option to remove 'owner' field we would have sometimes
to verify repository with an extra git command...

>> * don't verify that repository exists.
> 
>> Though perhaps the last possibility isn't a good idea, so it would be
>> left two-state, as in your patch. 
> 
> My tests show that forks are also a bottleneck in my setup.

What do you mean by "my tests" here?  Is it benchmark (perhaps just using
'timed' feature) with and without custom change removing fork(s)?  Or did
you used profiler (e.g. wondefull Devel::NYTProf) for that?

>                                                             On the other 
> hand I think that I can trust that my projects.list contains only valid
> repositories. So I would prefer to have a don't verify option. Or there
> is a possibility to write perl function with the same functionality as
> is_git_directory() from setup.c and use it to verify if the directory is a
> valid git repo.

Well, we can add those checks to check_export_ok()... well to function
it would call.

is_git_repository from setup.c checks that "/objects" and "/refs"
have executable permissions, and that "/HEAD" is valid via validate_headref
which does slightly more than (now slightly misnamed) check_head_link()
from gitweb...

...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable
is set, and path that it points to has executable permissions.  I don't
think we have to worry about this for gitweb.


I'll try to come up with a patch to gitweb that improves repository
verification, and perhaps a patch that uses the fact that "git config"
succeeded to verify repository.

-- 
Jakub Narębski

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

I'm sorry to Jakub for duplicate message, but I have forgotten CC: to
the group.

On Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:
> On Mon, 16 Apr 2012, Kacper Kornet wrote:
> > On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:
> >> On Wed, 4 Apr 2012, Kacper Kornet wrote:
> >>> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:
> >>>> On Wed, 4 April 2012, Kacper Kornet wrote:
> >>>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:
> >> So we would probably want to have said variable or set of variables
> >> describe three states:

> >> * find date of last change in repository with git-for-each-ref called
> >>   by git_get_last_activity(), which as a side effect verifies that
> >>   repository actually exist.  

> >>   git_get_last_activity() returns empty list in list context if repo
> >>   does not exist, hence

> >>   	my (@activity) = git_get_last_activity($pr->{'path'});
> >>   	unless (@activity) {
> >>   		next PROJECT;
> >>   	}

> >> * verify that repository exists with "git rev-parse --git-dir" or
> >>   "git rev-parse --verify HEAD", or "git symbolic-ref HEAD", redirecting
> >>   stderr to /dev/null (we would probably want to save output of the
> >>   latter two somewhere to use it later).

> >>   That saves I/O, but not fork.

> Actually if you look at the footer of projects list page with 'timed'
> feature enable you see that for N projects on list, gitweb uses 2*N+1
> git commands.  The "+1" part is from "git version", the "2*N" are from
> git-for-each-ref to get last activity (and verify repository as a
> side-effect)...

It is how I started to think about the problem. With my additional patch
to remove the owner I am able to reduce the number of git invocations to
1.

> ...and from call to "git config" to get owner (unconditional check for
> `gitweb.owner` override), description (if '.git/description' file got
> deleted), if applicable category (file then config), if applicable ctags
> (file(s) then config).

> So we can rely on "git config" being called, no need for separate
> verification.  My mistake.  (Though it might be hard to use this fact.)


> Well, with proposed option to remove 'owner' field we would have sometimes
> to verify repository with an extra git command...

> >> * don't verify that repository exists.

> >> Though perhaps the last possibility isn't a good idea, so it would be
> >> left two-state, as in your patch. 

> > My tests show that forks are also a bottleneck in my setup.

> What do you mean by "my tests" here?  Is it benchmark (perhaps just using
> 'timed' feature) with and without custom change removing fork(s)?  Or did
> you used profiler (e.g. wondefull Devel::NYTProf) for that?

Nothing fancy. I look at the footnote produced by "timed" feature. And
I see a difference between version with the following patch:

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 18cdf96..4a13807 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3156,6 +3156,18 @@ sub git_get_project_owner {
 	return $owner;
 }
 
+sub git_repo_exist {
+	my ($path) = @_;
+       my $fd;
+
+       $git_dir = "$projectroot/$path";
+       open($fd, "<", "$git_dir/HEAD") or return;
+       my $line = <$fd>;
+       close $fd or return;
+       return 1 if (defined $fd && substr($line, 0, 10) eq 'ref:refs/' 
+           || $line=~m/^[0-9a-z]{40}$/);
+       return 0;
+}
+
 sub git_get_last_activity {
 	my ($path) = @_;
 	my $fd;
@@ -5330,6 +5342,7 @@ sub fill_project_list_info {
 	my $show_ctags = gitweb_check_feature('ctags');
  PROJECT:
 	foreach my $pr (@$projlist) {
+             next PROJECT unless git_repo_exist($pr->{'path'});
 		if (project_info_needs_filling($pr, $filter_set->('age', 'age_string'))) {
 			my (@activity) = git_get_last_activity($pr->{'path'});
 			unless (@activity) {


and the one in which  git_repo_exist() uses invocation to /bin/true:

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 18cdf96..4bcc66f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -3156,6 +3156,13 @@ sub git_get_project_owner {
 	return $owner;
 }
 
+sub git_repo_exist {
+	my ($path) = @_;
+
+        $git_dir = "$projectroot/$path";
+        return not system('/bin/true');
+}
+
 sub git_get_last_activity {
 	my ($path) = @_;
 	my $fd;


> >                                                             On the other 
> > hand I think that I can trust that my projects.list contains only valid
> > repositories. So I would prefer to have a don't verify option. Or there
> > is a possibility to write perl function with the same functionality as
> > is_git_directory() from setup.c and use it to verify if the directory is a
> > valid git repo.

> Well, we can add those checks to check_export_ok()... well to function
> it would call.

> is_git_repository from setup.c checks that "/objects" and "/refs"
> have executable permissions, and that "/HEAD" is valid via validate_headref
> which does slightly more than (now slightly misnamed) check_head_link()
> from gitweb...

> ...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable
> is set, and path that it points to has executable permissions.  I don't
> think we have to worry about this for gitweb.

> I'll try to come up with a patch to gitweb that improves repository
> verification, and perhaps a patch that uses the fact that "git config"
> succeeded to verify repository.

As you see it is more or less what I have already written for my tests.
I only don't check if /objects and /refs are directories. If you want I
can send proper patch submission for this function

-- 
  Kacper Kornet


-- 
  Kacper Kornet

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Jakub Narebski]

On Mon, 16 Apr 2012, Kacper Kornet wrote:
> On Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:
>> On Mon, 16 Apr 2012, Kacper Kornet wrote:
>>> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:

>>>>   That saves I/O, but not fork.
>> 
>> Actually if you look at the footer of projects list page with 'timed'
>> feature enable you see that for N projects on list, gitweb uses 2*N+1
>> git commands.  The "+1" part is from "git version", the "2*N" are from
>> git-for-each-ref to get last activity (and verify repository as a
>> side-effect)...
> 
> It is how I started to think about the problem. With my additional patch
> to remove the owner I am able to reduce the number of git invocations to
> 1.

That is a very good thing.

Especially that adding caching to gitweb is long in coming (to core 
gitweb at least), and that not always one is able to turn on caching.

[...]
>>> My tests show that forks are also a bottleneck in my setup.
>> 
>> What do you mean by "my tests" here?  Is it benchmark (perhaps just using
>> 'timed' feature) with and without custom change removing fork(s)?  Or did
>> you used profiler (e.g. wondefull Devel::NYTProf) for that?
> 
> Nothing fancy. I look at the footnote produced by "timed" feature. And
> I see a difference between version with the following patch:
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 18cdf96..4a13807 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3156,6 +3156,18 @@ sub git_get_project_owner {
>  	return $owner;
>  }
>  
> +sub git_repo_exist {

Perhaps a better name would be validate_headref() or check_head(),
called from subroutine named either is_git_directory() as in setup.c,
or verify_repo()...

> +	my ($path) = @_;

BTW this could be written as

  +	my $path = shift;

though it is largely a matter of taste.

> +       my $fd;
> +
> +       $git_dir = "$projectroot/$path";
> +       open($fd, "<", "$git_dir/HEAD") or return;

It can be written as

  +	open my $fd, "<", "$projectroot/$path/HEAD"
  +		or return;

> +       my $line = <$fd>;
> +       close $fd or return;

Shouldn't we chomp($line)?

> +       return 1 if (defined $fd && substr($line, 0, 10) eq 'ref:refs/' 
> +           || $line=~m/^[0-9a-z]{40}$/);
> +       return 0;

I don't think we need to check that $fd is defined; if it isn't, we would
return earlier I think.

There can be space between "ref:" and fully qualified branch name, and in
fact git puts such space:

  $ cat .git/HEAD 
  ref: refs/heads/gitweb/web

Also, you can return boolean value.

So the above would reduce to:

  +	return ($line =~ m!^ref:\s*refs/! ||
  +	        $line =~ m!^[0-9a-z]{40}$!);

> +}
> +
>  sub git_get_last_activity {
>  	my ($path) = @_;
>  	my $fd;
> @@ -5330,6 +5342,7 @@ sub fill_project_list_info {
>  	my $show_ctags = gitweb_check_feature('ctags');
>   PROJECT:
>  	foreach my $pr (@$projlist) {
> +             next PROJECT unless git_repo_exist($pr->{'path'});

I understand that it is proof of concept patch, but I think that
in real patch iy would be better to update check_export_ok() instead
of this addition.

>  		if (project_info_needs_filling($pr, $filter_set->('age', 'age_string'))) {
>  			my (@activity) = git_get_last_activity($pr->{'path'});
>  			unless (@activity) {
> 
> 
> and the one in which  git_repo_exist() uses invocation to /bin/true:
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 18cdf96..4bcc66f 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -3156,6 +3156,13 @@ sub git_get_project_owner {
>  	return $owner;
>  }
>  
> +sub git_repo_exist {
> +	my ($path) = @_;
> +
> +        $git_dir = "$projectroot/$path";
> +        return not system('/bin/true');
> +}
> +

What were the differences in timing?

>>>                                                             On the other 
>>> hand I think that I can trust that my projects.list contains only valid
>>> repositories. So I would prefer to have a don't verify option. Or there
>>> is a possibility to write perl function with the same functionality as
>>> is_git_directory() from setup.c and use it to verify if the directory is a
>>> valid git repo.
>> 
>> Well, we can add those checks to check_export_ok()... well to function
>> it would call.
>> 
>> is_git_repository from setup.c checks that "/objects" and "/refs"
>> have executable permissions, and that "/HEAD" is valid via validate_headref
>> which does slightly more than (now slightly misnamed) check_head_link()
>> from gitweb...
>> 
>> ...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable
>> is set, and path that it points to has executable permissions.  I don't
>> think we have to worry about this for gitweb.
> 
>> I'll try to come up with a patch to gitweb that improves repository
>> verification, and perhaps a patch that uses the fact that "git config"
>> succeeded to verify repository.
> 
> As you see it is more or less what I have already written for my tests.
> I only don't check if /objects and /refs are directories. If you want I
> can send proper patch submission for this function

I don't know how strict we should be, but "/objects" and "/refs" are just
one stat more.


Anyway, if you plan on resending this patch series, then "gitweb: Improve
repository verification" should be be first, I think.

-- 
Jakub Narebski
Poland

[PATCH] gitweb: Improve repository verification

[Jakub Narebski]

Bring repository verification in check_export_ok() to standards of
is_git_directory function from setup.c (core git), and validate_headref()
to standards of the same function in path.c,... and a bit more.

validate_headref() replaces check_head_link(); note that the former
requires path to HEAD file, while the late latter path to repository.

Issues of note:
* is_git_directory() in gitweb is a bit stricter: it checks that
  "/objects" and "/refs" are directories, and not only 'executable'
  permission,
* validate_headref() in gitweb is a bit stricter: it checks that
  reference symlink or symref points to starts with "refs/heads/",
  and not only with "refs/",
* calls to check_head_link(), all of which were meant to check if
  given directory can be a git repository, were replaced by newly
  introduced is_git_directory().

This change is preparation for removing "Last change" column from list
of projects, which is currently used also for validating repository.

Suggested-by: Kacper Kornet <draenog@pld-linux.org>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
Here is how such first step could look like...

 gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++----------
 1 files changed, 42 insertions(+), 10 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 098e527..767d7a5 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -621,19 +621,51 @@ sub feature_avatar {
 	return @val ? @val : @_;
 }
 
-# checking HEAD file with -e is fragile if the repository was
-# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
-# and then pruned.
-sub check_head_link {
-	my ($dir) = @_;
-	my $headfile = "$dir/HEAD";
-	return ((-e $headfile) ||
-		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
+# Test if it looks like we're at a git directory.
+# We want to see:
+#
+#  - an objects/ directory,
+#  - a refs/ directory,
+#  - either a HEAD symlink or a HEAD file that is formatted as
+#    a proper "ref:", or a regular file HEAD that has a properly
+#    formatted sha1 object name.
+#
+# See is_git_directory() in setup.c
+sub is_git_directory {
+	my $dir = shift;
+	return
+		-x "$dir/objects" && -d _ &&
+		-x "$dir/refs"    && -d _ &&
+		validate_headref("$dir/HEAD");
+}
+
+# Check HEAD file, that it is either
+#
+#  - a "refs/heads/.." symlink, or
+#  - a symbolic ref to "refs/heads/..", or
+#  - a detached HEAD.
+#
+# See validate_headref() in path.c
+sub validate_headref {
+	my $headfile = shift;
+	if (-l $headfile) {
+		return readlink($headfile) =~ m!^refs/heads/!;
+
+	} elsif (-e _) {
+		open my $fh, '<', $headfile or return;
+		my $line = <$fh>;
+		close $fh or return;
+
+		return
+			$line =~ m!^ref:\s*refs/heads/! ||  # symref
+			$line =~ m!^[0-9a-z]{40}$!i;        # detached HEAD
+	}
+	return;
 }
 
 sub check_export_ok {
 	my ($dir) = @_;
-	return (check_head_link($dir) &&
+	return (is_git_directory($dir) &&
 		(!$export_ok || -e "$dir/$export_ok") &&
 		(!$export_auth_hook || $export_auth_hook->($dir)));
 }
@@ -842,7 +874,7 @@ sub evaluate_path_info {
 	# find which part of PATH_INFO is project
 	my $project = $path_info;
 	$project =~ s,/+$,,;
-	while ($project && !check_head_link("$projectroot/$project")) {
+	while ($project && !is_git_directory("$projectroot/$project")) {
 		$project =~ s,/*[^/]*$,,;
 	}
 	return unless $project;
-- 
1.7.9

Re: [PATCH] gitweb: Improve repository verification

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> Bring repository verification in check_export_ok() to standards of
> is_git_directory function from setup.c (core git), and validate_headref()
> to standards of the same function in path.c,... and a bit more.
>
> validate_headref() replaces check_head_link(); note that the former
> requires path to HEAD file, while the late latter path to repository.
>
> Issues of note:
> * is_git_directory() in gitweb is a bit stricter: it checks that
>   "/objects" and "/refs" are directories, and not only 'executable'
>   permission,
> * validate_headref() in gitweb is a bit stricter: it checks that
>   reference symlink or symref points to starts with "refs/heads/",
>   and not only with "refs/",
> * calls to check_head_link(), all of which were meant to check if
>   given directory can be a git repository, were replaced by newly
>   introduced is_git_directory().
>
> This change is preparation for removing "Last change" column from list
> of projects, which is currently used also for validating repository.
>
> Suggested-by: Kacper Kornet <draenog@pld-linux.org>
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> ---
> Here is how such first step could look like...

Do you mean by "could look like" that this is still an RFC, or is this
something we want to apply and see how well it makes people's lives in
the field?

By the way, I wonder (1) if it is worth adding support for the textual
".git" file that contains "gitdir: $path", and (2) if so how big a
change would we need to do so.

>  gitweb/gitweb.perl |   52 ++++++++++++++++++++++++++++++++++++++++++----------
>  1 files changed, 42 insertions(+), 10 deletions(-)
>
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 098e527..767d7a5 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -621,19 +621,51 @@ sub feature_avatar {
>  	return @val ? @val : @_;
>  }
>  
> -# checking HEAD file with -e is fragile if the repository was
> -# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed
> -# and then pruned.
> -sub check_head_link {
> -	my ($dir) = @_;
> -	my $headfile = "$dir/HEAD";
> -	return ((-e $headfile) ||
> -		(-l $headfile && readlink($headfile) =~ /^refs\/heads\//));
> +# Test if it looks like we're at a git directory.
> +# We want to see:
> +#
> +#  - an objects/ directory,
> +#  - a refs/ directory,
> +#  - either a HEAD symlink or a HEAD file that is formatted as
> +#    a proper "ref:", or a regular file HEAD that has a properly
> +#    formatted sha1 object name.
> +#
> +# See is_git_directory() in setup.c
> +sub is_git_directory {
> +	my $dir = shift;
> +	return
> +		-x "$dir/objects" && -d _ &&
> +		-x "$dir/refs"    && -d _ &&
> +		validate_headref("$dir/HEAD");
> +}
> +
> +# Check HEAD file, that it is either
> +#
> +#  - a "refs/heads/.." symlink, or
> +#  - a symbolic ref to "refs/heads/..", or
> +#  - a detached HEAD.
> +#
> +# See validate_headref() in path.c
> +sub validate_headref {
> +	my $headfile = shift;
> +	if (-l $headfile) {
> +		return readlink($headfile) =~ m!^refs/heads/!;
> +
> +	} elsif (-e _) {
> +		open my $fh, '<', $headfile or return;
> +		my $line = <$fh>;
> +		close $fh or return;
> +
> +		return
> +			$line =~ m!^ref:\s*refs/heads/! ||  # symref
> +			$line =~ m!^[0-9a-z]{40}$!i;        # detached HEAD
> +	}
> +	return;
>  }
>  
>  sub check_export_ok {
>  	my ($dir) = @_;
> -	return (check_head_link($dir) &&
> +	return (is_git_directory($dir) &&
>  		(!$export_ok || -e "$dir/$export_ok") &&
>  		(!$export_auth_hook || $export_auth_hook->($dir)));
>  }
> @@ -842,7 +874,7 @@ sub evaluate_path_info {
>  	# find which part of PATH_INFO is project
>  	my $project = $path_info;
>  	$project =~ s,/+$,,;
> -	while ($project && !check_head_link("$projectroot/$project")) {
> +	while ($project && !is_git_directory("$projectroot/$project")) {
>  		$project =~ s,/*[^/]*$,,;
>  	}
>  	return unless $project;

Re: [PATCH] gitweb: Improve repository verification

[Jakub Narebski]

On Thu, 19 April 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Bring repository verification in check_export_ok() to standards of
> > is_git_directory function from setup.c (core git), and validate_headref()
> > to standards of the same function in path.c,... and a bit more.
> >
> > validate_headref() replaces check_head_link(); note that the former
> > requires path to HEAD file, while the late latter path to repository.
> >
> > Issues of note:
> > * is_git_directory() in gitweb is a bit stricter: it checks that
> >   "/objects" and "/refs" are directories, and not only 'executable'
> >   permission,
> > * validate_headref() in gitweb is a bit stricter: it checks that
> >   reference symlink or symref points to starts with "refs/heads/",
> >   and not only with "refs/",
> > * calls to check_head_link(), all of which were meant to check if
> >   given directory can be a git repository, were replaced by newly
> >   introduced is_git_directory().
> >
> > This change is preparation for removing "Last change" column from list
> > of projects, which is currently used also for validating repository.
> >
> > Suggested-by: Kacper Kornet <draenog@pld-linux.org>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > Here is how such first step could look like...
> 
> Do you mean by "could look like" that this is still an RFC, or is this
> something we want to apply and see how well it makes people's lives in
> the field?

"Here is how such first step could look like" was directed to Kacper... :-)

Kacper Kornet (who started this thread with "[PATCH] gitweb: Option
to omit column with time of the last change") wants to have an option
to remove "Last Change" column from projects list page, and "Owner"
column and field from all gitweb views.  This will allow to generate
projects list page with 1 call to git command rather than 2*N+1, where
N is number of repositories shown...

...but we use the fact that "git --git-dir=$GIT_DIR for-each-ref ..."
succeed or fails to verify that given path points to git repository.
That is why I proposed this commit to be first patch in hopefully
upcoming Kacper's new version of patch series.

But in current gitweb (without Kacper's planned patches) this change
doesn't bring much, as git repositories are verified outside of
is_git_directory() check... well, perhaps with exception of possible
corner case when one is using path_info gitweb URL...
 
> By the way, I wonder (1) if it is worth adding support for the textual
> ".git" file that contains "gitdir: $path", and (2) if so how big a
> change would we need to do so.

I don't think that it would be big changeto add support for "gitlink"
files, assuming that 'git --git-dir=<gitlink file> ...' works correctly.
I would put that addition in a separate commit, though.

BTW. does core git limit number of redirections, or have some loop
detection?
-- 
Jakub Narebski
Poland

Re: [PATCH] gitweb: Improve repository verification

[Jakub Narebski]

On Thu, 19 April 2012, Jakub Narebski wrote:
> On Thu, 19 April 2012, Junio C Hamano wrote:

> > By the way, I wonder (1) if it is worth adding support for the textual
> > ".git" file that contains "gitdir: $path", and (2) if so how big a
> > change would we need to do so.
> 
> I don't think that it would be big change to add support for "gitlink"
> files, assuming that 'git --git-dir=<gitlink file> ...' works correctly.
> I would put that addition in a separate commit, though.

Well, I actually tried to write such commit, adding support for
'gitlink' files, and it turned out to be harder than I thought.
The problem that stumped me for now is that gitweb tries to read
many files inside git repository ('export-ok', 'description',
'cloneurl', 'category', etc.), allof which must be redirected to
real git directory.

I still think it is doable, but I wonder if it is worth it...

Below there is work in progress patch, which doesn't use resolve_gitdir
yet, and without any tests.

-- >8 ---------- >8 --
Subject: [PATCH] gitweb: Add support for "gitdir: <path>" gitfile

Suggested-by: Junio C Hamano <gitster@pobox.com>
Signed-off-by: Jakub Narebski <jnareb@gmail.com>
---
 gitweb/gitweb.perl |   54 ++++++++++++++++++++++++++++++++++++++++++++++++---
 1 files changed, 50 insertions(+), 4 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 767d7a5..8d70a0a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -639,6 +639,52 @@ sub is_git_directory {
 		validate_headref("$dir/HEAD");
 }
 
+# Try to read the location of the git directory from the .git file,
+# return path to git directory if found.
+#
+# See read_gitfile in setup.c
+sub read_gitfile {
+	my $path = shift;
+	# note: the "basename eq '.git'" check isn't in setup.c version
+	return unless ($path =~ m!(?:^|/)\.git/*$! && -f $path);
+
+	open my $fh, '<', $path or return;
+	my $contents = do { local $/ = undef; <$fh> };
+	close $fh or return;
+	return unless defined $contents;
+	chomp($contents);
+
+	return unless ($contents =~ s!^gitdir: !!);
+
+	if (!File::Spec->file_name_is_absolute($contents)) {
+		$contents = File::Spec->catfile(File::Basename::dirname($path), $contents);
+	}
+
+	return unless is_git_directory($contents);
+	return $contents;
+}
+
+# Test if it looks like we're at a git repository
+#
+#  - a '.git' file containing "gitdir: <path>",
+#  - a git directory.
+sub is_git_repository {
+	my $path = shift;
+	return defined(read_gitfile($path)) || is_git_directory($path);
+}
+
+# Return directory of a git repository, resolving '.git' files
+# (file containing "gitdir: <path>") if any
+#
+# See resolve_gitdir in setup.c
+sub resolve_gitdir {
+	my $suspect = shift;
+	if (is_git_directory($suspect)) {
+		return $suspect;
+	}
+	return read_gitfile($suspect);
+}
+
 # Check HEAD file, that it is either
 #
 #  - a "refs/heads/.." symlink, or
@@ -665,7 +711,7 @@ sub validate_headref {
 
 sub check_export_ok {
 	my ($dir) = @_;
-	return (is_git_directory($dir) &&
+	return (is_git_repository($dir) &&
 		(!$export_ok || -e "$dir/$export_ok") &&
 		(!$export_auth_hook || $export_auth_hook->($dir)));
 }
@@ -874,7 +920,7 @@ sub evaluate_path_info {
 	# find which part of PATH_INFO is project
 	my $project = $path_info;
 	$project =~ s,/+$,,;
-	while ($project && !is_git_directory("$projectroot/$project")) {
+	while ($project && !is_git_repository("$projectroot/$project")) {
 		$project =~ s,/*[^/]*$,,;
 	}
 	return unless $project;
@@ -3094,8 +3140,8 @@ sub git_get_projects_list {
 				our $projectroot;
 				# skip project-list toplevel, if we get it.
 				return if (m!^[/.]$!);
-				# only directories can be git repositories
-				return unless (-d $_);
+				# only directories or gitlink files can be git repositories
+				return unless (-d $_ || (-f _ && $File::Find::name =~ m!(?:^|/)\.git!));
 				# don't traverse too deep (Find is super slow on os x)
 				# $project_maxdepth excludes depth of $projectroot
 				if (($File::Find::name =~ tr!/!!) - $pfxdepth > $project_maxdepth) {
-- 
1.7.9

Re: [PATCH] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

I'm sorry for the late answer.

On Wed, Apr 18, 2012 at 01:36:08AM +0200, Jakub Narebski wrote:
> On Mon, 16 Apr 2012, Kacper Kornet wrote:
> > On Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:
> >> On Mon, 16 Apr 2012, Kacper Kornet wrote:
> >>> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:

> >>> My tests show that forks are also a bottleneck in my setup.

> >> What do you mean by "my tests" here?  Is it benchmark (perhaps just using
> >> 'timed' feature) with and without custom change removing fork(s)?  Or did
> >> you used profiler (e.g. wondefull Devel::NYTProf) for that?

> > Nothing fancy. I look at the footnote produced by "timed" feature. And
> > I see a difference between version with the following patch:

[...]

> > and the one in which  git_repo_exist() uses invocation to /bin/true:

> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> > index 18cdf96..4bcc66f 100755
> > --- a/gitweb/gitweb.perl
> > +++ b/gitweb/gitweb.perl
> > @@ -3156,6 +3156,13 @@ sub git_get_project_owner {
> >  	return $owner;
> >  }

> > +sub git_repo_exist {
> > +	my ($path) = @_;
> > +
> > +        $git_dir = "$projectroot/$path";
> > +        return not system('/bin/true');
> > +}
> > +

> What were the differences in timing?

The best results with the host disk caches were:

v1.7.10
This page took 66.960714 seconds  and 16517 git commands to generate.

Call /bin/true in git_repo_exist()
This page took 45.583935 seconds  and 1 git commands to generate

The patches applied:
This page took 6.090545 seconds  and 1 git commands to generate.

> Anyway, if you plan on resending this patch series, then "gitweb: Improve
> repository verification" should be be first, I think.

Thank you for writing that one for me. I will send my two patches to
omit owner and last modification time on top of it. 

-- 
  Kacper Kornet

[PATCH 1/2] gitweb: Option to omit column with time of the last change

[Kacper Kornet]

Generating information about last change for a large number of git
repositories can be very time consuming. This commit add an option to
omit 'Last Change' column when presenting the list of repositories.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Documentation/gitweb.conf.txt |    4 ++++
 gitweb/gitweb.perl            |   16 +++++++++++-----
 2 files changed, 15 insertions(+), 5 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index 7aba497..d240a2f 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -499,6 +499,10 @@ $maxload::
 Set `$maxload` to undefined value (`undef`) to turn this feature off.
 The default value is 300.
 
+$omit_age_column::
+	If true, omit the column with date of the most current commit on the
+	projects list page. It can save a bit of I/O and a fork per repository.
+
 $per_request_config::
 	If this is set to code reference, it will be run once for each request.
 	You can	set parts of configuration that change per session this way.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 350b59c..6bddbff 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -133,6 +133,9 @@ our $default_projects_order = "project";
 # (only effective if this variable evaluates to true)
 our $export_ok = "++GITWEB_EXPORT_OK++";
 
+# don't generate age column on the projects list page
+our $omit_age_column = 0;
+
 # show repository only if this subroutine returns true
 # when given the path to the project, for example:
 #    sub { return -e "$_[0]/git-daemon-export-ok"; }
@@ -5494,9 +5497,11 @@ sub git_project_list_rows {
 		                        : esc_html($pr->{'descr'})) .
 		      "</td>\n" .
 		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
-		print "<td class=\"". age_class($pr->{'age'}) . "\">" .
-		      (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n" .
-		      "<td class=\"link\">" .
+		unless ($omit_age_column) {
+		        print "<td class=\"". age_class($pr->{'age'}) . "\">" .
+		            (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n";
+		}
+		print"<td class=\"link\">" .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"summary")}, "summary")   . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"shortlog")}, "shortlog") . " | " .
 		      $cgi->a({-href => href(project=>$pr->{'path'}, action=>"log")}, "log") . " | " .
@@ -5527,7 +5532,8 @@ sub git_project_list_body {
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $search_regexp);
 	# fill the rest
-	@projects = fill_project_list_info(\@projects);
+	my @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
+	@projects = fill_project_list_info(\@projects, @all_fields);
 
 	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
@@ -5559,7 +5565,7 @@ sub git_project_list_body {
 		print_sort_th('project', $order, 'Project');
 		print_sort_th('descr', $order, 'Description');
 		print_sort_th('owner', $order, 'Owner');
-		print_sort_th('age', $order, 'Last Change');
+		print_sort_th('age', $order, 'Last Change') unless $omit_age_column;
 		print "<th></th>\n" . # for links
 		      "</tr>\n";
 	}
-- 
1.7.10



-- 
  Kacper Kornet

[PATCH 2/2] gitweb: Option to not display information about owner

[Kacper Kornet]

In some setups the repository owner is not a well defined concept
and administrator can prefer it to be not shown. This commit add
and an option that enable to reach this effect.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Documentation/gitweb.conf.txt |    3 +++
 gitweb/gitweb.perl            |   20 ++++++++++++++------
 2 files changed, 17 insertions(+), 6 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index d240a2f..4b8d1b1 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -503,6 +503,9 @@ $omit_age_column::
 	If true, omit the column with date of the most current commit on the
 	projects list page. It can save a bit of I/O and a fork per repository.
 
+$omit_owner::
+	If true prevents displaying information about repository owner.
+
 $per_request_config::
 	If this is set to code reference, it will be run once for each request.
 	You can	set parts of configuration that change per session this way.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6bddbff..7558def 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -136,6 +136,9 @@ our $export_ok = "++GITWEB_EXPORT_OK++";
 # don't generate age column on the projects list page
 our $omit_age_column = 0;
 
+# don't generate information about owners of repositories
+our $omit_owner=0;
+
 # show repository only if this subroutine returns true
 # when given the path to the project, for example:
 #    sub { return -e "$_[0]/git-daemon-export-ok"; }
@@ -5495,8 +5498,10 @@ sub git_project_list_rows {
 		                        ? esc_html_match_hl_chopped($pr->{'descr_long'},
 		                                                    $pr->{'descr'}, $search_regexp)
 		                        : esc_html($pr->{'descr'})) .
-		      "</td>\n" .
-		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
+		      "</td>\n";
+		unless ($omit_owner) {
+		        print "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
+		}
 		unless ($omit_age_column) {
 		        print "<td class=\"". age_class($pr->{'age'}) . "\">" .
 		            (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n";
@@ -5532,7 +5537,8 @@ sub git_project_list_body {
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $search_regexp);
 	# fill the rest
-	my @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
+	my @all_fields = $omit_age_column ? ('descr', 'descr_long', 'ctags', 'category') : ();
+	push @all_fields, 'owner' unless($omit_owner);
 	@projects = fill_project_list_info(\@projects, @all_fields);
 
 	$order ||= $default_projects_order;
@@ -5564,7 +5570,7 @@ sub git_project_list_body {
 		}
 		print_sort_th('project', $order, 'Project');
 		print_sort_th('descr', $order, 'Description');
-		print_sort_th('owner', $order, 'Owner');
+		print_sort_th('owner', $order, 'Owner') unless $omit_owner;
 		print_sort_th('age', $order, 'Last Change') unless $omit_age_column;
 		print "<th></th>\n" . # for links
 		      "</tr>\n";
@@ -6318,8 +6324,10 @@ sub git_summary {
 
 	print "<div class=\"title\">&nbsp;</div>\n";
 	print "<table class=\"projects_list\">\n" .
-	      "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
-	      "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
+	      "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n";
+        unless ($omit_owner) {
+	        print  "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
+        }
 	if (defined $cd{'rfc2822'}) {
 		print "<tr id=\"metadata_lchange\"><td>last change</td>" .
 		      "<td>".format_timestamp_html(\%cd)."</td></tr>\n";
-- 
1.7.10

-- 
  Kacper Kornet

Re: [PATCH 2/2] gitweb: Option to not display information about owner

[Junio C Hamano]

Kacper Kornet <draenog@pld-linux.org> writes:

> In some setups the repository owner is not a well defined concept
> and administrator can prefer it to be not shown. This commit add
> and an option that enable to reach this effect.
>
> Signed-off-by: Kacper Kornet <draenog@pld-linux.org>

Among your recent three patches, this one seems to break t9500; has it
been tested?

[Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in string comparison (cmp) at /srv/git/t/../gitweb/gitweb.perl line 5551.
[Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in string comparison (cmp) at /srv/git/t/../gitweb/gitweb.perl line 5551.
[Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in hash element at /srv/git/t/../gitweb/gitweb.perl line 5401.
[Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in hash element at /srv/git/t/../gitweb/gitweb.perl line 5401.

I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]
elements.

Re: [PATCH 2/2] gitweb: Option to not display information about owner

[Kacper Kornet]

On Wed, Apr 25, 2012 at 09:39:03PM -0700, Junio C Hamano wrote:
> Kacper Kornet <draenog@pld-linux.org> writes:

> > In some setups the repository owner is not a well defined concept
> > and administrator can prefer it to be not shown. This commit add
> > and an option that enable to reach this effect.

> > Signed-off-by: Kacper Kornet <draenog@pld-linux.org>

> Among your recent three patches, this one seems to break t9500; has it
> been tested?

> [Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in string comparison (cmp) at /srv/git/t/../gitweb/gitweb.perl line 5551.
> [Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in string comparison (cmp) at /srv/git/t/../gitweb/gitweb.perl line 5551.
> [Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in hash element at /srv/git/t/../gitweb/gitweb.perl line 5401.
> [Thu Apr 26 04:32:36 2012] gitweb.perl: Use of uninitialized value in hash element at /srv/git/t/../gitweb/gitweb.perl line 5401.

> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]
> elements.

Yes, I have tested the tree with:

gitweb: Improve repository verification
gitweb: Option to omit column with time of the last change
gitweb: Option to not display information about owner

applied on top of v1.7.10. And all tests except 't91??' are passed.
Could you write on top of which revision have you applied these three
patches?

-- 
  Kacper Kornet

Re: [PATCH 2/2] gitweb: Option to not display information about owner

[Junio C Hamano]

Kacper Kornet <draenog@pld-linux.org> writes:

>> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]
>> elements.
>
> Yes, I have tested the tree with:
>
> gitweb: Improve repository verification
> gitweb: Option to omit column with time of the last change
> gitweb: Option to not display information about owner
>
> applied on top of v1.7.10. And all tests except 't91??' are passed.
> Could you write on top of which revision have you applied these three
> patches?

Let's see...

$ git log --oneline --first-parent --boundary master..kk/gitweb-omit-expensive
37e2621 gitweb: Option to not display information about owner
5710be4 gitweb: Option to omit column with time of the last change
75e0dff gitweb: Don't set owner if got empty value from projects.list
- fdec2eb Merge branch 'maint-1.7.9' into maint

I replayed these three on v1.7.10^0

$ git checkout v1.7.10^0
$ git format-patch --stdout fdec2eb..kk/gitweb-omit-expensive | git am -s3c

and the result fails exactly the same way, though.

*** prove ***
t9500-gitweb-standalone-no-errors.sh .. Dubious, test returned 1 (wstat 256, 0x100)
Failed 2/117 subtests
        (less 2 skipped subtests: 113 okay)

Test Summary Report
-------------------
t9500-gitweb-standalone-no-errors.sh (Wstat: 256 Tests: 117 Failed: 2)
  Failed tests:  116-117
  Non-zero exit status: 1
Files=1, Tests=117, 15 wallclock secs ( 0.07 usr  0.00 sys + 10.18 cusr  1.42 csys = 11.67 CPU)
Result: FAIL
make[1]: *** [prove] Error 1
make[1]: Leaving directory `/srv/git/t'
make: *** [test] Error 2

Re: [PATCH 2/2] gitweb: Option to not display information about owner

[Kacper Kornet]

On Thu, Apr 26, 2012 at 08:53:22AM -0700, Junio C Hamano wrote:
> Kacper Kornet <draenog@pld-linux.org> writes:

> >> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]
> >> elements.

> > Yes, I have tested the tree with:

> > gitweb: Improve repository verification
> > gitweb: Option to omit column with time of the last change
> > gitweb: Option to not display information about owner

> > applied on top of v1.7.10. And all tests except 't91??' are passed.
> > Could you write on top of which revision have you applied these three
> > patches?

> Let's see...

> $ git log --oneline --first-parent --boundary master..kk/gitweb-omit-expensive
> 37e2621 gitweb: Option to not display information about owner
> 5710be4 gitweb: Option to omit column with time of the last change
> 75e0dff gitweb: Don't set owner if got empty value from projects.list
> - fdec2eb Merge branch 'maint-1.7.9' into maint

> I replayed these three on v1.7.10^0

> $ git checkout v1.7.10^0
> $ git format-patch --stdout fdec2eb..kk/gitweb-omit-expensive | git am -s3c

> and the result fails exactly the same way, though.

Appereantly I have managed to send the different patch then I have
applied in my private tree. I'm sorry for my mistake. In a short time I
will send the correct one.

-- 
  Kacper Kornet

[PATCH v2 2/2] gitweb: Option to not display information about owner

[Kacper Kornet]

In some setups the repository owner is not a well defined concept
and administrator can prefer it to be not shown. This commit add
and an option that enable to reach this effect.

Signed-off-by: Kacper Kornet <draenog@pld-linux.org>
---
 Documentation/gitweb.conf.txt |    3 +++
 gitweb/gitweb.perl            |   21 +++++++++++++++------
 2 files changed, 18 insertions(+), 6 deletions(-)

diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt
index d240a2f..4b8d1b1 100644
--- a/Documentation/gitweb.conf.txt
+++ b/Documentation/gitweb.conf.txt
@@ -503,6 +503,9 @@ $omit_age_column::
 	If true, omit the column with date of the most current commit on the
 	projects list page. It can save a bit of I/O and a fork per repository.
 
+$omit_owner::
+	If true prevents displaying information about repository owner.
+
 $per_request_config::
 	If this is set to code reference, it will be run once for each request.
 	You can	set parts of configuration that change per session this way.
diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6bddbff..6dbeb2f 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -136,6 +136,9 @@ our $export_ok = "++GITWEB_EXPORT_OK++";
 # don't generate age column on the projects list page
 our $omit_age_column = 0;
 
+# don't generate information about owners of repositories
+our $omit_owner=0;
+
 # show repository only if this subroutine returns true
 # when given the path to the project, for example:
 #    sub { return -e "$_[0]/git-daemon-export-ok"; }
@@ -5495,8 +5498,10 @@ sub git_project_list_rows {
 		                        ? esc_html_match_hl_chopped($pr->{'descr_long'},
 		                                                    $pr->{'descr'}, $search_regexp)
 		                        : esc_html($pr->{'descr'})) .
-		      "</td>\n" .
-		      "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
+		      "</td>\n";
+		unless ($omit_owner) {
+		        print "<td><i>" . chop_and_escape_str($pr->{'owner'}, 15) . "</i></td>\n";
+		}
 		unless ($omit_age_column) {
 		        print "<td class=\"". age_class($pr->{'age'}) . "\">" .
 		            (defined $pr->{'age_string'} ? $pr->{'age_string'} : "No commits") . "</td>\n";
@@ -5532,7 +5537,9 @@ sub git_project_list_body {
 	                                 'tagfilter'  => $tagfilter)
 		if ($tagfilter || $search_regexp);
 	# fill the rest
-	my @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();
+	my @all_fields = ('descr', 'descr_long', 'ctags', 'category');
+	push @all_fields, ('age', 'age_string') unless($omit_age_column);
+	push @all_fields, 'owner' unless($omit_owner);
 	@projects = fill_project_list_info(\@projects, @all_fields);
 
 	$order ||= $default_projects_order;
@@ -5564,7 +5571,7 @@ sub git_project_list_body {
 		}
 		print_sort_th('project', $order, 'Project');
 		print_sort_th('descr', $order, 'Description');
-		print_sort_th('owner', $order, 'Owner');
+		print_sort_th('owner', $order, 'Owner') unless $omit_owner;
 		print_sort_th('age', $order, 'Last Change') unless $omit_age_column;
 		print "<th></th>\n" . # for links
 		      "</tr>\n";
@@ -6318,8 +6325,10 @@ sub git_summary {
 
 	print "<div class=\"title\">&nbsp;</div>\n";
 	print "<table class=\"projects_list\">\n" .
-	      "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n" .
-	      "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
+	      "<tr id=\"metadata_desc\"><td>description</td><td>" . esc_html($descr) . "</td></tr>\n";
+        unless ($omit_owner) {
+	        print  "<tr id=\"metadata_owner\"><td>owner</td><td>" . esc_html($owner) . "</td></tr>\n";
+        }
 	if (defined $cd{'rfc2822'}) {
 		print "<tr id=\"metadata_lchange\"><td>last change</td>" .
 		      "<td>".format_timestamp_html(\%cd)."</td></tr>\n";
-- 
1.7.10

-- 
  Kacper Kornet