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

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 14, 2012, 13:16 UTC
Message-ID: <201204141516.02719.jnareb@gmail.com>
In-Reply-To: <20120404162208.GN10461@camk.edu.pl>

I'm sorry for the delay answering.

On Wed, 4 Apr 2012, Kacper Kornet wrote:

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

Show 23 quoted lines

>> 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.
> 
> I think that git_project_list_body always works with the list returned
> by git_get_projects_list. And git_get_projects_list validates if the
> path is a git repository. So it should not be a problem. Please correct
> me, if I am wrong.

 
If $projects_list points to plain file, git_get_projects_list() just
gets list of projects (and project owners) from $projects_list file
by reading and parsing this file.  No verification that repository
exists is done.

If $projects_list points to directory (which is the default), then git_get_projects_list() scans starting from $projects_list directory for somtehing that _looks like_ git repository with check_head_link() via check_export_ok(). But you still can encounter something that looks like git repository (has "HEAD" file in it), but isn't.

So we would probably want to have said variable or set of variables describe three states:

* find date of last change in repository with git-for-each-ref called
  by git_get_last_activity(), which as a side effect verifies that
  repository actually exist.

  git_get_last_activity() returns empty list in list context if repo
  does not exist, hence

  	my (@activity) = git_get_last_activity($pr->{'path'});
  	unless (@activity) {
  		next PROJECT;
  	}

* verify that repository exists with "git rev-parse --git-dir" or
  "git rev-parse --verify HEAD", or "git symbolic-ref HEAD", redirecting
  stderr to /dev/null (we would probably want to save output of the
  latter two somewhere to use it later).
  
  That saves I/O, but not fork.

* don't verify that repository exists.

Though perhaps the last possibility isn't a good idea, so it would be left two-state, as in your patch.

Show 20 quoted lines

>>> [...]. 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.
> 
> I thought about two different variables as that would have a slightly
> different functionality. While I want to get rid off Last Change from
> the projects list page, I still want to get this information on pages of
> single repositories. On the other hand I don't want repository owner to
> be shown anywhere.

Ah, in this case we want to keep $dont_show_repo_age / $no_age_column (or something like that) separate from $no_owner... and in this case these features can be controlled by separate scalar configuration variables.

-- 
Jakub Narebski
Poland