On Fri, 14 May 2010, Pavan Kumar Sunkara wrote: > On Fri, May 14, 2010 at 8:37 PM, Jakub Narebski <jnareb@xxxxxxxxx> 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 -- To unsubscribe from this list: send the line "unsubscribe git" in the body of a message to majordomo@xxxxxxxxxxxxxxx More majordomo info at http://vger.kernel.org/majordomo-info.html