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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 4, 2012, 14:31 UTC
Message-ID: <201204041631.42905.jnareb@gmail.com>
In-Reply-To: <20120404063939.GA17024@camk.edu.pl>

On Wed, 4 April 2012, Kacper Kornet wrote:

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

Show 23 quoted lines

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

Perhaps it would be better to say it like this:

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

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

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

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

Show 16 quoted lines

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

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

Show 26 quoted lines

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

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

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

-- 
Jakub Narebski
Poland