The build system added support minifying gitweb.js through a
JavaScript minifier, but most minifiers come with support to
minify CSS files as well, so we should use it if we can.

This patch will add the same facilities to gitweb.css that
gitweb.js has for minification. That does not mean that they
will use the same minifier though, as it is not safe to assume
that all JavaScript minifiers will also minify CSS files.

Though the bandwidth savings will not be as dramatic as with
the JavaScript minifier, every byte saved is important.

Signed-off-by: Mark Rada <marada@uwaterloo.ca>

---

Changes since v4:
	- Reworded some of the comments and documentation


 Makefile        |   21 +++++++++++++++------
 gitweb/INSTALL  |    5 +++++
 gitweb/Makefile |   28 +++++++++++++++++++++-------
 gitweb/README   |    3 ++-
 4 files changed, 43 insertions(+), 14 deletions(-)

diff --git a/Makefile b/Makefile
index 5384d33..450e4df 100644
--- a/Makefile
+++ b/Makefile
@@ -203,6 +203,9 @@ all::
 # Define JSMIN to point to JavaScript minifier that functions as
 # a filter to have gitweb.js minified.
 #
+# Define CSSMIN to point to a CSS minifier in order to generate a minified
+# version of gitweb.css
+#
 # Define DEFAULT_PAGER to a sensible pager command (defaults to "less") if
 # you want to use something different.  The value will be interpreted by the
 # shell at runtime when it is used.
@@ -279,8 +282,9 @@ lib = lib
 # DESTDIR=
 pathsep = :
 
-# JavaScript minifier invocation that can function as filter
+# JavaScript/CSS minifier invocation that can function as filter
 JSMIN =
+CSSMIN =
 
 export prefix bindir sharedir sysconfdir
 
@@ -1564,18 +1568,23 @@ gitweb:
 	$(QUIET_SUBDIR0)gitweb $(QUIET_SUBDIR1) all
 
 ifdef JSMIN
-OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
-gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
-else
-OTHER_PROGRAMS += gitweb/gitweb.cgi
-gitweb/gitweb.cgi: gitweb/gitweb.perl
+GITWEB_PROGRAMS += gitweb/gitweb.min.js
 ...
[ message continues ]

That's not everything this patch does, isn't it?  I think you should
have added to your commit message the following paragraph:

  While at it, introduce GITWEB_PROGRAMS variable to Makefile and
  use it for gitweb instead of adding to OTHER_PROGRAMS, to keep
  (possible) gitweb dependencies separate.


I think it is good idea.


-- 
Jakub Narebski
Poland
--

[ message continues ]

I have a question about this last line of the patch. Are GITWEB_JS and 
GITWEB_CSS supposed to be a source path or a URI?

The documentation for install (and my previous assumption) was that they 
represented the path on the target web server. I'm used to overriding 
them so that gitweb.cgi can live in my /cgi-bin directory, but the 
static files are served from /gitweb which is readable but not executable.

After this patch I had to removed $(GITWEB_JS) and $(GITWEB_CSS) from 
the list of dependencies for gitweb.cgi otherwise make failed.

Have I got the wrong end of the stick?

Thanks,

Charles.
--

[ message continues ]

Thanks a lot for noticing this bug.


GITWEB_JS and GITWEB_CSS were originally meant to be URI to file with
gitweb JavaScript code and default gitweb stylesheet,... but during work
on minification of JavaScript code and CSS file it somehow got confused
to mean source path.

If I remember correctly the original patch, before adding required
support for minified gitweb.js and gitweb.css to git-instaweb script,
and before support for CSS minification had

   ifdef JSMIN
   gitweb.cgi: gitweb.perl gitweb.min.js
   else
   gitweb.cgi: gitweb.perl
   endif

which should probably be replaced in current situation by

   ifdef JSMIN
   gitweb.cgi : gitweb.min.js
   endif
   ifdef CSSMIN
   gitweb.cgi : gitweb.min.css
   endif

just adding prerequisites to gitweb.css target in gitweb/Makefile


I guess that support for adding minifiction support to git-instaweb
would need to be more complicated.  Perhaps

  $(notdir $(GITWEB_JS))   # Makefile function

or

  $(basename $GITWEB_JS)   # shell command

But I guess that it wouldn't work for all cases...

-- 
Jakub Narebski
Poland
--

[ message continues ]

Aw, frig, never thought of using gitweb like that so I made some
assumptions to make things cleaner looking.

I think this can be fixed by just using different variable names? Or
perhaps some nested ifdef's? I'm not sure which will be better.

I wasn't at the computer today so I'm just getting to it now, I'll try
to have something when in the next day, going to bed now. Good night.

-- 
Mark Rada

--

[ message continues ]

I think the best solution for prerequisites would be to have multiple 
target-only (without commands) rules, which according to make 
documentation would get concatenated.  This means the following code
in gitweb/Makefile:

    ifdef JSMIN
    gitweb.cgi : gitweb.min.js
    endif
    ifdef CSSMIN
    gitweb.cgi : gitweb.min.css
    endif

in place of

   gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)


For git-instaweb I think that best solution would be to introduce new
variables holding _source_ of gitweb JavaScript code and CSS, e.g.

            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \

in place of

            -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \

...although GITWEB_CSS might mean something different for Makefile
and git-instaweb than for gitweb/Makefile and gitweb itself.
-- 
Jakub Narebski
Poland
--

[ message continues ]

Hmm, I like this because it is clear (if you know that dependancies
can be joined like that), I was thinking of trying to make a smaller fix
using pathsubst or subst, but that seems to not be as simple as I wanted it

Did you get those lines mixed up? I might be not understanding something
here.

I was actually planning something along the lines of 

             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_NAME)' \
             -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS_NAME)|' \

where I introduce the GITWEB_CSS_NAME variable, to be consistent with the
token in instaweb. This way we don't touch GITWEB_JS in the top level
makefile.

Also, I should update dependancies for instaweb, since those were
forgotten last time around. Just creating a short list of what the fix will
need for when I get home tonight.

-- 
Mark Rada
--

[ message continues ]

Ah, I'm sorry, I mixed up those two lines.  They should be in reverse
direction:


             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS_SOURCE)' \
 
  in place of
 
             -e '/@@GITWEB_CSS@@/r $(GITWEB_CSS)' \


Actually I'd like to rename @@GITWEB_CSS@@ placeholder etc. in 
git-instaweb.sh, as  @@GITWEB_CSS@@ in git-instaweb.sh means something

Why not:

            -e '/@@GITWEB_CSS_SOURCE@@/r $(GITWEB_CSS_SOURCE)' \
            -e '/@@GITWEB_CSS_SOURCE@@/d' \
            ...
            -e 's|@@GITWEB_CSS_NAME@@|$(GITWEB_CSS)|' \

(assuming that $(GITWEB_CSS) does not include '|' in it, I guess...

Something like

   git-instaweb: git-instaweb.sh gitweb/gitweb.cgi $(GITWEB_CSS_SOURCE) $(GITWEB_JS_SOURCE)


P.S. I have noticed additional complication: git-instaweb really needs
gitweb compiled with *specific* values of GITWEB_CSS and GITWEB_JS,
so that they point to git-instaweb's installed files.

-- 
Jakub Narebski
Poland
--

[ message continues ]

I am not touching instaweb part, but this would fix the build/clean side
of the things, no?

-- >8 --
gitweb: simplify gitweb.min.* generation and clean-up rules

GITWEB_CSS and GITWEB_JS are meant to be "what URI should the installed
cgi script use to refer to the stylesheet and JavaScript", never "this is
the name of the file we are building".

Lose incorrect assignment to them.

While we are at it, lose FILES that is used only for "clean" target in a
misguided way.  "make clean" should try to remove all the potential build
artifacts regardless of a minor configuration change.  Instead of trying
to remove only the build product "make clean" would have created if it
were run without "clean", explicitly list the three potential build
products for removal.

Signed-off-by: Junio C Hamano <gitster@pobox.com>
---
 gitweb/Makefile |   15 ++++-----------
 1 files changed, 4 insertions(+), 11 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index ffee4bd..1787633 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -80,16 +80,7 @@ endif
 
 all:: gitweb.cgi
 
-FILES = gitweb.cgi
-ifdef JSMIN
-FILES += gitweb.min.js
-GITWEB_JS = gitweb.min.js
-endif
-ifdef CSSMIN
-FILES += gitweb.min.css
-GITWEB_CSS = gitweb.min.css
-endif
-gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
+gitweb.cgi: gitweb.perl
 
 gitweb.cgi:
 	$(QUIET_GEN)$(RM) $@ $@+ && \
@@ -118,16 +109,18 @@ gitweb.cgi:
 	mv $@+ $@
 
 ifdef JSMIN
+all:: gitweb.min.js
 gitweb.min.js: gitweb.js
 	$(QUIET_GEN)$(JSMIN) <$< >$@
 endif # JSMIN
 
 ifdef CSSMIN
+all:: gitweb.min.css
 gitweb.min.css: gitweb.css
 	$(QUIET_GEN)$(CSSMIN) <$ >$@
 endif
 
 clean:
-	$(RM) $(FILES)
+	$(RM) gitweb.cgi gitweb.min.css gitweb.min.js
 
 .PHONY: all clean .FORCE-GIT-VERSION-FILE
--

[ message continues ]

Yes, this certainly fixes make and make clean for me with overridden
GITWEB_JS and GITWEB_CSS paths.

Charles.
--

[ message continues ]

Actually the assignment was intended to provide correct *default* values
for GITWEB_CSS and GITWEB_JS, so that if e.g. JSMIN is defined gitweb,
in absence of build time configuration, would link gitweb.min.js instead


I wonder about removing assigmnet to GITWEB_JS and GITWEB_CSS.  Without
it "make gitweb" (in top dir) would create gitweb/gitweb.cgi and 
gitweb/gitweb.min.js etc.... but generated gitweb/gitweb.cgi would
refer to gitweb.js, not gitweb.min.js.  Unless of course one provides

That makes gitweb.cgi not depend on gitweb.min.js, not gitweb.min.css.
It might be right... and I think the rightness or wrongness might be

-- 
Jakub Narebski
Poland
--

[ message continues ]

That is what I inteded to do.

Unless you are including gitweb.cgi (iow, the contents of the generated
file depends on the _contents_ of gitweb.min.js (or gitweb.js), gitweb.cgi
does _not_ depend on these files.  Of course if you generate gitweb.cgi
out of gitweb.perl with one setting of GITWEB_JS and then change your
mind, then you need to regenerate it, but that is not something you can do
by comparing file timestamp of gitweb.cgi and the file timestamp of
$(GITWEB_JS) anyway.  You would need to imitate something like how
GIT-BUILD-OPTIONS is used by the primary Makefile.
--

[ message continues ]

Right.  I agree.

P.S. This is nt the case with git-instaweb, which literally include 
gitweb.js or gitweb.min.js...

-- 
Jakub Narebski
Poland
--

[ message continues ]

I believe we can just change the variable names as planned before as well
as the dependancies, and that should fix up instaweb as far as will need
to be fixed, the rest has to happen at gitweb.cgi generation time.

My understanding (please correct me if I am wrong) is that comparing the
mtimes of the files is not reliable. So can't we just make gitweb.cgi
depend on source .js and .css files, since any modifications there will
also cause the minified versions to be regenerated? Can Junio's patch just
be updated to subst the suffix of GITWEB_JS and GITWEB_CSS, which makes sure
the correct version is being used?



-- 
Mark Rada
--

[ message continues ]

Here is a minimally tested patch.

In addition to the points raised by the proposed log message for the
previous one, this tries to make sure that the scripts are regenerated
whenever the replacement variables are modified.  For a good measure, if
you used different JSMIN/CSSMIN since the last time you produced minified
version of these files, they are regenerated.

If this seems to work Ok, please send it back to me with a proper commit
log message (concat of the above and the previous) with Tested-by: or
Acked-by: as appropriate, so that we can fix it at the tip of 'master'
before we tag 1.7.1-rc2.

Thanks.

 gitweb/Makefile |   75 ++++++++++++++++++++++++++++---------------------------
 1 files changed, 38 insertions(+), 37 deletions(-)

diff --git a/gitweb/Makefile b/gitweb/Makefile
index ffee4bd..f2e1d92 100644
--- a/gitweb/Makefile
+++ b/gitweb/Makefile
@@ -80,54 +80,55 @@ endif
 
 all:: gitweb.cgi
 
-FILES = gitweb.cgi
 ifdef JSMIN
-FILES += gitweb.min.js
 GITWEB_JS = gitweb.min.js
+all:: gitweb.min.js
+gitweb.min.js: gitweb.js GITWEB-BUILD-OPTIONS
+	$(QUIET_GEN)$(JSMIN) <$< >$@
 endif
+
 ifdef CSSMIN
-FILES += gitweb.min.css
 GITWEB_CSS = gitweb.min.css
+all:: gitweb.min.css
+gitweb.min.css: gitweb.css GITWEB-BUILD-OPTIONS
+	$(QUIET_GEN)$(CSSMIN) <$ >$@
 endif
-gitweb.cgi: gitweb.perl $(GITWEB_JS) $(GITWEB_CSS)
 
-gitweb.cgi:
+GITWEB_REPLACE = \
+	-e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \
+	-e 's|++GIT_BINDIR++|$(bindir)|g' \
+	-e 's|++GITWEB_CONFIG++|$(GITWEB_CONFIG)|g' \
+	-e 's|++GITWEB_CONFIG_SYSTEM++|$(GITWEB_CONFIG_SYSTEM)|g' \
+	-e 's|++GITWEB_HOME_LINK_STR++|$(GITWEB_HOME_LINK_STR)|g' \
+	-e 's|++GITWEB_SITENAME++|$(GITWEB_SITENAME)|g' \
+	-e 's|++GITWEB_PROJECTROOT++|$(GITWEB_PROJECTROOT)|g' \
+	-e 's|"++GITWEB_PROJECT_MAXDEPTH++"|$(GITWEB_PROJECT_MAXDEPTH)|g' \
+	-e 's|++GITWEB_EXPORT_OK++|$(GITWEB_EXPORT_OK)|g' \
+	-e 's|++GITWEB_STRICT_EXPORT++|$(GITWEB_STRICT_EXPORT)|g' \
+	-e 's|++GITWEB_BASE_URL++|$(GITWEB_BASE_URL)|g' \
+	-e ...
[ message continues ]