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

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

---
 Documentation/gitweb.conf.txt |    3 +++
 gitweb/gitweb.perl            |   16 +++++++++++-----
 2 files changed, 14 insertions(+), 5 deletions(-)

Documentation/gitweb.conf.txt +0 -0

index 7aba497..bfeef21 100644

gitweb/gitweb.perl +1 -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'})) .

gitweb/gitweb.perl +0 -0

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

Signature · 1 line

1.7.10.rc3

Documentation/gitweb.conf.txt $default_projects_order:: · line 408

Quoted 11 lines · Kacper Kornet

Documentation/gitweb.conf.txt +3 -0

index 7aba497..bfeef21 100644
@@ -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).

Quoted 7 lines

gitweb/gitweb.perl +0 -0

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

gitweb/gitweb.perl our $default_projects_order = "project"; · line 137

Quoted 2 lines

gitweb/gitweb.perl +2 -0

+# 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?

gitweb/gitweb.perl sub git_project_list_body { · line 5504

Quoted 7 lines

gitweb/gitweb.perl +2 -1

@@ -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".

Quoted 16 lines · Jakub Narebski

Documentation/gitweb.conf.txt $default_projects_order:: · line 408

Quoted 11 lines · Kacper Kornet

Documentation/gitweb.conf.txt +3 -0

index 7aba497..bfeef21 100644
@@ -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/

Quoted 3 lines

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.

Quoted 7 lines

Quoted 7 lines

gitweb/gitweb.perl +0 -0

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

Quoted 6 lines

gitweb/gitweb.perl our $default_projects_order = "project"; · line 137

Quoted 2 lines

gitweb/gitweb.perl +2 -0

+# 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?

Quoted 18 lines

gitweb/gitweb.perl sub git_project_list_body { · line 5504

Quoted 7 lines

gitweb/gitweb.perl +2 -1

@@ -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);

Quoted 4 lines

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.

Quoted 7 lines · Kacper Kornet

Quoted 1 line · Jakub Narebski

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.

Quoted 8 lines

Quoted 7 lines

Quoted 7 lines

gitweb/gitweb.perl +0 -0

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

Quoted 8 lines

Quoted 6 lines

gitweb/gitweb.perl our $default_projects_order = "project"; · line 137

Quoted 2 lines

gitweb/gitweb.perl +2 -0

+# 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.

Quoted 19 lines

Quoted 18 lines

gitweb/gitweb.perl sub git_project_list_body { · line 5504

Quoted 7 lines

gitweb/gitweb.perl +2 -1

@@ -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);

Quoted 7 lines

Quoted 4 lines

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.

Quoted 20 lines · Jakub Narebski

Quoted 2 lines · Kacper Kornet

Quoted 1 line · Jakub Narebski

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.

Quoted 2 lines

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.

Quoted 7 lines

Quoted 3 lines

[...]. 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.

Quoted 7 lines

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.

Quoted 21 lines · Kacper Kornet

Quoted 19 lines · Jakub Narebski

Quoted 2 lines · Kacper Kornet

Quoted 1 line · Jakub Narebski

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.

Quoted 7 lines

Quoted 2 lines

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.

Quoted 5 lines · Jakub Narebski

Quoted 4 lines · Kacper Kornet

Quoted 3 lines · Jakub Narebski

Quoted 2 lines · Kacper Kornet

Quoted 1 line · Jakub Narebski

On Tue, 3 Apr 2012, Kacper Kornet wrote:

Quoted 12 lines

Quoted 12 lines

Quoted 7 lines

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.

Quoted 4 lines

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.

Quoted 21 lines

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;
  	}

Quoted 6 lines

* 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.

Quoted 4 lines

* 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.

Quoted 24 lines · Kacper Kornet

Quoted 23 lines · Jakub Narebski

Quoted 3 lines · Kacper Kornet

Quoted 2 lines · Jakub Narebski

Quoted 1 line · Kacper Kornet

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).

Quoted 1 line

Quoted 1 line

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...

Quoted 1 line

Quoted 1 line

* don't verify that repository exists.

Quoted 4 lines

Quoted 2 lines

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?

Quoted 6 lines

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.

Quoted 10 lines · Jakub Narebski

Quoted 3 lines · Kacper Kornet

Quoted 1 line · Jakub Narebski

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.

Quoted 5 lines

Quoted 1 line

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:

gitweb/gitweb.perl +0 -0

index 18cdf96..4a13807 100755

gitweb/gitweb.perl +0 -0

 		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:

gitweb/gitweb.perl +0 -0

index 18cdf96..4bcc66f 100755

Quoted 18 lines

Quoted 6 lines

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.

Quoted 3 lines

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

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

Signed-off-by Jakub Narebski <[email protected]>

---
Here is how such first step could look like...

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

gitweb/gitweb.perl +42 -10

index 098e527..767d7a5 100755
@@ -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;

Quoted 21 lines · 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.

Quoted 4 lines

Suggested-by Kacper Kornet <[email protected]>

Signed-off-by Jakub Narebski <[email protected]>

---
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.

Quoted 3 lines · Junio C Hamano

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.

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

Quoted 5 lines · 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 <[email protected]>

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.

Quoted 2 lines

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?

Quoted 5 lines · Kacper Kornet

Quoted 2 lines

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

Yes, I have tested the tree with:

Quoted 7 lines

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.

Quoted 12 lines · Kacper Kornet

Quoted 8 lines · Jakub Narebski

Quoted 3 lines · Kacper Kornet

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:

Quoted 9 lines

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

gitweb/gitweb.perl +0 -0

index 18cdf96..4bcc66f 100755
@@ -3156,6 +3156,13 @@ sub git_get_project_owner {
 	return $owner;
 }

gitweb/gitweb.perl sub git_get_project_owner { · line 3165

Quoted 7 lines

gitweb/gitweb.perl +7 -0

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

What were the differences in timing?

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

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