Re: [PATCH v3 3/3] gitweb: Optional grouping of projects by category

From: Jakub Narebski <jnareb@gmail.com>
Date: Dec 5, 2008, 10:45 UTC
Message-ID: <200812051145.09732.jnareb@gmail.com>
In-Reply-To: <87bpvr1hee.wl%seb@cine7.net>

On Fri, 5 Dec 2008 at 03:32, Sébastien Cevey wrote:

Show 8 quoted lines

> At Fri, 5 Dec 2008 03:08:52 +0100, Jakub Narebski wrote:
> 
> > Nice... but perhaps it would be better to simply pass $from / $to to
> > build_projlist_by_category function, and have in %categories only
> > projects which are shown...
> 
> Ah you're right, we could do that, I hadn't thought of it.  Sounds
> cleaner than the $from/$to dance I did in this patch.

Usually simpler is better. The more complicated code, the more chances for bugs, and less maintability. I haven't even tried to follow $from/$to updating logic, but noticed that we can simply pass $from and $to to build_projlist_by_category(), and use loop $from..$to there.

Show 19 quoted lines

> > well, unless filtered out in print_project_rows() by $show_ctags; so
> > I think that there is nonzero probability of empty (no project
> > shown) categories.
> 
> Mh indeed, in fact this could happen any time we reach one of the
> 'next' statements in the loop in print_project_rows(), when performing
> search, tag filtering, etc...
> 
> Actually, assuming the project list is split into page, this can also
> lead to empty pages (if no project on the page matches the filter).
> To avoid empty categories, it's a bit tricky since the header is
> printed before we determine whether there are matching projects..
> 
> I need to read the code more carefully, but it seems that one solution
> would be to add a function that determines whether a project should be
> displayed or not (according to tags and search and forks); then we can
> map this function on the list of projects.
> 
> I could do it if it sounds sane to you.

Nah, I think it would be best to postpone dealing with this issue, and keep the code simple at the cost of possibly empty categories. It is not as empty categories are an actual error...

I guess that correct way to deal with this would be to filter projects earlier, before grouping by category, and not "at the last minute", when displaying them. It might be even better solution wrt. dividing projects list into pages. But that in my opinion is certainly matter for a separate patch.

> > I'll try to examine the code in more detail later... currently I don't
> > know why but I can't git-am the second patch (this patch) correctly...
> 
> This is the third patch, are you sure you applied 1 and 2 before?

 
Somehow I couldn't apply second patch in series:

 $ git am -3 -u "[PATCH v3 2_3] gitweb: Split git_project_list_body in two functions.txt"
 Applying: gitweb: Split git_project_list_body in two functions
 error: patch failed: gitweb/gitweb.perl:3972
 error: gitweb/gitweb.perl: patch does not apply
 fatal: sha1 information is lacking or useless (gitweb/gitweb.perl).
 Repository lacks necessary blobs to fall back on 3-way merge.
 Cannot fall back to three-way merge.
 Patch failed at 0001.

> Thanks for your careful and supportive comments!

You are welcome. Nice to have another gitweb developer :-)

-- 
Jakub Narebski
Poland