Re: [PATCHv2 00/11] Splitting gitweb

[Jakub Narebski <jnareb@gmail.com>]

On Sun, Aug 1, 2010, Sverre Rabbelier wrote:
> On Thu, Jul 15, 2010 at 02:29, Pavan Kumar Sunkara <pavan.sss1991@gmail.com> 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@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.

>   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