> > 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 -- 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