Re: [PATCHv2 GSOC 02/11] gitweb: Prepare for splitting gitweb

From: Jakub Narebski <jnareb@gmail.com>
Date: Jul 15, 2010, 18:05 UTC
Message-ID: <201007152005.54197.jnareb@gmail.com>
In-Reply-To: <1279178951-23712-3-git-send-email-pavan.sss1991@gmail.com>

On Thu, 15 Jul 2010, Pavan Kumar Sunkara wrote:

Show 11 quoted lines

> 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.
> 
> Signed-off-by: Jakub Narebski <jnareb@gmail.com>
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
> ---

First, it would be better if the comment from the cover letter about this patch (quoted below) was [also] put as the comment for this patch (i.e. in this area).

Show 12 quoted lines

> The second patch is produced by my commit ammend to Jakub Narębski's
> initial commit to prepare splitting of gitweb in the message-id:
> http://mid.gmane.org/1276531710-22945-4-git-send-email-jnareb@gmail.com
>
> 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.

Second, I don't quite understand the reason for your amendment. The difference is between two argument

  install [OPTION]... SOURCE DEST

in my original version of this patch, and two argument

  install [OPTION]... SOURCE DIRECTORY

after your change. In both cases we install _single_ file at once. What problems first version may cause if $(gitweblibdir) is used and it points elsewhere than $(gitwebdir)/lib?

Show 22 quoted lines

>  gitweb/Makefile    |    3 +++
>  gitweb/gitweb.perl |    9 +++++++++
>  2 files changed, 12 insertions(+), 0 deletions(-)
> 
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index d2584fe..c7610b3 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -55,6 +55,7 @@ PERL_PATH  ?= /usr/bin/perl
>  bindir_SQ = $(subst ','\'',$(bindir))#'
>  gitwebdir_SQ = $(subst ','\'',$(gitwebdir))#'
>  gitwebstaticdir_SQ = $(subst ','\'',$(gitwebdir)/static)#'
> +gitweblibdir_SQ = $(subst ','\'',$(gitwebdir)/lib)#'
>  SHELL_PATH_SQ = $(subst ','\'',$(SHELL_PATH))#'
>  PERL_PATH_SQ  = $(subst ','\'',$(PERL_PATH))#'
>  DESTDIR_SQ    = $(subst ','\'',$(DESTDIR))#'
> @@ -150,6 +151,8 @@ install: all
>  	$(INSTALL) -m 755 $(GITWEB_PROGRAMS) '$(DESTDIR_SQ)$(gitwebdir_SQ)'
>  	$(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
>  	$(INSTALL) -m 644 $(GITWEB_FILES) '$(DESTDIR_SQ)$(gitwebstaticdir_SQ)'
> +	$(foreach dir,$(sort $(dir $(GITWEB_MODULES))),test -d '$(DESTDIR_SQ)$(gitwebdir_SQ)/$(dir)' || $(INSTALL) -d -m 755 '$(DESTDIR_SQ)$(gitwebdir_SQ)/$(dir)';)
> +	$(foreach mod,$(GITWEB_MODULES),$(INSTALL) -m 644 $(mod) '$(DESTDIR_SQ)$(gitwebdir_SQ)/$(dir $(mod))';)

Third, if you were folloring git mailing list (or at least patches for gitweb on it), you would notice 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

which uses shell for loop, instead of make's $(foreach ...) function to avoid _possible_ problems with generating a command line that exceeded the maximum argument list length, as explained in comment section of

  http://thread.gmane.org/gmane.comp.version-control.git/150463/focus=150463

Note that this version uses SOURCE DIRECTORY version rather than SOURCE DEST, like in your patch... probably unnecessary.

Show 30 quoted lines

>  
>  ### Cleaning rules
>  
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 518328f..bda7da3 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -9,6 +9,14 @@
>  
>  use strict;
>  use warnings;
> +
> +use File::Spec;
> +# __DIR__ is taken from Dir::Self __DIR__ fragment
> +sub __DIR__ () {
> +	File::Spec->rel2abs(join '', (File::Spec->splitpath(__FILE__))[0, 1]);
> +}
> +use lib __DIR__ . '/lib';
> +
>  use CGI qw(:standard :escapeHTML -nosticky);
>  use CGI::Util qw(unescape);
>  use CGI::Carp qw(fatalsToBrowser set_message);
> @@ -16,6 +24,7 @@ use Encode;
>  use Fcntl ':mode';
>  use File::Find qw();
>  use File::Basename qw(basename);
> +
>  binmode STDOUT, ':utf8';
>  
>  our $t0;

In "[PATCHv3/RFC] gitweb: Prepare for splitting gitweb" this spurious change was removed from patch.

Anyway, eventual replacing of this patch by other version should be fairly easy, so it shouldn't hold this series.

-- 
Jakub Narebski
Poland