Re: [PATCH 00/11 GSoC] gitweb: Split gitweb into modules

Thank you very much for providing such detailed cover letter.  It makes
commenting on this series much easier.


On Tue, 22 June 2010 at 03:30:36 +0530, Pavan Kumar Sunkara wrote:

The patch series is based on 'pu' branch.

You should probably say also which commit (which branch) this depends
on, rather than blanket 'pu'.

10 patches out of the 11 patches in this patch series split gitweb into
several small sized modules which is one of the goal of my GSoC's project.

The first patch of this patch series is for fixing esc_url function which is
previously missed by commit 425e225.

I think it would be better to send this patch separately, standalone
(not in series), based on 'master' or even 'maint' to have it applied,
and not being held back by being in this series.  This is pure bugfix
for a (rare) bug.

The second patch is produced by my commit amend to Jakub Narebski's
initial commit to prepare splitting of gitweb in the message-id:
http://mid.gmane.org/1276531710-22945-4-git-send-email-jnareb@gmail.com

There is a small ammendment to that patch. 
$(INSTALL) -m 644 $(mod) '$(DESTDIR_SQ)$(gitwebdir_SQ)/$(mod)'
is changed to
$(INSTALL) -m 644 $(mod) '$(DESTDIR_SQ)$(gitwebdir_SQ)/$(dir $(mod))';)
because it is better if we gave 'dir $(mod)' as the target for install command
rather than '$(mod)' because it may cause problems in the future when gitweblibdir
is used and the modules are installed elsewhere rather than in /use/share/gitweb.

Thanks for finding it, and for improving it.

Actually, as long as we have only individual files in $(GITWEB_MODULES),
it should not matter.  The version before changes used

  install [OPTION]... SOURCE DEST

form, and after changes it uses

  install [OPTION]... SOURCE... DIRECTORY

with a single SOURCE.  So I don't think it is strictly necessary, but
it might be a good improvement anyway.

The modules created and being used by Gitweb are
	Gitweb::Git
	Gitweb::Config
	Gitweb::Request
	Gitweb::Escape
	Gitweb::RepoConfig
	Gitweb::View
	Gitweb::Util
	Gitweb::Format
	Gitweb::Parse

What I would like to see here is a dependency graph, i.e. which modules
include (use) which modules, and perhaps also how much this split
reduces size of main gitweb.perl file.

shortlog:
=========

Nitpick: I would say "Table of contents", because shortlog it is not ;-)))

1. gitweb: fix esc_url

The custom CGI escaping done in esc_url failed to escape UTF-8
properly. Fix by using CGI::escape on each sequence of matched
characters instead of sprintf()ing a custom escaping for each byte.

Additionally, the space -> + escape was being escaped due to greedy
matching on the first substitution. Fix by adding space to the
list of characters not handled on the first substitution.

Finally, remove an unnecessary escaping of the + sign.

commit 425e225 has missed fixing esc_url.

I would probably write this commit message differently, saying that
original author (Giuseppe Bilotta) fixed esc_param, but didn't finish
fixing this bug by not fixing esc_url, and that this commit finishes
it.  Probably also use "fix esc_url, use CGI::escape" in subject.
But that's me, not you.

Note: the commit is 452e225, not 425e225 !!!
                     ^^           ^^

2. gitweb: Prepare for splitting gitweb

Prepare gitweb for having been split into modules that are to be
installed alongside gitweb in 'lib/' subdirectory, by adding

  use lib __DIR__.'/lib';

to gitweb.perl (to main gitweb script), and preparing for putting
modules (relative path) in $(GITWEB_MODULES) in gitweb/Makefile.

It makes adding modules to install very easy, isn't it?
 

3. gitweb: Create Gitweb::Git module

Create a Gitweb::Git module in  'gitweb/lib/Gitweb/Git.pm'
to deal with running git commands (and also processing output
of git commands with external programs) from gitweb.

The "processing output of git commands with external programs"
looks a bit strange to me.  If it is about quote_command()
subroutine, it is more about shell escaping of commands, or
about running other programs... but it might be O.K., if what
you want to say that quote_command() is used in pipe-ing output
of git commands (e.g. git-archive) to other programs (e.g. gzip).

So take the above comment with a bit of salt...

This module is intended as standalone module, which does not require
(include) other gitweb' modules to avoid circular dependencies.  That
is why it includes $GIT variable, even though this variable is
configured during building gitweb.  On the other hand $GIT is more
about git configuration, than gitweb configuration.

Subroutines moved:
	evaluate_git_version
	git_cmd
	quote_command

Update gitweb/Makefile to install Gitweb::Git module alongside gitweb

O.K.

4. gitweb: Create Gitweb::Config module

Create Gitweb::Config module in 'gitweb/lib/Gitweb/Config.pm'
to store all the configuration variables and subroutines
regarding the gitweb.perl script.

This module depends only on $git_dir from Gitweb::Git and
includes most of the configuration related variables and
subroutines (Including those required for configuration
of gitweb features).

"Most" meaning what?  Which subroutines weren't moved, and why (they
should not be in Gitweb::Config, they cannot be moved for technical
reasons)?

Subroutines moved:
	evaluate_gitweb_config
	configure_gitweb_features

I'm not sure if configure_gitweb_features() should be there, as it
is run-once subroutine, about flow of the main program... but perhaps
it is O.K. to have it here.

	filter_snapshot_fmts
	gitweb_get_feature
	gitweb_check_feature

Update gitweb/Makefile to install Gitweb::Config module alongside gitweb

O.K.

5. gitweb: Create Gitweb::Request module

Create a Gitweb::Request module in 'gitweb/lib/Gitweb/Request.pm'
to store and handle all the cgi params and related variables
regarding the gitweb.perl script.

This module is intended as standalone module, which does not require
(include) other gitweb' modules to avoid circular dependencies.

validate_project and validate_action can't be included in this
module due to unmet dependencies.

What unment dependencies?  Can it be worked around by changing their
signatures to use additional parameters, e.g.:

  sub validate_action {
  	my $input = shift || return undef;
  	my $actions = shift || return undef;
  
  	return undef unless exists $actions->{$input};
  	return $input;
  }

But I guess it would be very hard to do the same with validate_project.
Also such change might be out of scope for _this_ series.

Subroutines moved:
	evaluate_uri
	evaluate_query_params
	validate_pathname
	validate_refname

I'm not completly sure if validate_* subroutines should be not separate,
as they do require more knowledge about setup, and about git repositories
served, than the rest of Gitweb::Request subroutines...

Update gitweb/Makefile to install Gitweb::Request module alongside gitweb

O.K.

6. gitweb: Create Gitweb::Escape module

Create a Gitweb::Escape module in 'gitweb/lib/Gitweb/Escape.pm'
to store all the quoting/unquoting and escaping subroutines
regarding the gitweb.perl script.

This module imports $fallback_encoding variable from
Gitweb::Config module to use it in sub 'to_utf8'

Subroutines moved:
	to_utf8
	esc_param
	esc_url
	esc_html
	esc_path
	quot_cec
	quot_upr

O.K.

	unquote

Shouldn't unquote be in Gitweb::Parse, as contrary to the rest of 
subroutines is is not about gitweb output escaping/quoting, but
about processing (parsing) output of git commands?  Perhaps it
could even be provate to Gitweb::Parse (i.e. not exported by
default)...

	untabify

Update gitweb/Makefile to install Gitweb::Escape module alongside gitweb


7. gitweb: Create Gitweb::RepoConfig module

Create a Gitweb::RepoConfig module in 'gitweb/lib/Gitweb/RepoConfig.pm'
to store and handle all the configuration and subroutines
related to a single repository regarding the gitweb.perl script.

This module depend on several other modules like Git.pm,
Config.pm, Request.pm and Escape.pm.

It also include subroutines regarding project_list and
it's handling.

Why?  Is it to not have too many tiny modules, or is simply there no
single good place where to put this subroutine?

Subroutines moved:
	check_head_link
	check_export_ok

O.K.

	hash_set_multi
	config_to_bool
	config_to_int
	config_to_multi
	git_parse_project_config

Hmmm... I think that in the future all the subroutines dealing with
reading of gitweb config and making its values available should be
encapsulated in a separate module... Git::Config perhaps?

But I guess that is for the future commit.

	git_get_project_config
	git_get_project_description
	git_get_project_ctags
	git_populate_project_tagcloud
	git_show_project_tagcloud
	git_get_project_url_list
	git_get_projects_list
	git_get_project_list_from_file
	git_get_project_owner
	get_file_owner

O.K.

	project_in_list

Hmmm...

Update gitweb/Makefile to install Gitweb::RepoConfig module
alongside gitweb


8. gitweb: Create Gitweb::View module

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

I find that this module looks a bit like a collection of fairly unrelated
subroutines at very different levels of abstractions.  

I guess that you don't want to split gitweb into too many modules,
and splitting gitweb is more of one of steps to final goal of adding
write functionality to gitweb, rather than the goal in and of itself.
Nevertheless it would be good to be able to immediately know from the
description of module what kind of subroutines should be there.

This module depends on Git.pm, Config.pm, Request.pm,
Escape.pm and RepoConfig.pm. Some subroutines which
output HTML but are not included in this module due
to unmet dependencies.

Which subroutines and what unmet dependencies?

Subroutines moved:

Perhaps this list should be grouped in categories, either by separating
groups with empty line, or by putting '*' before first subroutine in
group, as shown in example below: 

	href

Hmmm... that's the opposte side of Gitweb::Request, crating rather than
parsing gitweb links...

	get_feed_info

To be passed to href(); I think it is the only one.

	chop_str
	chop_and_escape_str

Those are pure utility functions.

    * age_class
      age_string

Those are about coloring based on relative time and (age_string - not
a great name) generating relative time ("nn units ago").

    * print_local_time
      format_local_time

Shouldn't this be in Gitweb::Format?  If not, why?

    * S_ISGITLINK
	mode_str
	file_type
	file_type_long

Those are all about types of files from git-ls-tree and friends, but
with single exception of S_ISGITLINK they are about gitweb output side.

	get_page_title
	git_header_html
	git_footer_html
	die_error

Main parts of page.

    * git_print_page_nav
	format_paging_nav
	git_print_header_div
	git_print_page_path

Fragments of page (used in most of views).  This decidely belong to
Gitweb::View.

    * insert_file

More of utility function.

    * git_get_link_target
	normalize_link_target
	git_print_tree_entry

Should it be here?  The *_body subroutines aren't there in Gitweb::View.
The *_link_target are utility subroutines for git_print_tree_entry.

	print_sort_th
	format_sort_th

Hmmm...

Update 'gitweb/Makefile' to install Gitweb::View alongside gitweb.

I have my doubts...

9. gitweb: Create Gitweb::Util module

Create Gitweb::Util module in 'gitweb/lib/Gitweb/Util.pm'
to store the git utility subroutines related to gitweb.

This module include subroutines in various categories
such as git utility subs invoking git commands, git
utility subs accessing git repository, mimetype related
subs and HTML output utility subs.

Subroutines moved:
	git_get_head_hash
	git_get_full_hash
	git_get_short_hash
	git_get_hash
	git_get_type
	git_get_hash_by_path
	git_get_path_by_hash
	git_get_last_activity
	git_get_references
	git_get_rev_name_tags
	git_get_heads_list
	git_get_tags_list
	mimetype_guess_file
	mimetype_guess
	blob_mimetype
	blob_contenttype
	guess_file_syntax
	fill_from_file_info
	is_deleted
	is_patch_split

O.K.

	run_highlighter

_Perhaps_ with exception of this subroutine.

Update 'gitweb/Makefile' to install Gitweb::Util alongside gitweb.


10. gitweb: Create Gitweb::Format module

Create Gitweb::Format module in 'gitweb/lib/Gitweb/Format.pm'
to store the subroutines related to formatting of HTML
fragments required for gitweb.

This module depends on Config.pm, View.pm, Escape.pm,
Util.pm and Request.pm. It mainly contain functions returning
short HTML fragments or transforming HTML fragments. Also
include subroutines regarding avatar formatting.

Why it depends on Gitweb::View, and not the reverse?  I understand why
it depends on Gitweb::Config and Gitweb::Escape, and I guess it needs
$cgi from Gitweb::Request (or is it something more?).

Subroutines moved:
	format_log_line_html
	format_ref_marker
	format_subject_html
	picon_url
	gravatar_url
	git_get_avatar
	format_search_author
	format_author_html
	format_git_diff_header_line
	format_extended_diff_header_line
	format_diff_from_to_header
	format_diff_cc_simplified
	format_diff_line
	format_snapshot_links

O.K., I think.

Update 'gitweb/Makefile' to install Gitweb::Format alongside gitweb.


11. gitweb: Create Gitweb::Parse module

Create Gitweb::Parse module in 'gitweb/lib/Gitweb/Parse.pm'
to store the subroutines which related to parsing functions
required for gitweb.

This module depends on Git.pm, Escape.pm, View.pm and Util.pm.

Why it depends on Gitweb::View?  It is a strange dependency.
I understand depending on Gitweb::Git to some extent, I'm not sure
if we shouldn't simply move unescape to it instead of requiring
Gitweb::Escape (unless there is more: to_utf8 perhaps?), and
I understand that Gitweb::Util has some required subroutines.

Subroutines moved:
	parse_date
	parse_tag
	parse_commit_text
	parse_commit
	parse_commits
	parse_difftree_raw_line
	parsed_difftree_line
	parse_ls_tree_line
	parse_from_to_diffinfo

O.K.

Update 'gitweb/Makefile' to install Gitweb::Parse alongside gitweb.


Pavan Kumar Sunkara (11):
  gitweb: fix esc_url
  gitweb: Prepare for splitting gitweb
  gitweb: Create Gitweb::Git module
  gitweb: Create Gitweb::Config module
  gitweb: Create Gitweb::Request module
  gitweb: Create Gitweb::Escape module
  gitweb: Create Gitweb::RepoConfig module
  gitweb: Create Gitweb::View module
  gitweb: Create Gitweb::Util module
  gitweb: Create Gitweb::Format module
  gitweb: Create Gitweb::Parse module

 gitweb/Makefile                 |   14 +
 gitweb/gitweb.perl              | 3855 +++------------------------------------

Nice!

 gitweb/lib/Gitweb/Config.pm     |  498 +++++
 gitweb/lib/Gitweb/Escape.pm     |  175 ++
 gitweb/lib/Gitweb/Format.pm     |  537 ++++++
 gitweb/lib/Gitweb/Git.pm        |   48 +
 gitweb/lib/Gitweb/Parse.pm      |  378 ++++
 gitweb/lib/Gitweb/RepoConfig.pm |  424 +++++
 gitweb/lib/Gitweb/Request.pm    |  153 ++
 gitweb/lib/Gitweb/Util.pm       |  447 +++++
 gitweb/lib/Gitweb/View.pm       | 1022 +++++++++++
 11 files changed, 3909 insertions(+), 3642 deletions(-)
 create mode 100644 gitweb/lib/Gitweb/Config.pm
 create mode 100644 gitweb/lib/Gitweb/Escape.pm
 create mode 100644 gitweb/lib/Gitweb/Format.pm
 create mode 100644 gitweb/lib/Gitweb/Git.pm
 create mode 100644 gitweb/lib/Gitweb/Parse.pm
 create mode 100644 gitweb/lib/Gitweb/RepoConfig.pm
 create mode 100644 gitweb/lib/Gitweb/Request.pm
 create mode 100644 gitweb/lib/Gitweb/Util.pm
 create mode 100644 gitweb/lib/Gitweb/View.pm

Keep up good work!

-- 
Jakub Narebski
Poland