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@xxxxxxxxx > > 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 -- 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