2010/6/22 Jakub Narebski <jnareb@xxxxxxxxx>: > 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. Thanks. >> >> 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. The dependency is written in individual commits. >> >> 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 !!! > ^^ ^^ ok. >> >> >> 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? yes. >> 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... actually that message has been written by you itself in one of the earlier threads. >> >> This module is intended as standalone module, which does n>ot 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)? Yes, due to either unment dependency or circular dependency problem. But no need to worry as they are very small. >> >> 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. That's what I thought. >> 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; > } > Yes. I think we can but don't you think that we need to import $actions everytime we use validate_action. So, I figured it would be better If i leave it untouched for now. > 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... But they are being used in Gitweb::Request subroutines, so I included them in there. >> >> 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)... It would result in circular dependency. So, Escape module is best for it's place. >> 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? There is no single good place. Anyhow, as they are regarding project configuration, I thought it would be better place for it. >> >> 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? action specific HTML divs. They include format_* and parse_* subs. >> >> 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... Ok. >> >> 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. Well. It was in the utility category in gitweb.perl script. So, I added it in here without giving much thought. >> >> 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?). $hash, $hash_parent also. >> >> 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. for die_error. >> >> 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 > So, If you can tell me what I need to change in this series, I would like to complete it and send a second version so that you can ack it and junio can merge it soon. Thanks, Pavan. -- 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