Re: [PATCHv2 00/11] Splitting gitweb

On Sun, Aug 1, 2010, Sverre Rabbelier wrote:

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.

Comitted: 109988f (gitweb: fix esc_url, 2010-07-15)

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@gmail.com>
  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.

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.

Here it would be nice to have description which related subroutines were not included, and why.

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).

ACK-ed.

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.

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.

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.

Straighforward moving of format_* subroutines and avatar formatting. The only thing that could be improved is describing why avatar related subroutines were put there.

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.