Re: [PATCH] gitweb: Improve repository verification

From: Jakub Narebski <jnareb@gmail.com>
Date: Apr 19, 2012, 19:46 UTC
Message-ID: <201204192146.09853.jnareb@gmail.com>
In-Reply-To: <xmqq397zwp4c.fsf@junio.mtv.corp.google.com>

On Thu, 19 April 2012, Junio C Hamano wrote:

Show 31 quoted lines

> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > Bring repository verification in check_export_ok() to standards of
> > is_git_directory function from setup.c (core git), and validate_headref()
> > to standards of the same function in path.c,... and a bit more.
> >
> > validate_headref() replaces check_head_link(); note that the former
> > requires path to HEAD file, while the late latter path to repository.
> >
> > Issues of note:
> > * is_git_directory() in gitweb is a bit stricter: it checks that
> >   "/objects" and "/refs" are directories, and not only 'executable'
> >   permission,
> > * validate_headref() in gitweb is a bit stricter: it checks that
> >   reference symlink or symref points to starts with "refs/heads/",
> >   and not only with "refs/",
> > * calls to check_head_link(), all of which were meant to check if
> >   given directory can be a git repository, were replaced by newly
> >   introduced is_git_directory().
> >
> > This change is preparation for removing "Last change" column from list
> > of projects, which is currently used also for validating repository.
> >
> > Suggested-by: Kacper Kornet <draenog@pld-linux.org>
> > Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> > ---
> > Here is how such first step could look like...
> 
> Do you mean by "could look like" that this is still an RFC, or is this
> something we want to apply and see how well it makes people's lives in
> the field?

"Here is how such first step could look like" was directed to Kacper... :-)

Kacper Kornet (who started this thread with "[PATCH] gitweb: Option to omit column with time of the last change") wants to have an option to remove "Last Change" column from projects list page, and "Owner" column and field from all gitweb views. This will allow to generate projects list page with 1 call to git command rather than 2*N+1, where N is number of repositories shown...

...but we use the fact that "git --git-dir=$GIT_DIR for-each-ref ..." succeed or fails to verify that given path points to git repository. That is why I proposed this commit to be first patch in hopefully upcoming Kacper's new version of patch series.

But in current gitweb (without Kacper's planned patches) this change
doesn't bring much, as git repositories are verified outside of
is_git_directory() check... well, perhaps with exception of possible
corner case when one is using path_info gitweb URL...

> By the way, I wonder (1) if it is worth adding support for the textual
> ".git" file that contains "gitdir: $path", and (2) if so how big a
> change would we need to do so.

I don't think that it would be big changeto add support for "gitlink" files, assuming that 'git --git-dir=<gitlink file> ...' works correctly. I would put that addition in a separate commit, though.

BTW. does core git limit number of redirections, or have some loop detection?

-- 
Jakub Narebski
Poland