{"thread":{"id":"32305","subject":"[PATCH] gitweb: Sort projects with undefined ages last","startedAt":"2012-12-10T04:34:21Z","lastAt":"2012-12-11T18:08:29Z","messageCount":4,"participants":["Matthew Daley","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"204639","messageId":"1355114061-4652-1-git-send-email-mattjd@gmail.com","threadId":"32305","inReplyTo":null,"subject":"[PATCH] gitweb: Sort projects with undefined ages last","fromName":"Matthew Daley","fromEmail":"mattjd@gmail.com","sentAt":"2012-12-10T04:34:21Z","receivedAt":"2012-12-10T04:34:21Z","isPatch":true,"sender":{"key":"mattjd@gmail.com","avatar":null},"body":"Sorting gitweb's project list by age ('Last Change') currently shows\nprojects with undefined ages at the head of the list. This results in a\nless useful result when there are a number of projects that are missing\nor otherwise faulty and one is trying to see what projects have been\nupdated recently.\n\nFix by sorting these projects with undefined ages at the bottom of the\nlist when sorting by age.\n\nSigned-off-by: Matthew Daley <mattjd@gmail.com>\n---\nI realize this might be a bit bikesheddy, but it does improve the listing\nin the given use case. For an example of the problem, see ie.\nhttp://git.kernel.org/?o=age or http://repo.or.cz/w?a=project_list;o=age .\n\nI'm also not a Perl native, so any advice on making the patch good Perl is\nappreciated.\n\n gitweb/gitweb.perl |    4 +++-\n 1 file changed, 3 insertions(+), 1 deletion(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0f207f2..21da1b5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5541,7 +5541,9 @@ sub sort_projects_list {\n \tif ($oi->{'type'} eq 'str') {\n \t\t@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @$projlist;\n \t} else {\n-\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @$projlist;\n+\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}}\n+\t\t            grep {defined $_->{$oi->{'key'}}} @$projlist;\n+\t\tpush @projects, grep {!defined $_->{$oi->{'key'}}} @$projlist;\n \t}\n \n \treturn @projects;\n-- \n1.7.10.4\n"},{"id":"204640","messageId":"7vip8actz3.fsf@alter.siamese.dyndns.org","threadId":"32305","inReplyTo":"1355114061-4652-1-git-send-email-mattjd@gmail.com","subject":"Re: [PATCH] gitweb: Sort projects with undefined ages last","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-10T06:16:32Z","receivedAt":"2012-12-10T06:16:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Daley <mattjd@gmail.com> writes:\n\n> Sorting gitweb's project list by age ('Last Change') currently shows\n> projects with undefined ages at the head of the list. This results in a\n> less useful result when there are a number of projects that are missing\n> or otherwise faulty and one is trying to see what projects have been\n> updated recently.\n>\n> Fix by sorting these projects with undefined ages at the bottom of the\n> list when sorting by age.\n>\n> Signed-off-by: Matthew Daley <mattjd@gmail.com>\n> ---\n> I realize this might be a bit bikesheddy, but it does improve the listing\n> in the given use case. For an example of the problem, see ie.\n> http://git.kernel.org/?o=age or http://repo.or.cz/w?a=project_list;o=age .\n\nYeah, it could be argued that in a very minor corner case showing\nnew and empty ones at the top might attract more attention to them,\nbut new and empty ones can stay inactive, so this change would be an\noverall improvement for these two sites.  An alternative could be to\ngive the mtime of the git directory to the age field if there is no\ncommits in the repository, to sink the empty and inactive ones to\nthe bottom quickly while showing newly created ones at the top, but\nit shouldn't make any practical difference.\n\n> I'm also not a Perl native, so any advice on making the patch good Perl is\n> appreciated.\n>\n>  gitweb/gitweb.perl |    4 +++-\n>  1 file changed, 3 insertions(+), 1 deletion(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 0f207f2..21da1b5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5541,7 +5541,9 @@ sub sort_projects_list {\n>  \tif ($oi->{'type'} eq 'str') {\n>  \t\t@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @$projlist;\n>  \t} else {\n> -\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @$projlist;\n> +\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}}\n> +\t\t            grep {defined $_->{$oi->{'key'}}} @$projlist;\n> +\t\tpush @projects, grep {!defined $_->{$oi->{'key'}}} @$projlist;\n>  \t}\n\nTwo observations:\n\n * This iterates over the same @$projlist twice with grep, with one\n   \"defined\" and the other \"!defined\", which may risk these two\n   complementary grep conditions to go out of sync (it also may\n   affect performance but that is a lessor issue).\n\n   An alternative may be to change the expression used inside sort()\n   to treat an undef as if it were a very large value, something\n   like:\n\n\tsort {\n        \tdefined $a->{$oi->{'key'}}\n                ? (defined $b->{$oi->{'key'}}\n                  ? ($a->{$oi->{'key'}} <=> $b->{$oi->{'key'}})\n\t\t  : -1)\n                : (defined $b->{$oi->{'key'}} ? 1 : 0);\n        }\n\n * This \"sort undefs at the end is better than at the beginning\" is\n   good only for the \"age\" field, and we wouldn't know if we would\n   add other keys for which it may be better to sort undef at the\n   beginning.  The order_info{} currently has only one field of the\n   'num' type, so this is not an immediate issue, but in order to\n   future proof, it may make sense to rewrite the sort_projects_list\n   function to map the order field name to a function given to sort,\n   e.g.\n\n\tmy %order_sort = (\n\t\tproject => sub { $a->{'path'} cmp $b->{'path'} },\n                descr => sub { $a->{'descr_long'} cmp $b->{'descr_long'} },\n\t\towner => sub { $a->{'owner'} cmp $b->{'owner'} },\n\t\tage => sub { ... the num cmp with undef above ... },\n\t);\n\tif (!exists $order_sort{$order}) {\n        \treturn @$projlist;\n\t}\n\treturn sort $order_sort{$order} @$projlist;\n\nI am not sure the second one is worth it, though.\n"},{"id":"204675","messageId":"1355223367-5894-1-git-send-email-mattjd@gmail.com","threadId":"32305","inReplyTo":"7vip8actz3.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH] gitweb: Sort projects with undefined ages last","fromName":"Matthew Daley","fromEmail":"mattjd@gmail.com","sentAt":"2012-12-11T10:56:07Z","receivedAt":"2012-12-11T10:56:07Z","isPatch":true,"sender":{"key":"mattjd@gmail.com","avatar":null},"body":"On Mon, Dec 10, 2012 at 7:16 PM, Junio C Hamano <gitster@pobox.com> wrote:\n> Yeah, it could be argued that in a very minor corner case showing\n> new and empty ones at the top might attract more attention to them,\n> but new and empty ones can stay inactive, so this change would be an\n> overall improvement for these two sites.  An alternative could be to\n> give the mtime of the git directory to the age field if there is no\n> commits in the repository, to sink the empty and inactive ones to\n> the bottom quickly while showing newly created ones at the top, but\n> it shouldn't make any practical difference.\n\nAgreed.\n\n> Two observations:\n>\n>  * This iterates over the same @$projlist twice with grep, with one\n>    \"defined\" and the other \"!defined\", which may risk these two\n>    complementary grep conditions to go out of sync (it also may\n>    affect performance but that is a lessor issue).\n>\n>    An alternative may be to change the expression used inside sort()\n>    to treat an undef as if it were a very large value, something\n>    like:\n>\n>         sort {\n>                 defined $a->{$oi->{'key'}}\n>                 ? (defined $b->{$oi->{'key'}}\n>                   ? ($a->{$oi->{'key'}} <=> $b->{$oi->{'key'}})\n>                   : -1)\n>                 : (defined $b->{$oi->{'key'}} ? 1 : 0);\n>         }\n>\n>  * This \"sort undefs at the end is better than at the beginning\" is\n>    good only for the \"age\" field, and we wouldn't know if we would\n>    add other keys for which it may be better to sort undef at the\n>    beginning.  The order_info{} currently has only one field of the\n>    'num' type, so this is not an immediate issue, but in order to\n>    future proof, it may make sense to rewrite the sort_projects_list\n>    function to map the order field name to a function given to sort,\n>    e.g.\n>\n>         my %order_sort = (\n>                 project => sub { $a->{'path'} cmp $b->{'path'} },\n>                 descr => sub { $a->{'descr_long'} cmp $b->{'descr_long'} },\n>                 owner => sub { $a->{'owner'} cmp $b->{'owner'} },\n>                 age => sub { ... the num cmp with undef above ... },\n>         );\n>         if (!exists $order_sort{$order}) {\n>                 return @$projlist;\n>         }\n>         return sort $order_sort{$order} @$projlist;\n>\n> I am not sure the second one is worth it, though.\n\nI thought about both of those variants as well. What about this:\n\n-- >8 --\nSubject: [PATCH] gitweb: Sort projects with undefined ages last\n\nSorting gitweb's project list by age ('Last Change') currently shows\nprojects with undefined ages at the head of the list. This gives a less\nuseful result when there are a number of projects that are missing or\notherwise faulty and one is trying to see what projects have been\nupdated recently.\n\nFix by sorting these projects with undefined ages at the bottom of the\nlist when sorting by age.\n\nSigned-off-by: Matthew Daley <mattjd@gmail.com>\n---\n gitweb/gitweb.perl |   35 +++++++++++++++++++++--------------\n 1 file changed, 21 insertions(+), 14 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 0f207f2..656b324 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -5528,23 +5528,30 @@ sub fill_project_list_info {\n \n sub sort_projects_list {\n \tmy ($projlist, $order) = @_;\n-\tmy @projects;\n \n-\tmy %order_info = (\n-\t\tproject => { key => 'path', type => 'str' },\n-\t\tdescr => { key => 'descr_long', type => 'str' },\n-\t\towner => { key => 'owner', type => 'str' },\n-\t\tage => { key => 'age', type => 'num' }\n-\t);\n-\tmy $oi = $order_info{$order};\n-\treturn @$projlist unless defined $oi;\n-\tif ($oi->{'type'} eq 'str') {\n-\t\t@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @$projlist;\n-\t} else {\n-\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @$projlist;\n+\tsub order_str {\n+\t\tmy $key = shift;\n+\t\treturn sub { $a->{$key} cmp $b->{$key} };\n \t}\n \n-\treturn @projects;\n+\tsub order_num_then_undef {\n+\t\tmy $key = shift;\n+\t\treturn sub {\n+\t\t\tdefined $a->{$key} ?\n+\t\t\t\t(defined $b->{$key} ? $a->{$key} <=> $b->{$key} : -1) :\n+\t\t\t\t(defined $b->{$key} ? 1 : 0)\n+\t\t};\n+\t}\n+\n+\tmy %orderings = (\n+\t\tproject => order_str('path'),\n+\t\tdescr => order_str('descr_long'),\n+\t\towner => order_str('owner'),\n+\t\tage => order_num_then_undef('age'),\n+\t);\n+\n+\tmy $ordering = $orderings{$order};\n+\treturn defined $ordering ? sort $ordering @$projlist : @$projlist;\n }\n \n # returns a hash of categories, containing the list of project\n-- \n1.7.10.4\n"},{"id":"204680","messageId":"7vhans8ns2.fsf@alter.siamese.dyndns.org","threadId":"32305","inReplyTo":"1355223367-5894-1-git-send-email-mattjd@gmail.com","subject":"Re: [PATCH] gitweb: Sort projects with undefined ages last","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-12-11T18:08:29Z","receivedAt":"2012-12-11T18:08:29Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Matthew Daley <mattjd@gmail.com> writes:\n\n> I thought about both of those variants as well. What about this:\n>\n> -- >8 --\n> Subject: [PATCH] gitweb: Sort projects with undefined ages last\n>\n> Sorting gitweb's project list by age ('Last Change') currently shows\n> projects with undefined ages at the head of the list. This gives a less\n> useful result when there are a number of projects that are missing or\n> otherwise faulty and one is trying to see what projects have been\n> updated recently.\n>\n> Fix by sorting these projects with undefined ages at the bottom of the\n> list when sorting by age.\n>\n> Signed-off-by: Matthew Daley <mattjd@gmail.com>\n> ---\n\nLooks sensible to me.  Thanks; will queue.\n\n>  gitweb/gitweb.perl |   35 +++++++++++++++++++++--------------\n>  1 file changed, 21 insertions(+), 14 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 0f207f2..656b324 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -5528,23 +5528,30 @@ sub fill_project_list_info {\n>  \n>  sub sort_projects_list {\n>  \tmy ($projlist, $order) = @_;\n> -\tmy @projects;\n>  \n> -\tmy %order_info = (\n> -\t\tproject => { key => 'path', type => 'str' },\n> -\t\tdescr => { key => 'descr_long', type => 'str' },\n> -\t\towner => { key => 'owner', type => 'str' },\n> -\t\tage => { key => 'age', type => 'num' }\n> -\t);\n> -\tmy $oi = $order_info{$order};\n> -\treturn @$projlist unless defined $oi;\n> -\tif ($oi->{'type'} eq 'str') {\n> -\t\t@projects = sort {$a->{$oi->{'key'}} cmp $b->{$oi->{'key'}}} @$projlist;\n> -\t} else {\n> -\t\t@projects = sort {$a->{$oi->{'key'}} <=> $b->{$oi->{'key'}}} @$projlist;\n> +\tsub order_str {\n> +\t\tmy $key = shift;\n> +\t\treturn sub { $a->{$key} cmp $b->{$key} };\n>  \t}\n>  \n> -\treturn @projects;\n> +\tsub order_num_then_undef {\n> +\t\tmy $key = shift;\n> +\t\treturn sub {\n> +\t\t\tdefined $a->{$key} ?\n> +\t\t\t\t(defined $b->{$key} ? $a->{$key} <=> $b->{$key} : -1) :\n> +\t\t\t\t(defined $b->{$key} ? 1 : 0)\n> +\t\t};\n> +\t}\n> +\n> +\tmy %orderings = (\n> +\t\tproject => order_str('path'),\n> +\t\tdescr => order_str('descr_long'),\n> +\t\towner => order_str('owner'),\n> +\t\tage => order_num_then_undef('age'),\n> +\t);\n> +\n> +\tmy $ordering = $orderings{$order};\n> +\treturn defined $ordering ? sort $ordering @$projlist : @$projlist;\n>  }\n>  \n>  # returns a hash of categories, containing the list of project\n"}]}