[PATCH GSoC] gitweb: Add global installation target for gitweb

[PATCH GSoC] gitweb: Add global installation target for gitweb

[Pavan Kumar Sunkara]

The current installation of gitweb requires us to give it a
target directory. But splitting of gitweb makes it difficult
for git instaweb to continue with the current method.

This commit allow installation of gitweb files into the target
'$(sharedir)/gitweb' by default when user type 'make install'.
This target act as root directory for instaweb servers.

Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
---

This is necessary step to acheive the goals of my GSoC project.
Currently instaweb script creates gitweb.* files in every repository
which is unnecessary. So if we have global folder for gitweb files we
can configure the instaweb server root to point to that direction.

 Makefile |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/Makefile b/Makefile
index de7f680..0b262a9 100644
--- a/Makefile
+++ b/Makefile
@@ -269,6 +269,7 @@ mandir = share/man
 infodir = share/info
 gitexecdir = libexec/git-core
 sharedir = $(prefix)/share
+gitwebdir = $(sharedir)/gitweb
 template_dir = share/git-core/templates
 htmldir = share/doc/git-doc
 ifeq ($(prefix),/usr)
@@ -1971,6 +1972,7 @@ install: all
 	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
 ifndef NO_PERL
 	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
+	$(MAKE) -C gitweb gitwebdir=$(gitwebdir) install
 endif
 ifndef NO_PYTHON
 	$(MAKE) -C git_remote_helpers prefix='$(prefix_SQ)'
DESTDIR='$(DESTDIR_SQ)' install
-- 
1.7.1.16.g5d405c.dirty

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Jakub Narebski]

Cc-ed Eric Wong, the main author and maintainer of git-instaweb

In short: I think that this patch should be split into two patches, one
which sets default value of 'gitwebdir' (in Makefile or gitweb/Makefile;
please explain why you chosen one or the other), and second that "fixes"
git-instaweb (and might include installing gitweb, in $(gitwebdir) or in
$(sharedir)/gitweb).


On Thu, 13 May 2010, Pavan Kumar Sunkara wrote:

> Subject: gitweb: Add global installation target for gitweb

The name "target" is here a bit misleading in the context of Makefile.
You meant that you set default installation destination (default
installation directory) for install-gitweb target in Makefile, by adding
default value of 'gitwebdir' variable.  To clarify this issue I think it
would be better to write:

  gitweb: Set default destination directory for installing gitweb

or something like that.

>
> The current installation of gitweb requires us to give it a
> target directory. But splitting of gitweb makes it difficult
> for git instaweb to continue with the current method.

This paragraph has not a best grammar, and is also slightly inaccurate.
Also having default value for build variables (for a destination
directories for install targets) is a good thing in itself, independent
of the issue of git-instaweb and splitting gitweb.

What about this?

  Currently installing gitweb requires to give a target directory
  (via 'gitwebdir' build variable).  Giving it a default value
  protects against user errors.

  Also git-instaweb in its current form (re)creates gitweb.cgi and
  (some of) required static files in $GIT_DIR/gitweb/ directory for
  each repository it is ran.  Splitting gitweb would make it difficult
  for git-instaweb to continue with this method.

See also comment about git-instaweb below.

> 
> This commit allow installation of gitweb files into the target
> '$(sharedir)/gitweb' by default when user type 'make install'.

I would phrase it a bit differently myself, emphasizing that 
"make install" would now also install gitweb (!):

  This commit sets default installation directory for gitweb files
  to "$(sharedir)/gitweb".  The 'install' target ("make install")
  now also installs gitweb. 

> This target act as root directory for instaweb servers.

It does not, as you have not provided required changes to
git-instaweb.sh and 'git-instaweb' target in main Makefile.

> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

Two things.

First, I think providing default value for 'gitwebdir' could be I good
idea.  I think that all other build variables containing installation
directories have default values.  But I do wonder whether the
"$(sharedir)/gitweb" is a good default value for 'gitwebdir' (see also
comment about git-instaweb below).


Second, the issue with git-instaweb in its current form, and splitting
gitweb is very important.  I am not sure though if this is correct
solution for this problem.  It is NOT A FULL SOLUTION, that is sure.

The gitweb.cgi file that git-instaweb puts into $GIT_DIR/gitweb/gitweb.cgi
is *modified* to browse given git repository; the gitweb_cgi() function
in git-instaweb that creates gitweb.cgi also changes $projectroot in it:

  script='
  s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
  s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
  s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
  s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'

  gitweb_cgi () {
        cat > "$1.tmp" <<\EOFGITWEB
  @@GITWEB_CGI@@
  EOFGITWEB
        # Use the configured full path to perl to match the generated
        # scripts' 'hashpling' line
        "$PERL" -p -e "$script" "$1.tmp"  > "$1" # <------ '-e "$script"'
        chmod +x "$1"
        rm -f "$1.tmp"
  }

So in its current for git-instaweb wouldn't be able to use generic
gitweb.cgi that is installed in "$(sharedir)/gitweb".

Also git-instaweb when run (when starting server) puts appropriate
configuration into $GIT_DIR/gitweb/httpd.conf (the same file for any web
server chosen).  This configuration includes paths to static files that
gitweb requires, and that currently git-instaweb puts (installs) in
$GIT_DIR/gitweb (in $fqgitdir).  So you would need to change that for a
full solution of git-instaweb problem.

The solution to modifying gitweb.cgi in git-instaweb could be for
git-instaweb to put (install) appropriately configured
gitweb_config.perl file into $GIT_DIR/gitweb, and set GITWEB_CONFIG (and
export) environmental variable to it inside git-instaweb.


By the way, perhaps in the future split gitweb should install its
subpackages (the *.pm files) in the same place that Git.pm is installed,
and for gitweb.perl to have

  use lib (split(/:/, $ENV{GITPERLLIB}));

Or not.  Food for though.

> ---
> 
> This is necessary step to achieve the goals of my GSoC project.
> Currently instaweb script creates gitweb.* files in every repository
> which is unnecessary. So if we have global folder for gitweb files we
> can configure the instaweb server root to point to that direction.

*Can* configure, but you actually doesn't do this.

> 
>  Makefile |    2 ++
>  1 files changed, 2 insertions(+), 0 deletions(-)
> 
> diff --git a/Makefile b/Makefile
> index de7f680..0b262a9 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -269,6 +269,7 @@ mandir = share/man
>  infodir = share/info
>  gitexecdir = libexec/git-core
>  sharedir = $(prefix)/share
> +gitwebdir = $(sharedir)/gitweb

Why in Makefile, and not in gitweb/Makefile?

>  template_dir = share/git-core/templates
>  htmldir = share/doc/git-doc
>  ifeq ($(prefix),/usr)
> @@ -1971,6 +1972,7 @@ install: all
>  	$(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
>  ifndef NO_PERL
>  	$(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
> +	$(MAKE) -C gitweb gitwebdir=$(gitwebdir) install

First, I think that gitwebdir=$(gitwebdir) is not necessary here. Make
automatically passes variables to invoked submakes. And then it would be
as easy as adding 'install-gitweb' (or new 'install-git-instaweb') as
additional dependency for 'install' target.

Second, why don't you set default value of 'gitwebdir' in gitweb/Makefile
instead?

The above are questions that needs to be answered, but not necessarily
require changes to patch.


Please also note that if user wants to install gitweb in for example
'/var/www/cgi-bin', and to do that sets "gitwebdir=/var/www/cgi-bin" in
config.mak file, so that it would be present for "make install" (and not
only "make install-gitweb"), gitweb files would be not present in
"$(sharedir)/gitweb" for git-instaweb to find (well, unless git-instaweb
search for them in "$(gitwebdir)" instead...).


Most important of all, without changes to git-instaweb.sh and
git-instaweb target in main Makefile, installing gitweb by default, be
it

  install: all install-gitweb

or

  	$(MAKE) -C gitweb gitwebdir="$(sharedir)/gitweb" install

doesn't make absoultely no sense.

NAK to this part of patch.

>  endif
>  ifndef NO_PYTHON
>  	$(MAKE) -C git_remote_helpers prefix='$(prefix_SQ)'
> DESTDIR='$(DESTDIR_SQ)' install
> -- 
> 1.7.1.16.g5d405c.dirty
> 

-- 
Jakub Narebski
Poland

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Pavan Kumar Sunkara]

On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> Cc-ed Eric Wong, the main author and maintainer of git-instaweb
>
> In short: I think that this patch should be split into two patches, one
> which sets default value of 'gitwebdir' (in Makefile or gitweb/Makefile;
> please explain why you chosen one or the other), and second that "fixes"
> git-instaweb (and might include installing gitweb, in $(gitwebdir) or in
> $(sharedir)/gitweb).

Yes, I agree. This is the first patch.
The second patch which fixes git-instaweb is in discussion with my
mentors. after that I will be sending it to the git mailing list.

I choose Makefile rather than chossing gitweb/Makefile becuase
git-instaweb is a package of git not a package of gitweb. So, I choose
Makefile rather than choosing gitweb/Makefile

> On Thu, 13 May 2010, Pavan Kumar Sunkara wrote:
>
>> Subject: gitweb: Add global installation target for gitweb
>
> The name "target" is here a bit misleading in the context of Makefile.
> You meant that you set default installation destination (default
> installation directory) for install-gitweb target in Makefile, by adding
> default value of 'gitwebdir' variable.  To clarify this issue I think it
> would be better to write:
>
>  gitweb: Set default destination directory for installing gitweb
>
> or something like that.

Ok.

>> The current installation of gitweb requires us to give it a
>> target directory. But splitting of gitweb makes it difficult
>> for git instaweb to continue with the current method.
>
> This paragraph has not a best grammar, and is also slightly inaccurate.
> Also having default value for build variables (for a destination
> directories for install targets) is a good thing in itself, independent
> of the issue of git-instaweb and splitting gitweb.
>
> What about this?
>
>  Currently installing gitweb requires to give a target directory
>  (via 'gitwebdir' build variable).  Giving it a default value
>  protects against user errors.
>
>  Also git-instaweb in its current form (re)creates gitweb.cgi and
>  (some of) required static files in $GIT_DIR/gitweb/ directory for
>  each repository it is ran.  Splitting gitweb would make it difficult
>  for git-instaweb to continue with this method.
>
> See also comment about git-instaweb below.
>
>>
>> This commit allow installation of gitweb files into the target
>> '$(sharedir)/gitweb' by default when user type 'make install'.
>
> I would phrase it a bit differently myself, emphasizing that
> "make install" would now also install gitweb (!):
>
>  This commit sets default installation directory for gitweb files
>  to "$(sharedir)/gitweb".  The 'install' target ("make install")
>  now also installs gitweb.
>
>> This target act as root directory for instaweb servers.
>
> It does not, as you have not provided required changes to
> git-instaweb.sh and 'git-instaweb' target in main Makefile.
>
>>
>> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>

Ok.

> Two things.
>
> First, I think providing default value for 'gitwebdir' could be I good
> idea.  I think that all other build variables containing installation
> directories have default values.  But I do wonder whether the
> "$(sharedir)/gitweb" is a good default value for 'gitwebdir' (see also
> comment about git-instaweb below).

I chose it because the lib files of gitk and git-gui are being
installed in sharedir.
I am happy to accept if you have any other better idea. :-)

> Second, the issue with git-instaweb in its current form, and splitting
> gitweb is very important.  I am not sure though if this is correct
> solution for this problem.  It is NOT A FULL SOLUTION, that is sure.
>

Yes, It is not a full solution. There is another patch that is
currently in discussion with my mentors.
Petr and Christian told me to make sure that I send patches as small
as possibl so that they will be merged into the mainstream. That is my
GSoC aim too.

So, I sent this small patch which justs installs gitweb with git's
"make install" without breaking the git-instaweb.sh script.
The patch for modifying git-instaweb will be sent soon to his mailing list.

> The gitweb.cgi file that git-instaweb puts into $GIT_DIR/gitweb/gitweb.cgi
> is *modified* to browse given git repository; the gitweb_cgi() function
> in git-instaweb that creates gitweb.cgi also changes $projectroot in it:
>
>  script='
>  s#^(my|our) \$projectroot =.*#$1 \$projectroot = "'$(dirname "$fqgitdir")'";#;
>  s#(my|our) \$gitbin =.*#$1 \$gitbin = "'$GIT_EXEC_PATH'";#;
>  s#(my|our) \$projects_list =.*#$1 \$projects_list = \$projectroot;#;
>  s#(my|our) \$git_temp =.*#$1 \$git_temp = "'$fqgitdir/gitweb/tmp'";#;'
>
>  gitweb_cgi () {
>        cat > "$1.tmp" <<\EOFGITWEB
>  @@GITWEB_CGI@@
>  EOFGITWEB
>        # Use the configured full path to perl to match the generated
>        # scripts' 'hashpling' line
>        "$PERL" -p -e "$script" "$1.tmp"  > "$1" # <------ '-e "$script"'
>        chmod +x "$1"
>        rm -f "$1.tmp"
>  }
>
> So in its current for git-instaweb wouldn't be able to use generic
> gitweb.cgi that is installed in "$(sharedir)/gitweb".
>
> Also git-instaweb when run (when starting server) puts appropriate
> configuration into $GIT_DIR/gitweb/httpd.conf (the same file for any web
> server chosen).  This configuration includes paths to static files that
> gitweb requires, and that currently git-instaweb puts (installs) in
> $GIT_DIR/gitweb (in $fqgitdir).  So you would need to change that for a
> full solution of git-instaweb problem.
>
> The solution to modifying gitweb.cgi in git-instaweb could be for
> git-instaweb to put (install) appropriately configured
> gitweb_config.perl file into $GIT_DIR/gitweb, and set GITWEB_CONFIG (and
> export) environmental variable to it inside git-instaweb.
>

As we discussed earlier, I will be making use of git-instaweb script
and gitweb to create a local server for user which takes a file list
of repositories and provide client UI for them. According to this, the
user needs to execute 'git instaweb' only once, not in every
repository.

The next patch will take care of modifying git-instaweb.sh and
configuring gitweb.perl

> By the way, perhaps in the future split gitweb should install its
> subpackages (the *.pm files) in the same place that Git.pm is installed,
> and for gitweb.perl to have
>
>  use lib (split(/:/, $ENV{GITPERLLIB}));
>
> Or not.  Food for though.
>
>> ---
>>
>> This is necessary step to achieve the goals of my GSoC project.
>> Currently instaweb script creates gitweb.* files in every repository
>> which is unnecessary. So if we have global folder for gitweb files we
>> can configure the instaweb server root to point to that direction.
>
> *Can* configure, but you actually doesn't do this.
>
>>
>>  Makefile |    2 ++
>>  1 files changed, 2 insertions(+), 0 deletions(-)
>>
>> diff --git a/Makefile b/Makefile
>> index de7f680..0b262a9 100644
>> --- a/Makefile
>> +++ b/Makefile
>> @@ -269,6 +269,7 @@ mandir = share/man
>>  infodir = share/info
>>  gitexecdir = libexec/git-core
>>  sharedir = $(prefix)/share
>> +gitwebdir = $(sharedir)/gitweb
>
> Why in Makefile, and not in gitweb/Makefile?
>
>>  template_dir = share/git-core/templates
>>  htmldir = share/doc/git-doc
>>  ifeq ($(prefix),/usr)
>> @@ -1971,6 +1972,7 @@ install: all
>>       $(MAKE) -C templates DESTDIR='$(DESTDIR_SQ)' install
>>  ifndef NO_PERL
>>       $(MAKE) -C perl prefix='$(prefix_SQ)' DESTDIR='$(DESTDIR_SQ)' install
>> +     $(MAKE) -C gitweb gitwebdir=$(gitwebdir) install
>
> First, I think that gitwebdir=$(gitwebdir) is not necessary here. Make
> automatically passes variables to invoked submakes. And then it would be
> as easy as adding 'install-gitweb' (or new 'install-git-instaweb') as
> additional dependency for 'install' target.

So, you want to me to do something like this ?
+     $(MAKE) -c gitweb gitwebdir=$(sharedir)/gitweb install

> Second, why don't you set default value of 'gitwebdir' in gitweb/Makefile
> instead?

It's explained above.
I don't want the user to install gitweb specifically, from now onwards
gitweb will installed along with main program so that git-instaweb
script will be working properly and the users will be happy to get a
web client along with git.

> The above are questions that needs to be answered, but not necessarily
> require changes to patch.
>
>
> Please also note that if user wants to install gitweb in for example
> '/var/www/cgi-bin', and to do that sets "gitwebdir=/var/www/cgi-bin" in
> config.mak file, so that it would be present for "make install" (and not
> only "make install-gitweb"), gitweb files would be not present in
> "$(sharedir)/gitweb" for git-instaweb to find (well, unless git-instaweb
> search for them in "$(gitwebdir)" instead...).

User need to specify instaweb.root in config file to point to the
gitweb files which defaults to /usr/share/gitweb
This is covered in my next patch.

> Most important of all, without changes to git-instaweb.sh and
> git-instaweb target in main Makefile, installing gitweb by default, be
> it
>
>  install: all install-gitweb
>
> or
>
>        $(MAKE) -C gitweb gitwebdir="$(sharedir)/gitweb" install
>
> doesn't make absoultely no sense.

Yes, it doesn't make sense. But petr and christian asked me to send my
patches as small as possible without breaking any of the current
functionalities. That is the reason I sent this.

I need to complete my GSoC, so you can be sure that every patch of
mine has a reason.

> NAK to this part of patch.
>

Just rethink about this or you can wait until my next patch :-)

Thanks
- Pavan

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Jakub Narebski]

On Fri, May 14, 2010, Pavan Kumar Sunkara wrote:
> On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > Cc-ed Eric Wong, the main author and maintainer of git-instaweb
> >
> > In short: I think that this patch should be split into two patches, one
> > which sets default value of 'gitwebdir' (in Makefile or gitweb/Makefile;
> > please explain why you chosen one or the other), and second that "fixes"
> > git-instaweb (and might include installing gitweb, in $(gitwebdir) or in
> > $(sharedir)/gitweb).
> 
> Yes, I agree. This is the first patch.
>
> The second patch which fixes git-instaweb is in discussion with my
> mentors. after that I will be sending it to the git mailing list.

I agree about splitting the patch.

What I disagree with is having addition of installing gitweb in first
patch. Especially that (as I think was shown in discussion) git-instaweb
must know where gitweb.cgi it can use is installed, so it has to be
synchronized.


P.S. About whether to add default value for 'gitwebdir' to Makefile or
to gitweb/Makefile - why not add it to both?

-- 
Jakub Narebski
Poland

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Jakub Narebski]

On Fri, 14 May 2010, Pavan Kumar Sunkara wrote:
> On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski <jnareb@gmail.com> wrote:
> >
> > In short: I think that this patch should be split into two patches, one
> > which sets default value of 'gitwebdir' (in Makefile or gitweb/Makefile;
> > please explain why you chosen one or the other), and second that "fixes"
> > git-instaweb (and might include installing gitweb, in $(gitwebdir) or in
> > $(sharedir)/gitweb).
> 
> Yes, I agree. This is the first patch.
>
> The second patch which fixes git-instaweb is in discussion with my
> mentors. after that I will be sending it to the git mailing list.

As I said, I agree with the reasoning and the fact that patch is split
into two. What I disagree with is the splitting itself.

Making 'install-gitweb' prerequisite to 'install' target, or in other
words adding installing gitweb *somewhere*, should in my opinion by in
this second patch.
 
> I choose Makefile rather than choosing gitweb/Makefile becuase
> git-instaweb is a package of git not a package of gitweb. So, I choose
> Makefile rather than choosing gitweb/Makefile.

This is important fact; the description that 'gitwebdir' has default
value set (also) in Makefile because of the future change to the way
git-instaweb is build / installed should be in commit message of a new
first patch.


BTW. if 'gitwebdir' would be set to some default value both in Makefile
and in gitweb/Makefile, then in gitweb/Makefile the "?=" operator must
be used (set if undefined).

Note that default value in Makefile can be "$(sharedir)/gitweb", and in
gitweb/Makefile can be '/var/www/cgi-bin'.

[...]
> > Two things.
> >
> > First, I think providing default value for 'gitwebdir' could be I good
> > idea.  I think that all other build variables containing installation
> > directories have default values.  But I do wonder whether the
> > "$(sharedir)/gitweb" is a good default value for 'gitwebdir' (see also
> > comment about git-instaweb below).
> 
> I chose it because the lib files of gitk and git-gui are being
> installed in sharedir.

O.K., I can agree with this. You might want to put this substantiation
in the commit message, though.

> > Second, the issue with git-instaweb in its current form, and splitting
> > gitweb is very important.  I am not sure though if this is correct
> > solution for this problem.  It is NOT A FULL SOLUTION, that is sure.
> 
> Yes, It is not a full solution. There is another patch that is
> currently in discussion with my mentors.
>
> Petr and Christian told me to make sure that I send patches as small
> as possibl so that they will be merged into the mainstream. That is my
> GSoC aim too.
> 
> So, I sent this small patch which just installs gitweb with git's
> "make install" without breaking the git-instaweb.sh script.
> The patch for modifying git-instaweb will be sent soon to his mailing list.

That is a good practice, and a good idea.

My complaints are about putting too much in this first patch (I think
that making 'install' target install gitweb should be put in the commit
that changes git-instaweb, as those two changes have to be
synchronized), and about that commit message seems to claim that this
commit does more than it really does.

-- 
Jakub Narebski
Poland

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Pavan Kumar Sunkara]

>
> As I said, I agree with the reasoning and the fact that patch is split
> into two. What I disagree with is the splitting itself.
>
> Making 'install-gitweb' prerequisite to 'install' target, or in other
> words adding installing gitweb *somewhere*, should in my opinion by in
> this second patch.
>
>> I choose Makefile rather than choosing gitweb/Makefile becuase
>> git-instaweb is a package of git not a package of gitweb. So, I choose
>> Makefile rather than choosing gitweb/Makefile.
>
> This is important fact; the description that 'gitwebdir' has default
> value set (also) in Makefile because of the future change to the way
> git-instaweb is build / installed should be in commit message of a new
> first patch.
>
>
> BTW. if 'gitwebdir' would be set to some default value both in Makefile
> and in gitweb/Makefile, then in gitweb/Makefile the "?=" operator must
> be used (set if undefined).
>
> Note that default value in Makefile can be "$(sharedir)/gitweb", and in
> gitweb/Makefile can be '/var/www/cgi-bin'.
>
> [...]
>> > Two things.
>> >
>> > First, I think providing default value for 'gitwebdir' could be I good
>> > idea.  I think that all other build variables containing installation
>> > directories have default values.  But I do wonder whether the
>> > "$(sharedir)/gitweb" is a good default value for 'gitwebdir' (see also
>> > comment about git-instaweb below).
>>
>> I chose it because the lib files of gitk and git-gui are being
>> installed in sharedir.
>
> O.K., I can agree with this. You might want to put this substantiation
> in the commit message, though.
>
>> > Second, the issue with git-instaweb in its current form, and splitting
>> > gitweb is very important.  I am not sure though if this is correct
>> > solution for this problem.  It is NOT A FULL SOLUTION, that is sure.
>>
>> Yes, It is not a full solution. There is another patch that is
>> currently in discussion with my mentors.
>>
>> Petr and Christian told me to make sure that I send patches as small
>> as possibl so that they will be merged into the mainstream. That is my
>> GSoC aim too.
>>
>> So, I sent this small patch which just installs gitweb with git's
>> "make install" without breaking the git-instaweb.sh script.
>> The patch for modifying git-instaweb will be sent soon to his mailing list.
>
> That is a good practice, and a good idea.
>
> My complaints are about putting too much in this first patch (I think
> that making 'install' target install gitweb should be put in the commit
> that changes git-instaweb, as those two changes have to be
> synchronized), and about that commit message seems to claim that this
> commit does more than it really does.
>
> --
> Jakub Narebski
> Poland
>

So, what I need to do in this patch is add default values. Not
installing in 'install' target. ??
Or any thing else.

If you could specify what needs to be done, I will be happy resend the patch.

- Pavan

Re: [PATCH GSoC] gitweb: Add global installation target for gitweb

[Jakub Narebski]

On Sat, 15 May 2010, Pavan Kumar Sunkara wrote:

> So, what I need to do in this patch is add default values. Not
> installing in 'install' target. ??
> Or any thing else.
> 
> If you could specify what needs to be done, I will be happy resend
> the patch. 

To get my ACK for this patch it is enough to *not* add installing gitweb
to 'install' target in main Makefile.  IMVHO it should be left for next
patch in this series.


P.S. By the way, you should write in comments to patch, i.e. between 
"---" separator and the diffstat, that this patch is based on 'next'
branch, or perhaps even that it is based on commit 152d943 in 
'jn/gitweb-install'.

>From Documentation/SubmittingChanges:

   If you are preparing a work based on "next" branch, that is fine,
   but please mark it as such.
              ^^^^^^^^^^^^^^^

Keep up good work!
-- 
Jakub Narebski
Poland