{"thread":{"id":"23279","subject":"[PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","startedAt":"2010-04-01T05:36:03Z","lastAt":"2010-04-15T01:42:56Z","messageCount":15,"participants":["Mark Rada","Jakub Narebski","Charles Bailey","Junio C Hamano"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"138321","messageId":"4BB430C3.9030000@mailservices.uwaterloo.ca","threadId":"23279","inReplyTo":null,"subject":"[PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2010-04-01T05:36:03Z","receivedAt":"2010-04-01T05:36:03Z","isPatch":false,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"The build system added support minifying gitweb.js through a\nJavaScript minifier, but most minifiers come with support to\nminify CSS files as well, so we should use it if we can.\n\nThis patch will add the same facilities to gitweb.css that\ngitweb.js has for minification. That does not mean that they\nwill use the same minifier though, as it is not safe to assume\nthat all JavaScript minifiers will also minify CSS files.\n\nThough the bandwidth savings will not be as dramatic as with\nthe JavaScript minifier, every byte saved is important.\n\nSigned-off-by: Mark Rada <marada@uwaterloo.ca>\n\n---\n\nChanges since v4:\n\t- Reworded some of the comments and documentation\n\n\n Makefile        |   21 +++++++++++++++------\n gitweb/INSTALL  |    5 +++++\n gitweb/Makefile |   28 +++++++++++++++++++++-------\n gitweb/README   |    3 ++-\n 4 files changed, 43 insertions(+), 14 deletions(-)\n\ndiff --git a/Makefile b/Makefile\nindex 5384d33..450e4df 100644\n--- a/Makefile\n+++ b/Makefile\n@@ -203,6 +203,9 @@ all::\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n+# Define CSSMIN to point to a CSS minifier in order to generate a minified\n+# version of gitweb.css\n+#\n # Define DEFAULT_PAGER to a sensible pager command (defaults to \"less\") if\n # you want to use something different.  The value will be interpreted by the\n # shell at runtime when it is used.\n@@ -279,8 +282,9 @@ lib = lib\n # DESTDIR=\n pathsep = :\n \n-# JavaScript minifier invocation that can function as filter\n+# JavaScript/CSS minifier invocation that can function as filter\n JSMIN =\n+CSSMIN =\n \n export prefix bindir sharedir sysconfdir\n \n@@ -1564,18 +1568,23 @@ gitweb:\n \t$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all\n \n ifdef JSMIN\n-OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js\n-gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js\n-else\n-OTHER_PROGRAMS += gitweb/gitweb.cgi\n-gitweb/gitweb.cgi: gitweb/gitweb.perl\n+GITWEB_PROGRAMS += gitweb/gitweb.min.js\n endif\n+ifdef CSSMIN\n+GITWEB_PROGRAMS += gitweb/gitweb.min.css\n+endif\n+OTHER_PROGRAMS +=  gitweb/gitweb.cgi  $(GITWEB_PROGRAMS)\n+gitweb/gitweb.cgi: gitweb/gitweb.perl $(GITWEB_PROGRAMS)\n \t$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)\n \n ifdef JSMIN\n gitweb/gitweb.min.js: gitweb/gitweb.js\n \t$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)\n endif # JSMIN\n+ifdef CSSMIN\n+gitweb/gitweb.min.css: gitweb/gitweb.css\n+\t$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)\n+endif # CSSMIN\n \n \n git-instaweb: git-instaweb.sh gitweb/gitweb.cgi gitweb/gitweb.css gitweb/gitweb.js\ndiff --git a/gitweb/INSTALL b/gitweb/INSTALL\nindex b76a0cf..b75a90b 100644\n--- a/gitweb/INSTALL\n+++ b/gitweb/INSTALL\n@@ -66,6 +66,11 @@ file for gitweb (in gitweb/README).\n   build configuration variables. By default gitweb tries to find them\n   in the same directory as gitweb.cgi script.\n \n+- You can optionally generate a minified version of gitweb.css by defining\n+  the CSSMIN build configuration variable. By default the non-minified\n+  version of gitweb.css will be used. NOTE: if you enable this option,\n+  substitute gitweb.min.css for all uses of gitweb.css in the help files.\n+\n Build example\n ~~~~~~~~~~~~~\n \ndiff --git a/gitweb/Makefile b/gitweb/Makefile\nindex c9eb1ee..fffe700 100644\n--- a/gitweb/Makefile\n+++ b/gitweb/Makefile\n@@ -6,13 +6,17 @@ all::\n # Define JSMIN to point to JavaScript minifier that functions as\n # a filter to have gitweb.js minified.\n #\n+# Define CSSMIN to point to a CSS minifier in order to generate a minified\n+# version of gitweb.css\n+#\n \n prefix ?= $(HOME)\n bindir ?= $(prefix)/bin\n RM ?= rm -f\n \n-# JavaScript minifier invocation that can function as filter\n+# JavaScript/CSS minifier invocation that can function as filter\n JSMIN ?=\n+CSSMIN ?=\n \n # default configuration for gitweb\n GITWEB_CONFIG = gitweb_config.perl\n@@ -26,7 +30,11 @@ GITWEB_STRICT_EXPORT =\n GITWEB_BASE_URL =\n GITWEB_LIST =\n GITWEB_HOMETEXT = indextext.html\n+ifdef CSSMIN\n+GITWEB_CSS = gitweb.min.css\n+else\n GITWEB_CSS = gitweb.css\n+endif\n GITWEB_LOGO = git-logo.png\n GITWEB_FAVICON = git-favicon.png\n ifdef JSMIN\n@@ -84,13 +92,14 @@ endif\n \n all:: gitweb.cgi\n \n+FILES = gitweb.cgi\n ifdef JSMIN\n-FILES=gitweb.cgi gitweb.min.js\n-gitweb.cgi: gitweb.perl gitweb.min.js\n-else # !JSMIN\n-FILES=gitweb.cgi\n-gitweb.cgi: gitweb.perl\n-endif # JSMIN\n+FILES += gitweb.min.js\n+endif\n+ifdef CSSMIN\n+FILES += gitweb.min.css\n+endif\n+gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n \n gitweb.cgi:\n \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n@@ -123,6 +132,11 @@ gitweb.min.js: gitweb.js\n \t$(QUIET_GEN)$(JSMIN) <$< >$@\n endif # JSMIN\n \n+ifdef CSSMIN\n+gitweb.min.css: gitweb.css\n+\t$(QUIET_GEN)$(CSSMIN) <$ >$@\n+endif\n+\n clean:\n \t$(RM) $(FILES)\n \ndiff --git a/gitweb/README b/gitweb/README\nindex ad6a04c..71742b3 100644\n--- a/gitweb/README\n+++ b/gitweb/README\n@@ -80,7 +80,8 @@ You can specify the following configuration variables when building GIT:\n    Points to the location where you put gitweb.css on your web server\n    (or to be more generic, the URI of gitweb stylesheet).  Relative to the\n    base URI of gitweb.  Note that you can setup multiple stylesheets from\n-   the gitweb config file.  [Default: gitweb.css]\n+   the gitweb config file.  [Default: gitweb.css (or gitweb.min.css if the\n+   CSSMIN variable is defined / CSS minifier is used)]\n  * GITWEB_LOGO\n    Points to the location where you put git-logo.png on your web server\n    (or to be more generic URI of logo, 72x27 size, displayed in top right\n-- \n1.7.0.3.519.g7e0613\n"},{"id":"138334","messageId":"201004011051.39711.jnareb@gmail.com","threadId":"23279","inReplyTo":"4BB430C3.9030000@mailservices.uwaterloo.ca","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-01T08:51:38Z","receivedAt":"2010-04-01T08:51:38Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 1 April 2010, Mark Rada wrote:\n\n> The build system added support minifying gitweb.js through a\n> JavaScript minifier, but most minifiers come with support to\n> minify CSS files as well, so we should use it if we can.\n> \n> This patch will add the same facilities to gitweb.css that\n> gitweb.js has for minification. That does not mean that they\n> will use the same minifier though, as it is not safe to assume\n> that all JavaScript minifiers will also minify CSS files.\n> \n> Though the bandwidth savings will not be as dramatic as with\n> the JavaScript minifier, every byte saved is important.\n\nThat's not everything this patch does, isn't it?  I think you should\nhave added to your commit message the following paragraph:\n\n  While at it, introduce GITWEB_PROGRAMS variable to Makefile and\n  use it for gitweb instead of adding to OTHER_PROGRAMS, to keep\n  (possible) gitweb dependencies separate.\n\nOr something like that.\n\n> \n> Signed-off-by: Mark Rada <marada@uwaterloo.ca>\n\nI think it is good idea.\n\n[...]\n>  ifdef JSMIN\n> -OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js\n> -gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js\n> -else\n> -OTHER_PROGRAMS += gitweb/gitweb.cgi\n> -gitweb/gitweb.cgi: gitweb/gitweb.perl\n> +GITWEB_PROGRAMS += gitweb/gitweb.min.js\n>  endif\n> +ifdef CSSMIN\n> +GITWEB_PROGRAMS += gitweb/gitweb.min.css\n> +endif\n> +OTHER_PROGRAMS +=  gitweb/gitweb.cgi  $(GITWEB_PROGRAMS)\n> +gitweb/gitweb.cgi: gitweb/gitweb.perl $(GITWEB_PROGRAMS)\n>  \t$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) $(patsubst gitweb/%,%,$@)\n\n-- \nJakub Narebski\nPoland\n"},{"id":"139434","messageId":"4BC4D3F0.5020107@hashpling.org","threadId":"23279","inReplyTo":"4BB430C3.9030000@mailservices.uwaterloo.ca","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-04-13T20:28:32Z","receivedAt":"2010-04-13T20:28:32Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On 01/04/2010 06:36, Mark Rada wrote:\n> @@ -84,13 +92,14 @@ endif\n>\n>   all:: gitweb.cgi\n>\n> +FILES = gitweb.cgi\n>   ifdef JSMIN\n> -FILES=gitweb.cgi gitweb.min.js\n> -gitweb.cgi: gitweb.perl gitweb.min.js\n> -else # !JSMIN\n> -FILES=gitweb.cgi\n> -gitweb.cgi: gitweb.perl\n> -endif # JSMIN\n> +FILES += gitweb.min.js\n> +endif\n> +ifdef CSSMIN\n> +FILES += gitweb.min.css\n> +endif\n> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>\n\nI have a question about this last line of the patch. Are GITWEB_JS and \nGITWEB_CSS supposed to be a source path or a URI?\n\nThe documentation for install (and my previous assumption) was that they \nrepresented the path on the target web server. I'm used to overriding \nthem so that gitweb.cgi can live in my /cgi-bin directory, but the \nstatic files are served from /gitweb which is readable but not executable.\n\nAfter this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from \nthe list of dependencies for gitweb.cgi otherwise make failed.\n\nHave I got the wrong end of the stick?\n\nThanks,\n\nCharles.\n"},{"id":"139438","messageId":"201004140030.47222.jnareb@gmail.com","threadId":"23279","inReplyTo":"4BC4D3F0.5020107@hashpling.org","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-13T22:30:45Z","receivedAt":"2010-04-13T22:30:45Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Tue, 13 April 2010, Charles Bailey wrote:\n> On 01/04/2010 06:36, Mark Rada wrote:\n> > @@ -84,13 +92,14 @@ endif\n> >\n> >   all:: gitweb.cgi\n> >\n> > +FILES = gitweb.cgi\n> >   ifdef JSMIN\n> > -FILES=gitweb.cgi gitweb.min.js\n> > -gitweb.cgi: gitweb.perl gitweb.min.js\n> > -else # !JSMIN\n> > -FILES=gitweb.cgi\n> > -gitweb.cgi: gitweb.perl\n> > -endif # JSMIN\n> > +FILES += gitweb.min.js\n> > +endif\n> > +ifdef CSSMIN\n> > +FILES += gitweb.min.css\n> > +endif\n> > +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n> >\n> \n> I have a question about this last line of the patch. Are GITWEB_JS and \n> GITWEB_CSS supposed to be a source path or a URI?\n> \n> The documentation for install (and my previous assumption) was that they \n> represented the path on the target web server. I'm used to overriding \n> them so that gitweb.cgi can live in my /cgi-bin directory, but the \n> static files are served from /gitweb which is readable but not executable.\n> \n> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from \n> the list of dependencies for gitweb.cgi otherwise make failed.\n> \n> Have I got the wrong end of the stick?\n\nThanks a lot for noticing this bug.\n\n\nGITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\ngitweb JavaScript code and default gitweb stylesheet,... but during work\non minification of JavaScript code and CSS file it somehow got confused\nto mean source path.\n\nIf I remember correctly the original patch, before adding required\nsupport for minified gitweb.js and gitweb.css to git-instaweb script,\nand before support for CSS minification had\n\n   ifdef JSMIN\n   gitweb.cgi: gitweb.perl gitweb.min.js\n   else\n   gitweb.cgi: gitweb.perl\n   endif\n\nwhich should probably be replaced in current situation by\n\n   ifdef JSMIN\n   gitweb.cgi : gitweb.min.js\n   endif\n   ifdef CSSMIN\n   gitweb.cgi : gitweb.min.css\n   endif\n\njust adding prerequisites to gitweb.css target in gitweb/Makefile\n\n\nI guess that support for adding minifiction support to git-instaweb\nwould need to be more complicated.  Perhaps\n\n  $(notdir $(GITWEB_JS))   # Makefile function\n\nor\n\n  $(basename $GITWEB_JS)   # shell command\n\nBut I guess that it wouldn't work for all cases...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"139453","messageId":"4BC55558.1060608@mailservices.uwaterloo.ca","threadId":"23279","inReplyTo":"201004140030.47222.jnareb@gmail.com","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2010-04-14T05:40:40Z","receivedAt":"2010-04-14T05:40:40Z","isPatch":false,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"On 10-04-13 6:30 PM, Jakub Narebski wrote:\n> On Tue, 13 April 2010, Charles Bailey wrote:\n>> On 01/04/2010 06:36, Mark Rada wrote:\n>>> @@ -84,13 +92,14 @@ endif\n>>>\n>>>   all:: gitweb.cgi\n>>>\n>>> +FILES = gitweb.cgi\n>>>   ifdef JSMIN\n>>> -FILES=gitweb.cgi gitweb.min.js\n>>> -gitweb.cgi: gitweb.perl gitweb.min.js\n>>> -else # !JSMIN\n>>> -FILES=gitweb.cgi\n>>> -gitweb.cgi: gitweb.perl\n>>> -endif # JSMIN\n>>> +FILES += gitweb.min.js\n>>> +endif\n>>> +ifdef CSSMIN\n>>> +FILES += gitweb.min.css\n>>> +endif\n>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>>>\n>>\n>> I have a question about this last line of the patch. Are GITWEB_JS and \n>> GITWEB_CSS supposed to be a source path or a URI?\n>>\n>> The documentation for install (and my previous assumption) was that they \n>> represented the path on the target web server. I'm used to overriding \n>> them so that gitweb.cgi can live in my /cgi-bin directory, but the \n>> static files are served from /gitweb which is readable but not executable.\n>>\n>> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from \n>> the list of dependencies for gitweb.cgi otherwise make failed.\n>>\n>> Have I got the wrong end of the stick?\n> \n> Thanks a lot for noticing this bug.\n> \n> \n> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n> gitweb JavaScript code and default gitweb stylesheet,... but during work\n> on minification of JavaScript code and CSS file it somehow got confused\n> to mean source path.\n> \n> If I remember correctly the original patch, before adding required\n> support for minified gitweb.js and gitweb.css to git-instaweb script,\n> and before support for CSS minification had\n> \n>    ifdef JSMIN\n>    gitweb.cgi: gitweb.perl gitweb.min.js\n>    else\n>    gitweb.cgi: gitweb.perl\n>    endif\n> \n> which should probably be replaced in current situation by\n> \n>    ifdef JSMIN\n>    gitweb.cgi : gitweb.min.js\n>    endif\n>    ifdef CSSMIN\n>    gitweb.cgi : gitweb.min.css\n>    endif\n> \n> just adding prerequisites to gitweb.css target in gitweb/Makefile\n> \n> \n> I guess that support for adding minifiction support to git-instaweb\n> would need to be more complicated.  Perhaps\n> \n>   $(notdir $(GITWEB_JS))   # Makefile function\n> \n> or\n> \n>   $(basename $GITWEB_JS)   # shell command\n> \n> But I guess that it wouldn't work for all cases...\n>\n\nAw, frig, never thought of using gitweb like that so I made some\nassumptions to make things cleaner looking.\n\nI think this can be fixed by just using different variable names? Or\nperhaps some nested ifdef's? I'm not sure which will be better.\n\nI wasn't at the computer today so I'm just getting to it now, I'll try\nto have something when in the next day, going to bed now. Good night.\n\n-- \nMark Rada\n"},{"id":"139493","messageId":"201004141922.20213.jnareb@gmail.com","threadId":"23279","inReplyTo":"4BC55558.1060608@mailservices.uwaterloo.ca","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-14T17:22:18Z","receivedAt":"2010-04-14T17:22:18Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 14 Apr 2010, Mark Rada wrote:\n> On 10-04-13 6:30 PM, Jakub Narebski wrote:\n>> On Tue, 13 April 2010, Charles Bailey wrote:\n>>> On 01/04/2010 06:36, Mark Rada wrote:\n\n[...]\n>>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>>>>\n>>>\n>>> I have a question about this last line of the patch. Are GITWEB_JS and \n>>> GITWEB_CSS supposed to be a source path or a URI?\n>>>\n>>> The documentation for install (and my previous assumption) was that they \n>>> represented the path on the target web server. I'm used to overriding \n>>> them so that gitweb.cgi can live in my /cgi-bin directory, but the \n>>> static files are served from /gitweb which is readable but not executable.\n>>>\n>>> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from \n>>> the list of dependencies for gitweb.cgi otherwise make failed.\n>>>\n>>> Have I got the wrong end of the stick?\n>> \n>> Thanks a lot for noticing this bug.\n>> \n>> \n>> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n>> gitweb JavaScript code and default gitweb stylesheet,... but during work\n>> on minification of JavaScript code and CSS file it somehow got confused\n>> to mean source path.\n>> \n>> If I remember correctly the original patch, before adding required\n>> support for minified gitweb.js and gitweb.css to git-instaweb script,\n>> and before support for CSS minification had\n>> \n>>    ifdef JSMIN\n>>    gitweb.cgi: gitweb.perl gitweb.min.js\n>>    else\n>>    gitweb.cgi: gitweb.perl\n>>    endif\n>> \n>> which should probably be replaced in current situation by\n>> \n>>    ifdef JSMIN\n>>    gitweb.cgi : gitweb.min.js\n>>    endif\n>>    ifdef CSSMIN\n>>    gitweb.cgi : gitweb.min.css\n>>    endif\n>> \n>> just adding prerequisites to gitweb.css target in gitweb/Makefile\n>> \n>> \n>> I guess that support for adding minifiction support to git-instaweb\n>> would need to be more complicated. [...]\n> \n> Aw, frig, never thought of using gitweb like that so I made some\n> assumptions to make things cleaner looking.\n> \n> I think this can be fixed by just using different variable names? Or\n> perhaps some nested ifdef's? I'm not sure which will be better.\n> \n> I wasn't at the computer today so I'm just getting to it now, I'll try\n> to have something when in the next day, going to bed now. Good night.\n\nI think the best solution for prerequisites would be to have multiple \ntarget-only (without commands) rules, which according to make \ndocumentation would get concatenated.  This means the following code\nin gitweb/Makefile:\n\n    ifdef JSMIN\n    gitweb.cgi : gitweb.min.js\n    endif\n    ifdef CSSMIN\n    gitweb.cgi : gitweb.min.css\n    endif\n\nin place of\n\n   gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n\n\nFor git-instaweb I think that best solution would be to introduce new\nvariables holding _source_ of gitweb JavaScript code and CSS, e.g.\n\n            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \\\n\nin place of\n\n            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \\\n\n...although GITWEB_CSS might mean something different for Makefile\nand git-instaweb than for gitweb/Makefile and gitweb itself.\n-- \nJakub Narebski\nPoland\n"},{"id":"139496","messageId":"4BC614D8.2000208@mailservices.uwaterloo.ca","threadId":"23279","inReplyTo":"201004141922.20213.jnareb@gmail.com","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2010-04-14T19:17:44Z","receivedAt":"2010-04-14T19:17:44Z","isPatch":false,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"On 10-04-14 1:22 PM, Jakub Narebski wrote:\n> On Wed, 14 Apr 2010, Mark Rada wrote:\n>> On 10-04-13 6:30 PM, Jakub Narebski wrote:\n>>> On Tue, 13 April 2010, Charles Bailey wrote:\n>>>> On 01/04/2010 06:36, Mark Rada wrote:\n> \n> [...]\n>>>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>>>>>\n>>>>\n>>>> I have a question about this last line of the patch. Are GITWEB_JS and \n>>>> GITWEB_CSS supposed to be a source path or a URI?\n>>>>\n>>>> The documentation for install (and my previous assumption) was that they \n>>>> represented the path on the target web server. I'm used to overriding \n>>>> them so that gitweb.cgi can live in my /cgi-bin directory, but the \n>>>> static files are served from /gitweb which is readable but not executable.\n>>>>\n>>>> After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from \n>>>> the list of dependencies for gitweb.cgi otherwise make failed.\n>>>>\n>>>> Have I got the wrong end of the stick?\n>>>\n>>> Thanks a lot for noticing this bug.\n>>>\n>>>\n>>> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n>>> gitweb JavaScript code and default gitweb stylesheet,... but during work\n>>> on minification of JavaScript code and CSS file it somehow got confused\n>>> to mean source path.\n>>>\n>>> If I remember correctly the original patch, before adding required\n>>> support for minified gitweb.js and gitweb.css to git-instaweb script,\n>>> and before support for CSS minification had\n>>>\n>>>    ifdef JSMIN\n>>>    gitweb.cgi: gitweb.perl gitweb.min.js\n>>>    else\n>>>    gitweb.cgi: gitweb.perl\n>>>    endif\n>>>\n>>> which should probably be replaced in current situation by\n>>>\n>>>    ifdef JSMIN\n>>>    gitweb.cgi : gitweb.min.js\n>>>    endif\n>>>    ifdef CSSMIN\n>>>    gitweb.cgi : gitweb.min.css\n>>>    endif\n>>>\n>>> just adding prerequisites to gitweb.css target in gitweb/Makefile\n>>>\n>>>\n>>> I guess that support for adding minifiction support to git-instaweb\n>>> would need to be more complicated. [...]\n>>\n>> Aw, frig, never thought of using gitweb like that so I made some\n>> assumptions to make things cleaner looking.\n>>\n>> I think this can be fixed by just using different variable names? Or\n>> perhaps some nested ifdef's? I'm not sure which will be better.\n>>\n>> I wasn't at the computer today so I'm just getting to it now, I'll try\n>> to have something when in the next day, going to bed now. Good night.\n> \n> I think the best solution for prerequisites would be to have multiple \n> target-only (without commands) rules, which according to make \n> documentation would get concatenated.  This means the following code\n> in gitweb/Makefile:\n> \n>     ifdef JSMIN\n>     gitweb.cgi : gitweb.min.js\n>     endif\n>     ifdef CSSMIN\n>     gitweb.cgi : gitweb.min.css\n>     endif\n> \n> in place of\n> \n>    gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n> \n\nHmm, I like this because it is clear (if you know that dependancies\ncan be joined like that), I was thinking of trying to make a smaller fix\nusing pathsubst or subst, but that seems to not be as simple as I wanted it\nto be.\n\n> For git-instaweb I think that best solution would be to introduce new\n> variables holding _source_ of gitweb JavaScript code and CSS, e.g.\n> \n>             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \\\n> \n> in place of\n> \n>             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \\\n> \n> ...although GITWEB_CSS might mean something different for Makefile\n> and git-instaweb than for gitweb/Makefile and gitweb itself.\n\nDid you get those lines mixed up? I might be not understanding something\nhere.\n\nI was actually planning something along the lines of \n\n             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_NAME)' \\\n             -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS_NAME)|' \\\n\nwhere I introduce the GITWEB_CSS_NAME variable, to be consistent with the\ntoken in instaweb. This way we don't touch GITWEB_JS in the top level\nmakefile.\n\nAlso, I should update dependancies for instaweb, since those were\nforgotten last time around. Just creating a short list of what the fix will\nneed for when I get home tonight.\n\n-- \nMark Rada\n"},{"id":"139502","messageId":"201004142204.57323.jnareb@gmail.com","threadId":"23279","inReplyTo":"4BC614D8.2000208@mailservices.uwaterloo.ca","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-14T20:04:56Z","receivedAt":"2010-04-14T20:04:56Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Wed, 14 April 2010, Mark Rada wrote:\n> On 10-04-14 1:22 PM, Jakub Narebski wrote:\n\n> > For git-instaweb I think that best solution would be to introduce new\n> > variables holding _source_ of gitweb JavaScript code and CSS, e.g.\n> > \n> >             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \\\n> > \n> > in place of\n> > \n> >             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \\\n> > \n> > ...although GITWEB_CSS might mean something different for Makefile\n> > and git-instaweb than for gitweb/Makefile and gitweb itself.\n> \n> Did you get those lines mixed up? I might be not understanding something\n> here.\n\nAh, I'm sorry, I mixed up those two lines.  They should be in reverse\ndirection:\n\n\n             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \\\n \n  in place of\n \n             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \\\n\n\nActually I'd like to rename @@GITWEB_CSS@@ placeholder etc. in \ngit-instaweb.sh, as  @@GITWEB_CSS@@ in git-instaweb.sh means something\nquite different from ++GITWEB_CSS++ in gitweb/gitweb.perl...\n\n> \n> I was actually planning something along the lines of \n> \n>              -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_NAME)' \\\n>              -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS_NAME)|' \\\n> \n> where I introduce the GITWEB_CSS_NAME variable, to be consistent with the\n> token in instaweb. This way we don't touch GITWEB_JS in the top level\n> makefile.\n\nWhy not:\n\n            -e '/@@GITWEB_CSS_SOURCE@@/r $(GITWEB_CSS_SOURCE)' \\\n            -e '/@@GITWEB_CSS_SOURCE@@/d' \\\n            ...\n            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \\\n\n(assuming that $(GITWEB_CSS) does not include '|' in it, I guess...\nbut see below).\n\n> \n> Also, I should update dependancies for instaweb, since those were\n> forgotten last time around. Just creating a short list of what the fix will\n> need for when I get home tonight.\n\nSomething like\n\n   git-instaweb: git-instaweb.sh gitweb/gitweb.cgi $(GITWEB_CSS_SOURCE) $(GITWEB_JS_SOURCE)\n\n\nP.S. I have noticed additional complication: git-instaweb really needs\ngitweb compiled with *specific* values of GITWEB_CSS and GITWEB_JS,\nso that they point to git-instaweb's installed files.\n\n-- \nJakub Narebski\nPoland\n"},{"id":"139528","messageId":"7viq7tmvsb.fsf@alter.siamese.dyndns.org","threadId":"23279","inReplyTo":"201004140030.47222.jnareb@gmail.com","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-14T23:58:12Z","receivedAt":"2010-04-14T23:58:12Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n> On Tue, 13 April 2010, Charles Bailey wrote:\n>> On 01/04/2010 06:36, Mark Rada wrote:\n>> > @@ -84,13 +92,14 @@ endif\n>> >\n>> >   all:: gitweb.cgi\n>> >\n>> > +FILES = gitweb.cgi\n>> >   ifdef JSMIN\n>> > +FILES += gitweb.min.js\n>> > +endif\n>> > +ifdef CSSMIN\n>> > +FILES += gitweb.min.css\n>> > +endif\n>> > +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>\n> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n> gitweb JavaScript code and default gitweb stylesheet,... but during work\n> on minification of JavaScript code and CSS file it somehow got confused\n> to mean source path.\n\nI am not touching instaweb part, but this would fix the build/clean side\nof the things, no?\n\n-- >8 --\ngitweb: simplify gitweb.min.* generation and clean-up rules\n\nGITWEB_CSS and GITWEB_JS are meant to be \"what URI should the installed\ncgi script use to refer to the stylesheet and JavaScript\", never \"this is\nthe name of the file we are building\".\n\nLose incorrect assignment to them.\n\nWhile we are at it, lose FILES that is used only for \"clean\" target in a\nmisguided way.  \"make clean\" should try to remove all the potential build\nartifacts regardless of a minor configuration change.  Instead of trying\nto remove only the build product \"make clean\" would have created if it\nwere run without \"clean\", explicitly list the three potential build\nproducts for removal.\n\nSigned-off-by: Junio C Hamano <gitster@pobox.com>\n---\n gitweb/Makefile |   15 ++++-----------\n 1 files changed, 4 insertions(+), 11 deletions(-)\n\ndiff --git a/gitweb/Makefile b/gitweb/Makefile\nindex ffee4bd..1787633 100644\n--- a/gitweb/Makefile\n+++ b/gitweb/Makefile\n@@ -80,16 +80,7 @@ endif\n \n all:: gitweb.cgi\n \n-FILES = gitweb.cgi\n-ifdef JSMIN\n-FILES += gitweb.min.js\n-GITWEB_JS = gitweb.min.js\n-endif\n-ifdef CSSMIN\n-FILES += gitweb.min.css\n-GITWEB_CSS = gitweb.min.css\n-endif\n-gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n+gitweb.cgi: gitweb.perl\n \n gitweb.cgi:\n \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n@@ -118,16 +109,18 @@ gitweb.cgi:\n \tmv $@+ $@\n \n ifdef JSMIN\n+all:: gitweb.min.js\n gitweb.min.js: gitweb.js\n \t$(QUIET_GEN)$(JSMIN) <$< >$@\n endif # JSMIN\n \n ifdef CSSMIN\n+all:: gitweb.min.css\n gitweb.min.css: gitweb.css\n \t$(QUIET_GEN)$(CSSMIN) <$ >$@\n endif\n \n clean:\n-\t$(RM) $(FILES)\n+\t$(RM) gitweb.cgi gitweb.min.css gitweb.min.js\n \n .PHONY: all clean .FORCE-GIT-VERSION-FILE\n"},{"id":"139530","messageId":"20100415001830.GA26416@hashpling.org","threadId":"23279","inReplyTo":"7viq7tmvsb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Charles Bailey","fromEmail":"charles@hashpling.org","sentAt":"2010-04-15T00:18:30Z","receivedAt":"2010-04-15T00:18:30Z","isPatch":false,"sender":{"key":"charles@hashpling.org","avatar":"https://avatars.githubusercontent.com/u/1668475?v=4"},"body":"On Wed, Apr 14, 2010 at 04:58:12PM -0700, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> > On Tue, 13 April 2010, Charles Bailey wrote:\n> >> On 01/04/2010 06:36, Mark Rada wrote:\n> >> > @@ -84,13 +92,14 @@ endif\n> >> >\n> >> >   all:: gitweb.cgi\n> >> >\n> >> > +FILES = gitweb.cgi\n> >> >   ifdef JSMIN\n> >> > +FILES += gitweb.min.js\n> >> > +endif\n> >> > +ifdef CSSMIN\n> >> > +FILES += gitweb.min.css\n> >> > +endif\n> >> > +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n> >\n> > GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n> > gitweb JavaScript code and default gitweb stylesheet,... but during work\n> > on minification of JavaScript code and CSS file it somehow got confused\n> > to mean source path.\n> \n> I am not touching instaweb part, but this would fix the build/clean side\n> of the things, no?\n> \n\nYes, this certainly fixes make and make clean for me with overridden\nGITWEB_JS and GITWEB_CSS paths.\n\nCharles.\n"},{"id":"139534","messageId":"201004150225.42101.jnareb@gmail.com","threadId":"23279","inReplyTo":"7viq7tmvsb.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-15T00:25:41Z","receivedAt":"2010-04-15T00:25:41Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"On Thu, 15 Apr 2010, Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n>> On Tue, 13 April 2010, Charles Bailey wrote:\n>>> On 01/04/2010 06:36, Mark Rada wrote:\n>>>> @@ -84,13 +92,14 @@ endif\n>>>>\n>>>>   all:: gitweb.cgi\n>>>>\n>>>> +FILES = gitweb.cgi\n>>>>   ifdef JSMIN\n>>>> +FILES += gitweb.min.js\n>>>> +endif\n>>>> +ifdef CSSMIN\n>>>> +FILES += gitweb.min.css\n>>>> +endif\n>>>> +gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>>\n>> GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with\n>> gitweb JavaScript code and default gitweb stylesheet,... but during work\n>> on minification of JavaScript code and CSS file it somehow got confused\n>> to mean source path.\n> \n> I am not touching instaweb part, but this would fix the build/clean side\n> of the things, no?\n\nClose, see below.\n\n> \n> -->8 --\n> gitweb: simplify gitweb.min.* generation and clean-up rules\n> \n> GITWEB_CSS and GITWEB_JS are meant to be \"what URI should the installed\n> cgi script use to refer to the stylesheet and JavaScript\", never \"this is\n> the name of the file we are building\".\n> \n> Lose incorrect assignment to them.\n\nActually the assignment was intended to provide correct *default* values\nfor GITWEB_CSS and GITWEB_JS, so that if e.g. JSMIN is defined gitweb,\nin absence of build time configuration, would link gitweb.min.js instead\nof gitweb.js.\n\n> \n> While we are at it, lose FILES that is used only for \"clean\" target in a\n> misguided way.  \"make clean\" should try to remove all the potential build\n> artifacts regardless of a minor configuration change.  Instead of trying\n> to remove only the build product \"make clean\" would have created if it\n> were run without \"clean\", explicitly list the three potential build\n> products for removal.\n\nGood.\n\n> \n> Signed-off-by: Junio C Hamano <gitster@pobox.com>\n> ---\n>  gitweb/Makefile |   15 ++++-----------\n>  1 files changed, 4 insertions(+), 11 deletions(-)\n> \n> diff --git a/gitweb/Makefile b/gitweb/Makefile\n> index ffee4bd..1787633 100644\n> --- a/gitweb/Makefile\n> +++ b/gitweb/Makefile\n> @@ -80,16 +80,7 @@ endif\n>  \n>  all:: gitweb.cgi\n>  \n> -FILES = gitweb.cgi\n> -ifdef JSMIN\n> -FILES += gitweb.min.js\n> -GITWEB_JS = gitweb.min.js\n> -endif\n> -ifdef CSSMIN\n> -FILES += gitweb.min.css\n> -GITWEB_CSS = gitweb.min.css\n> -endif\n\nI wonder about removing assigmnet to GITWEB_JS and GITWEB_CSS.  Without\nit \"make gitweb\" (in top dir) would create gitweb/gitweb.cgi and \ngitweb/gitweb.min.js etc.... but generated gitweb/gitweb.cgi would\nrefer to gitweb.js, not gitweb.min.js.  Unless of course one provides\nvalues for GITWEB_JS during build time.\n\n> -gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n> +gitweb.cgi: gitweb.perl\n>  \n>  gitweb.cgi:\n>  \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n> @@ -118,16 +109,18 @@ gitweb.cgi:\n>  \tmv $@+ $@\n>  \n>  ifdef JSMIN\n> +all:: gitweb.min.js\n>  gitweb.min.js: gitweb.js\n>  \t$(QUIET_GEN)$(JSMIN) <$<>$@\n>  endif # JSMIN\n>  \n>  ifdef CSSMIN\n> +all:: gitweb.min.css\n>  gitweb.min.css: gitweb.css\n>  \t$(QUIET_GEN)$(CSSMIN) <$>$@\n>  endif\n\nThat makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css.\nIt might be right... and I think the rightness or wrongness might be\ntied with values of GITWEB>JS and GITWEB_CSS.\n\n>  \n>  clean:\n> -\t$(RM) $(FILES)\n> +\t$(RM) gitweb.cgi gitweb.min.css gitweb.min.js\n>  \n>  .PHONY: all clean .FORCE-GIT-VERSION-FILE\n> \n\n-- \nJakub Narebski\nPoland\n"},{"id":"139538","messageId":"7veiihmtjw.fsf@alter.siamese.dyndns.org","threadId":"23279","inReplyTo":"201004150225.42101.jnareb@gmail.com","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-15T00:46:27Z","receivedAt":"2010-04-15T00:46:27Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jakub Narebski <jnareb@gmail.com> writes:\n\n>> -gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>> +gitweb.cgi: gitweb.perl\n> ...\n> That makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css.\n\nThat is what I inteded to do.\n\nUnless you are including gitweb.cgi (iow, the contents of the generated\nfile depends on the _contents_ of gitweb.min.js (or gitweb.js), gitweb.cgi\ndoes _not_ depend on these files.  Of course if you generate gitweb.cgi\nout of gitweb.perl with one setting of GITWEB_JS and then change your\nmind, then you need to regenerate it, but that is not something you can do\nby comparing file timestamp of gitweb.cgi and the file timestamp of\n$(GITWEB_JS) anyway.  You would need to imitate something like how\nGIT-BUILD-OPTIONS is used by the primary Makefile.\n"},{"id":"139540","messageId":"201004150302.51242.jnareb@gmail.com","threadId":"23279","inReplyTo":"7veiihmtjw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2010-04-15T01:02:47Z","receivedAt":"2010-04-15T01:02:47Z","isPatch":false,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"Junio C Hamano wrote:\n> Jakub Narebski <jnareb@gmail.com> writes:\n> \n> >> -gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n> >> +gitweb.cgi: gitweb.perl\n> > ...\n> > That makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css.\n> \n> That is what I inteded to do.\n> \n> Unless you are including gitweb.cgi (iow, the contents of the generated\n> file depends on the _contents_ of gitweb.min.js (or gitweb.js), gitweb.cgi\n> does _not_ depend on these files.  Of course if you generate gitweb.cgi\n> out of gitweb.perl with one setting of GITWEB_JS and then change your\n> mind, then you need to regenerate it, but that is not something you can do\n> by comparing file timestamp of gitweb.cgi and the file timestamp of\n> $(GITWEB_JS) anyway.  You would need to imitate something like how\n> GIT-BUILD-OPTIONS is used by the primary Makefile.\n\nRight.  I agree.\n\nP.S. This is nt the case with git-instaweb, which literally include \ngitweb.js or gitweb.min.js...\n\n-- \nJakub Narebski\nPoland\n"},{"id":"139541","messageId":"4BC66A30.7030107@mailservices.uwaterloo.ca","threadId":"23279","inReplyTo":"201004150302.51242.jnareb@gmail.com","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Mark Rada","fromEmail":"marada@uwaterloo.ca","sentAt":"2010-04-15T01:21:52Z","receivedAt":"2010-04-15T01:21:52Z","isPatch":false,"sender":{"key":"marada@uwaterloo.ca","avatar":"https://avatars.githubusercontent.com/u/38430?v=4"},"body":"On 10-04-14 9:02 PM, Jakub Narebski wrote:\n> Junio C Hamano wrote:\n>> Jakub Narebski <jnareb@gmail.com> writes:\n>>\n>>>> -gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n>>>> +gitweb.cgi: gitweb.perl\n>>> ...\n>>> That makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css.\n>>\n>> That is what I inteded to do.\n>>\n>> Unless you are including gitweb.cgi (iow, the contents of the generated\n>> file depends on the _contents_ of gitweb.min.js (or gitweb.js), gitweb.cgi\n>> does _not_ depend on these files.  Of course if you generate gitweb.cgi\n>> out of gitweb.perl with one setting of GITWEB_JS and then change your\n>> mind, then you need to regenerate it, but that is not something you can do\n>> by comparing file timestamp of gitweb.cgi and the file timestamp of\n>> $(GITWEB_JS) anyway.  You would need to imitate something like how\n>> GIT-BUILD-OPTIONS is used by the primary Makefile.\n> \n> Right.  I agree.\n> \n> P.S. This is nt the case with git-instaweb, which literally include \n> gitweb.js or gitweb.min.js...\n> \n\nI believe we can just change the variable names as planned before as well\nas the dependancies, and that should fix up instaweb as far as will need\nto be fixed, the rest has to happen at gitweb.cgi generation time.\n\nMy understanding (please correct me if I am wrong) is that comparing the\nmtimes of the files is not reliable. So can't we just make gitweb.cgi\ndepend on source .js and .css files, since any modifications there will\nalso cause the minified versions to be regenerated? Can Junio's patch just\nbe updated to subst the suffix of GITWEB_JS and GITWEB_CSS, which makes sure\nthe correct version is being used?\n\n\n\n-- \nMark Rada\n"},{"id":"139544","messageId":"7v1vehmqxr.fsf@alter.siamese.dyndns.org","threadId":"23279","inReplyTo":"7veiihmtjw.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCHv5 2/6] Gitweb: add support for minifying gitweb.css","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2010-04-15T01:42:56Z","receivedAt":"2010-04-15T01:42:56Z","isPatch":false,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Junio C Hamano <gitster@pobox.com> writes:\n\n> Unless you are including gitweb.cgi (iow, the contents of the generated\n> file depends on the _contents_ of gitweb.min.js (or gitweb.js), gitweb.cgi\n> does _not_ depend on these files.  Of course if you generate gitweb.cgi\n> out of gitweb.perl with one setting of GITWEB_JS and then change your\n> mind, then you need to regenerate it, but that is not something you can do\n> by comparing file timestamp of gitweb.cgi and the file timestamp of\n> $(GITWEB_JS) anyway.  You would need to imitate something like how\n> GIT-BUILD-OPTIONS is used by the primary Makefile.\n\nHere is a minimally tested patch.\n\nIn addition to the points raised by the proposed log message for the\nprevious one, this tries to make sure that the scripts are regenerated\nwhenever the replacement variables are modified.  For a good measure, if\nyou used different JSMIN/CSSMIN since the last time you produced minified\nversion of these files, they are regenerated.\n\nIf this seems to work Ok, please send it back to me with a proper commit\nlog message (concat of the above and the previous) with Tested-by: or\nAcked-by: as appropriate, so that we can fix it at the tip of 'master'\nbefore we tag 1.7.1-rc2.\n\nThanks.\n\n gitweb/Makefile |   75 ++++++++++++++++++++++++++++---------------------------\n 1 files changed, 38 insertions(+), 37 deletions(-)\n\ndiff --git a/gitweb/Makefile b/gitweb/Makefile\nindex ffee4bd..f2e1d92 100644\n--- a/gitweb/Makefile\n+++ b/gitweb/Makefile\n@@ -80,54 +80,55 @@ endif\n \n all:: gitweb.cgi\n \n-FILES = gitweb.cgi\n ifdef JSMIN\n-FILES += gitweb.min.js\n GITWEB_JS = gitweb.min.js\n+all:: gitweb.min.js\n+gitweb.min.js: gitweb.js GITWEB-BUILD-OPTIONS\n+\t$(QUIET_GEN)$(JSMIN) <$< >$@\n endif\n+\n ifdef CSSMIN\n-FILES += gitweb.min.css\n GITWEB_CSS = gitweb.min.css\n+all:: gitweb.min.css\n+gitweb.min.css: gitweb.css GITWEB-BUILD-OPTIONS\n+\t$(QUIET_GEN)$(CSSMIN) <$ >$@\n endif\n-gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)\n \n-gitweb.cgi:\n+GITWEB_REPLACE = \\\n+\t-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \\\n+\t-e 's|++GIT_BINDIR++|$(bindir)|g' \\\n+\t-e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \\\n+\t-e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \\\n+\t-e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \\\n+\t-e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \\\n+\t-e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \\\n+\t-e 's|\"++GITWEB_PROJECT_MAXDEPTH++\"|$(GITWEB_PROJECT_MAXDEPTH)|g' \\\n+\t-e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \\\n+\t-e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \\\n+\t-e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \\\n+\t-e 's|++GITWEB_LIST++|$(GITWEB_LIST)|g' \\\n+\t-e 's|++GITWEB_HOMETEXT++|$(GITWEB_HOMETEXT)|g' \\\n+\t-e 's|++GITWEB_CSS++|$(GITWEB_CSS)|g' \\\n+\t-e 's|++GITWEB_LOGO++|$(GITWEB_LOGO)|g' \\\n+\t-e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \\\n+\t-e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \\\n+\t-e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \\\n+\t-e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g'\n+\n+GITWEB-BUILD-OPTIONS: FORCE\n+\t@rm -f $@+\n+\t@echo \"x\" '$(PERL_PATH_SQ)' $(GITWEB_REPLACE) \"$(JSMIN)|$(CSSMIN)\" >$@+\n+\t@cmp -s $@+ $@ && rm -f $@+ || mv -f $@+ $@\n+\n+gitweb.cgi: gitweb.perl GITWEB-BUILD-OPTIONS\n \t$(QUIET_GEN)$(RM) $@ $@+ && \\\n \tsed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \\\n-\t    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \\\n-\t    -e 's|++GIT_BINDIR++|$(bindir)|g' \\\n-\t    -e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \\\n-\t    -e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \\\n-\t    -e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \\\n-\t    -e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \\\n-\t    -e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \\\n-\t    -e 's|\"++GITWEB_PROJECT_MAXDEPTH++\"|$(GITWEB_PROJECT_MAXDEPTH)|g' \\\n-\t    -e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \\\n-\t    -e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \\\n-\t    -e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \\\n-\t    -e 's|++GITWEB_LIST++|$(GITWEB_LIST)|g' \\\n-\t    -e 's|++GITWEB_HOMETEXT++|$(GITWEB_HOMETEXT)|g' \\\n-\t    -e 's|++GITWEB_CSS++|$(GITWEB_CSS)|g' \\\n-\t    -e 's|++GITWEB_LOGO++|$(GITWEB_LOGO)|g' \\\n-\t    -e 's|++GITWEB_FAVICON++|$(GITWEB_FAVICON)|g' \\\n-\t    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \\\n-\t    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \\\n-\t    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \\\n-\t    $< >$@+ && \\\n+\t\t$(GITWEB_REPLACE) $< >$@+ && \\\n \tchmod +x $@+ && \\\n \tmv $@+ $@\n \n-ifdef JSMIN\n-gitweb.min.js: gitweb.js\n-\t$(QUIET_GEN)$(JSMIN) <$< >$@\n-endif # JSMIN\n-\n-ifdef CSSMIN\n-gitweb.min.css: gitweb.css\n-\t$(QUIET_GEN)$(CSSMIN) <$ >$@\n-endif\n-\n clean:\n-\t$(RM) $(FILES)\n+\t$(RM) gitweb.cgi gitweb.min.js gitweb.min.css GITWEB-BUILD-OPTIONS\n+\n+.PHONY: all clean .FORCE-GIT-VERSION-FILE FORCE\n \n-.PHONY: all clean .FORCE-GIT-VERSION-FILE\n"}]}