Re: [PATCH v2] gitweb: Optional grouping of projects by category

Sébastien Cevey [off-list ref] writes:

This adds the $projects_list_group_categories option which, if enabled,
will result in grouping projects by category on the project list page.
The category is specified for each project by the $GIT_DIR/category file
or the 'category' variable in its configuration file. By default, projects
are put in the $project_list_default_category category.

Note:
- Categories are always sorted alphabetically, with projects in
  each category sorted according to the globally selected $order.

I am not sure if always sorting the categories alphabetically is a severe
enough restriction, but if it was, you can use @project_list_categories
array that disables the feature when empty and otherwise enumerates the
categories in order.  Just an idle thought.

- When displaying a subset of all the projects, only the categories with
  projects in the chosen subset are displayed.

Could you clarify this bit?

It is not very clear to me how this subset selection happens.  As far as I
can see, the user does not choose the category to view, but lets the usual
page limiting to decide at which project to start and stop placing on the
page, and only show the ones in the same category as the one that happened
to be the first on the page.

While I think both the introduction of "git_get_project_config_or_file"
which is a generalized interface usable between description and the new
feature and the refactoring of project_list_body into a seprate function
"print_project_rows" is a good idea, the patch would have been much easier
to read if these preparatory refactoring steps (without any new feature)
were done as a separate patch followed by the main patch to introduce the
new feature.

On Wed, 3 Dec 2008, Junio C Hamano wrote:

Sébastien Cevey [off-list ref] writes:

quoted

This adds the $projects_list_group_categories option which, if enabled,
will result in grouping projects by category on the project list page.
The category is specified for each project by the $GIT_DIR/category file
or the 'category' variable in its configuration file. By default, projects
are put in the $project_list_default_category category.

Note:
- Categories are always sorted alphabetically, with projects in
  each category sorted according to the globally selected $order.

I am not sure if always sorting the categories alphabetically is a severe
enough restriction, but if it was, you can use @project_list_categories
array that disables the feature when empty and otherwise enumerates the
categories in order.  Just an idle thought.

Well, this way of sorting, i.e. keeping order of categories constant,
and changing order _within_ categories has the advantage that changing
order (the column we sort by) doesn't change page layout, i.e. in this
case doesn't change categories layout.


It means that if we had:

  project1
  project2
  [category A]
  A/pr1
  A/pr2
  [category B]
  B/pr1
  B/pr2
  B/pr3

when sorting by project name, we will have similarly looking

  project2
  project1
  [category A]
  A/pr1
  A/pr2
  [category B]
  B/pr3
  B/pr1
  B/pr2

for example if we order by age (not shown here)
 

quoted

- When displaying a subset of all the projects, only the categories with
  projects in the chosen subset are displayed.

Could you clarify this bit?

It is not very clear to me how this subset selection happens.  As far as I
can see, the user does not choose the category to view, but lets the usual
page limiting to decide at which project to start and stop placing on the
page, and only show the ones in the same category as the one that happened
to be the first on the page.

Currently the only selection is by tag, but lets assume that we have
some page length limit and only first N projects (first N in given sort
order) are shown.

If we have selection by tag, only those categories that have at least
one project tagged with selected tag, and only projects with given tag
would be shown. If we have page limit on projects list page, and we
sort for example by age, then only categories with freshest projects
will be shown.

At least it is how I understand it...

 

While I think both the introduction of "git_get_project_config_or_file"

Errr... I think it is git_get_file_or_project_config ;-))))

which is a generalized interface usable between description and the new
feature and the refactoring of project_list_body into a seprate function
"print_project_rows" is a good idea, the patch would have been much easier
to read if these preparatory refactoring steps (without any new feature)
were done as a separate patch followed by the main patch to introduce the
new feature.

 
I agree, especially the second part, as adding print_project_rows patch
looks harder than it actually is.

-- 
Jakub Narebski
Poland

At Wed, 03 Dec 2008 14:51:07 -0800, Junio C Hamano wrote:

Good evening,

I am not sure if always sorting the categories alphabetically is a
severe enough restriction, but if it was, you can use
@project_list_categories array that disables the feature when empty
and otherwise enumerates the categories in order.  Just an idle
thought.

I could add that if requested to, though Jakub seems to think
alphabetical order is also fine.  I'll be pushing v3 in a minute with
commits split by feature update and the rename to
git_get_file_or_project_config.

All this gymnastics helped me getting more familiar with add -i and
other fun workflow, thanks for the exercise[1] ;-)

quoted

- When displaying a subset of all the projects, only the categories with
  projects in the chosen subset are displayed.

Could you clarify this bit?

It is not very clear to me how this subset selection happens.  As far as I
can see, the user does not choose the category to view, but lets the usual
page limiting to decide at which project to start and stop placing on the
page

Right,

and only show the ones in the same category as the one that happened
to be the first on the page.

I'm not sure I understand what you meant by this.

What I meant is that, as you said, the subset of projects to display
is defined by the usual $from/$to mechanism.  And that the category
header for all the displayed projects is present; in other words, the
header for some category C is shown iff one or more of the projects in
C is present on the page.  It's really that simple.

It also means that we might only see a subset of the projects in the
first and last category but hey, if you asked for range N-M of the
projects, that's what you get!

I hope I did not confuse you further.


[1] OT: I didn't find a simple command to do this:

    $ git diff ..the-end-state > finish.patch
    $ patch -p1 < finish.patch
    $ git commit -a -s

    (where the original HEAD and the-end-state have an older MRCA with
     rewritten history inbetween, and I don't want to apply that
     history and solve conflicts, just "get my tree to the end state".)

    Any tip?

-- 
Sébastien Cevey / inso.cc

" Rest is for the weak and the dead. "

Introduce a git_get_file_or_project_config utility function to
retrieve a repository variable either from a plain text file in the
$GIT_DIR or else from 'gitweb.$variable' in the repository config
(e.g. 'description').

Signed-off-by: Sebastien Cevey <redacted>
---
 gitweb/gitweb.perl |   24 ++++++++++++++++--------
 1 files changed, 16 insertions(+), 8 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 933e137..b31274c 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -2001,18 +2001,26 @@ sub git_get_path_by_hash {
 ## ......................................................................
 ## git utility functions, directly accessing git repository
 
-sub git_get_project_description {
-	my $path = shift;
+# get the value of a config variable either from a file with the same
+# name in the repository, or the gitweb.$name value in the repository
+# config file.
+sub git_get_file_or_project_config {
+	my ($name, $path) = @_;
 
 	$git_dir = "$projectroot/$path";
-	open my $fd, "$git_dir/description"
-		or return git_get_project_config('description');
-	my $descr = <$fd>;
+	open my $fd, "$git_dir/$name"
+		or return git_get_project_config($name);
+	my $conf = <$fd>;
 	close $fd;
-	if (defined $descr) {
-		chomp $descr;
+	if (defined $conf) {
+		chomp $conf;
 	}
-	return $descr;
+	return $conf;
+}
+
+sub git_get_project_description {
+	my $path = shift;
+	return git_get_file_or_project_config('description', $path);
 }
 
 sub git_get_project_ctags {

-- 
1.5.6.5

Extract the printing of project rows on the project page into a
separate print_project_rows function. This makes it easier to reuse
the code to print different subsets of the whole project list.

The row printing code is merely moved into a separate function, but
note that $projects is passed as a reference now.

Signed-off-by: Sebastien Cevey <redacted>
---

Boo, the diff is still quite scarier than it really should be..

 gitweb/gitweb.perl |  103 +++++++++++++++++++++++++++++-----------------------
 1 files changed, 57 insertions(+), 46 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index b31274c..a6bb702 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -3972,59 +3972,19 @@ sub print_sort_th {
 	}
 }
 
-sub git_project_list_body {
+# print a row for each project in the given list, using the given
+# range and extra display options
+sub print_project_rows {
 	# actually uses global variable $project
-	my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
-
-	my ($check_forks) = gitweb_check_feature('forks');
-	my @projects = fill_project_list_info($projlist, $check_forks);
+	my ($projects, $from, $to, $check_forks, $show_ctags) = @_;
 
-	$order ||= $default_projects_order;
 	$from = 0 unless defined $from;
-	$to = $#projects if (!defined $to || $#projects < $to);
-
-	my %order_info = (
-		project => { key => 'path', type => 'str' },
-		descr => { key => 'descr_long', type => 'str' },
-		owner => { key => 'owner', type => 'str' },
-		age => { key => 'age', type => 'num' }
-	);
-	my $oi = $order_info{$order};
-	if ($oi->{'type'} eq 'str') {
-		@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @projects;
-	} else {
-		@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;
-	}
+	$to = $#$projects if (!defined $to || $#$projects < $to);
 
-	my $show_ctags = gitweb_check_feature('ctags');
-	if ($show_ctags) {
-		my %ctags;
-		foreach my $p (@projects) {
-			foreach my $ct (keys %{$p->{'ctags'}}) {
-				$ctags{$ct} += $p->{'ctags'}->{$ct};
-			}
-		}
-		my $cloud = git_populate_project_tagcloud(\%ctags);
-		print git_show_project_tagcloud($cloud, 64);
-	}
-
-	print "<table class=\"project_list\">\n";
-	unless ($no_header) {
-		print "<tr>\n";
-		if ($check_forks) {
-			print "<th></th>\n";
-		}
-		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 "<th></th>\n" . # for links
-		      "</tr>\n";
-	}
 	my $alternate = 1;
 	my $tagfilter = $cgi->param('by_tag');
 	for (my $i = $from; $i <= $to; $i++) {
-		my $pr = $projects[$i];
+		my $pr = $projects->[$i];
 
 		next if $tagfilter and $show_ctags and not grep { lc $_ eq lc $tagfilter } keys %{$pr->{'ctags'}};
 		next if $searchtext and not $pr->{'path'} =~ /$searchtext/

@@ -4068,6 +4028,57 @@ sub git_project_list_body {
 		      "</td>\n" .
 		      "</tr>\n";
 	}
+}
+
+sub git_project_list_body {
+	my ($projlist, $order, $from, $to, $extra, $no_header) = @_;
+
+	my ($check_forks) = gitweb_check_feature('forks');
+	my @projects = fill_project_list_info($projlist, $check_forks);
+
+	$order ||= $default_projects_order;
+
+	my %order_info = (
+		project => { key => 'path', type => 'str' },
+		descr => { key => 'descr_long', type => 'str' },
+		owner => { key => 'owner', type => 'str' },
+		age => { key => 'age', type => 'num' }
+	);
+	my $oi = $order_info{$order};
+	if ($oi->{'type'} eq 'str') {
+		@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @projects;
+	} else {
+		@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @projects;
+	}
+
+	my $show_ctags = gitweb_check_feature('ctags');
+	if ($show_ctags) {
+		my %ctags;
+		foreach my $p (@projects) {
+			foreach my $ct (keys %{$p->{'ctags'}}) {
+				$ctags{$ct} += $p->{'ctags'}->{$ct};
+			}
+		}
+		my $cloud = git_populate_project_tagcloud(\%ctags);
+		print git_show_project_tagcloud($cloud, 64);
+	}
+
+	print "<table class=\"project_list\">\n";
+	unless ($no_header) {
+		print "<tr>\n";
+		if ($check_forks) {
+			print "<th></th>\n";
+		}
+		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 "<th></th>\n" . # for links
+		      "</tr>\n";
+	}
+
+	print_project_rows(\@projects, $from, $to, $check_forks, $show_ctags);
+
 	if (defined $extra) {
 		print "<tr>\n";
 		if ($check_forks) {

-- 
1.5.6.5

This adds the $projects_list_group_categories option which, if enabled,
will result in grouping projects by category on the project list page.
The category is specified for each project by the $GIT_DIR/category file
or the 'category' variable in its configuration file. By default, projects
are put in the $project_list_default_category category.

Note:
- Categories are always sorted alphabetically, with projects in
  each category sorted according to the globally selected $order.
- When displaying a subset of all the projects (page limiting), the
  category headers are only displayed for projects present on the page.

The feature is inspired from Sham Chukoury's patch for the XMMS2
gitweb, but has been rewritten for the current gitweb development
HEAD. The CSS for categories is inspired from Gustavo Sverzut Barbieri's
patch to group projects by path.

Thanks to Florian Ragwitz for Perl tips.

Signed-off-by: Sebastien Cevey <redacted>
---

Cleaner patch this time indeed.  Still no fancy sorting of categories,
only alphabetical.

 gitweb/README      |   16 ++++++++++++
 gitweb/gitweb.css  |    7 +++++
 gitweb/gitweb.perl |   67 +++++++++++++++++++++++++++++++++++++++++++++++++--
 3 files changed, 87 insertions(+), 3 deletions(-)

diff --git a/gitweb/README b/gitweb/README
index 825162a..f8f8872 100644
--- a/gitweb/README
+++ b/gitweb/README

@@ -188,6 +188,15 @@ not include variables usually directly set during build):
    full description is available as 'title' attribute (usually shown on
    mouseover).  By default set to 25, which might be too small if you
    use long project descriptions.
+ * $projects_list_group_categories
+   Enables the grouping of projects by category on the project list page.
+   The category of a project is determined by the $GIT_DIR/category
+   file or the 'gitweb.category' variable in its repository configuration.
+   Disabled by default.
+ * $project_list_default_category
+   Default category for projects for which none is specified.  If set
+   to the empty string, such projects will remain uncategorized and
+   listed at the top, above categorized projects.
  * @git_base_url_list
    List of git base URLs used for URL to where fetch project from, shown
    in project summary page.  Full URL is "$git_base_url/$project".

@@ -269,6 +278,13 @@ You can use the following files in repository:
    from the template during repository creation. You can use the
    gitweb.description repo configuration variable, but the file takes
    precedence.
+ * category (or gitweb.category)
+   Singe line category of a project, used to group projects if
+   $projects_list_group_categories is enabled. By default (file and
+   configuration variable absent), uncategorized projects are put in
+   the $project_list_default_category category. You can use the
+   gitweb.category repo configuration variable, but the file takes
+   precedence.
  * cloneurl (or multiple-valued gitweb.url)
    File with repository URL (used for clone and fetch), one per line.
    Displayed in the project summary page. You can use multiple-valued

diff --git a/gitweb/gitweb.css b/gitweb/gitweb.css
index a01eac8..64f2a41 100644
--- a/gitweb/gitweb.css
+++ b/gitweb/gitweb.css

@@ -264,6 +264,13 @@ td.current_head {
 	text-decoration: underline;
 }
 
+td.category {
+	background-color: #d9d8d1;
+	border-top: 1px solid #000000;
+	border-left: 1px solid #000000;
+	font-weight: bold;
+}
+
 table.diff_tree span.file_status.new {
 	color: #008000;
 }

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a6bb702..97a9b73 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl

@@ -87,6 +87,14 @@ our $projects_list = "++GITWEB_LIST++";
 # the width (in characters) of the projects list "Description" column
 our $projects_list_description_width = 25;
 
+# group projects by category on the projects list
+# (enabled if this variable evaluates to true)
+our $projects_list_group_categories = 0;
+
+# default category if none specified
+# (leave the empty string for no category)
+our $project_list_default_category = "";
+
 # default order of projects list
 # valid values are none, project, descr, owner, and age
 our $default_projects_order = "project";

@@ -2023,6 +2031,11 @@ sub git_get_project_description {
 	return git_get_file_or_project_config('description', $path);
 }
 
+sub git_get_project_category {
+	my $path = shift;
+	return git_get_file_or_project_config('category', $path);
+}
+
 sub git_get_project_ctags {
 	my $path = shift;
 	my $ctags = {};

@@ -3915,8 +3928,9 @@ sub git_patchset_body {
 
 # . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . . .
 
-# fills project list info (age, description, owner, forks) for each
-# project in the list, removing invalid projects from returned list
+# fills project list info (age, description, owner, category, forks)
+# for each project in the list, removing invalid projects from
+# returned list
 # NOTE: modifies $projlist, but does not remove entries from it
 sub fill_project_list_info {
 	my ($projlist, $check_forks) = @_;

@@ -3939,6 +3953,11 @@ sub fill_project_list_info {
 		if (!defined $pr->{'owner'}) {
 			$pr->{'owner'} = git_get_project_owner("$pr->{'path'}") || "";
 		}
+		if ($projects_list_group_categories && !defined $pr->{'category'}) {
+			my $cat = git_get_project_category($pr->{'path'}) ||
+			                                   $project_list_default_category;
+			$pr->{'category'} = to_utf8($cat);
+		}
 		if ($check_forks) {
 			my $pname = $pr->{'path'};
 			if (($pname =~ s/\.git$//) &&

@@ -3956,6 +3975,19 @@ sub fill_project_list_info {
 	return @projects;
 }
 
+# returns a hash of categories, containing the list of project
+# belonging to each category
+sub build_projlist_by_category {
+	my $projlist = shift;
+	my %categories;
+
+	for my $pr (@$projlist) {
+		push @{$categories{ $pr->{'category'} }}, $pr;
+	}
+
+	return %categories;
+}
+
 # print 'sort by' <th> element, generating 'sort by $name' replay link
 # if that order is not selected
 sub print_sort_th {

@@ -4077,7 +4109,36 @@ sub git_project_list_body {
 		      "</tr>\n";
 	}
 
-	print_project_rows(\@projects, $from, $to, $check_forks, $show_ctags);
+	if ($projects_list_group_categories) {
+		# only display categories with projects in the $from-$to window
+		my %categories = build_projlist_by_category(\@projects);
+		foreach my $cat (sort keys %categories) {
+			my $num_cats = @{$categories{$cat}};
+
+			# out of the window to display, done
+			last if defined $to and $to < 0;
+
+			# in the window to display
+			if (!defined $from or $from < $num_cats) {
+				unless ($cat eq "") {
+					print "<tr>\n";
+					if ($check_forks) {
+						print "<td></td>\n";
+					}
+					print "<td class=\"category\" colspan=\"5\">$cat</td>\n";
+					print "</tr>\n";
+				}
+
+				print_project_rows($categories{$cat}, $from, $to, $check_forks, $show_ctags);
+			}
+
+			# adjust $from/$to offset, keep $from positive
+			$from = ($from > $num_cats) ? $from - $num_cats : 0 if defined $from;
+			$to -= $num_cats if defined $to;
+		}
+	} else {
+		print_project_rows(\@projects, $from, $to, $check_forks, $show_ctags);
+	}
 
 	if (defined $extra) {
 		print "<tr>\n";

-- 
1.5.6.5

Thanks, will queue so that they won't get lost in the noise.

On Thu, 4 Dec 2008, at 01:42, Sébastien Cevey wrote:

Introduce a git_get_file_or_project_config utility function to
retrieve a repository variable either from a plain text file in the
$GIT_DIR 

I would say that we try $GIT_DIR/$variable file.

or else from 'gitweb.$variable' in the repository config 
(e.g. 'description').

It _might_ also be added (just in case) that currently the only user
of this new subroutine is git_get_project_description, but this is
to change, and that is why this split was introduced.

Signed-off-by: Sebastien Cevey <redacted>

But those are minor issues. So, FWIW

Acked-by: Jakub Narebski <redacted>

It would probably be better to explicitly say that we use $git_dir/$name
file, or if it doesn't exist, gitweb.$name configuration variable.

+sub git_get_file_or_project_config {
+	my ($name, $path) = @_;

I think that $project, or $projectpath _might_ be better name for the
second argument to this subroutine.

 
 	$git_dir = "$projectroot/$path";
-	open my $fd, "$git_dir/description"
-		or return git_get_project_config('description');
-	my $descr = <$fd>;
+	open my $fd, "$git_dir/$name"
+		or return git_get_project_config($name);
+	my $conf = <$fd>;
 	close $fd;
-	if (defined $descr) {
-		chomp $descr;
+	if (defined $conf) {
+		chomp $conf;
 	}
-	return $descr;
+	return $conf;
+}
+
+sub git_get_project_description {
+	my $path = shift;
+	return git_get_file_or_project_config('description', $path);
 }

Nicely done.

 
 sub git_get_project_ctags {
-- 
1.5.6.5

-- 
Jakub Narębski
Poland

On Tue, 4 Dec 2008 at 01:44, Sébastien Cevey wrote:

Extract the printing of project rows on the project page into a
separate print_project_rows function. This makes it easier to reuse
the code to print different subsets of the whole project list.

Err... it was not obvious for me that 'project rows' are rows of
projects list table, i.e. currently the body of projects list table.

But I think it is a good description.

The row printing code is merely moved into a separate function, but
note that $projects is passed as a reference now.

Signed-off-by: Sebastien Cevey <redacted>

Nicely done.

For what it is worth:

Acked-by: Jakub Narebski <redacted>

---

Boo, the diff is still quite scarier than it really should be...

Well, nothing short of blame -C would make it work; the issue is that
we split subroutine into two, extracting from the middle, but with
some common/similar header

  2        1
  1        1
  2        1
  2
  2   ->   2
  1        2
  1        2
  2        2
  2        2
           2

And diff is forward preferring, i.e. it finds match then second part
of split as add, rather than first first part of split as add then
match.

I see that print_project_rows gets @$projects list already sorted, and
it passes explicitly $check_forks and $show_ctags from caller.

-- 
Jakub Narebski
Poland

On Thu, 4 Dec 2008, Sébastien Cevey wrote:

This adds the $projects_list_group_categories option which, if enabled,
will result in grouping projects by category on the project list page.
The category is specified for each project by the $GIT_DIR/category file
or the 'category' variable in its configuration file. By default, projects
are put in the $project_list_default_category category.

Note:
- Categories are always sorted alphabetically, with projects in
  each category sorted according to the globally selected $order.
- When displaying a subset of all the projects (page limiting), the
  category headers are only displayed for projects present on the page.

The feature is inspired from Sham Chukoury's patch for the XMMS2
gitweb, but has been rewritten for the current gitweb development
HEAD. The CSS for categories is inspired from Gustavo Sverzut Barbieri's
patch to group projects by path.

Thanks to Florian Ragwitz for Perl tips.

Very nice, and nicely done and thought out, idea.

Good.

Good.

Nice.

Good. Nicely uses earlier patch, which adds this infrastructure.

Nice. I see that you choose to go with $pr->{'category'} like existing
$pr->{'owner'}, rather than $pr->{'cat'} like existing $pr->{'descr'}.

This is very nice and simple way to group by categories, and it works
quite well with sorting (assuming that @$projlist is already sorted),
but I wonder how it works with $from / $to, i.e. with selecting
projects.

Nice... but perhaps it would be better to simply pass $from / $to to
build_projlist_by_category function, and have in %categories only
projects which are shown... well, unless filtered out in 
print_project_rows() by $show_ctags; so I think that there is nonzero
probability of empty (no project shown) categories.

+		foreach my $cat (sort keys %categories) {
+			my $num_cats = @{$categories{$cat}};
+
+			# out of the window to display, done
+			last if defined $to and $to < 0;
+
+			# in the window to display
+			if (!defined $from or $from < $num_cats) {
+				unless ($cat eq "") {
+					print "<tr>\n";
+					if ($check_forks) {
+						print "<td></td>\n";
+					}
+					print "<td class=\"category\" colspan=\"5\">$cat</td>\n";
+					print "</tr>\n";
+				}
+
+				print_project_rows($categories{$cat}, $from, $to, $check_forks, $show_ctags);
+			}
+
+			# adjust $from/$to offset, keep $from positive
+			$from = ($from > $num_cats) ? $from - $num_cats : 0 if defined $from;
+			$to -= $num_cats if defined $to;

I don't think that the games we play with $from / $to would be enough.
Check what happens (I think that it wouldn't work correctly) if we have
something like that:

 project | category | shown
 --------------------------
  1      | A        |
  2      | A        |
  3      | B        | Y
  4      | C        | Y
  5      | B        | Y
  6      | C        |
  7      | C        |

It means that we display for example second page in projects list.

+		}
+	} else {
+		print_project_rows(\@projects, $from, $to, $check_forks, $show_ctags);
+	}

Nice.

 
 	if (defined $extra) {
 		print "<tr>\n";
-- 
1.5.6.5

I'll try to examine the code in more detail later... currently I don't
know why but I can't git-am the second patch (this patch) correctly...

-- 
Jakub Narębski
Poland

At Fri, 5 Dec 2008 03:08:52 +0100, Jakub Narebski wrote:

Nice... but perhaps it would be better to simply pass $from / $to to
build_projlist_by_category function, and have in %categories only
projects which are shown...

Ah you're right, we could do that, I hadn't thought of it.  Sounds
cleaner than the $from/$to dance I did in this patch.

well, unless filtered out in print_project_rows() by $show_ctags; so
I think that there is nonzero probability of empty (no project
shown) categories.

Mh indeed, in fact this could happen any time we reach one of the
'next' statements in the loop in print_project_rows(), when performing
search, tag filtering, etc...

Actually, assuming the project list is split into page, this can also
lead to empty pages (if no project on the page matches the filter).
To avoid empty categories, it's a bit tricky since the header is
printed before we determine whether there are matching projects..

I need to read the code more carefully, but it seems that one solution
would be to add a function that determines whether a project should be
displayed or not (according to tags and search and forks); then we can
map this function on the list of projects.

I could do it if it sounds sane to you.

I don't think that the games we play with $from / $to would be enough.
Check what happens (I think that it wouldn't work correctly) if we have
something like that:

 project | category | shown
 --------------------------
  1      | A        |
  2      | A        |
  3      | B        | Y
  4      | C        | Y
  5      | B        | Y
  6      | C        |
  7      | C        |

AFAIK this cannot happen with the current code, since projects are
grouped by category (according to the foreach on categories).

I'll try to examine the code in more detail later... currently I don't
know why but I can't git-am the second patch (this patch) correctly...

This is the third patch, are you sure you applied 1 and 2 before?


Thanks for your careful and supportive comments!

-- 
Sébastien Cevey / inso.cc

On Fri, 5 Dec 2008 at 03:32, Sébastien Cevey wrote:

At Fri, 5 Dec 2008 03:08:52 +0100, Jakub Narebski wrote:

quoted

Nice... but perhaps it would be better to simply pass $from / $to to
build_projlist_by_category function, and have in %categories only
projects which are shown...

Ah you're right, we could do that, I hadn't thought of it.  Sounds
cleaner than the $from/$to dance I did in this patch.

Usually simpler is better. The more complicated code, the more chances
for bugs, and less maintability. I haven't even tried to follow 
$from/$to updating logic, but noticed that we can simply pass $from
and $to to build_projlist_by_category(), and use loop $from..$to there.

quoted

well, unless filtered out in print_project_rows() by $show_ctags; so
I think that there is nonzero probability of empty (no project
shown) categories.

Mh indeed, in fact this could happen any time we reach one of the
'next' statements in the loop in print_project_rows(), when performing
search, tag filtering, etc...

Actually, assuming the project list is split into page, this can also
lead to empty pages (if no project on the page matches the filter).
To avoid empty categories, it's a bit tricky since the header is
printed before we determine whether there are matching projects..

I need to read the code more carefully, but it seems that one solution
would be to add a function that determines whether a project should be
displayed or not (according to tags and search and forks); then we can
map this function on the list of projects.

I could do it if it sounds sane to you.

Nah, I think it would be best to postpone dealing with this issue, and
keep the code simple at the cost of possibly empty categories. It is
not as empty categories are an actual error...

I guess that correct way to deal with this would be to filter projects
earlier, before grouping by category, and not "at the last minute", 
when displaying them. It might be even better solution wrt. dividing
projects list into pages. But that in my opinion is certainly matter
for a separate patch.

quoted

I'll try to examine the code in more detail later... currently I don't
know why but I can't git-am the second patch (this patch) correctly...

This is the third patch, are you sure you applied 1 and 2 before?

 
Somehow I couldn't apply second patch in series:

 $ git am -3 -u "[PATCH v3 2_3] gitweb: Split git_project_list_body in two functions.txt"
 Applying: gitweb: Split git_project_list_body in two functions
 error: patch failed: gitweb/gitweb.perl:3972
 error: gitweb/gitweb.perl: patch does not apply
 fatal: sha1 information is lacking or useless (gitweb/gitweb.perl).
 Repository lacks necessary blobs to fall back on 3-way merge.
 Cannot fall back to three-way merge.
 Patch failed at 0001.
 

Thanks for your careful and supportive comments!

You are welcome. Nice to have another gitweb developer :-)
-- 
Jakub Narebski
Poland

At Fri, 5 Dec 2008 11:45:08 +0100, Jakub Narebski wrote:

quoted

quoted

Nice... but perhaps it would be better to simply pass $from / $to to
build_projlist_by_category function, and have in %categories only
projects which are shown...

Ah you're right, we could do that, I hadn't thought of it.  Sounds
cleaner than the $from/$to dance I did in this patch.

[...] we can simply pass $from and $to to
build_projlist_by_category(), and use loop $from..$to there.

I just tried, it works but we first need to sort @projects by
category.

Somehow I couldn't apply second patch in series:

 $ git am -3 -u "[PATCH v3 2_3] gitweb: Split git_project_list_body in two functions.txt"
 Applying: gitweb: Split git_project_list_body in two functions
 error: patch failed: gitweb/gitweb.perl:3972
 error: gitweb/gitweb.perl: patch does not apply
 fatal: sha1 information is lacking or useless (gitweb/gitweb.perl).
 Repository lacks necessary blobs to fall back on 3-way merge.
 Cannot fall back to three-way merge.
 Patch failed at 0001.

Where does it find the reference to the "necessary blobs"?  Could it
be because I didn't leave the From OBJECTID DATE line in my email
header?

Seems like there was a recent change on the line

  my $check_forks = gitweb_check_feature('forks');

so I had to update my patch to apply cleanly on the current master
HEAD.

I'm gonna re-send the three patches as a new version.

-- 
Sébastien Cevey / inso.cc

On Fri, 12 Dec 2008, Sébastien Cevey wrote:

At Fri, 5 Dec 2008 11:45:08 +0100, Jakub Narebski wrote:

quoted

quoted

quoted

Nice... but perhaps it would be better to simply pass $from / $to to
build_projlist_by_category function, and have in %categories only
projects which are shown...

Ah you're right, we could do that, I hadn't thought of it.  Sounds
cleaner than the $from/$to dance I did in this patch.

[...] we can simply pass $from and $to to
build_projlist_by_category(), and use loop $from..$to there.

I just tried, it works but we first need to sort @projects by
category.

I don't understand. You have the following code:

+# returns a hash of categories, containing the list of project
+# belonging to each category
+sub build_projlist_by_category {
+       my $projlist = shift;
+       my %categories;
+
+       for my $pr (@$projlist) {
+               push @{$categories{ $pr->{'category'} }}, $pr;
+       }
+
+       return %categories;
+}

I propose to change it to:

+# returns a hash of categories, containing the list of project
+# belonging to each category
+sub build_projlist_by_category {
+       my ($projlist, $from, $to) = shift;                       # <<<<<<<<<<<<<<<<<<<<<<<
+	$from = 0 unless defined $from;                           # +++++++++++++++++++++++
+	$to = $#$projlist if (!defined $to || $#$projlist < $to); # +++++++++++++++++++++++
+       my %categories;
+
+       for (my $i = $from; $i <= $to; $i++) {                    # <<<<<<<<<<<<<<<<<<<<<<<
+		$pr = $projlist->[$i];                            # +++++++++++++++++++++++
+               push @{$categories{ $pr->{'category'} }}, $pr;
+       }
+
+       return %categories;
+}

And of course update callers to pass $from and $to parameters.

This code doesn't change _anything_ beside the fact that it can operate
only on part of @$projlist.


But as you have noticed some kinds of limiting shown project list size
make sense only if done _after_ dividing into categories. Some but not
all. For example page length limiting after sorting by name, 
description, owner or age (sorting which takes place before dividing
into categories) makes sense if it is done _before_ categorization:
we want to show e.g. 100 freshest projects, and not up to 100 projects
from first categories, with last category showing maybe only freshest
projects in this category.


Besides making selection of projects to show operate on list of projects
instead of being done just before display is IMHO better, more flexible,
and more robust solution.

I'm gonna re-send the three patches as a new version.

Thanks in advance.

-- 
Jakub Narebski
Poland

At Fri, 12 Dec 2008 01:13:45 +0100, Jakub Narebski wrote:

I just tried, it works but we first need to sort @projects by
category.

I don't understand.
[...]
I propose to change it to:

Well in my previous iteration of the patch (v3), the printing of
projects with categories is done using:

  foreach my $cat (sort keys %categories) {

So everything was already sorted by category (and then by whichever
property you picked inside each category).  You seemed okay with it,
but requested that I documented that behaviour in the commit log.

To maintain the same result with your proposed change (which is what I
submitted in my patch), we need to sort by categories first (AFAIK
Perl sort retains the original order inside equivalence classes of
comparison key?), otherwise splice(projlist, from, to) doesn't return
the expected subset.

-- 
Sébastien Cevey / inso.cc

On Fri, 12 Dec 2008, Sébastien Cevey wrote:

At Fri, 12 Dec 2008 01:13:45 +0100, Jakub Narebski wrote:

quoted

I just tried, it works but we first need to sort @projects by
category.

I don't understand.
[...]
I propose to change it to:

Well in my previous iteration of the patch (v3), the printing of
projects with categories is done using:

  foreach my $cat (sort keys %categories) {

So everything was already sorted by category (and then by whichever
property you picked inside each category).  You seemed okay with it,
but requested that I documented that behaviour in the commit log.

But this does not mean that sorting by categories is necessary, or
even wanted (see below). This foreach _sorts_ by categories as primary
key using kind of bucket sort algorithm.
 

To maintain the same result with your proposed change (which is what I
submitted in my patch), we need to sort by categories first (AFAIK
Perl sort retains the original order inside equivalence classes of
comparison key?), otherwise splice(projlist, from, to) doesn't return 
the expected subset.

Perl requires "use sort 'stable';" pragma to ensure stable sort.

And no, we don't need to sort by categories first.  Let me explain
in more detail a bit.

Let us assume that $from and $to is actually used to divide projects
list into categories (which goal is incompatible with searching
projects, limiting to given tag/tagset and hiding forked as it is done
now, at the display time; it has to be done _before_ pagination).
They are used to display first page, i.e. repositories numbered 1..N
in current ordering, or N..2*N, or 2*N..3*N to show next pages. Let us
have the following project list:

  Project       Category
  ---------------------------
  1             a
  2             b
  3             b
  4             a

If categories are not shown, and page limit is 2, then project would
be displayed like this:


  page 1          page 2
  ------          ------
  1               3
  2               4

Now _without_ sorting by category upfront, those pages would look like
the following if grouping by category is enabled:

A.page 1          page 2
  ------          ------
  [a]             [a]
  1               4
  [b]             [b]
  2               3

What is not visible in this example is that projects inside category
would be sorted by given order.


Now if you would sort by categories _before_ pagination, like you
(from what I understand) proposed, you would have (assuming that
you used "use sort 'stable'" inside block):

  Project       Category
  ---------------------------
  1             a
  4             a
  2             b
  3             b

Pagination would then look like the following:

B.page 1          page 2
  ------          ------
  [a]             [b]
  1               2
  4               3


Now which result you consider correct depends on the point of view.
First is sort, paginate, sort; second is sort, sort, paginate.
First have first N repositories in given order on first page, perhaps
reordered a bit by categories, second doesn't have this feature.
I think that the case A is more correct, but you might disagree.


Let us change example a bit:

  Project       Category
  ---------------------------
  1             b
  2             a
  3             a
  4             b

A.page 1          page 2
  ------          ------
  [a]             [a]
  2               3
  [b]             [b]
  1               4

B.page 1          page 2
  ------          ------
  [a]             [b]
  2               1
  3               4

P.S. It is IMHO better to use

 	for (my $i = $from; $i <= $to; $i++) {

than the style which is not used elsewhere in gitweb, from what
I remember

 	foreach my $i ($from..$to) {

I might also be inefficient as it generates temporary array which
might be quite big; I don't know if Perl 5.8.x, the oldest version
one can sensibly use with gitweb I think, has this bug or not.
-- 
Jakub Narebski
Poland

At Fri, 12 Dec 2008 03:03:55 +0100, Jakub Narebski wrote:

And no, we don't need to sort by categories first.  Let me explain
in more detail a bit.

Thanks for the detailed explanation, I understand your preference.
But as you said, it's a bit arbitrary, I think none is completely
obvious.


I don't really have a strong opinion about which is best, but just to
illustrate what made me go for the solution B, let me show another
example:

name / date / cat
1      2006    A
2      2003    B
3      2005    B
4      2003    A
5      2000    A
6      2008    C
7      2007    C
8      2001    B
9      2005    A

We then sort by name and split in pages of N=3 (sorted by cat on each
page):

A:sort(name) split(max=3) subsort(cat)
  1  2006  A     4  2003  A     9  2005  A
  2  2003  B     5  2000  A     8  2001  B
  3  2005  B     6  2008  C     7  2007  C

B:sort(cat) subsort(name) split(max=4)
  1  2006  A     9  2005  A     8  2001  B
  4  2003  A     2  2003  B     6  2008  C
  5  2000  A     3  2005  B     7  2007  C

With B, we have the second top-entry (2) relegated to the second page,
which might be surprising because we ordered by name.  But what I find
weird about A is that because of the per-page category sorting, the
"top-sorted entries at the top" isn't true either (page 3).  We have
"reshuffled" the result of 'sort(name) split(max=3)' on each page.

To be truly fair to the main sorting, we should not try to group
categories and display the header for each consecutive group of
entries from a distinct category:

A-:sort(name) split(max=3)
  1  2006  A     4  2003  A     7  2007  C
  2  2003  B     5  2000  A     8  2001  B
  3  2005  B     6  2008  C     9  2005  A

Which in this case is painless as it only affects page 3, but it could
lead to a mess of interleaved categories, and we kind of lose the
purpose of category groups in the first place..

The point is that with A, you cannot determine whether you're on the
right page to find project P (even if you know its category) by
checking whether it's in the interval between the top and bottom
entries.


It's perhaps even more apparent if we sort by date:

A:sort(year) split(max=3) subsort(cat)
  1  2006  A     9  2005  A     4  2003  A
  6  2008  C     3  2005  B     5  2000  A
  7  2007  C     2  2003  B     8  2001  B

B:sort(cat) subsort(year) split(max=4)
  1  2006  A     4  2003  A     3  2005  B
  9  2005  A     8  2001  B     7  2007  C
  5  2000  A     2  2003  B     6  2008  C

It feels kind of unnatural that not only projects are not sorted by
date on each page (they are inside categories), but moreover
categories are spread over all pages.


I guess it depends on your use case, and whether categories are
important or known by the user.  I personally don't really care (I
never split stuff into pages in the gitweb I use), so I can do a new
version of my patch that does A if you prefer, just let me know.  I
just wanted to clarify that both solutions sort of suck :-)

P.S. It is IMHO better to use

 	for (my $i = $from; $i <= $to; $i++) {

Ah sorry, I took the words from your earlier email too literally ("we
can [...] use loop $from..$to there"). That's what happens when
pseudocode is actually valid syntax in a language!

Cheers,

-- 
Sébastien Cevey / inso.cc

On Fri, 12 Dec 2008, Sébastien Cevey wrote:

At Fri, 12 Dec 2008 03:03:55 +0100, Jakub Narebski wrote:

quoted

And no, we don't need to sort by categories first.  Let me explain
in more detail a bit.

Thanks for the detailed explanation, I understand your preference.
But as you said, it's a bit arbitrary, I think none is completely
obvious.

First, I feel a bit bad for derailing this patch. Currently gitweb
does not do pagination of projects list; it is not even possible in
a sane way with current way project searching/selecting is implemented.
So the whole build_projlist_by_category() respecting $from and $to is
moot point.

So if we don't use it, even if it is nice to have for the future, we
don't need to pay cost of extra stable sorting.

I don't really have a strong opinion about which is best, but just to
illustrate what made me go for the solution B, let me show another
example:

name / date / cat
1      2006    A
2      2003    B
3      2005    B
4      2003    A
5      2000    A
6      2008    C
7      2007    C
8      2001    B
9      2005    A

We then sort by name and split in pages of N=3 (sorted by cat on each
page):

A:sort(name) split(max=3) subsort(cat)
  1  2006  A     4  2003  A     9  2005  A
  2  2003  B     5  2000  A     8  2001  B
  3  2005  B     6  2008  C     7  2007  C

B:sort(cat) subsort(name) split(max=3)
  1  2006  A     9  2005  A     8  2001  B
  4  2003  A     2  2003  B     6  2008  C
  5  2000  A     3  2005  B     7  2007  C

With B, we have the second top-entry (2) relegated to the second page,
which might be surprising because we ordered by name.  But what I find
weird about A is that because of the per-page category sorting, the
"top-sorted entries at the top" isn't true either (page 3).  We have
"reshuffled" the result of 'sort(name) split(max=3)' on each page.

[...]

It's perhaps even more apparent if we sort by date:

A:sort(year) split(max=3) subsort(cat)
  1  2006  A     9  2005  A     4  2003  A
  6  2008  C     3  2005  B     5  2000  A
  7  2007  C     2  2003  B     8  2001  B

B:sort(cat) subsort(year) split(max=4)
  1  2006  A     4  2003  A     3  2005  B
  9  2005  A     8  2001  B     7  2007  C
  5  2000  A     2  2003  B     6  2008  C

It feels kind of unnatural that not only projects are not sorted by
date on each page (they are inside categories), but moreover
categories are spread over all pages.


I guess it depends on your use case, and whether categories are
important or known by the user.  I personally don't really care (I
never split stuff into pages in the gitweb I use), so I can do a new
version of my patch that does A if you prefer, just let me know.  I
just wanted to clarify that both solutions sort of suck :-)

Well, with version A you can (I think) simply change

  foreach my $cat (sort keys %categories) {

to

  foreach my $cat (sort 
  	{ cmp_cat($projlist, \%categories, $oi, $a, $b) } keys %categories) {

to have the following output (see the difference on page 3)

A':sort(name) split(max=3) subsort(sort(cat,name))
  1  2006  A     4  2003  A     7  2007  C
  2  2003  B     5  2000  A     8  2001  B
  3  2005  B     6  2008  C     9  2005  A

where sort_cat might be something like (we took advantage that
categories in %categories have at least one project):

  sub cmp_cat {
  	my ($projlist, $cats, $oi, $a, $b) = @_;
  	my ($aa, $bb);
  	# projects in categories are sorted, so we can compare first
  	# project from a category to sort categories in given ordering
  	$aa = $projlist->{$cats->{$a}[0]};
  	$bb = $projlist->{$cats->{$b}[0]};
  	if ($oi->{'type'} eq 'str') {
		return $aa->{$oi->{'key'}} cmp $bb->{$oi->{'key'}};
	} else {
		return $aa->{$oi->{'key'}} <=> $bb->{$oi->{'key'}};
	}
  }

-- 
Jakub Narebski
Poland