On Sun, Aug 1, 2010, Sverre Rabbelier wrote: > On Thu, Jul 15, 2010 at 02:29, Pavan Kumar Sunkara <pavan.sss1991@xxxxxxxxx> wrote: > > 10 patches out of the 11 patches in this patch series split gitweb into > > several small sized modules > > What is the status of this series? Is anyone going to carry it > forward? I remember multiple people wishing for gitweb to be more > modular to make it easier to hack on? What shape is the series in, is > it almost done, or will it need a lot more work? It is almost done. The only major thing seems to be the "Prepare for splitting gitweb" patch, which should be I think replaced by updated version that uses shell loop in place of make's $(foreach ...) function to avoid possibility with generating a command line that exceeded the maximum argument list length. Might be unnecessary. Most if not all comments that I have to this version of series was about commit messages, not the content of the patch itself. If Pavan would not do re-roll of this series in a few days time, I will pick it up myself, and resubmit. > Pavan Kumar Sunkara (11): > gitweb: fix esc_url Comitted: 109988f (gitweb: fix esc_url, 2010-07-15) > gitweb: Prepare for splitting gitweb To be replaced by new version of my "gitweb: Prepare for splitting gitweb" patch: "[PATCHv3/RFC] gitweb: Prepare for splitting gitweb" Message-ID: <201007080920.38724.jnareb@xxxxxxxxx> http://thread.gmane.org/gmane.comp.version-control.git/150463/focus=150544 Also it would need explanation that moving from install SOURCE DEST to install SOURCE DIRECTORY was done for security reason (I think), and because we cannot use portably `-T' / `--no-target-directory' option to install, as 1.) it is GNU-ism, 2.) it is not present in older GNU install. > gitweb: Create Gitweb::Git module There was an issue about where to put descriptions of build-time variables: in gitweb.perl or in individual modules. I think the issue was addressed somewhat in later commit, but I think it should be addressed here too. > gitweb: Create Gitweb::Config module Here it would be nice to have description which related subroutines were not included, and why. > gitweb: Create Gitweb::Request module It is quite strange for me that evaluate_query_params is in this module, but evaluate_path_info is not. Also this module needs to be updated to newer codebase (lacks reset_timer subroutine). > gitweb: Create Gitweb::Escape module ACK-ed. > gitweb: Create Gitweb::RepoConfig module I'd like to have here better description of the intent behind this module, i.e. what kinds of subroutines it is to contain. Here, and in later commits, the modules it depends on are named like Git.pm instead of Gitweb::Git in the commit message. This is a minor issue. > gitweb: Create Gitweb::View module 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, it should be stated so in the commit message. Also in the commit messages dividing moved subroutines into groups should be done better. > gitweb: Create Gitweb::Util module Just a question: shouldn't git_get_last_activity subroutine be in Gitweb::RepoConfig module? Or is Gitweb::RepoConfig only about "static" properties of a repository? This is result of not well enough described goal of Gitweb::RepoConfig module, see above. > gitweb: Create Gitweb::Format module Straighforward moving of format_* subroutines and avatar formatting. The only thing that could be improved is describing why avatar related subroutines were put there. > gitweb: Create Gitweb::Parse module The commit message might be improved by stating that it is output of git commands that gets parsed. There is also odd duck of parsed_difftree_line subroutine. -- 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