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:
> >> 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.
I think that even in this case check_export_ok is called. But there is
still the problem you have mentioned below.
> 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.
My tests show that forks are also a bottleneck in my setup. 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.
--
Kacper Kornet
--
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