On Thu, May 17, 2007 at 04:18:58AM CEST, Shawn O. Pearce wrote: > I have a couple of problems with the patch as-is. The first is > of course that the patch needs to be split into two; one patch for > the git-gui subdirectory itself and one for git.git. Hmm, why? It's an atomic change, one part doesn't make sense without the other. > My other problem is > > > ifeq ($(findstring $(MAKEFLAGS),s),s) > > @@ -92,7 +92,7 @@ install: all > > $(INSTALL) git-gui '$(DESTDIR_SQ)$(gitexecdir_SQ)' > > $(foreach p,$(GITGUI_BUILT_INS), rm -f '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' && ln '$(DESTDIR_SQ)$(gitexecdir_SQ)/git-gui' '$(DESTDIR_SQ)$(gitexecdir_SQ)/$p' ;) > > $(INSTALL) -d -m755 '$(DESTDIR_SQ)$(libdir_SQ)' > > - $(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)' > > + [ ! -e lib/tclIndex ] || $(INSTALL) -m644 lib/tclIndex '$(DESTDIR_SQ)$(libdir_SQ)' > > $(foreach p,$(ALL_LIBFILES), $(INSTALL) -m644 $p '$(DESTDIR_SQ)$(libdir_SQ)' ;) > > git-gui won't work if lib/tclIndex is missing or invalid. So not > installing it means we should just disable git-gui entirely. Aha, ouch - I understood that it is only an optimization. :-( So AIUI, there are several possibilities: (i) Makefile will autodecide on whether git-gui will be built+installed or not (ii) ./configure will, people not using configure and building on servers will be left to tweak config manually (iii) ./configure will, git-gui will default to not to be built and people not using configure and wanting git-gui will be left to tweak config manually I suspect that (ii) will be chosen, and even though I don't like it *personally* I guess it's the most reasonable approach for the general public. I didn't know that tclIndex is vital for git-gui when I submitted the patch, the /Makefile comment suggests otherwise. -- Petr "Pasky" Baudis Stuff: http://pasky.or.cz/ Ever try. Ever fail. No matter. // Try again. Fail again. Fail better. -- Samuel Beckett - 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
