Re: [Patch 001/GSoC] Move static files into subdir

[Jakub Narebski <jnareb@gmail.com>]

Pavan Kumar Sunkara <pavan.sss1991@gmail.com> writes:

> Hi,
> 
> It's been a while I mail to this list since I got GSoC. But I have
> been in contact with Christian and Petr (mentors) everyday. As I am
> having my vacation, I decided to statrt the project earlier itself.
> Here's is my first patch in the process of my GSoC.
> 
> One of my project goals is to split gitweb. This patch initiates the splitting.
> 
> PATCH:
> 
> From e25db0b62b481e029354ad33af8f0615a8353633 Mon Sep 17 00:00:00 2001
> From: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
> Date: Wed, 5 May 2010 21:44:57 -0700
> Subject: [PATCH] Gitweb: Move all static files into a seperate directory

Almost all right.  Please read Documentation/SubmittingChanges first.

There are two ways of submitting a patch with some comment about patch
that should not be put in commit message.

The first is to put first line (summary) of a commit message, in your
case
  [PATCH GSoC] gitweb: Move all static files into a seperate directory
in *email* subject, and put comments about patch itself between "---\n"
line and diffstat (see also below).

The second, used for example if the patch is byproduct of an email, is
to write comment in the email, and add patch itself after a separator.
Commonly used separator is 'scissors' line (see git-mailinfo(1)), e.g.
"-- >8 --\n" line.  You need to write 'Subject:' line _only_ if the
subject of an email is different.

The first is more commonly used, as you can see by looking at emails
marked with '[PATCH]' in git mailing list; you can use mailing list
archive like MARC or GMane, or GMane NNTP (Usenet) interface reading
it with news reader.
 
> This commit creates a new subdirectory called 'static' in gitweb
> which will contain all the static files required by gitweb.cgi
> while executing. By doing so, the gitweb source will be more
> readable and maintainable.

A minor issue: commit messages are usually written in imperative;
also I'd rather avoid marketspeak-sounding "will be":

  Create a new subdirectory called 'static' in gitweb/, and move
  all static files required by gitweb.cgi when running, which means
  styles, images and Javascript code. This should make gitweb more
  readable and easier to maintain.


I also wonder how other project solve this issue. Well, Gitalist
uses 'root/static/'...

> 
> Also changed INSTALL, README, Makefile and test files
> according to this change.

A minor issue: "Also changed" doesn't look like correct English
grammar for me.  I am not a native English speaker either, but I would
personally write:

  Update t/gitweb-lib.sh to reflect this change.  The default is
  now to install static files also in 'static' subdirectory in target
  directory: update gitweb's INSTALL, README and Makefile accordingly.


Note that favicon.ico should be installed in top directory, but I
think it doesn't really matter where is git-favicon.png as long as
'<link rel="shortcut icon" ... />' points in correct place...

> 
> Signed-off-by: Pavan Kumar Sunkara <pavan.sss1991@gmail.com>
> ---
>  gitweb/INSTALL                      |   20 ++++++++++----------
>  gitweb/Makefile                     |   20 ++++++++++----------
>  gitweb/README                       |    8 ++++----
>  gitweb/{ => static}/git-favicon.png |  Bin 115 -> 115 bytes
>  gitweb/{ => static}/git-logo.png    |  Bin 207 -> 207 bytes
>  gitweb/{ => static}/gitweb.css      |    0
>  gitweb/{ => static}/gitweb.js       |    0
>  t/gitweb-lib.sh                     |    6 +++---
>  8 files changed, 27 insertions(+), 27 deletions(-)
>  rename gitweb/{ => static}/git-favicon.png (100%)
>  rename gitweb/{ => static}/git-logo.png (100%)
>  rename gitweb/{ => static}/gitweb.css (100%)
>  rename gitweb/{ => static}/gitweb.js (100%)

You might need to update also git-instaweb.sh and generating
git-instaweb in main Makefile.

> 
> diff --git a/gitweb/INSTALL b/gitweb/INSTALL
> index cbdc136..60c25ff 100644
> --- a/gitweb/INSTALL
> +++ b/gitweb/INSTALL
> @@ -7,7 +7,8 @@ gitweb.css, git-logo.png and git-favicon.png) to their
> destination.
>  For example if git was (or is) installed with /usr prefix, you can do
> 
>  	$ make prefix=/usr gitweb             ;# as yourself
> -	# cp gitweb/git* /var/www/cgi-bin/    ;# as root
> +	# cp gitweb/gitweb.cgi /var/www/cgi-bin/   ;# as root
> +	# cp -r gitweb/static  /var/www/cgi-bin/   ;# as root

This assumes that ones 'cp' support -r / --recursive option.
How portable is this?

But I think this might be all right in an example.

> @@ -79,17 +81,15 @@ Build example
>    we want to display are under /home/local/scm, you can do
> 
>  	make GITWEB_PROJECTROOT="/home/local/scm" \
> -	     GITWEB_JS="/gitweb/gitweb.js" \
> -	     GITWEB_CSS="/gitweb/gitweb.css" \
> -	     GITWEB_LOGO="/gitweb/git-logo.png" \
> -	     GITWEB_FAVICON="/gitweb/git-favicon.png" \
> +	     GITWEB_JS="/gitweb/static/gitweb.js" \
> +	     GITWEB_CSS="/gitweb/static/gitweb.css" \
> +	     GITWEB_LOGO="/gitweb/static/git-logo.png" \
> +	     GITWEB_FAVICON="/gitweb/static/git-favicon.png" \
>  	     bindir=/usr/local/bin \
>  	     gitweb
> 
> -	cp -fv ~/git/gitweb/gitweb.{cgi,js,css} \
> -	       ~/git/gitweb/git-{favicon,logo}.png \
> -	     /var/www/cgi-bin/gitweb/
> -
> +	cp -fv gitweb/gitweb.cgi /var/www/cgi-bin/gitweb/
> +	cp -r gitweb/static /var/www/cgi-bin/gitweb/

Sidenote (something to think about in the future): we might want to
put static files in /var/www/html/gitweb/ (but not in 
/var/www/html/gitweb/static/), outside of where gitweb.cgi is put.
 
> diff --git a/gitweb/Makefile b/gitweb/Makefile
> index f2e1d92..c0d5da3 100644
> --- a/gitweb/Makefile
> +++ b/gitweb/Makefile
> @@ -26,10 +26,10 @@ GITWEB_STRICT_EXPORT =
>  GITWEB_BASE_URL =
>  GITWEB_LIST =
>  GITWEB_HOMETEXT = indextext.html
> -GITWEB_CSS = gitweb.css
> -GITWEB_LOGO = git-logo.png
> -GITWEB_FAVICON = git-favicon.png
> -GITWEB_JS = gitweb.js
> +GITWEB_CSS = static/gitweb.css
> +GITWEB_LOGO = static/git-logo.png
> +GITWEB_FAVICON = static/git-favicon.png
> +GITWEB_JS = static/gitweb.js
>  GITWEB_SITE_HEADER =
>  GITWEB_SITE_FOOTER =
> 
> @@ -81,16 +81,16 @@ endif
>  all:: gitweb.cgi
> 
>  ifdef JSMIN
> -GITWEB_JS = gitweb.min.js
> -all:: gitweb.min.js
> -gitweb.min.js: gitweb.js GITWEB-BUILD-OPTIONS
> +GITWEB_JS = static/gitweb.min.js
> +all:: static/gitweb.min.js
> +static/gitweb.min.js: static/gitweb.js GITWEB-BUILD-OPTIONS
>  	$(QUIET_GEN)$(JSMIN) <$< >$@
>  endif
> 
>  ifdef CSSMIN
> -GITWEB_CSS = gitweb.min.css
> -all:: gitweb.min.css
> -gitweb.min.css: gitweb.css GITWEB-BUILD-OPTIONS
> +GITWEB_CSS = static/gitweb.min.css
> +all:: static/gitweb.min.css
> +static/gitweb.min.css: static/gitweb.css GITWEB-BUILD-OPTIONS
>  	$(QUIET_GEN)$(CSSMIN) <$ >$@
>  endif

You need to update 'clean' target in gitweb/Makefile too.
 
> diff --git a/gitweb/README b/gitweb/README
> index 71742b3..eeac204 100644
> --- a/gitweb/README
> +++ b/gitweb/README
> @@ -80,23 +80,23 @@ You can specify the following configuration
> variables when building GIT:
>     Points to the location where you put gitweb.css on your web server
>     (or to be more generic, the URI of gitweb stylesheet).  Relative to the
>     base URI of gitweb.  Note that you can setup multiple stylesheets from
> -   the gitweb config file.  [Default: gitweb.css (or gitweb.min.css if the
> +   the gitweb config file.  [Default: static/gitweb.css (or
> gitweb.min.css if the
>     CSSMIN variable is defined / CSS minifier is used)]

Word wrapped patch. It should read:

  @@ -80,23 +80,23 @@ You can specify the following configuration
  variables when building GIT:
      Points to the location where you put gitweb.css on your web server
      (or to be more generic, the URI of gitweb stylesheet).  Relative to the
      base URI of gitweb.  Note that you can setup multiple stylesheets from
  -   the gitweb config file.  [Default: gitweb.css (or gitweb.min.css if the
  +   the gitweb config file.  [Default: static/gitweb.css (or gitweb.min.css if the
      CSSMIN variable is defined / CSS minifier is used)]

You need to turn off line wrapping in your email client when sending a
patch.  Here it is minor issue.

Also you forgot to update gitweb.min.css to static/gitweb.min.css...
in which case you should probably rewrap this line to:

  @@ -80,24 +80,24 @@ You can specify the following configuration
  variables when building GIT:
      Points to the location where you put gitweb.css on your web server
      (or to be more generic, the URI of gitweb stylesheet).  Relative to the
      base URI of gitweb.  Note that you can setup multiple stylesheets from
  -   the gitweb config file.  [Default: gitweb.css (or gitweb.min.css if the
  -   CSSMIN variable is defined / CSS minifier is used)]
  +   the gitweb config file.  [Default: static/gitweb.css (or static/gitweb.min.css
  +   if the CSSMIN variable is defined / CSS minifier is used)]

[...]

> -   Relative to base URI of gitweb.  [Default: gitweb.js (or gitweb.min.js
> +   Relative to base URI of gitweb.  [Default: static/gitweb.js (or
> gitweb.min.js

Word-wrapped.

> diff --git a/gitweb/gitweb.css b/gitweb/static/gitweb.css
> similarity index 100%
> rename from gitweb/gitweb.css
> rename to gitweb/static/gitweb.css

Good!

[...]
> diff --git a/t/gitweb-lib.sh b/t/gitweb-lib.sh
> index 5a734b1..b70b891 100644
> --- a/t/gitweb-lib.sh
> +++ b/t/gitweb-lib.sh
> @@ -19,9 +19,9 @@ our \$site_name = '[localhost]';
>  our \$site_header = '';
>  our \$site_footer = '';
>  our \$home_text = 'indextext.html';
> -our @stylesheets = ('file:///$TEST_DIRECTORY/../gitweb/gitweb.css');
> -our \$logo = 'file:///$TEST_DIRECTORY/../gitweb/git-logo.png';
> -our \$favicon = 'file:///$TEST_DIRECTORY/../gitweb/git-favicon.png';
> +our @stylesheets = ('file:///$TEST_DIRECTORY/../gitweb/static/gitweb.css');
> +our \$logo = 'file:///$TEST_DIRECTORY/../gitweb/static/git-logo.png';
> +our \$favicon = 'file:///$TEST_DIRECTORY/../gitweb/static/git-favicon.png';
>  our \$projects_list = '';
>  our \$export_ok = '';
>  our \$strict_export = '';

Thanks for thinking about this... although it doesn't really matter,
as it is not served to a web browser.

-- 
Jakub Narebski
Poland
ShadeHawk on #git