On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:

quoted text
> Create Gitweb::View module in 'gitweb/lib/Gitweb/View.pm'
> to store the subroutines related to the HTML output
> for gitweb.

If it contains subroutines related only to HTML output, why isn't it
called Gitweb::HTML then?  If it contains some subroutines which are
not strictly about HTML, please state it in the commit message.

quoted text
> 
> This module depends on Git.pm, Config.pm, Request.pm,
> Escape.pm and RepoConfig.pm. Action specific HTML div
> subroutines are not included in this module due to
> unmet dependencies of Gitweb::Parse and Gitweb::Format.

Whether you use Gitweb::Git or Git.pm, you should be consistent in the
naming you use: either Git.pm, or Gitweb::Parse.

Do I understand correctly trhat Gitweb::Parse and Gitweb::Format are to
be added in subsequent patches?  Please state this fact in the commit
message.

quoted text
> 
> Subroutines moved:
> 	*href
> 	 get_feed_info
> 	*chop_str
> 	 chop_and_escape_str
> 	*age_class
> 	 age_string
> 	*S_ISGITLINK
> 	 mode_str
> 	 file_type
> 	 file_type_long
> 	*get_page_title
> 	 git_header_html
> 	 git_footer_html
> 	 die_error
> 	*git_print_page_nav
> 	 format_paging_nav
> 	 git_print_header_div
> 	 git_print_page_path
> 	*print_local_time
> 	 format_local_time
> 	*insert_file
> 	*git_get_link_target
> 	 normalize_link_target
> 	 git_print_tree_entry
> 	*print_sort_th
> 	 format_sort_th

Nitpick: my first thought was "why some of those subroutines are marked
with a star?".  If you want to divide this list of subroutines into
groups by function, you should leave a space between '*' and name of
subroutine (i.e. '* href'), and perhaps also leave empty lines between
groups, though this might make commit this list overly long.

I guess that this separation of subroutines into Gitweb::View module 
was a matter of some compromise: between having too large modules
(containing only barely related subroutines) and having too many
modules.

quoted text
> 
> Update 'gitweb/Makefile' to install Gitweb::View alongside gitweb.
> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

I have not checked the diff itself...

-- 
Jakub Narebski
Poland
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@vger.kernel.org
More majordomo info at  http://vger.kernel.org/majordomo-info.html