On Mon, 16 Apr 2012, Kacper Kornet wrote:
> On Sat, Apr 14, 2012 at 03:16:01PM +0200, Jakub Narebski wrote:
>> 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:
[...]
>>>> 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.
>
> I think that even in this case check_export_ok is called. But there is
> still the problem you have mentioned below.
True, somehow I missed the fact that check_export_ok() is called to
check if it looks like repository and if it is to be exported in both
cases ($projects_list a directory or a file).
>> 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.
Actually if you look at the footer of projects list page with 'timed'
feature enable you see that for N projects on list, gitweb uses 2*N+1
git commands. The "+1" part is from "git version", the "2*N" are from
git-for-each-ref to get last activity (and verify repository as a
side-effect)...
...and from call to "git config" to get owner (unconditional check for
`gitweb.owner` override), description (if '.git/description' file got
deleted), if applicable category (file then config), if applicable ctags
(file(s) then config).
So we can rely on "git config" being called, no need for separate
verification. My mistake. (Though it might be hard to use this fact.)
Well, with proposed option to remove 'owner' field we would have sometimes
to verify repository with an extra git command...
>> * 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.
>
> My tests show that forks are also a bottleneck in my setup.
What do you mean by "my tests" here? Is it benchmark (perhaps just using
'timed' feature) with and without custom change removing fork(s)? Or did
you used profiler (e.g. wondefull Devel::NYTProf) for that?
> On the other
> hand I think that I can trust that my projects.list contains only valid
> repositories. So I would prefer to have a don't verify option. Or there
> is a possibility to write perl function with the same functionality as
> is_git_directory() from setup.c and use it to verify if the directory is a
> valid git repo.
Well, we can add those checks to check_export_ok()... well to function
it would call.
is_git_repository from setup.c checks that "/objects" and "/refs"
have executable permissions, and that "/HEAD" is valid via validate_headref
which does slightly more than (now slightly misnamed) check_head_link()
from gitweb...
...or that DB_ENVIRONMENT i.e. GIT_OBJECT_DIRECTORY environment variable
is set, and path that it points to has executable permissions. I don't
think we have to worry about this for gitweb.
I'll try to come up with a patch to gitweb that improves repository
verification, and perhaps a patch that uses the fact that "git config"
succeeded to verify repository.
--
Jakub Narębski
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html