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

From

Kacper Kornet <draenog@pld-linux.org>

Date

Apr 4, 2012, 16:22 UTC

Message-ID

<20120404162208.GN10461@camk.edu.pl>

In-Reply-To

<201204041631.42905.jnareb@gmail.com>

On Wed, Apr 04, 2012 at 04:31:42PM +0200, Jakub Narebski wrote:

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

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

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

-- 
  Kacper Kornet