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

[Jakub Narebski <jnareb@gmail.com>]

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