{"thread":{"id":"30138","subject":"[PATCH] gitweb: Option to omit column with time of the last change","startedAt":"2012-04-03T13:27:36Z","lastAt":"2012-04-26T16:45:44Z","messageCount":23,"participants":["Kacper Kornet","Jakub Narebski","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"188413","messageId":"20120403132735.GA12389@camk.edu.pl","threadId":"30138","inReplyTo":null,"subject":"[PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-03T13:27:36Z","receivedAt":"2012-04-03T13:27:36Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"Generating information about last change for a large number of git\nrepositories can be time consuming. This commit adds an option to\nomit 'Last Change' column when presenting the list of repositories.\n\nSigned-off-by: Kacper Kornet \n---\n Documentation/gitweb.conf.txt | 3 +++\n gitweb/gitweb.perl | 16 +++++++++++-----\n 2 files changed, 14 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex 7aba497..bfeef21 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -403,6 +403,9 @@ $default_projects_order::\n \ti.e. path to repository relative to `$projectroot`), \"descr\"\n \t(project description), \"owner\", and \"age\" (by date of most current\n \tcommit).\n+\n+$no_list_age::\n+\tOmit column with date of the most curren commit\n +\n Default value is \"project\". Unknown value means unsorted.\n \ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..f42468c 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n # (only effective if this variable evaluates to true)\n our $export_ok = \"++GITWEB_EXPORT_OK++\";\n \n+# don't generate age column\n+our $no_list_age = 0;\n+\n # show repository only if this subroutine returns true\n # when given the path to the project, for example:\n # sub { return -e \"$_[0]/git-daemon-export-ok\"; }\n@@ -5462,9 +5465,11 @@ sub git_project_list_rows {\n \t\t : esc_html($pr->{'descr'})) .\n \t\t \"\\n\" .\n \t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n-\t\tprint \"{'age'}) . \"\\\">\" .\n-\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n-\t\t \"\" .\n+\t\tunless ($no_list_age) {\n+\t\t print \"{'age'}) . \"\\\">\" .\n+\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\";\n+\t\t}\n+\t\tprint\"\" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\")}, \"summary\") . \" | \" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"shortlog\")}, \"shortlog\") . \" | \" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"log\")}, \"log\") . \" | \" .\n@@ -5495,7 +5500,8 @@ sub git_project_list_body {\n \t 'tagfilter' => $tagfilter)\n \t\tif ($tagfilter || $search_regexp);\n \t# fill the rest\n-\t@projects = fill_project_list_info(\\@projects);\n+\tmy @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n+\t@projects = fill_project_list_info(\\@projects, @all_fields);\n \n \t$order ||= $default_projects_order;\n \t$from = 0 unless defined $from;\n@@ -5527,7 +5533,7 @@ sub git_project_list_body {\n \t\tprint_sort_th('project', $order, 'Project');\n \t\tprint_sort_th('descr', $order, 'Description');\n \t\tprint_sort_th('owner', $order, 'Owner');\n-\t\tprint_sort_th('age', $order, 'Last Change');\n+\t\tprint_sort_th('age', $order, 'Last Change') unless $no_list_age;\n \t\tprint \"\\n\" . # for links\n \t\t \"\\n\";\n \t}\n-- \n1.7.10.rc3\n"},{"id":"188460","messageId":"201204040112.02269.jnareb@gmail.com","threadId":"30138","inReplyTo":"20120403132735.GA12389@camk.edu.pl","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-03T23:12:01Z","receivedAt":"2012-04-03T23:12:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 3 Apr 2012, Kacper Kornet wrote:\n\n> Generating information about last change for a large number of git\n> repositories can be time consuming. This commit adds an option to\n> omit 'Last Change' column when presenting the list of repositories.\n>\nThis is quite a good idea, I think.\n\nEven a better solution would be to add caching support to gitweb, but\nfirst it is a long way from being ready, and second not in all cases\nyou are able to / wants to have a cache.\n \n> Signed-off-by: Kacper Kornet \n> ---\n> Documentation/gitweb.conf.txt | 3 +++\n> gitweb/gitweb.perl | 16 +++++++++++-----\n> 2 files changed, 14 insertions(+), 5 deletions(-)\n> \n> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> index 7aba497..bfeef21 100644\n> --- a/Documentation/gitweb.conf.txt\n> +++ b/Documentation/gitweb.conf.txt\n> @@ -403,6 +403,9 @@ $default_projects_order::\n> \ti.e. path to repository relative to `$projectroot`), \"descr\"\n> \t(project description), \"owner\", and \"age\" (by date of most current\n> \tcommit).\n> +\n> +$no_list_age::\n> +\tOmit column with date of the most curren commit\n\ns/curren/current/\n\nThanks for adding documentation, though I would prefer if you expanded\nthis description (for example including the information that it touches\nprojects list page).\n\n> +\n\nThis '+' here means \"continuation\". You by accident inserted\ndescription of new $no_list_age in the middle of description for\n$default_projects_order variable...\n\n> Default value is \"project\". Unknown value means unsorted.\n\n...as you can see here.\n\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a8b5fad..f42468c 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n> # (only effective if this variable evaluates to true)\n> our $export_ok = \"++GITWEB_EXPORT_OK++\";\n> \n> +# don't generate age column\n> +our $no_list_age = 0;\n\n\"age\" column where?\n\nHmmm... can't we come with a better name than $no_list_age?\n\n> +\n> # show repository only if this subroutine returns true\n> # when given the path to the project, for example:\n> # sub { return -e \"$_[0]/git-daemon-export-ok\"; }\n> @@ -5462,9 +5465,11 @@ sub git_project_list_rows {\n> \t\t : esc_html($pr->{'descr'})) .\n> \t\t \"\\n\" .\n> \t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n> -\t\tprint \"{'age'}) . \"\\\">\" .\n> -\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n> -\t\t \"\" .\n> +\t\tunless ($no_list_age) {\n> +\t\t print \"{'age'}) . \"\\\">\" .\n> +\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\";\n> +\t\t}\n> +\t\tprint\"\" .\n\nO.K. I guess that it ismore readable than\n\n +\t\tprint \"{'age'}) . \"\\\">\" .\n +\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n +\t\t \"\"\n +\t\t\tunless ($no_list_age);\n\n> \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\")}, \"summary\") . \" | \" .\n> \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"shortlog\")}, \"shortlog\") . \" | \" .\n> \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"log\")}, \"log\") . \" | \" .\n> @@ -5495,7 +5500,8 @@ sub git_project_list_body {\n> \t 'tagfilter' => $tagfilter)\n> \t\tif ($tagfilter || $search_regexp);\n> \t# fill the rest\n> -\t@projects = fill_project_list_info(\\@projects);\n> +\tmy @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n> +\t@projects = fill_project_list_info(\\@projects, @all_fields);\n\nThat looks quite strange on first glance. I know that empty list means\nfilling all fields, but the casual reader migh wonder about this\nconditional expression.\n\nPerhaps it would be better to write it this way:\n\n -\t@projects = fill_project_list_info(\\@projects);\n +\tmy @fields = qw(descr descr_long owner ctags category);\n +\tpush @fields, 'age' unless ($no_list_age);\n +\t@projects = fill_project_list_info(\\@projects, @fields);\n\nor something like that.\n\nWell, at least until we come up with a better way to specify \"all fields\nexcept those specified\".\n\n> \n> \t$order ||= $default_projects_order;\n> \t$from = 0 unless defined $from;\n> @@ -5527,7 +5533,7 @@ sub git_project_list_body {\n> \t\tprint_sort_th('project', $order, 'Project');\n> \t\tprint_sort_th('descr', $order, 'Description');\n> \t\tprint_sort_th('owner', $order, 'Owner');\n> -\t\tprint_sort_th('age', $order, 'Last Change');\n> +\t\tprint_sort_th('age', $order, 'Last Change') unless $no_list_age;\n\n +\t\tprint_sort_th('age', $order, 'Last Change')\n +\t\t\tunless $no_list_age;\n\nmight be more readable.\n\n> \t\tprint \"\\n\" . # for links\n> \t\t \"\\n\";\n> \t}\n> -- \n> 1.7.10.rc3\n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"188470","messageId":"20120404063939.GA17024@camk.edu.pl","threadId":"30138","inReplyTo":"201204040112.02269.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-04T06:39:39Z","receivedAt":"2012-04-04T06:39:39Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n> > diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n> > index 7aba497..bfeef21 100644\n> > --- a/Documentation/gitweb.conf.txt\n> > +++ b/Documentation/gitweb.conf.txt\n> > @@ -403,6 +403,9 @@ $default_projects_order::\n> > \ti.e. path to repository relative to `$projectroot`), \"descr\"\n> > \t(project description), \"owner\", and \"age\" (by date of most current\n> > \tcommit).\n> > +\n> > +$no_list_age::\n> > +\tOmit column with date of the most curren commit\n\n> s/curren/current/\n\n> Thanks for adding documentation, though I would prefer if you expanded\n> this description (for example including the information that it touches\n> projects list page).\n\nWhat about:\n\n$no_list_age::\n\tWhether to show the column with date of the most current commit on the\n\tprojects list page. It can save a bit of I/O.\n\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index a8b5fad..f42468c 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n> > # (only effective if this variable evaluates to true)\n> > our $export_ok = \"++GITWEB_EXPORT_OK++\";\n\n> > +# don't generate age column\n> > +our $no_list_age = 0;\n\n> \"age\" column where?\n\n> Hmmm... can't we come with a better name than $no_list_age?\n\nAny of $no_age_column, $omit_age_column, $no_last_commit would be better?\n\n\n> > @@ -5495,7 +5500,8 @@ sub git_project_list_body {\n> > \t 'tagfilter' => $tagfilter)\n> > \t\tif ($tagfilter || $search_regexp);\n> > \t# fill the rest\n> > -\t@projects = fill_project_list_info(\\@projects);\n> > +\tmy @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n> > +\t@projects = fill_project_list_info(\\@projects, @all_fields);\n\n> That looks quite strange on first glance. I know that empty list means\n> filling all fields, but the casual reader migh wonder about this\n> conditional expression.\n\n> Perhaps it would be better to write it this way:\n\n> -\t@projects = fill_project_list_info(\\@projects);\n> +\tmy @fields = qw(descr descr_long owner ctags category);\n> +\tpush @fields, 'age' unless ($no_list_age);\n> +\t@projects = fill_project_list_info(\\@projects, @fields);\n\n> or something like that.\n\n> Well, at least until we come up with a better way to specify \"all fields\n> except those specified\".\n\nYes, that's better. Especially that I would like also to introduce\noption to prevent printing repository owner everywhere.\n\n-- \n Kacper Kornet\n"},{"id":"188489","messageId":"201204041631.42905.jnareb@gmail.com","threadId":"30138","inReplyTo":"20120404063939.GA17024@camk.edu.pl","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-04T14:31:42Z","receivedAt":"2012-04-04T14:31:42Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 4 April 2012, Kacper Kornet wrote:\n> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n>> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n\n>>> diff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\n>>> index 7aba497..bfeef21 100644\n>>> --- a/Documentation/gitweb.conf.txt\n>>> +++ b/Documentation/gitweb.conf.txt\n>>> @@ -403,6 +403,9 @@ $default_projects_order::\n>>> \ti.e. path to repository relative to `$projectroot`), \"descr\"\n>>> \t(project description), \"owner\", and \"age\" (by date of most current\n>>> \tcommit).\n>>> +\n>>> +$no_list_age::\n>>> +\tOmit column with date of the most curren commit\n> \n>> s/curren/current/\n> \n>> Thanks for adding documentation, though I would prefer if you expanded\n>> this description (for example including the information that it touches\n>> projects list page).\n> \n> What about:\n> \n> $no_list_age::\n> \tWhether to show the column with date of the most current commit on the\n> \tprojects list page. It can save a bit of I/O.\n\nPerhaps it would be better to say it like this:\n\n $no_list_age::\n \tIf true, omit the column with date of the most current commit on the\n \tprojects list page. [...]\n\nIt is true that it can save a bit of I/O: the git_get_last_activity()\nexamines all branches (some of which are usually loose), and must hit\nthe object database, unpacking/getting commit objects to get at commit\ndate.\n\nBut the fact that it also saves a fork (a git command call) per repository\nreminds me of something which I missed in first round of review, namely\nthat generating 'age' and 'age_string' fields serve also as a check if\nrepository really exist.\n\nSo either we document this fact, or use some other way to verify that\ngit repository is valid.\n\n>>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n>>> index a8b5fad..f42468c 100755\n>>> --- a/gitweb/gitweb.perl\n>>> +++ b/gitweb/gitweb.perl\n>>> @@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n>>> # (only effective if this variable evaluates to true)\n>>> our $export_ok = \"++GITWEB_EXPORT_OK++\";\n> \n>>> +# don't generate age column\n>>> +our $no_list_age = 0;\n> \n>> \"age\" column where?\n> \n>> Hmmm... can't we come with a better name than $no_list_age?\n> \n> Any of $no_age_column, $omit_age_column, $no_last_commit would be better?\n\n$no_age_column seems better than $no_list_age... but see below. \n \n>>> @@ -5495,7 +5500,8 @@ sub git_project_list_body {\n>>> \t 'tagfilter' => $tagfilter)\n>>> \t\tif ($tagfilter || $search_regexp);\n>>> \t# fill the rest\n>>> -\t@projects = fill_project_list_info(\\@projects);\n>>> +\tmy @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n>>> +\t@projects = fill_project_list_info(\\@projects, @all_fields);\n> \n>> That looks quite strange on first glance. I know that empty list means\n>> filling all fields, but the casual reader migh wonder about this\n>> conditional expression.\n> \n>> Perhaps it would be better to write it this way:\n> \n>> -\t@projects = fill_project_list_info(\\@projects);\n>> +\tmy @fields = qw(descr descr_long owner ctags category);\n>> +\tpush @fields, 'age' unless ($no_list_age);\n>> +\t@projects = fill_project_list_info(\\@projects, @fields);\n> \n>> or something like that.\n> \n>> Well, at least until we come up with a better way to specify \"all fields\n>> except those specified\".\n> \n> Yes, that's better. Especially that I would like also to introduce\n> option to prevent printing repository owner everywhere.\n\nWell, because this patch affects gitweb configuration, and because we\nneed to preserve (as far as possible) the backward compatibility with\nexisting gitweb configuration files we need to be careful with changes.\n\nPerhaps instead of $no_age_column that can be single configuration\nvariable like @excluded_project_list_fields instead of one variable\nper column. Somebody might want to omit project description as well\n(though then project search must be limited to project names only).\nThough this approach will have problem that some of columns simply\nhave to be present... maybe one variable per column (perhaps hidden\nin a hash) is a better solution.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"188494","messageId":"20120404162208.GN10461@camk.edu.pl","threadId":"30138","inReplyTo":"201204041631.42905.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-04T16:22:08Z","receivedAt":"2012-04-04T16:22:08Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:\n> On Wed, 4 April 2012, Kacper Kornet wrote:\n> > On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n> >> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n> > What about:\n\n> > $no_list_age::\n> > \tWhether to show the column with date of the most current commit on the\n> > \tprojects list page. It can save a bit of I/O.\n\n> Perhaps it would be better to say it like this:\n\n> $no_list_age::\n> \tIf true, omit the column with date of the most current commit on the\n> \tprojects list page. [...]\n\n> It is true that it can save a bit of I/O: the git_get_last_activity()\n> examines all branches (some of which are usually loose), and must hit\n> the object database, unpacking/getting commit objects to get at commit\n> date.\n\n> But the fact that it also saves a fork (a git command call) per repository\n> reminds me of something which I missed in first round of review, namely\n> that generating 'age' and 'age_string' fields serve also as a check if\n> repository really exist.\n\n> So either we document this fact, or use some other way to verify that\n> git repository is valid.\n\nI think that git_project_list_body always works with the list returned\nby git_get_projects_list. And git_get_projects_list validates if the\npath is a git repository. So it should not be a problem. Please correct\nme, if I am wrong.\n\n\n\n> >>> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> >>> index a8b5fad..f42468c 100755\n> >>> --- a/gitweb/gitweb.perl\n> >>> +++ b/gitweb/gitweb.perl\n> >>> @@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n> >>> # (only effective if this variable evaluates to true)\n> >>> our $export_ok = \"++GITWEB_EXPORT_OK++\";\n\n> >>> +# don't generate age column\n> >>> +our $no_list_age = 0;\n\n> >> \"age\" column where?\n\n> >> Hmmm... can't we come with a better name than $no_list_age?\n\n> > Any of $no_age_column, $omit_age_column, $no_last_commit would be better?\n\n> $no_age_column seems better than $no_list_age... but see below. \n\n> >>> @@ -5495,7 +5500,8 @@ sub git_project_list_body {\n> >>> \t 'tagfilter' => $tagfilter)\n> >>> \t\tif ($tagfilter || $search_regexp);\n> >>> \t# fill the rest\n> >>> -\t@projects = fill_project_list_info(\\@projects);\n> >>> +\tmy @all_fields = $no_list_age ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n> >>> +\t@projects = fill_project_list_info(\\@projects, @all_fields);\n\n> >> That looks quite strange on first glance. I know that empty list means\n> >> filling all fields, but the casual reader migh wonder about this\n> >> conditional expression.\n\n> >> Perhaps it would be better to write it this way:\n\n> >> -\t@projects = fill_project_list_info(\\@projects);\n> >> +\tmy @fields = qw(descr descr_long owner ctags category);\n> >> +\tpush @fields, 'age' unless ($no_list_age);\n> >> +\t@projects = fill_project_list_info(\\@projects, @fields);\n\n> >> or something like that.\n\n> >> Well, at least until we come up with a better way to specify \"all fields\n> >> except those specified\".\n\n> > Yes, that's better. Especially that I would like also to introduce\n> > option to prevent printing repository owner everywhere.\n\n> Well, because this patch affects gitweb configuration, and because we\n> need to preserve (as far as possible) the backward compatibility with\n> existing gitweb configuration files we need to be careful with changes.\n\n> Perhaps instead of $no_age_column that can be single configuration\n> variable like @excluded_project_list_fields instead of one variable\n> per column. Somebody might want to omit project description as well\n> (though then project search must be limited to project names only).\n> Though this approach will have problem that some of columns simply\n> have to be present... maybe one variable per column (perhaps hidden\n> in a hash) is a better solution.\n\nI thought about two different variables as that would have a slightly\ndifferent functionality. While I want to get rid off Last Change from\nthe projects list page, I still want to get this information on pages of\nsingle repositories. On the other hand I don't want repository owner to\nbe shown anywhere.\n\n-- \n Kacper Kornet\n"},{"id":"188499","messageId":"7vlimba07b.fsf@alter.siamese.dyndns.org","threadId":"30138","inReplyTo":"201204041631.42905.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-04T17:14:32Z","receivedAt":"2012-04-04T17:14:32Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski writes:\n\n> Perhaps instead of $no_age_column that can be single configuration\n> variable like @excluded_project_list_fields instead of one variable\n> per column.\n\nTrue.\n\nIn general we avoid no_anything because it tends to introduce hard-to-read\ndouble negation in the code and the documentation (e.g. \"if (!$no_frotz)\"),\nbut \"next if (exists $exclude{$it})\" is probably not so bad.\n"},{"id":"189261","messageId":"201204141516.02719.jnareb@gmail.com","threadId":"30138","inReplyTo":"20120404162208.GN10461@camk.edu.pl","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-14T13:16:01Z","receivedAt":"2012-04-14T13:16:01Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I'm sorry for the delay answering.\n\nOn Wed, 4 Apr 2012, Kacper Kornet wrote:\n> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:\n>> On Wed, 4 April 2012, Kacper Kornet wrote:\n>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n>>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n \n>> Perhaps it would be better to say it like this:\n>> \n>> $no_list_age::\n>> \tIf true, omit the column with date of the most current commit on the\n>> \tprojects list page. [...]\n>> \n>> It is true that it can save a bit of I/O: the git_get_last_activity()\n>> examines all branches (some of which are usually loose), and must hit\n>> the object database, unpacking/getting commit objects to get at commit\n>> date.\n>> \n>> But the fact that it also saves a fork (a git command call) per repository\n>> reminds me of something which I missed in first round of review, namely\n>> that generating 'age' and 'age_string' fields serve also as a check if\n>> repository really exist.\n>> \n>> So either we document this fact, or use some other way to verify that\n>> git repository is valid.\n> \n> I think that git_project_list_body always works with the list returned\n> by git_get_projects_list. And git_get_projects_list validates if the\n> path is a git repository. So it should not be a problem. Please correct\n> me, if I am wrong.\n \nIf $projects_list points to plain file, git_get_projects_list() just\ngets list of projects (and project owners) from $projects_list file\nby reading and parsing this file. No verification that repository\nexists is done.\n\nIf $projects_list points to directory (which is the default), then\ngit_get_projects_list() scans starting from $projects_list directory\nfor somtehing that _looks like_ git repository with check_head_link()\nvia check_export_ok(). But you still can encounter something that\nlooks like git repository (has \"HEAD\" file in it), but isn't.\n\n\nSo we would probably want to have said variable or set of variables\ndescribe three states:\n\n* find date of last change in repository with git-for-each-ref called\n by git_get_last_activity(), which as a side effect verifies that\n repository actually exist. \n\n git_get_last_activity() returns empty list in list context if repo\n does not exist, hence\n\n \tmy (@activity) = git_get_last_activity($pr->{'path'});\n \tunless (@activity) {\n \t\tnext PROJECT;\n \t}\n\n* verify that repository exists with \"git rev-parse --git-dir\" or\n \"git rev-parse --verify HEAD\", or \"git symbolic-ref HEAD\", redirecting\n stderr to /dev/null (we would probably want to save output of the\n latter two somewhere to use it later).\n \n That saves I/O, but not fork.\n\n* don't verify that repository exists.\n\n\nThough perhaps the last possibility isn't a good idea, so it would be\nleft two-state, as in your patch. \n\n>>> [...]. Especially that I would like also to introduce\n>>> option to prevent printing repository owner everywhere.\n>>> \n>> Well, because this patch affects gitweb configuration, and because we\n>> need to preserve (as far as possible) the backward compatibility with\n>> existing gitweb configuration files we need to be careful with changes.\n>> \n>> Perhaps instead of $no_age_column that can be single configuration\n>> variable like @excluded_project_list_fields instead of one variable\n>> per column. Somebody might want to omit project description as well\n>> (though then project search must be limited to project names only).\n>> Though this approach will have problem that some of columns simply\n>> have to be present... maybe one variable per column (perhaps hidden\n>> in a hash) is a better solution.\n> \n> I thought about two different variables as that would have a slightly\n> different functionality. While I want to get rid off Last Change from\n> the projects list page, I still want to get this information on pages of\n> single repositories. On the other hand I don't want repository owner to\n> be shown anywhere.\n\nAh, in this case we want to keep $dont_show_repo_age / $no_age_column\n(or something like that) separate from $no_owner... and in this case\nthese features can be controlled by separate scalar configuration\nvariables.\n\n\n-- \nJakub Narebski\nPoland\n"},{"id":"189377","messageId":"20120416101242.GK17753@camk.edu.pl","threadId":"30138","inReplyTo":"201204141516.02719.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-16T10:12:42Z","receivedAt":"2012-04-16T10:12:42Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:\n> On Wed, 4 Apr 2012, Kacper Kornet wrote:\n> > On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:\n> >> On Wed, 4 April 2012, Kacper Kornet wrote:\n> >>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n> >>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n\n> >> Perhaps it would be better to say it like this:\n\n> >> $no_list_age::\n> >> \tIf true, omit the column with date of the most current commit on the\n> >> \tprojects list page. [...]\n\n> >> It is true that it can save a bit of I/O: the git_get_last_activity()\n> >> examines all branches (some of which are usually loose), and must hit\n> >> the object database, unpacking/getting commit objects to get at commit\n> >> date.\n\n> >> But the fact that it also saves a fork (a git command call) per repository\n> >> reminds me of something which I missed in first round of review, namely\n> >> that generating 'age' and 'age_string' fields serve also as a check if\n> >> repository really exist.\n\n> >> So either we document this fact, or use some other way to verify that\n> >> git repository is valid.\n\n> > I think that git_project_list_body always works with the list returned\n> > by git_get_projects_list. And git_get_projects_list validates if the\n> > path is a git repository. So it should not be a problem. Please correct\n> > me, if I am wrong.\n\n> If $projects_list points to plain file, git_get_projects_list() just\n> gets list of projects (and project owners) from $projects_list file\n> by reading and parsing this file. No verification that repository\n> exists is done.\n\nI think that even in this case check_export_ok is called. But there is\nstill the problem you have mentioned below.\n\n> If $projects_list points to directory (which is the default), then\n> git_get_projects_list() scans starting from $projects_list directory\n> for somtehing that _looks like_ git repository with check_head_link()\n> via check_export_ok(). But you still can encounter something that\n> looks like git repository (has \"HEAD\" file in it), but isn't.\n\n\n> So we would probably want to have said variable or set of variables\n> describe three states:\n\n> * find date of last change in repository with git-for-each-ref called\n> by git_get_last_activity(), which as a side effect verifies that\n> repository actually exist. \n\n> git_get_last_activity() returns empty list in list context if repo\n> does not exist, hence\n\n> \tmy (@activity) = git_get_last_activity($pr->{'path'});\n> \tunless (@activity) {\n> \t\tnext PROJECT;\n> \t}\n\n> * verify that repository exists with \"git rev-parse --git-dir\" or\n> \"git rev-parse --verify HEAD\", or \"git symbolic-ref HEAD\", redirecting\n> stderr to /dev/null (we would probably want to save output of the\n> latter two somewhere to use it later).\n\n> That saves I/O, but not fork.\n\n> * don't verify that repository exists.\n\n> Though perhaps the last possibility isn't a good idea, so it would be\n> left two-state, as in your patch. \n\nMy tests show that forks are also a bottleneck in my setup. On the other\nhand I think that I can trust that my projects.list contains only valid\nrepositories. So I would prefer to have a don't verify option. Or there\nis a possibility to write perl function with the same functionality as\nis_git_directory() from setup.c and use it to verify if the directory is a\nvalid git repo.\n\n-- \n Kacper Kornet\n"},{"id":"189452","messageId":"201204162206.50631.jnareb@gmail.com","threadId":"30138","inReplyTo":"20120416101242.GK17753@camk.edu.pl","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-16T20:06:49Z","receivedAt":"2012-04-16T20:06:49Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 16 Apr 2012, Kacper Kornet wrote:\n> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:\n>> On Wed, 4 Apr 2012, Kacper Kornet wrote:\n>>> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:\n>>>> On Wed, 4 April 2012, Kacper Kornet wrote:\n>>>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n>>>>>> On Tue, 3 Apr 2012, Kacper Kornet wrote:\n[...] \n>>>> But the fact that it also saves a fork (a git command call) per repository\n>>>> reminds me of something which I missed in first round of review, namely\n>>>> that generating 'age' and 'age_string' fields serve also as a check if\n>>>> repository really exist.\n>>>> \n>>>> So either we document this fact, or use some other way to verify that\n>>>> git repository is valid.\n>>> \n>>> I think that git_project_list_body always works with the list returned\n>>> by git_get_projects_list. And git_get_projects_list validates if the\n>>> path is a git repository. So it should not be a problem. Please correct\n>>> me, if I am wrong.\n> \n>> If $projects_list points to plain file, git_get_projects_list() just\n>> gets list of projects (and project owners) from $projects_list file\n>> by reading and parsing this file. No verification that repository\n>> exists is done.\n> \n> I think that even in this case check_export_ok is called. But there is\n> still the problem you have mentioned below.\n\nTrue, somehow I missed the fact that check_export_ok() is called to\ncheck if it looks like repository and if it is to be exported in both\ncases ($projects_list a directory or a file).\n\n>> If $projects_list points to directory (which is the default), then\n>> git_get_projects_list() scans starting from $projects_list directory\n>> for somtehing that _looks like_ git repository with check_head_link()\n>> via check_export_ok(). But you still can encounter something that\n>> looks like git repository (has \"HEAD\" file in it), but isn't.\n>> \n>> So we would probably want to have said variable or set of variables\n>> describe three states:\n> \n>> * find date of last change in repository with git-for-each-ref called\n>> by git_get_last_activity(), which as a side effect verifies that\n>> repository actually exist. \n>> \n>> git_get_last_activity() returns empty list in list context if repo\n>> does not exist, hence\n>> \n>> \tmy (@activity) = git_get_last_activity($pr->{'path'});\n>> \tunless (@activity) {\n>> \t\tnext PROJECT;\n>> \t}\n>> \n>> * verify that repository exists with \"git rev-parse --git-dir\" or\n>> \"git rev-parse --verify HEAD\", or \"git symbolic-ref HEAD\", redirecting\n>> stderr to /dev/null (we would probably want to save output of the\n>> latter two somewhere to use it later).\n>> \n>> That saves I/O, but not fork.\n\nActually if you look at the footer of projects list page with 'timed'\nfeature enable you see that for N projects on list, gitweb uses 2*N+1\ngit commands. The \"+1\" part is from \"git version\", the \"2*N\" are from\ngit-for-each-ref to get last activity (and verify repository as a\nside-effect)...\n\n...and from call to \"git config\" to get owner (unconditional check for\n`gitweb.owner` override), description (if '.git/description' file got\ndeleted), if applicable category (file then config), if applicable ctags\n(file(s) then config).\n\nSo we can rely on \"git config\" being called, no need for separate\nverification. My mistake. (Though it might be hard to use this fact.)\n\n\nWell, with proposed option to remove 'owner' field we would have sometimes\nto verify repository with an extra git command...\n\n>> * don't verify that repository exists.\n> \n>> Though perhaps the last possibility isn't a good idea, so it would be\n>> left two-state, as in your patch. \n> \n> My tests show that forks are also a bottleneck in my setup.\n\nWhat do you mean by \"my tests\" here? Is it benchmark (perhaps just using\n'timed' feature) with and without custom change removing fork(s)? Or did\nyou used profiler (e.g. wondefull Devel::NYTProf) for that?\n\n> On the other \n> hand I think that I can trust that my projects.list contains only valid\n> repositories. So I would prefer to have a don't verify option. Or there\n> is a possibility to write perl function with the same functionality as\n> is_git_directory() from setup.c and use it to verify if the directory is a\n> valid git repo.\n\nWell, we can add those checks to check_export_ok()... well to function\nit would call.\n\nis_git_repository from setup.c checks that \"/objects\" and \"/refs\"\nhave executable permissions, and that \"/HEAD\" is valid via validate_headref\nwhich does slightly more than (now slightly misnamed) check_head_link()\nfrom gitweb...\n\n...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable\nis set, and path that it points to has executable permissions. I don't\nthink we have to worry about this for gitweb.\n\n\nI'll try to come up with a patch to gitweb that improves repository\nverification, and perhaps a patch that uses the fact that \"git config\"\nsucceeded to verify repository.\n\n-- \nJakub Narębski\n"},{"id":"189468","messageId":"20120416213938.GB22574@camk.edu.pl","threadId":"30138","inReplyTo":"201204162206.50631.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-16T21:39:38Z","receivedAt":"2012-04-16T21:39:38Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"I'm sorry to Jakub for duplicate message, but I have forgotten CC: to\nthe group.\n\nOn Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:\n> On Mon, 16 Apr 2012, Kacper Kornet wrote:\n> > On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:\n> >> On Wed, 4 Apr 2012, Kacper Kornet wrote:\n> >>> On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:\n> >>>> On Wed, 4 April 2012, Kacper Kornet wrote:\n> >>>>> On Wed, Apr 04, 2012 at 01:12:01AM +0200, Jakub Narebski wrote:\n> >> So we would probably want to have said variable or set of variables\n> >> describe three states:\n\n> >> * find date of last change in repository with git-for-each-ref called\n> >> by git_get_last_activity(), which as a side effect verifies that\n> >> repository actually exist. \n\n> >> git_get_last_activity() returns empty list in list context if repo\n> >> does not exist, hence\n\n> >> \tmy (@activity) = git_get_last_activity($pr->{'path'});\n> >> \tunless (@activity) {\n> >> \t\tnext PROJECT;\n> >> \t}\n\n> >> * verify that repository exists with \"git rev-parse --git-dir\" or\n> >> \"git rev-parse --verify HEAD\", or \"git symbolic-ref HEAD\", redirecting\n> >> stderr to /dev/null (we would probably want to save output of the\n> >> latter two somewhere to use it later).\n\n> >> That saves I/O, but not fork.\n\n> Actually if you look at the footer of projects list page with 'timed'\n> feature enable you see that for N projects on list, gitweb uses 2*N+1\n> git commands. The \"+1\" part is from \"git version\", the \"2*N\" are from\n> git-for-each-ref to get last activity (and verify repository as a\n> side-effect)...\n\nIt is how I started to think about the problem. With my additional patch\nto remove the owner I am able to reduce the number of git invocations to\n1.\n\n> ...and from call to \"git config\" to get owner (unconditional check for\n> `gitweb.owner` override), description (if '.git/description' file got\n> deleted), if applicable category (file then config), if applicable ctags\n> (file(s) then config).\n\n> So we can rely on \"git config\" being called, no need for separate\n> verification. My mistake. (Though it might be hard to use this fact.)\n\n\n> Well, with proposed option to remove 'owner' field we would have sometimes\n> to verify repository with an extra git command...\n\n> >> * don't verify that repository exists.\n\n> >> Though perhaps the last possibility isn't a good idea, so it would be\n> >> left two-state, as in your patch. \n\n> > My tests show that forks are also a bottleneck in my setup.\n\n> What do you mean by \"my tests\" here? Is it benchmark (perhaps just using\n> 'timed' feature) with and without custom change removing fork(s)? Or did\n> you used profiler (e.g. wondefull Devel::NYTProf) for that?\n\nNothing fancy. I look at the footnote produced by \"timed\" feature. And\nI see a difference between version with the following patch:\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 18cdf96..4a13807 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3156,6 +3156,18 @@ sub git_get_project_owner {\n \treturn $owner;\n }\n \n+sub git_repo_exist {\n+\tmy ($path) = @_;\n+ my $fd;\n+\n+ $git_dir = \"$projectroot/$path\";\n+ open($fd, \"<\", \"$git_dir/HEAD\") or return;\n+ my $line = <$fd>;\n+ close $fd or return;\n+ return 1 if (defined $fd && substr($line, 0, 10) eq 'ref:refs/' \n+ || $line=~m/^[0-9a-z]{40}$/);\n+ return 0;\n+}\n+\n sub git_get_last_activity {\n \tmy ($path) = @_;\n \tmy $fd;\n@@ -5330,6 +5342,7 @@ sub fill_project_list_info {\n \tmy $show_ctags = gitweb_check_feature('ctags');\n PROJECT:\n \tforeach my $pr (@$projlist) {\n+ next PROJECT unless git_repo_exist($pr->{'path'});\n \t\tif (project_info_needs_filling($pr, $filter_set->('age', 'age_string'))) {\n \t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n \t\t\tunless (@activity) {\n\n\nand the one in which git_repo_exist() uses invocation to /bin/true:\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 18cdf96..4bcc66f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -3156,6 +3156,13 @@ sub git_get_project_owner {\n \treturn $owner;\n }\n \n+sub git_repo_exist {\n+\tmy ($path) = @_;\n+\n+ $git_dir = \"$projectroot/$path\";\n+ return not system('/bin/true');\n+}\n+\n sub git_get_last_activity {\n \tmy ($path) = @_;\n \tmy $fd;\n\n\n> > On the other \n> > hand I think that I can trust that my projects.list contains only valid\n> > repositories. So I would prefer to have a don't verify option. Or there\n> > is a possibility to write perl function with the same functionality as\n> > is_git_directory() from setup.c and use it to verify if the directory is a\n> > valid git repo.\n\n> Well, we can add those checks to check_export_ok()... well to function\n> it would call.\n\n> is_git_repository from setup.c checks that \"/objects\" and \"/refs\"\n> have executable permissions, and that \"/HEAD\" is valid via validate_headref\n> which does slightly more than (now slightly misnamed) check_head_link()\n> from gitweb...\n\n> ...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable\n> is set, and path that it points to has executable permissions. I don't\n> think we have to worry about this for gitweb.\n\n> I'll try to come up with a patch to gitweb that improves repository\n> verification, and perhaps a patch that uses the fact that \"git config\"\n> succeeded to verify repository.\n\nAs you see it is more or less what I have already written for my tests.\nI only don't check if /objects and /refs are directories. If you want I\ncan send proper patch submission for this function\n\n-- \n Kacper Kornet\n\n\n-- \n Kacper Kornet\n"},{"id":"189589","messageId":"201204180136.08570.jnareb@gmail.com","threadId":"30138","inReplyTo":"20120416213938.GB22574@camk.edu.pl","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-17T23:36:08Z","receivedAt":"2012-04-17T23:36:08Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Mon, 16 Apr 2012, Kacper Kornet wrote:\n> On Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:\n>> On Mon, 16 Apr 2012, Kacper Kornet wrote:\n>>> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:\n\n>>>> That saves I/O, but not fork.\n>> \n>> Actually if you look at the footer of projects list page with 'timed'\n>> feature enable you see that for N projects on list, gitweb uses 2*N+1\n>> git commands. The \"+1\" part is from \"git version\", the \"2*N\" are from\n>> git-for-each-ref to get last activity (and verify repository as a\n>> side-effect)...\n> \n> It is how I started to think about the problem. With my additional patch\n> to remove the owner I am able to reduce the number of git invocations to\n> 1.\n\nThat is a very good thing.\n\nEspecially that adding caching to gitweb is long in coming (to core \ngitweb at least), and that not always one is able to turn on caching.\n\n[...]\n>>> My tests show that forks are also a bottleneck in my setup.\n>> \n>> What do you mean by \"my tests\" here? Is it benchmark (perhaps just using\n>> 'timed' feature) with and without custom change removing fork(s)? Or did\n>> you used profiler (e.g. wondefull Devel::NYTProf) for that?\n> \n> Nothing fancy. I look at the footnote produced by \"timed\" feature. And\n> I see a difference between version with the following patch:\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 18cdf96..4a13807 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3156,6 +3156,18 @@ sub git_get_project_owner {\n> \treturn $owner;\n> }\n> \n> +sub git_repo_exist {\n\nPerhaps a better name would be validate_headref() or check_head(),\ncalled from subroutine named either is_git_directory() as in setup.c,\nor verify_repo()...\n\n> +\tmy ($path) = @_;\n\nBTW this could be written as\n\n +\tmy $path = shift;\n\nthough it is largely a matter of taste.\n\n> + my $fd;\n> +\n> + $git_dir = \"$projectroot/$path\";\n> + open($fd, \"<\", \"$git_dir/HEAD\") or return;\n\nIt can be written as\n\n +\topen my $fd, \"<\", \"$projectroot/$path/HEAD\"\n +\t\tor return;\n\n> + my $line = <$fd>;\n> + close $fd or return;\n\nShouldn't we chomp($line)?\n\n> + return 1 if (defined $fd && substr($line, 0, 10) eq 'ref:refs/' \n> + || $line=~m/^[0-9a-z]{40}$/);\n> + return 0;\n\nI don't think we need to check that $fd is defined; if it isn't, we would\nreturn earlier I think.\n\nThere can be space between \"ref:\" and fully qualified branch name, and in\nfact git puts such space:\n\n $ cat .git/HEAD \n ref: refs/heads/gitweb/web\n\nAlso, you can return boolean value.\n\nSo the above would reduce to:\n\n +\treturn ($line =~ m!^ref:\\s*refs/! ||\n +\t $line =~ m!^[0-9a-z]{40}$!);\n\n> +}\n> +\n> sub git_get_last_activity {\n> \tmy ($path) = @_;\n> \tmy $fd;\n> @@ -5330,6 +5342,7 @@ sub fill_project_list_info {\n> \tmy $show_ctags = gitweb_check_feature('ctags');\n> PROJECT:\n> \tforeach my $pr (@$projlist) {\n> + next PROJECT unless git_repo_exist($pr->{'path'});\n\nI understand that it is proof of concept patch, but I think that\nin real patch iy would be better to update check_export_ok() instead\nof this addition.\n\n> \t\tif (project_info_needs_filling($pr, $filter_set->('age', 'age_string'))) {\n> \t\t\tmy (@activity) = git_get_last_activity($pr->{'path'});\n> \t\t\tunless (@activity) {\n> \n> \n> and the one in which git_repo_exist() uses invocation to /bin/true:\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 18cdf96..4bcc66f 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -3156,6 +3156,13 @@ sub git_get_project_owner {\n> \treturn $owner;\n> }\n> \n> +sub git_repo_exist {\n> +\tmy ($path) = @_;\n> +\n> + $git_dir = \"$projectroot/$path\";\n> + return not system('/bin/true');\n> +}\n> +\n\nWhat were the differences in timing?\n\n>>> On the other \n>>> hand I think that I can trust that my projects.list contains only valid\n>>> repositories. So I would prefer to have a don't verify option. Or there\n>>> is a possibility to write perl function with the same functionality as\n>>> is_git_directory() from setup.c and use it to verify if the directory is a\n>>> valid git repo.\n>> \n>> Well, we can add those checks to check_export_ok()... well to function\n>> it would call.\n>> \n>> is_git_repository from setup.c checks that \"/objects\" and \"/refs\"\n>> have executable permissions, and that \"/HEAD\" is valid via validate_headref\n>> which does slightly more than (now slightly misnamed) check_head_link()\n>> from gitweb...\n>> \n>> ...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable\n>> is set, and path that it points to has executable permissions. I don't\n>> think we have to worry about this for gitweb.\n> \n>> I'll try to come up with a patch to gitweb that improves repository\n>> verification, and perhaps a patch that uses the fact that \"git config\"\n>> succeeded to verify repository.\n> \n> As you see it is more or less what I have already written for my tests.\n> I only don't check if /objects and /refs are directories. If you want I\n> can send proper patch submission for this function\n\nI don't know how strict we should be, but \"/objects\" and \"/refs\" are just\none stat more.\n\n\nAnyway, if you plan on resending this patch series, then \"gitweb: Improve\nrepository verification\" should be be first, I think.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"189709","messageId":"201204191807.32410.jnareb@gmail.com","threadId":"30138","inReplyTo":"201204180136.08570.jnareb@gmail.com","subject":"[PATCH] gitweb: Improve repository verification","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-19T16:07:31Z","receivedAt":"2012-04-19T16:07:31Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Bring repository verification in check_export_ok() to standards of\nis_git_directory function from setup.c (core git), and validate_headref()\nto standards of the same function in path.c,... and a bit more.\n\nvalidate_headref() replaces check_head_link(); note that the former\nrequires path to HEAD file, while the late latter path to repository.\n\nIssues of note:\n* is_git_directory() in gitweb is a bit stricter: it checks that\n \"/objects\" and \"/refs\" are directories, and not only 'executable'\n permission,\n* validate_headref() in gitweb is a bit stricter: it checks that\n reference symlink or symref points to starts with \"refs/heads/\",\n and not only with \"refs/\",\n* calls to check_head_link(), all of which were meant to check if\n given directory can be a git repository, were replaced by newly\n introduced is_git_directory().\n\nThis change is preparation for removing \"Last change\" column from list\nof projects, which is currently used also for validating repository.\n\nSuggested-by: Kacper Kornet \nSigned-off-by: Jakub Narebski \n---\nHere is how such first step could look like...\n\n gitweb/gitweb.perl | 52 ++++++++++++++++++++++++++++++++++++++++++----------\n 1 files changed, 42 insertions(+), 10 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 098e527..767d7a5 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -621,19 +621,51 @@ sub feature_avatar {\n \treturn @val ? @val : @_;\n }\n \n-# checking HEAD file with -e is fragile if the repository was\n-# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n-# and then pruned.\n-sub check_head_link {\n-\tmy ($dir) = @_;\n-\tmy $headfile = \"$dir/HEAD\";\n-\treturn ((-e $headfile) ||\n-\t\t(-l $headfile && readlink($headfile) =~ /^refs\\/heads\\//));\n+# Test if it looks like we're at a git directory.\n+# We want to see:\n+#\n+# - an objects/ directory,\n+# - a refs/ directory,\n+# - either a HEAD symlink or a HEAD file that is formatted as\n+# a proper \"ref:\", or a regular file HEAD that has a properly\n+# formatted sha1 object name.\n+#\n+# See is_git_directory() in setup.c\n+sub is_git_directory {\n+\tmy $dir = shift;\n+\treturn\n+\t\t-x \"$dir/objects\" && -d _ &&\n+\t\t-x \"$dir/refs\" && -d _ &&\n+\t\tvalidate_headref(\"$dir/HEAD\");\n+}\n+\n+# Check HEAD file, that it is either\n+#\n+# - a \"refs/heads/..\" symlink, or\n+# - a symbolic ref to \"refs/heads/..\", or\n+# - a detached HEAD.\n+#\n+# See validate_headref() in path.c\n+sub validate_headref {\n+\tmy $headfile = shift;\n+\tif (-l $headfile) {\n+\t\treturn readlink($headfile) =~ m!^refs/heads/!;\n+\n+\t} elsif (-e _) {\n+\t\topen my $fh, '<', $headfile or return;\n+\t\tmy $line = <$fh>;\n+\t\tclose $fh or return;\n+\n+\t\treturn\n+\t\t\t$line =~ m!^ref:\\s*refs/heads/! || # symref\n+\t\t\t$line =~ m!^[0-9a-z]{40}$!i; # detached HEAD\n+\t}\n+\treturn;\n }\n \n sub check_export_ok {\n \tmy ($dir) = @_;\n-\treturn (check_head_link($dir) &&\n+\treturn (is_git_directory($dir) &&\n \t\t(!$export_ok || -e \"$dir/$export_ok\") &&\n \t\t(!$export_auth_hook || $export_auth_hook->($dir)));\n }\n@@ -842,7 +874,7 @@ sub evaluate_path_info {\n \t# find which part of PATH_INFO is project\n \tmy $project = $path_info;\n \t$project =~ s,/+$,,;\n-\twhile ($project && !check_head_link(\"$projectroot/$project\")) {\n+\twhile ($project && !is_git_directory(\"$projectroot/$project\")) {\n \t\t$project =~ s,/*[^/]*$,,;\n \t}\n \treturn unless $project;\n-- \n1.7.9\n"},{"id":"189717","messageId":"xmqq397zwp4c.fsf@junio.mtv.corp.google.com","threadId":"30138","inReplyTo":"201204191807.32410.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Improve repository verification","fromName":"Junio C Hamano","fromEmail":"jch@google.com","sentAt":"2012-04-19T18:30:43Z","receivedAt":"2012-04-19T18:30:43Z","isPatch":true,"sender":{"key":"jch@google.com","avatar":null},"body":"Jakub Narebski writes:\n\n> Bring repository verification in check_export_ok() to standards of\n> is_git_directory function from setup.c (core git), and validate_headref()\n> to standards of the same function in path.c,... and a bit more.\n>\n> validate_headref() replaces check_head_link(); note that the former\n> requires path to HEAD file, while the late latter path to repository.\n>\n> Issues of note:\n> * is_git_directory() in gitweb is a bit stricter: it checks that\n> \"/objects\" and \"/refs\" are directories, and not only 'executable'\n> permission,\n> * validate_headref() in gitweb is a bit stricter: it checks that\n> reference symlink or symref points to starts with \"refs/heads/\",\n> and not only with \"refs/\",\n> * calls to check_head_link(), all of which were meant to check if\n> given directory can be a git repository, were replaced by newly\n> introduced is_git_directory().\n>\n> This change is preparation for removing \"Last change\" column from list\n> of projects, which is currently used also for validating repository.\n>\n> Suggested-by: Kacper Kornet \n> Signed-off-by: Jakub Narebski \n> ---\n> Here is how such first step could look like...\n\nDo you mean by \"could look like\" that this is still an RFC, or is this\nsomething we want to apply and see how well it makes people's lives in\nthe field?\n\nBy the way, I wonder (1) if it is worth adding support for the textual\n\".git\" file that contains \"gitdir: $path\", and (2) if so how big a\nchange would we need to do so.\n\n> gitweb/gitweb.perl | 52 ++++++++++++++++++++++++++++++++++++++++++----------\n> 1 files changed, 42 insertions(+), 10 deletions(-)\n>\n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index 098e527..767d7a5 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -621,19 +621,51 @@ sub feature_avatar {\n> \treturn @val ? @val : @_;\n> }\n> \n> -# checking HEAD file with -e is fragile if the repository was\n> -# initialized long time ago (i.e. symlink HEAD) and was pack-ref'ed\n> -# and then pruned.\n> -sub check_head_link {\n> -\tmy ($dir) = @_;\n> -\tmy $headfile = \"$dir/HEAD\";\n> -\treturn ((-e $headfile) ||\n> -\t\t(-l $headfile && readlink($headfile) =~ /^refs\\/heads\\//));\n> +# Test if it looks like we're at a git directory.\n> +# We want to see:\n> +#\n> +# - an objects/ directory,\n> +# - a refs/ directory,\n> +# - either a HEAD symlink or a HEAD file that is formatted as\n> +# a proper \"ref:\", or a regular file HEAD that has a properly\n> +# formatted sha1 object name.\n> +#\n> +# See is_git_directory() in setup.c\n> +sub is_git_directory {\n> +\tmy $dir = shift;\n> +\treturn\n> +\t\t-x \"$dir/objects\" && -d _ &&\n> +\t\t-x \"$dir/refs\" && -d _ &&\n> +\t\tvalidate_headref(\"$dir/HEAD\");\n> +}\n> +\n> +# Check HEAD file, that it is either\n> +#\n> +# - a \"refs/heads/..\" symlink, or\n> +# - a symbolic ref to \"refs/heads/..\", or\n> +# - a detached HEAD.\n> +#\n> +# See validate_headref() in path.c\n> +sub validate_headref {\n> +\tmy $headfile = shift;\n> +\tif (-l $headfile) {\n> +\t\treturn readlink($headfile) =~ m!^refs/heads/!;\n> +\n> +\t} elsif (-e _) {\n> +\t\topen my $fh, '<', $headfile or return;\n> +\t\tmy $line = <$fh>;\n> +\t\tclose $fh or return;\n> +\n> +\t\treturn\n> +\t\t\t$line =~ m!^ref:\\s*refs/heads/! || # symref\n> +\t\t\t$line =~ m!^[0-9a-z]{40}$!i; # detached HEAD\n> +\t}\n> +\treturn;\n> }\n> \n> sub check_export_ok {\n> \tmy ($dir) = @_;\n> -\treturn (check_head_link($dir) &&\n> +\treturn (is_git_directory($dir) &&\n> \t\t(!$export_ok || -e \"$dir/$export_ok\") &&\n> \t\t(!$export_auth_hook || $export_auth_hook->($dir)));\n> }\n> @@ -842,7 +874,7 @@ sub evaluate_path_info {\n> \t# find which part of PATH_INFO is project\n> \tmy $project = $path_info;\n> \t$project =~ s,/+$,,;\n> -\twhile ($project && !check_head_link(\"$projectroot/$project\")) {\n> +\twhile ($project && !is_git_directory(\"$projectroot/$project\")) {\n> \t\t$project =~ s,/*[^/]*$,,;\n> \t}\n> \treturn unless $project;\n"},{"id":"189723","messageId":"201204192146.09853.jnareb@gmail.com","threadId":"30138","inReplyTo":"xmqq397zwp4c.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH] gitweb: Improve repository verification","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-19T19:46:09Z","receivedAt":"2012-04-19T19:46:09Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 19 April 2012, Junio C Hamano wrote:\n> Jakub Narebski writes:\n> \n> > Bring repository verification in check_export_ok() to standards of\n> > is_git_directory function from setup.c (core git), and validate_headref()\n> > to standards of the same function in path.c,... and a bit more.\n> >\n> > validate_headref() replaces check_head_link(); note that the former\n> > requires path to HEAD file, while the late latter path to repository.\n> >\n> > Issues of note:\n> > * is_git_directory() in gitweb is a bit stricter: it checks that\n> > \"/objects\" and \"/refs\" are directories, and not only 'executable'\n> > permission,\n> > * validate_headref() in gitweb is a bit stricter: it checks that\n> > reference symlink or symref points to starts with \"refs/heads/\",\n> > and not only with \"refs/\",\n> > * calls to check_head_link(), all of which were meant to check if\n> > given directory can be a git repository, were replaced by newly\n> > introduced is_git_directory().\n> >\n> > This change is preparation for removing \"Last change\" column from list\n> > of projects, which is currently used also for validating repository.\n> >\n> > Suggested-by: Kacper Kornet \n> > Signed-off-by: Jakub Narebski \n> > ---\n> > Here is how such first step could look like...\n> \n> Do you mean by \"could look like\" that this is still an RFC, or is this\n> something we want to apply and see how well it makes people's lives in\n> the field?\n\n\"Here is how such first step could look like\" was directed to Kacper... :-)\n\nKacper Kornet (who started this thread with \"[PATCH] gitweb: Option\nto omit column with time of the last change\") wants to have an option\nto remove \"Last Change\" column from projects list page, and \"Owner\"\ncolumn and field from all gitweb views. This will allow to generate\nprojects list page with 1 call to git command rather than 2*N+1, where\nN is number of repositories shown...\n\n...but we use the fact that \"git --git-dir=$GIT_DIR for-each-ref ...\"\nsucceed or fails to verify that given path points to git repository.\nThat is why I proposed this commit to be first patch in hopefully\nupcoming Kacper's new version of patch series.\n\nBut in current gitweb (without Kacper's planned patches) this change\ndoesn't bring much, as git repositories are verified outside of\nis_git_directory() check... well, perhaps with exception of possible\ncorner case when one is using path_info gitweb URL...\n \n> By the way, I wonder (1) if it is worth adding support for the textual\n> \".git\" file that contains \"gitdir: $path\", and (2) if so how big a\n> change would we need to do so.\n\nI don't think that it would be big changeto add support for \"gitlink\"\nfiles, assuming that 'git --git-dir= ...' works correctly.\nI would put that addition in a separate commit, though.\n\nBTW. does core git limit number of redirections, or have some loop\ndetection?\n-- \nJakub Narebski\nPoland\n"},{"id":"189805","messageId":"201204211328.25162.jnareb@gmail.com","threadId":"30138","inReplyTo":"201204192146.09853.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Improve repository verification","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-04-21T11:28:24Z","receivedAt":"2012-04-21T11:28:24Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 19 April 2012, Jakub Narebski wrote:\n> On Thu, 19 April 2012, Junio C Hamano wrote:\n\n> > By the way, I wonder (1) if it is worth adding support for the textual\n> > \".git\" file that contains \"gitdir: $path\", and (2) if so how big a\n> > change would we need to do so.\n> \n> I don't think that it would be big change to add support for \"gitlink\"\n> files, assuming that 'git --git-dir= ...' works correctly.\n> I would put that addition in a separate commit, though.\n\nWell, I actually tried to write such commit, adding support for\n'gitlink' files, and it turned out to be harder than I thought.\nThe problem that stumped me for now is that gitweb tries to read\nmany files inside git repository ('export-ok', 'description',\n'cloneurl', 'category', etc.), allof which must be redirected to\nreal git directory.\n\nI still think it is doable, but I wonder if it is worth it...\n\nBelow there is work in progress patch, which doesn't use resolve_gitdir\nyet, and without any tests.\n\n-- >8 ---------- >8 --\nSubject: [PATCH] gitweb: Add support for \"gitdir: \" gitfile\n\nSuggested-by: Junio C Hamano \nSigned-off-by: Jakub Narebski \n---\n gitweb/gitweb.perl | 54 ++++++++++++++++++++++++++++++++++++++++++++++++---\n 1 files changed, 50 insertions(+), 4 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 767d7a5..8d70a0a 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -639,6 +639,52 @@ sub is_git_directory {\n \t\tvalidate_headref(\"$dir/HEAD\");\n }\n \n+# Try to read the location of the git directory from the .git file,\n+# return path to git directory if found.\n+#\n+# See read_gitfile in setup.c\n+sub read_gitfile {\n+\tmy $path = shift;\n+\t# note: the \"basename eq '.git'\" check isn't in setup.c version\n+\treturn unless ($path =~ m!(?:^|/)\\.git/*$! && -f $path);\n+\n+\topen my $fh, '<', $path or return;\n+\tmy $contents = do { local $/ = undef; <$fh> };\n+\tclose $fh or return;\n+\treturn unless defined $contents;\n+\tchomp($contents);\n+\n+\treturn unless ($contents =~ s!^gitdir: !!);\n+\n+\tif (!File::Spec->file_name_is_absolute($contents)) {\n+\t\t$contents = File::Spec->catfile(File::Basename::dirname($path), $contents);\n+\t}\n+\n+\treturn unless is_git_directory($contents);\n+\treturn $contents;\n+}\n+\n+# Test if it looks like we're at a git repository\n+#\n+# - a '.git' file containing \"gitdir: \",\n+# - a git directory.\n+sub is_git_repository {\n+\tmy $path = shift;\n+\treturn defined(read_gitfile($path)) || is_git_directory($path);\n+}\n+\n+# Return directory of a git repository, resolving '.git' files\n+# (file containing \"gitdir: \") if any\n+#\n+# See resolve_gitdir in setup.c\n+sub resolve_gitdir {\n+\tmy $suspect = shift;\n+\tif (is_git_directory($suspect)) {\n+\t\treturn $suspect;\n+\t}\n+\treturn read_gitfile($suspect);\n+}\n+\n # Check HEAD file, that it is either\n #\n # - a \"refs/heads/..\" symlink, or\n@@ -665,7 +711,7 @@ sub validate_headref {\n \n sub check_export_ok {\n \tmy ($dir) = @_;\n-\treturn (is_git_directory($dir) &&\n+\treturn (is_git_repository($dir) &&\n \t\t(!$export_ok || -e \"$dir/$export_ok\") &&\n \t\t(!$export_auth_hook || $export_auth_hook->($dir)));\n }\n@@ -874,7 +920,7 @@ sub evaluate_path_info {\n \t# find which part of PATH_INFO is project\n \tmy $project = $path_info;\n \t$project =~ s,/+$,,;\n-\twhile ($project && !is_git_directory(\"$projectroot/$project\")) {\n+\twhile ($project && !is_git_repository(\"$projectroot/$project\")) {\n \t\t$project =~ s,/*[^/]*$,,;\n \t}\n \treturn unless $project;\n@@ -3094,8 +3140,8 @@ sub git_get_projects_list {\n \t\t\t\tour $projectroot;\n \t\t\t\t# skip project-list toplevel, if we get it.\n \t\t\t\treturn if (m!^[/.]$!);\n-\t\t\t\t# only directories can be git repositories\n-\t\t\t\treturn unless (-d $_);\n+\t\t\t\t# only directories or gitlink files can be git repositories\n+\t\t\t\treturn unless (-d $_ || (-f _ && $File::Find::name =~ m!(?:^|/)\\.git!));\n \t\t\t\t# don't traverse too deep (Find is super slow on os x)\n \t\t\t\t# $project_maxdepth excludes depth of $projectroot\n \t\t\t\tif (($File::Find::name =~ tr!/!!) - $pfxdepth > $project_maxdepth) {\n-- \n1.7.9\n"},{"id":"189974","messageId":"20120424173618.GA15600@camk.edu.pl","threadId":"30138","inReplyTo":"201204180136.08570.jnareb@gmail.com","subject":"Re: [PATCH] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-24T17:36:18Z","receivedAt":"2012-04-24T17:36:18Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"I'm sorry for the late answer.\n\nOn Wed, Apr 18, 2012 at 01:36:08AM +0200, Jakub Narebski wrote:\n> On Mon, 16 Apr 2012, Kacper Kornet wrote:\n> > On Mon, Apr 16, 2012 at 10:06:49PM +0200, Jakub Narebski wrote:\n> >> On Mon, 16 Apr 2012, Kacper Kornet wrote:\n> >>> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:\n\n> >>> My tests show that forks are also a bottleneck in my setup.\n\n> >> What do you mean by \"my tests\" here? Is it benchmark (perhaps just using\n> >> 'timed' feature) with and without custom change removing fork(s)? Or did\n> >> you used profiler (e.g. wondefull Devel::NYTProf) for that?\n\n> > Nothing fancy. I look at the footnote produced by \"timed\" feature. And\n> > I see a difference between version with the following patch:\n\n[...]\n\n> > and the one in which git_repo_exist() uses invocation to /bin/true:\n\n> > diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> > index 18cdf96..4bcc66f 100755\n> > --- a/gitweb/gitweb.perl\n> > +++ b/gitweb/gitweb.perl\n> > @@ -3156,6 +3156,13 @@ sub git_get_project_owner {\n> > \treturn $owner;\n> > }\n\n> > +sub git_repo_exist {\n> > +\tmy ($path) = @_;\n> > +\n> > + $git_dir = \"$projectroot/$path\";\n> > + return not system('/bin/true');\n> > +}\n> > +\n\n> What were the differences in timing?\n\nThe best results with the host disk caches were:\n\nv1.7.10\nThis page took 66.960714 seconds and 16517 git commands to generate.\n\nCall /bin/true in git_repo_exist()\nThis page took 45.583935 seconds and 1 git commands to generate\n\nThe patches applied:\nThis page took 6.090545 seconds and 1 git commands to generate.\n\n> Anyway, if you plan on resending this patch series, then \"gitweb: Improve\n> repository verification\" should be be first, I think.\n\nThank you for writing that one for me. I will send my two patches to\nomit owner and last modification time on top of it. \n\n-- \n Kacper Kornet\n"},{"id":"189975","messageId":"20120424173914.GB15600@camk.edu.pl","threadId":"30138","inReplyTo":"201204191807.32410.jnareb@gmail.com","subject":"[PATCH 1/2] gitweb: Option to omit column with time of the last change","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-24T17:39:15Z","receivedAt":"2012-04-24T17:39:15Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"Generating information about last change for a large number of git\nrepositories can be very time consuming. This commit add an option to\nomit 'Last Change' column when presenting the list of repositories.\n\nSigned-off-by: Kacper Kornet \n---\n Documentation/gitweb.conf.txt | 4 ++++\n gitweb/gitweb.perl | 16 +++++++++++-----\n 2 files changed, 15 insertions(+), 5 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex 7aba497..d240a2f 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -499,6 +499,10 @@ $maxload::\n Set `$maxload` to undefined value (`undef`) to turn this feature off.\n The default value is 300.\n \n+$omit_age_column::\n+\tIf true, omit the column with date of the most current commit on the\n+\tprojects list page. It can save a bit of I/O and a fork per repository.\n+\n $per_request_config::\n \tIf this is set to code reference, it will be run once for each request.\n \tYou can\tset parts of configuration that change per session this way.\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 350b59c..6bddbff 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -133,6 +133,9 @@ our $default_projects_order = \"project\";\n # (only effective if this variable evaluates to true)\n our $export_ok = \"++GITWEB_EXPORT_OK++\";\n \n+# don't generate age column on the projects list page\n+our $omit_age_column = 0;\n+\n # show repository only if this subroutine returns true\n # when given the path to the project, for example:\n # sub { return -e \"$_[0]/git-daemon-export-ok\"; }\n@@ -5494,9 +5497,11 @@ sub git_project_list_rows {\n \t\t : esc_html($pr->{'descr'})) .\n \t\t \"\\n\" .\n \t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n-\t\tprint \"{'age'}) . \"\\\">\" .\n-\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\" .\n-\t\t \"\" .\n+\t\tunless ($omit_age_column) {\n+\t\t print \"{'age'}) . \"\\\">\" .\n+\t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\";\n+\t\t}\n+\t\tprint\"\" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"summary\")}, \"summary\") . \" | \" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"shortlog\")}, \"shortlog\") . \" | \" .\n \t\t $cgi->a({-href => href(project=>$pr->{'path'}, action=>\"log\")}, \"log\") . \" | \" .\n@@ -5527,7 +5532,8 @@ sub git_project_list_body {\n \t 'tagfilter' => $tagfilter)\n \t\tif ($tagfilter || $search_regexp);\n \t# fill the rest\n-\t@projects = fill_project_list_info(\\@projects);\n+\tmy @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n+\t@projects = fill_project_list_info(\\@projects, @all_fields);\n \n \t$order ||= $default_projects_order;\n \t$from = 0 unless defined $from;\n@@ -5559,7 +5565,7 @@ sub git_project_list_body {\n \t\tprint_sort_th('project', $order, 'Project');\n \t\tprint_sort_th('descr', $order, 'Description');\n \t\tprint_sort_th('owner', $order, 'Owner');\n-\t\tprint_sort_th('age', $order, 'Last Change');\n+\t\tprint_sort_th('age', $order, 'Last Change') unless $omit_age_column;\n \t\tprint \"\\n\" . # for links\n \t\t \"\\n\";\n \t}\n-- \n1.7.10\n\n\n\n-- \n Kacper Kornet\n"},{"id":"189976","messageId":"20120424174114.GC15600@camk.edu.pl","threadId":"30138","inReplyTo":"201204191807.32410.jnareb@gmail.com","subject":"[PATCH 2/2] gitweb: Option to not display information about owner","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-24T17:41:14Z","receivedAt":"2012-04-24T17:41:14Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"In some setups the repository owner is not a well defined concept\nand administrator can prefer it to be not shown. This commit add\nand an option that enable to reach this effect.\n\nSigned-off-by: Kacper Kornet \n---\n Documentation/gitweb.conf.txt | 3 +++\n gitweb/gitweb.perl | 20 ++++++++++++++------\n 2 files changed, 17 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex d240a2f..4b8d1b1 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -503,6 +503,9 @@ $omit_age_column::\n \tIf true, omit the column with date of the most current commit on the\n \tprojects list page. It can save a bit of I/O and a fork per repository.\n \n+$omit_owner::\n+\tIf true prevents displaying information about repository owner.\n+\n $per_request_config::\n \tIf this is set to code reference, it will be run once for each request.\n \tYou can\tset parts of configuration that change per session this way.\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6bddbff..7558def 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -136,6 +136,9 @@ our $export_ok = \"++GITWEB_EXPORT_OK++\";\n # don't generate age column on the projects list page\n our $omit_age_column = 0;\n \n+# don't generate information about owners of repositories\n+our $omit_owner=0;\n+\n # show repository only if this subroutine returns true\n # when given the path to the project, for example:\n # sub { return -e \"$_[0]/git-daemon-export-ok\"; }\n@@ -5495,8 +5498,10 @@ sub git_project_list_rows {\n \t\t ? esc_html_match_hl_chopped($pr->{'descr_long'},\n \t\t $pr->{'descr'}, $search_regexp)\n \t\t : esc_html($pr->{'descr'})) .\n-\t\t \"\\n\" .\n-\t\t \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n+\t\t \"\\n\";\n+\t\tunless ($omit_owner) {\n+\t\t print \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\\n\";\n+\t\t}\n \t\tunless ($omit_age_column) {\n \t\t print \"{'age'}) . \"\\\">\" .\n \t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"\\n\";\n@@ -5532,7 +5537,8 @@ sub git_project_list_body {\n \t 'tagfilter' => $tagfilter)\n \t\tif ($tagfilter || $search_regexp);\n \t# fill the rest\n-\tmy @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n+\tmy @all_fields = $omit_age_column ? ('descr', 'descr_long', 'ctags', 'category') : ();\n+\tpush @all_fields, 'owner' unless($omit_owner);\n \t@projects = fill_project_list_info(\\@projects, @all_fields);\n \n \t$order ||= $default_projects_order;\n@@ -5564,7 +5570,7 @@ sub git_project_list_body {\n \t\t}\n \t\tprint_sort_th('project', $order, 'Project');\n \t\tprint_sort_th('descr', $order, 'Description');\n-\t\tprint_sort_th('owner', $order, 'Owner');\n+\t\tprint_sort_th('owner', $order, 'Owner') unless $omit_owner;\n \t\tprint_sort_th('age', $order, 'Last Change') unless $omit_age_column;\n \t\tprint \"\\n\" . # for links\n \t\t \"\\n\";\n@@ -6318,8 +6324,10 @@ sub git_summary {\n \n \tprint \"
 
\\n\";\n \tprint \"\\n\" .\n-\t \"\\n\" .\n-\t \"\\n\";\n+\t \"\\n\";\n+ unless ($omit_owner) {\n+\t print \"\\n\";\n+ }\n \tif (defined $cd{'rfc2822'}) {\n \t\tprint \"\" .\n \t\t \"\\n\";\n-- \n1.7.10\n\n-- \n Kacper Kornet\n"},{"id":"190094","messageId":"xmqqy5pj9kew.fsf@junio.mtv.corp.google.com","threadId":"30138","inReplyTo":"20120424174114.GC15600@camk.edu.pl","subject":"Re: [PATCH 2/2] gitweb: Option to not display information about owner","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-26T04:39:03Z","receivedAt":"2012-04-26T04:39:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n> In some setups the repository owner is not a well defined concept\n> and administrator can prefer it to be not shown. This commit add\n> and an option that enable to reach this effect.\n>\n> Signed-off-by: Kacper Kornet \n\nAmong your recent three patches, this one seems to break t9500; has it\nbeen tested?\n\n[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.\n[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.\n[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.\n[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.\n\nI am guessing both #5401 and #5551 are $it->{'category'} of @projects[]\nelements.\n"},{"id":"190115","messageId":"20120426150721.GG16489@camk.edu.pl","threadId":"30138","inReplyTo":"xmqqy5pj9kew.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 2/2] gitweb: Option to not display information about owner","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-26T15:07:21Z","receivedAt":"2012-04-26T15:07:21Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Wed, Apr 25, 2012 at 09:39:03PM -0700, Junio C Hamano wrote:\n> Kacper Kornet writes:\n\n> > In some setups the repository owner is not a well defined concept\n> > and administrator can prefer it to be not shown. This commit add\n> > and an option that enable to reach this effect.\n\n> > Signed-off-by: Kacper Kornet \n\n> Among your recent three patches, this one seems to break t9500; has it\n> been tested?\n\n> [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.\n> [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.\n> [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.\n> [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.\n\n> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]\n> elements.\n\nYes, I have tested the tree with:\n\ngitweb: Improve repository verification\ngitweb: Option to omit column with time of the last change\ngitweb: Option to not display information about owner\n\napplied on top of v1.7.10. And all tests except 't91??' are passed.\nCould you write on top of which revision have you applied these three\npatches?\n\n-- \n Kacper Kornet\n"},{"id":"190119","messageId":"xmqqaa1ya3rh.fsf@junio.mtv.corp.google.com","threadId":"30138","inReplyTo":"20120426150721.GG16489@camk.edu.pl","subject":"Re: [PATCH 2/2] gitweb: Option to not display information about owner","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2012-04-26T15:53:22Z","receivedAt":"2012-04-26T15:53:22Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Kacper Kornet writes:\n\n>> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]\n>> elements.\n>\n> Yes, I have tested the tree with:\n>\n> gitweb: Improve repository verification\n> gitweb: Option to omit column with time of the last change\n> gitweb: Option to not display information about owner\n>\n> applied on top of v1.7.10. And all tests except 't91??' are passed.\n> Could you write on top of which revision have you applied these three\n> patches?\n\nLet's see...\n\n$ git log --oneline --first-parent --boundary master..kk/gitweb-omit-expensive\n37e2621 gitweb: Option to not display information about owner\n5710be4 gitweb: Option to omit column with time of the last change\n75e0dff gitweb: Don't set owner if got empty value from projects.list\n- fdec2eb Merge branch 'maint-1.7.9' into maint\n\nI replayed these three on v1.7.10^0\n\n$ git checkout v1.7.10^0\n$ git format-patch --stdout fdec2eb..kk/gitweb-omit-expensive | git am -s3c\n\nand the result fails exactly the same way, though.\n\n*** prove ***\nt9500-gitweb-standalone-no-errors.sh .. Dubious, test returned 1 (wstat 256, 0x100)\nFailed 2/117 subtests\n (less 2 skipped subtests: 113 okay)\n\nTest Summary Report\n-------------------\nt9500-gitweb-standalone-no-errors.sh (Wstat: 256 Tests: 117 Failed: 2)\n Failed tests: 116-117\n Non-zero exit status: 1\nFiles=1, Tests=117, 15 wallclock secs ( 0.07 usr 0.00 sys + 10.18 cusr 1.42 csys = 11.67 CPU)\nResult: FAIL\nmake[1]: *** [prove] Error 1\nmake[1]: Leaving directory `/srv/git/t'\nmake: *** [test] Error 2\n"},{"id":"190124","messageId":"20120426163502.GH16489@camk.edu.pl","threadId":"30138","inReplyTo":"xmqqaa1ya3rh.fsf@junio.mtv.corp.google.com","subject":"Re: [PATCH 2/2] gitweb: Option to not display information about owner","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-26T16:35:02Z","receivedAt":"2012-04-26T16:35:02Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"On Thu, Apr 26, 2012 at 08:53:22AM -0700, Junio C Hamano wrote:\n> Kacper Kornet writes:\n\n> >> I am guessing both #5401 and #5551 are $it->{'category'} of @projects[]\n> >> elements.\n\n> > Yes, I have tested the tree with:\n\n> > gitweb: Improve repository verification\n> > gitweb: Option to omit column with time of the last change\n> > gitweb: Option to not display information about owner\n\n> > applied on top of v1.7.10. And all tests except 't91??' are passed.\n> > Could you write on top of which revision have you applied these three\n> > patches?\n\n> Let's see...\n\n> $ git log --oneline --first-parent --boundary master..kk/gitweb-omit-expensive\n> 37e2621 gitweb: Option to not display information about owner\n> 5710be4 gitweb: Option to omit column with time of the last change\n> 75e0dff gitweb: Don't set owner if got empty value from projects.list\n> - fdec2eb Merge branch 'maint-1.7.9' into maint\n\n> I replayed these three on v1.7.10^0\n\n> $ git checkout v1.7.10^0\n> $ git format-patch --stdout fdec2eb..kk/gitweb-omit-expensive | git am -s3c\n\n> and the result fails exactly the same way, though.\n\nAppereantly I have managed to send the different patch then I have\napplied in my private tree. I'm sorry for my mistake. In a short time I\nwill send the correct one.\n\n-- \n Kacper Kornet\n"},{"id":"190125","messageId":"20120426164544.GI16489@camk.edu.pl","threadId":"30138","inReplyTo":"20120426163502.GH16489@camk.edu.pl","subject":"[PATCH v2 2/2] gitweb: Option to not display information about owner","fromName":"Kacper Kornet","fromEmail":"draenog@pld-linux.org","sentAt":"2012-04-26T16:45:44Z","receivedAt":"2012-04-26T16:45:44Z","isPatch":true,"sender":{"key":"draenog@pld-linux.org","avatar":"https://avatars.githubusercontent.com/u/608762?v=4"},"body":"In some setups the repository owner is not a well defined concept\nand administrator can prefer it to be not shown. This commit add\nand an option that enable to reach this effect.\n\nSigned-off-by: Kacper Kornet \n---\n Documentation/gitweb.conf.txt | 3 +++\n gitweb/gitweb.perl | 21 +++++++++++++++------\n 2 files changed, 18 insertions(+), 6 deletions(-)\n\ndiff --git a/Documentation/gitweb.conf.txt b/Documentation/gitweb.conf.txt\nindex d240a2f..4b8d1b1 100644\n--- a/Documentation/gitweb.conf.txt\n+++ b/Documentation/gitweb.conf.txt\n@@ -503,6 +503,9 @@ $omit_age_column::\n \tIf true, omit the column with date of the most current commit on the\n \tprojects list page. It can save a bit of I/O and a fork per repository.\n \n+$omit_owner::\n+\tIf true prevents displaying information about repository owner.\n+\n $per_request_config::\n \tIf this is set to code reference, it will be run once for each request.\n \tYou can\tset parts of configuration that change per session this way.\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex 6bddbff..6dbeb2f 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -136,6 +136,9 @@ our $export_ok = \"++GITWEB_EXPORT_OK++\";\n # don't generate age column on the projects list page\n our $omit_age_column = 0;\n \n+# don't generate information about owners of repositories\n+our $omit_owner=0;\n+\n # show repository only if this subroutine returns true\n # when given the path to the project, for example:\n # sub { return -e \"$_[0]/git-daemon-export-ok\"; }\n@@ -5495,8 +5498,10 @@ sub git_project_list_rows {\n \t\t ? esc_html_match_hl_chopped($pr->{'descr_long'},\n \t\t $pr->{'descr'}, $search_regexp)\n \t\t : esc_html($pr->{'descr'})) .\n-\t\t \"\\n\" .\n-\t\t \"\\n\";\n+\t\t \"\\n\";\n+\t\tunless ($omit_owner) {\n+\t\t print \"\\n\";\n+\t\t}\n \t\tunless ($omit_age_column) {\n \t\t print \"\\n\";\n@@ -5532,7 +5537,9 @@ sub git_project_list_body {\n \t 'tagfilter' => $tagfilter)\n \t\tif ($tagfilter || $search_regexp);\n \t# fill the rest\n-\tmy @all_fields = $omit_age_column ? ('descr', 'descr_long', 'owner', 'ctags', 'category') : ();\n+\tmy @all_fields = ('descr', 'descr_long', 'ctags', 'category');\n+\tpush @all_fields, ('age', 'age_string') unless($omit_age_column);\n+\tpush @all_fields, 'owner' unless($omit_owner);\n \t@projects = fill_project_list_info(\\@projects, @all_fields);\n \n \t$order ||= $default_projects_order;\n@@ -5564,7 +5571,7 @@ sub git_project_list_body {\n \t\t}\n \t\tprint_sort_th('project', $order, 'Project');\n \t\tprint_sort_th('descr', $order, 'Description');\n-\t\tprint_sort_th('owner', $order, 'Owner');\n+\t\tprint_sort_th('owner', $order, 'Owner') unless $omit_owner;\n \t\tprint_sort_th('age', $order, 'Last Change') unless $omit_age_column;\n \t\tprint \"\\n\" . # for links\n \t\t \"\\n\";\n@@ -6318,8 +6325,10 @@ sub git_summary {\n \n \tprint \"
 
\\n\";\n \tprint \"
description\" . esc_html($descr) . \"
owner\" . esc_html($owner) . \"
description\" . esc_html($descr) . \"
owner\" . esc_html($owner) . \"
last change\".format_timestamp_html(\\%cd).\"
\" . chop_and_escape_str($pr->{'owner'}, 15) . \"\" . chop_and_escape_str($pr->{'owner'}, 15) . \"{'age'}) . \"\\\">\" .\n \t\t (defined $pr->{'age_string'} ? $pr->{'age_string'} : \"No commits\") . \"
\\n\" .\n-\t \"\\n\" .\n-\t \"\\n\";\n+\t \"\\n\";\n+ unless ($omit_owner) {\n+\t print \"\\n\";\n+ }\n \tif (defined $cd{'rfc2822'}) {\n \t\tprint \"\" .\n \t\t \"\\n\";\n-- \n1.7.10\n\n-- \n Kacper Kornet\n"}]}
description\" . esc_html($descr) . \"
owner\" . esc_html($owner) . \"
description\" . esc_html($descr) . \"
owner\" . esc_html($owner) . \"
last change\".format_timestamp_html(\\%cd).\"