Gitweb caching changes v2

This is an attempt to break out the default values & associated documentation from the main gitweb file so that it's easier to browse / read and understand without the associated code involved.

This helps by making defaults self contained with their documentation making it easier for someone to read through things and find what they want

This is also a not-so-subtle start of trying to break up gitweb into separate files for easier maintainability, having everything in a single file is just a mess and makes the whole thing more complicated than it needs to be. This is a bit of a baby step towards breaking it up for easier maintenance.

Signed-off-by John 'Warthog9' Hawley <[email protected]>

---
 .gitignore                  |    1 +
 Makefile                    |   15 +-
 gitweb/Makefile             |    2 +-
 gitweb/gitweb.perl          |  515 +++++--------------------------------------
 gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 537 insertions(+), 464 deletions(-)
 create mode 100644 gitweb/gitweb_defaults.perl

.gitignore +0 -0

index ac02a58..5e48102 100644

.gitignore +0 -0

 /test-chmtime
 /test-ctype
 /test-date

Makefile +2 -3

index 8db9d01..2c5f139 100644
@@ -1510,14 +1510,16 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	mv $@+ $@
 
 .PHONY: gitweb
-gitweb: gitweb/gitweb.cgi
+gitweb: gitweb/gitweb.cgi gitweb/gitweb_defaults.pl
 ifdef JSMIN
-OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
-gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js
+OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js gitweb/gitweb_defaults.pl

Makefile +1 -0

 	chmod +x $@+ && \
 	mv $@+ $@
 

@@ -1913,6 +1915,7 @@ clean:
 	$(MAKE) -C Documentation/ clean
 ifndef NO_PERL
 	$(RM) gitweb/gitweb.cgi
+	$(RM) gitweb/gitweb_defaults.pl
 	$(MAKE) -C perl clean
 endif
 	$(MAKE) -C templates/ clean

gitweb/Makefile +1 -1

index 8d318b3..2bd421a 100644
@@ -1,6 +1,6 @@
 SHELL = /bin/bash
 
-FILES = gitweb.cgi
+FILES = gitweb.cgi gitweb_defaults.pl
 
 .PHONY: $(FILES)
 

gitweb/gitweb.perl +0 -0

index 3b44371..fd41539 100755

gitweb/gitweb.perl +48 -0

+# Define and than setup our configuration 
+#
+our(
+	$VERSION,
+	$path_info,
+	$GIT,
+	$projectroot,
+	$project_maxdepth,
+	$home_link,
+	$home_link_str,
+	$site_name,
+	$site_header,
+	$home_text,
+	$site_footer,
+	@stylesheets,
+	$stylesheet,
+	$logo,
+	$favicon,
+	$javascript,
+	$logo_url,
+	$logo_label,
+	$projects_list,
+	$projects_list_description_width,
+	$default_projects_order,
+	$export_ok,
+	$export_auth_hook,
+	$strict_export,
+	@git_base_url_list,
+	$default_blob_plain_mimetype,
+	$default_text_plain_charset,
+	$mimetypes_file,
+	$missmatch_git,
+	$gitlinkurl,
+	$maxload,
+	$cache_enable,
+	$minCacheTime,
+	$maxCacheTime,
+	$cachedir,
+	$backgroundCache,
+	$nocachedata,
+	$nocachedatabin,
+	$fullhashpath,
+	$fullhashbinpath,
+	$export_auth_hook,
+	%known_snapshot_format_aliases,
+	%known_snapshot_formats,
+	$path_info,
+	$fallback_encoding,

gitweb/gitweb.perl +1 -0

+do 'gitweb_defaults.pl';
 
 sub gitweb_get_feature {
 	my ($name) = @_;

gitweb/gitweb_defaults.perl new file +0 -0

new file mode 100644
index 0000000..ede0daf

gitweb/gitweb_defaults.perl new file +1 -0

+1;

Quoted 5 lines

This is also a not-so-subtle start of trying to break up gitweb into separate files for easier maintainability, having everything in a single file is just a mess and makes the whole thing more complicated than it needs to be. This is a bit of a baby step towards breaking it up for easier maintenance.

The question is if easier maintenance and development by spliting gitweb for developers offsets ease of install for users.

Quoted 2 lines

Signed-off-by John 'Warthog9' Hawley <[email protected]>

Signoff mismatch.

Quoted 10 lines

---
 .gitignore                  |    1 +
 Makefile                    |   15 +-
 gitweb/Makefile             |    2 +-
 gitweb/gitweb.perl          |  515 +++++--------------------------------------
 gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 537 insertions(+), 464 deletions(-)
 create mode 100644 gitweb/gitweb_defaults.perl

.gitignore line 154

Quoted 9 lines

.gitignore +1 -0

index ac02a58..5e48102 100644
@@ -151,6 +151,7 @@
 /git-core-*/?*
 /gitk-git/gitk-wish
 /gitweb/gitweb.cgi
+/gitweb/gitweb_defaults.pl

Hmmm... gitweb/gitweb_defaults.perl as source file, and gitweb/gitweb_defaults.pl as generated file? Wouldn't it be better to go with the convention used elsewhere in gitweb and use gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as source file?

.gitignore line 155

Quoted 1 line

.gitignore +1 -0

+	#$(QUIET_GEN)$(RM) $@ $@+ &&

What this line is about?

.gitignore endif · line 1544

Quoted 10 lines

.gitignore +1 -1

 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \

@@ -1539,7 +1541,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \

Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?

Also wouldn't all replacements be in the new gitweb_defaults file, so there would be no need then to do replacements for gitweb.cgi?

Oh, I see there is at least one that stayed in gitweb.perl: $version

Quoted 53 lines

.gitignore +53 -0

+# Define and than setup our configuration 
+#
+our(
+	$VERSION,
+	$path_info,
+	$GIT,
+	$projectroot,
+	$project_maxdepth,
+	$home_link,
+	$home_link_str,
+	$site_name,
+	$site_header,
+	$home_text,
+	$site_footer,
+	@stylesheets,
+	$stylesheet,
+	$logo,
+	$favicon,
+	$javascript,
+	$logo_url,
+	$logo_label,
+	$projects_list,
+	$projects_list_description_width,
+	$default_projects_order,
+	$export_ok,
+	$export_auth_hook,
+	$strict_export,
+	@git_base_url_list,
+	$default_blob_plain_mimetype,
+	$default_text_plain_charset,
+	$mimetypes_file,
+	$missmatch_git,
+	$gitlinkurl,
+	$maxload,
+	$cache_enable,
+	$minCacheTime,
+	$maxCacheTime,
+	$cachedir,
+	$backgroundCache,
+	$nocachedata,
+	$nocachedatabin,
+	$fullhashpath,
+	$fullhashbinpath,
+	$export_auth_hook,
+	%known_snapshot_format_aliases,
+	%known_snapshot_formats,
+	$path_info,
+	$fallback_encoding,
+	%avatar_size,
+	$project_maxdepth,
+	$headerRefresh,
+	$base_url,
+	$projects_list_description_width,

.gitignore

Quoted 5 lines

.gitignore +4 -0

+	$default_projects_order,
+	$prevent_xss,
+	@diff_opts,
+	%feature
 );

Why this block is required? Why not have variables defined (using "our") in gitweb_defaults file?

[cut deletion]

Quoted 14 lines

.gitignore +1 -0

+do 'gitweb_defaults.pl';
 
 sub gitweb_get_feature {
 	my ($name) = @_;

gitweb/gitweb_defaults.perl new file +4 -0

new file mode 100644
index 0000000..ede0daf
@@ -0,0 +1,468 @@
+# gitweb - simple web interface to track changes in git repositories
+#
+# (C) 2005-2006, Kay Sievers <[email protected]>
+# (C) 2005, Christian Gierke

gitweb/gitweb_defaults.perl line 10

Quoted 6 lines

gitweb/gitweb_defaults.perl new file +6 -0

+#
+# This program is licensed under the GPLv2
+
+# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
+# needed and used only for URLs with nonempty PATH_INFO
+$base_url = $my_url;

Why not "our $base_url = $my_url;"?

Quoted 6 lines

Quoted 5 lines

This is also a not-so-subtle start of trying to break up gitweb into separate files for easier maintainability, having everything in a single file is just a mess and makes the whole thing more complicated than it needs to be. This is a bit of a baby step towards breaking it up for easier maintenance.

Quoted 2 lines

The question is if easier maintenance and development by spliting gitweb for developers offsets ease of install for users.

This would just get dropped into the same location that gitweb.cgi exists in, there is no real difference in installation, and thus I can't see this as an issue for users.

Quoted 10 lines

Quoted 10 lines

---
 .gitignore                  |    1 +
 Makefile                    |   15 +-
 gitweb/Makefile             |    2 +-
 gitweb/gitweb.perl          |  515 +++++--------------------------------------
 gitweb/gitweb_defaults.perl |  468 +++++++++++++++++++++++++++++++++++++++
 5 files changed, 537 insertions(+), 464 deletions(-)
 create mode 100644 gitweb/gitweb_defaults.perl

Quoted 15 lines

.gitignore line 154

Quoted 9 lines

.gitignore +1 -0

index ac02a58..5e48102 100644
@@ -151,6 +151,7 @@
 /git-core-*/?*
 /gitk-git/gitk-wish
 /gitweb/gitweb.cgi
+/gitweb/gitweb_defaults.pl

Hmmm... gitweb/gitweb_defaults.perl as source file, and gitweb/gitweb_defaults.pl as generated file? Wouldn't it be better to go with the convention used elsewhere in gitweb and use gitweb/gitweb_defaults.perl.in or gitweb/gitweb_defaults.pl.in as source file?

I think you got confused, the committed file is .perl the generated file is .pl.

Quoted 11 lines

.gitignore endif · line 1544

Quoted 9 lines

.gitignore +1 -1

 	$(QUIET_GEN)$(RM) $@ $@+ && \
 	sed -e '1s|#!.*perl|#!$(PERL_PATH_SQ)|' \
 	    -e 's|++GIT_VERSION++|$(GIT_VERSION)|g' \

@@ -1539,7 +1541,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $(patsubst %.cgi,%.perl,$@) >$@+ && \
+	    $(patsubst %.cgi,%.perl,$(patsubst %.pl, %.perl, $@)) >$@+ && \

Why the slightly inconsistent style ("%.cgi,%perl" vs "%.pl, %perl")?

Considering that the defaults is more of an include vs. a cgi it probably shouldn't share the standard expected executable suffix, thus I used .pl. Could just as easily change it to .pm, or something else but I think it would make the most sense to leave things we are expecting the webserver to directly execute as .cgi, and includes as a different suffix.

Quoted 2 lines

Also wouldn't all replacements be in the new gitweb_defaults file, so there would be no need then to do replacements for gitweb.cgi?

Not all replacements are done in one or the other, and since it's basically a NOP to perform the full set of replacements on both files that seemed the easiest way to ensure they were done in both places.

Quoted 2 lines

Oh, I see there is at least one that stayed in gitweb.perl: $version

<snip>

Quoted 6 lines

Quoted 6 lines

.gitignore +6 -0

+# Define and than setup our configuration 
+#
+our(
+	$VERSION,
+	$path_info,
+	$GIT,

Quoted 7 lines

.gitignore

Quoted 4 lines

.gitignore +3 -0

+	$prevent_xss,
+	@diff_opts,
+	%feature
 );

Why this block is required? Why not have variables defined (using "our") in gitweb_defaults file?

Wanted to make sure things were properly defined, if in an unexpected state, should a user have gitweb.cgi in place but not the defaults.

Quoted 4 lines

Quoted 4 lines

gitweb/gitweb_defaults.perl new file +0 -1

new file mode 100644
index 0000000..ede0daf
--- /dev/null

Quoted 8 lines

gitweb/gitweb_defaults.perl line 6

Quoted 8 lines

+++ b/gitweb/gitweb_defaults.perl

gitweb/gitweb_defaults.perl new file +6 -0

@@ -0,0 +1,468 @@
+# gitweb - simple web interface to track changes in git repositories
+#
+# (C) 2005-2006, Kay Sievers <[email protected]>
+# (C) 2005, Christian Gierke
+#
+# This program is licensed under the GPLv2

Quoted 6 lines

gitweb/gitweb_defaults.perl line 10

Quoted 4 lines

gitweb/gitweb_defaults.perl new file +4 -0

+
+# Base URL for relative URLs in gitweb ($logo, $favicon, ...),
+# needed and used only for URLs with nonempty PATH_INFO
+$base_url = $my_url;

Why not "our $base_url = $my_url;"?

same reason as the other 'our' includes above,

though why this ended up
as a separate patch vs. the rest of the file I don't know.

Quoted 6 lines · J.H.

Quoted 6 lines

Quoted 5 lines

This is also a not-so-subtle start of trying to break up gitweb into separate files for easier maintainability, having everything in a single file is just a mess and makes the whole thing more complicated than it needs to be. This is a bit of a baby step towards breaking it up for easier maintenance.

Quoted 6 lines

Quoted 2 lines

The question is if easier maintenance and development by spliting gitweb for developers offsets ease of install for users.

This would just get dropped into the same location that gitweb.cgi exists in, there is no real difference in installation, and thus I can't see this as an issue for users.

To be more exact you have to know that you have to drop _generated files_, which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl (or whatever the generated file with config variables would be named).

ATTENTION!

Not that I am anxious to queue new topics to 'next' right now (we are frozen for 1.6.6), but I think having what is proven to work well at a real site like k.org is much better than waiting for an unproven reimplementation using somebody else's framework only for your theoretical cleanliness.  John has better things to do than doing such a rewrite himself, and even if you helped the process by producing a competing caching scheme based on existing web caching engines, the aggregated result (not just the web caching engine you base your work on) needs to get a similar field exposure to prove itself that it can scale to the load k.org sees, which would be quite a lot of work, no?

Quoted 15 lines · Jakub Narebski

Quoted 8 lines · J.H.

Quoted 7 lines

Quoted 5 lines

This is also a not-so-subtle start of trying to break up gitweb into separate files for easier maintainability, having everything in a single file is just a mess and makes the whole thing more complicated than it needs to be. This is a bit of a baby step towards breaking it up for easier maintenance.

The question is if easier maintenance and development by spliting gitweb for developers offsets ease of install for users.

This would just get dropped into the same location that gitweb.cgi exists in, there is no real difference in installation, and thus I can't see this as an issue for users.

To be more exact you have to know that you have to drop _generated files_, which means (for this version of patch) gitweb.cgi and gitweb_defaults.pl (or whatever the generated file with config variables would be named).

Quoted 1 line

ATTENTION!

You didn't have to shout.

Any progress on this front?

Not that I am anxious to queue new topics to 'next' right now (we are frozen for 1.6.6), but I think having what is proven to work well at a real site like k.org is much better than waiting for an unproven reimplementation using somebody else's framework only for your theoretical cleanliness. John has better things to do than doing such a rewrite himself, and even if you helped the process by producing a competing caching scheme based on existing web caching engines, the aggregated result (not just the web caching engine you base your work on) needs to get a similar field exposure to prove itself that it can scale to the load k.org sees, which would be quite a lot of work, no?

Quoted 1 line · Junio C Hamano

Any progress on this front?

Sadly, no. Busy weekend and a need to get some of the kernel.org servers upgraded has taken some precedence. I should be circling back around on this tomorrow I think.

Quoted 4 lines · J.H.

Quoted 1 line · Junio C Hamano

Any progress on this front?

Sadly, no. Busy weekend and a need to get some of the kernel.org servers upgraded has taken some precedence. I should be circling back around on this tomorrow I think.

So should I wait for reroll with proposals for improvements (modified patches)?

This adjust the makefiles so that you can do such things as

make gitweb

from the top level make tree,

or if your in the gitweb directory
itself typing

make

will call back up to the main Makefile and build gitweb

Signed-off-by John 'Warthog9' Hawley <[email protected]>

---
 Makefile        |    4 +++-
 gitweb/Makefile |   14 ++++++++++++++
 2 files changed, 17 insertions(+), 1 deletions(-)
 create mode 100644 gitweb/Makefile

gitweb/Makefile new file +9 -0

+
+all: $(FILES)
+
+$(FILES):
+	$(MAKE) $(MFLAGS) -C ../ -f Makefile gitweb/$@
+
+clean:
+	rm -rf $(FILES)
+

IMPORTANT!

A note about this change: I think it would be better to move creating gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make main Makefile call gitweb/Makefile, and not vice versa like in your solution.

If it is possible.

Quoted 9 lines

Makefile +0 -0

index 4a1e5bc..8db9d01 100644
@@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	chmod +x $@+ && \
 	mv $@+ $@
 

Makefile $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl · line 1512

Quoted 1 line

Makefile +1 -0

+.PHONY: gitweb

Why it is here, and not with the .PHONY block at line 1924 of Makefile? It would be nice to have comment supporting this choice in email with this patch (or in commit message).

Makefile endif · line 1542

Quoted 11 lines

Makefile +2 -1

+gitweb: gitweb/gitweb.cgi
 ifdef JSMIN
 OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
 gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js

@@ -1537,7 +1539,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $< >$@+ && \
+	    $(patsubst %.cgi,%.perl,$@) >$@+ && \

Why this change?

gitweb/Makefile line 1

Quoted 11 lines

Makefile +0 -0

 	chmod +x $@+ && \
 	mv $@+ $@
 

gitweb/Makefile new file +1 -0

new file mode 100644
index 0000000..8d318b3
@@ -0,0 +1,14 @@
+SHELL = /bin/bash

Why is this needed?

gitweb/Makefile line 5

Quoted 4 lines

gitweb/Makefile new file +4 -0

+
+FILES = gitweb.cgi
+
+.PHONY: $(FILES)

Why .PHONY? $(FILES) are created.

Quoted 9 lines

gitweb/Makefile new file +9 -0

+
+all: $(FILES)
+
+$(FILES):
+	$(MAKE) $(MFLAGS) -C ../ -f Makefile gitweb/$@
+
+clean:
+	rm -rf $(FILES)
+

Quoted 7 lines

IMPORTANT!

A note about this change: I think it would be better to move creating gitweb.cgi (and optionally gitweb.min.js) to gitweb/Makefile, and make main Makefile call gitweb/Makefile, and not vice versa like in your solution.

Quoted 1 line

If it is possible.

It's quite possible, and I'm fine with doing that. If no one has any objections I can re-work those with the understanding that the build process for gitweb shift to the gitweb/ directory instead of the main Makefile.

Quoted 8 lines

Quoted 8 lines

Makefile +0 -0

index 4a1e5bc..8db9d01 100644
@@ -1509,6 +1509,8 @@ $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl
 	chmod +x $@+ && \
 	mv $@+ $@
 

Quoted 5 lines

Makefile $(patsubst %.perl,%,$(SCRIPT_PERL)): % : %.perl · line 1512

Quoted 1 line

Makefile +1 -0

+.PHONY: gitweb

Why it is here, and not with the .PHONY block at line 1924 of Makefile? It would be nice to have comment supporting this choice in email with this patch (or in commit message).

There are 6 other instances of .PHONY in the makefile, having the .PHONY localized seemed to make it the most obvious since it was right next to the actual target.

Quoted 4 lines

Quoted 4 lines

Makefile +1 -0

+gitweb: gitweb/gitweb.cgi
 ifdef JSMIN
 OTHER_PROGRAMS += gitweb/gitweb.cgi   gitweb/gitweb.min.js
 gitweb/gitweb.cgi: gitweb/gitweb.perl gitweb/gitweb.min.js

Quoted 8 lines

Makefile endif · line 1542

Quoted 6 lines

Makefile +1 -1

@@ -1537,7 +1539,7 @@ endif
 	    -e 's|++GITWEB_JS++|$(GITWEB_JS)|g' \
 	    -e 's|++GITWEB_SITE_HEADER++|$(GITWEB_SITE_HEADER)|g' \
 	    -e 's|++GITWEB_SITE_FOOTER++|$(GITWEB_SITE_FOOTER)|g' \
-	    $< >$@+ && \
+	    $(patsubst %.cgi,%.perl,$@) >$@+ && \

Why this change?

Preparation for a later change. The change could happen all at the same time if it makes more logical sense.

Quoted 12 lines

gitweb/Makefile line 1

Quoted 10 lines

Makefile +0 -0

 	chmod +x $@+ && \
 	mv $@+ $@
 

gitweb/Makefile new file +1 -0

new file mode 100644
index 0000000..8d318b3
@@ -0,0 +1,14 @@
+SHELL = /bin/bash

Why is this needed?

Quoted 6 lines

gitweb/Makefile line 5

Quoted 4 lines

gitweb/Makefile new file +4 -0

+
+FILES = gitweb.cgi
+
+.PHONY: $(FILES)

Why .PHONY? $(FILES) are created.

From this makefile I wanted to explicitly call up to the main makefile
no matter what, the main makefile doesn't consider the targets .PHONY and it has all the dependencies that it would expect.

This adds a git:// link to the summary pages should a common $gitlinkurl be defined (default is nothing defined, thus nothing shown)

This adds a git:// link to the summary pages should a common $gitlinkurl be defined (default is nothing defined, thus nothing shown)

This does make the assumption that the git trees share a common path, and nothing to date is known to actually make use of the link

Signed-off-by John 'Warthog9' Hawley <[email protected]>

---
 gitweb/gitweb.perl |    8 ++++++++
 1 files changed, 8 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +0 -0

index d84f4c0..7ad096c 100755

This adds $missmatch_git so that gitweb can run with a miss-matched git install.

This adds $missmatch_git so that gitweb can run with a miss-matched git install. Gitweb, generally, runs fine on a very broad range of git versions, but it's not always practicle or useful to upgrade it every time you upgrade git.

This allows the administrator to realize they are miss-matched, and should they be so inclined, disable the check entirely and run in a miss-matched fasion.

This is more here to give an obvious warning as to whats going on vs. silently failing.

Signed-off-by John 'Warthog9' Hawley <[email protected]>

---
 gitweb/gitweb.perl |   22 ++++++++++++++++++++++
 1 files changed, 22 insertions(+), 0 deletions(-)

gitweb/gitweb.perl +0 -0

index 813e48f..d84f4c0 100755

gitweb/gitweb.perl +5 -0

+	print "</p>\n";
+	git_footer_html();
+	exit;
+}
+
 $projects_list ||= $projectroot;
 
 # ======================================================================

"John 'Warthog9' Hawley" <[email protected]> :
+ open($load, '<', '/proc/loadavg') or return 0;

 gitweb/gitweb.perl |   24 ++++++++++++++++++++++++

 1 files changed, 24 insertions(+), 0 deletions(-)

This changes the behavior, slightly, of gitweb so that it verifies that the box isn't inundated with before attempting to serve gitweb. If the box is overloaded, it basically returns a 503 server unavailable until the load falls below the defined threshold. This helps dramatically if you have a box that's I/O bound, reaches a certain load and you don't want gitweb, the I/O hog that it is, increasing the pain the server is already undergoing.

adds $maxload configuration variable. Default is a load of 300, which for most cases should never be hit.

Please note this makes the assumption that /proc/loadavg exists as there is no good way to read load averages on a great number of platforms [READ: Windows], or that it's reasonably accurate.

Signed-off-by John 'Warthog9' Hawley <[email protected]>

---
 gitweb/gitweb.perl |   24 ++++++++++++++++++++++++
 1 files changed, 24 insertions(+), 0 deletions(-) 

gitweb/gitweb.perl +2 -0

+our $maxload = 300;
+
 # You define site-wide feature defaults here; override them with
 # $GITWEB_CONFIG as necessary.
 our %feature = (

gitweb/gitweb.perl +2 -0

+}
+
 # version of the core git binary
 our $git_version = qx("$GIT" --version) =~ m/git version (.*)$/ ? $1 : "unknown";
 $number_of_git_cmds++;

Quoted 2 lines

adds $maxload configuration variable. Default is a load of 300, which for most cases should never be hit.

Your patch doesn't allow for *turning off* this feature. Reasonable solution would be to use 'undef' or negative number to turn off this check (this feature).

Quoted 3 lines

Please note this makes the assumption that /proc/loadavg exists as there is no good way to read load averages on a great number of platforms [READ: Windows], or that it's reasonably accurate.

What about MacOS X, or FreeBSD, or OpenSolaris?

You should mention that it is intended that if gitweb cannot read load average (for example /proc/loadavg does not exist), then the feature is turned off, i.e. the check always succeeds. Which is reasonable.

Quoted 2 lines

Signed-off-by John 'Warthog9' Hawley <[email protected]>

Why signoff is different from author ([email protected])? Why this email for signoff? Just curious...

gitweb/gitweb.perl if (-e $GITWEB_CONFIG) { · line 564

Quoted 6 lines

gitweb/gitweb.perl +6 -0

+# loadavg throttle
+sub get_loadavg() {
+    my $load;
+    my @loads;
+
+    open($load, '<', '/proc/loadavg') or return 0;

Why not use one of existing CPAN modules: Sys::Info::Device::CPU, BSD::getloadavg, Sys::CpuLoad?

gitweb/gitweb.perl if (-e $GITWEB_CONFIG) { · line 575

Quoted 6 lines

gitweb/gitweb.perl +6 -0

+if (get_loadavg() > $maxload) {
+    print "Content-Type: text/plain\n";
+    print "Status: 503 Excessive load on server\n";
+    print "\n";
+    print "The load average on the server is too high\n";
+    exit 0;

Why not use die_error subroutine? Is it to have generate absolutely minimal load, and that is why you do not use die_error(), or even $cgi->header()?

Wouldn't a better solution be to use here-doc syntax?

<snip>

Quoted 6 lines

Quoted 2 lines

adds $maxload configuration variable. Default is a load of 300, which for most cases should never be hit.

Your patch doesn't allow for *turning off* this feature. Reasonable solution would be to use 'undef' or negative number to turn off this check (this feature).

Well there's the opposite argument that setting the number arbitrarily high, 4096 for instance would also in essence negate this (though I'll admit I've reached and exceeded those numbers before)

That said I agree, being able to turn this off needs to be added and will be shortly.

Quoted 5 lines

Quoted 3 lines

Please note this makes the assumption that /proc/loadavg exists as there is no good way to read load averages on a great number of platforms [READ: Windows], or that it's reasonably accurate.

What about MacOS X, or FreeBSD, or OpenSolaris?

Will comment on this further down

Quoted 3 lines

You should mention that it is intended that if gitweb cannot read load average (for example /proc/loadavg does not exist), then the feature is turned off, i.e. the check always succeeds. Which is reasonable.

That's fine.

Quoted 9 lines

gitweb/gitweb.perl if (-e $GITWEB_CONFIG) { · line 564

Quoted 6 lines

gitweb/gitweb.perl +6 -0

+# loadavg throttle
+sub get_loadavg() {
+    my $load;
+    my @loads;
+
+    open($load, '<', '/proc/loadavg') or return 0;

Why not use one of existing CPAN modules: Sys::Info::Device::CPU, BSD::getloadavg, Sys::CpuLoad?

Here's the fundamental problem:

Sys:Info:Device:CPU
Windows:
Using this method under Windows is not recommended since, the WMI interface will possibly take at least 2 seconds to complete the request.

BSD::getloadavg
While this more or less supports anything with a libc getloadavg (and thus might be the best one I've seen, I'll admit I didn't notice this one when I looked years ago) getting it to work on windows looks, exciting.

Sys::CpuLoad:
http://cpansearch.perl.org/src/CLINTDW/Sys-CpuLoad-0.03/README
Specifically:
- Currently FreeBSD and OpenBSD are supported.
- Wanted: HPUX 11.11 ...
- Todo: Win32 support

So this doesn't really buy me anything but, maybe, BSD support.

So at the end of the day, none of those really gets me a "useful" cross platform load checker (though like I said BSD::getloadavg looks to be the best of the ones you mentioned) and more or less Windows is going to lose this as a usable feature no matter what.

I think I'd almost rather set this up so that if it can't get something useful (I.E. /proc/loadavg is missing) it just skips past it as if the load was 0.

I might try out the BSD::getloadavg but I want to take a look and see if that's easily installed or not, if it's not it might be difficult to justify that as a dependency.

Quoted 13 lines

gitweb/gitweb.perl if (-e $GITWEB_CONFIG) { · line 575

Quoted 6 lines

gitweb/gitweb.perl +6 -0

+if (get_loadavg()> $maxload) {
+    print "Content-Type: text/plain\n";
+    print "Status: 503 Excessive load on server\n";
+    print "\n";
+    print "The load average on the server is too high\n";
+    exit 0;

Why not use die_error subroutine? Is it to have generate absolutely minimal load, and that is why you do not use die_error(), or even $cgi->header()?

Wouldn't a better solution be to use here-doc syntax?

Review

Quoted 7 lines

(quoted diff) +7 -0

+    print <<'EOF';
+Content-Type: text/plain; charset=utf-8
+Status: 503 Excessive load on server
+
+The load average on the server is too high
+EOF
+    exit 0;

It was intended to be the most minimal possible, mainly get in, get out.

Also not sure the die_error existed in gitweb when this was originally
written. Probably worth switching to it now since it's there either way, and I don't think using it would add enough overhead to matter.

It was intended to be the most minimal possible, mainly get in, get out. Also not sure the die_error existed in gitweb when this was originally written. Probably worth switching to it now since it's there either way, and I don't think using it would add enough overhead to matter.

Quoted 5 lines · J.H.

It was intended to be the most minimal possible, mainly get in, get out. Also not sure the die_error existed in gitweb when this was originally written. Probably worth switching to it now since it's there either way, and I don't think using it would add enough overhead to matter.

Thanks; all sounded a reasonable response to the review. Are you re-rolling the series anytime soon (I am asking because then I'd rather not to queue this round especially because I didn't see 5/6).

Quoted 11 lines · Junio C Hamano

Quoted 5 lines · J.H.

It was intended to be the most minimal possible, mainly get in, get out. Also not sure the die_error existed in gitweb when this was originally written. Probably worth switching to it now since it's there either way, and I don't think using it would add enough overhead to matter.

Thanks; all sounded a reasonable response to the review. Are you re-rolling the series anytime soon (I am asking because then I'd rather not to queue this round especially because I didn't see 5/6).

I'll probably have some changes up and about tomorrow, and it's a little troubling that 5/6 didn't come through for you

6 at least made it to marc.info: http://marc.info/?l=git&m=126048884825985&w=2

and 5 seems to have been eaten by a grue somewhere. It was a *big* patch mainly because all the caching flips over in a single go. If you want I can privately bounce 5 & 6 to you so you have a complete tree right now?

Quoted 3 lines

Thanks; all sounded a reasonable response to the review. Are you re-rolling the series anytime soon (I am asking because then I'd rather not to queue this round especially because I didn't see 5/6).

I'll probably have some changes up and about tomorrow, and it's a little troubling that 5/6 didn't come through for you

6 at least made it to marc.info:
http://marc.info/?l=git&m=126048884825985&w=2

and 5 seems to have been eaten by a grue somewhere. It was a *big* patch mainly because all the caching flips over in a single go. If you want I can privately bounce 5 & 6 to you so you have a complete tree right now?

gitweb/gitweb.perl if (-e $GITWEB_CONFIG) { · line 564

Quoted 6 lines

gitweb/gitweb.perl +6 -0

+# loadavg throttle
+sub get_loadavg() {
+    my $load;
+    my @loads;
+
+    open($load, '<', '/proc/loadavg') or return 0;

Why not use one of existing CPAN modules: Sys::Info::Device::CPU, BSD::getloadavg, Sys::CpuLoad?

John 'Warthog9' Hawley (6):
 GITWEB - Load Checking
 GITWEB - Missmatching git w/ gitweb
 GITWEB - Add git:// link to summary pages
 GITWEB - Makefile changes
 GITWEB - File based caching layer
 GITWEB - Separate defaults from main file

Evening everyone,

This is the latest incarnation of gitweb w/ caching. This is finally at the point where it should probably start either being considered for inclusion or mainline, or I need to accept that this will never get in and more perminantely fork (as is the case with Fedora where this is going in as gitweb-caching as a parrallel rpm package).

That said this brings the base up to mainline (again), it updates a number of elements in the caching engine, and this is a much cleaner break-out of the tree vs. what I am currently developing against.

New things known to work:
- Better breakout
- You can actually disable the cache now

- John 'Warthog9' Hawley

John 'Warthog9' Hawley (6):
GITWEB - Load Checking
GITWEB - Missmatching git w/ gitweb
GITWEB - Add git:// link to summary pages
GITWEB - Makefile changes
GITWEB - File based caching layer

GITWEB - Separate defaults from main file

Quoted 9 lines · Jakub Narebski

Quoted 6 lines · "John 'Warthog9' Hawley" <[email protected]> writes:

John 'Warthog9' Hawley (6):
GITWEB - Load Checking
GITWEB - Missmatching git w/ gitweb
GITWEB - Add git:// link to summary pages
GITWEB - Makefile changes
GITWEB - File based caching layer

Quoted 5 lines

This patch didn't made it to git mailing list. I suspect that you ran afoul vger anti-SPAM filter.

Does this "File based caching layer" have anything common with GSoC 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?

Yeah, it does seem that way (like I said eaten by a grue), it *currently* has nothing to do with Lea's GSoC code but it is still my intention, long term, to integrate the two.

The patch, in all it's glory can be viewed at:
http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db

It is anything but a small patch to gitweb, the patch is 117K and comprises 3539 lines (including git header commit information). There's not any real good way to break it up as it's a bit of an all or nothing patch.

Quoted 16 lines · On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:

Quoted 8 lines · Jakub Narebski

Quoted 6 lines · "John 'Warthog9' Hawley" <[email protected]> writes:

John 'Warthog9' Hawley (6):
GITWEB - Load Checking
GITWEB - Missmatching git w/ gitweb
GITWEB - Add git:// link to summary pages
GITWEB - Makefile changes
GITWEB - File based caching layer

Quoted 5 lines

This patch didn't made it to git mailing list. I suspect that you ran afoul vger anti-SPAM filter.

Does this "File based caching layer" have anything common with GSoC 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?

Yeah, it does seem that way (like I said eaten by a grue), it *currently* has nothing to do with Lea's GSoC code but it is still my intention, long term, to integrate the two.

Quoted 3 lines

The patch, in all it's glory can be viewed at: http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db

It is anything but a small patch to gitweb, the patch is 117K and comprises 3539 lines (including git header commit information). There's not any real good way to break it up as it's a bit of an all or nothing patch.

First, why do you reinvent the wheel instead of using one of existing caching interfaces like CHI or Cache::Cache (perhaps creating a custom backend or middle layer which incorporates required features, like being load-aware)?

This way changing from file-based cache to e.g. mmap based
one or to memcached would be very simple.

And you would avoid pitfals
in doing your own cache management. perl-Cache-Cache should be available package in extras repositories.

If module is no available this would simply mean no caching, like in many (or not so many) other cases with optional features in gitweb.

Second, if you can't use CGI::Cache directly, you can always steal the idea from it, then the change to gitweb itself would be minimal:

"Internally, the CGI::Cache module ties the output file descriptor (usually STDOUT) to an internal variable to which all output is saved."

P.S. I'll postpone critique of the patch itself for now. The above issues are much more important.

Quoted 10 lines · Jakub Narebski

Quoted 9 lines · On Fri, 11 Dec 2009, J.H. (John 'Warthog9' Hawley) wrote:

Quoted 6 lines · Jakub Narebski

Quoted 3 lines · "John 'Warthog9' Hawley" <[email protected]> writes:

GITWEB - File based caching layer

This patch didn't made it to git mailing list. I suspect that you ran afoul vger anti-SPAM filter.

Yeah, it does seem that way (like I said eaten by a grue),

Quoted 5 lines

Quoted 5 lines

Quoted 2 lines

Does this "File based caching layer" have anything common with GSoC 2008 project, available at git://repo.or.cz/git/gitweb-caching.git ?

It *currently* has nothing to do with Lea's GSoC code but it is still my intention, in long term, to integrate the two.

Quoted 8 lines

Quoted 7 lines

The patch, in all it's glory can be viewed at:
http://git.kernel.org/?p=git/warthog9/gitweb.git;a=commitdiff;h=42641b1e3bfae14d5cc2e0150355e89cb87951db

It is anything but a small patch to gitweb, the patch is 117K and comprises 3539 lines (including git header commit information). There's not any real good way to break it up as it's a bit of an all or nothing patch.

Quoted 4 lines

First, why do you reinvent the wheel instead of using one of existing caching interfaces like CHI or Cache::Cache (perhaps creating a custom backend or middle layer which incorporates required features, like being load-aware)?

Well for starters this isn't exactly a reinvention of the wheel, and this isn't something "new" per-se. This code has been actively running on git.kernel.org for something like 3 - 4 years so there's something to be said for the devil we know and understand.

As well using the other
caching strategies involves adding dramatically more complex interactions with caching layer.

The caching layer is actually quite
specific to how git + gitweb works and solves more than just "caching" on the surface.

Specifically it solves the stampeding herd problem
which would have to be solved either way even if I didn't implement my own caching, and since I had to do that caching was barely a step beyond that to implement.

Quoted 2 lines

This way changing from file-based cache to e.g. mmap based
one or to memcached would be very simple.

True but these are *VERY* different caching strategies than the one I've got here, yes it's using files as a backend but it's doing so with specific goals in mind. As I've said I plan to integrate Lea's memcached based caching into this in the future and that has different advantages and disadvantages.

At the end of the day the "normal" caching engines aren't as efficient as mine and there is the case the very high performance sites are going to have to investigate a number of different solutions to see what works best for them. Mine is also *dramatically* simpler to setup as well, turn it on, point it at a directory and you're done.

Quoted 3 lines

And you would avoid pitfals
in doing your own cache management. perl-Cache-Cache should be available package in extras repositories.

There's pitfalls if I do it myself, or I use one of the other "common" perl modules. I did it this way years ago, I've maintained it and it works pretty well. I won't admit that it's the smartest caching engine on the planet, far from it, but it has evolved specifically for gitweb and that itself saves me a lot of pitfalls from cache engine + gitweb integration.

Quoted 2 lines

If module is no available this would simply mean no caching, like in many (or not so many) other cases with optional features in gitweb.

Yes, but as can be seen from how you enable various other caching engines the setup of those is non-trivial, this is and either way caching *HAS* to be explicitly turned on by the admin/user since they are going to have to do *some* configuration, or at least be aware that their webapp is going to chew up some sort of resource.

Quoted 5 lines

Second, if you can't use CGI::Cache directly, you can always steal the idea from it, then the change to gitweb itself would be minimal:

"Internally, the CGI::Cache module ties the output file descriptor (usually STDOUT) to an internal variable to which all output is saved."

I thought about that 3 years ago, and decided it wasn't a good option for gitweb. Why? There's too many assumptions throughout the code that when you do a print it will go immediately out. Things like error messages and such. Breaking out the prints into prints (which will do what is expected) and passing around the output in the $output variables makes it a lot simpler easier to differentiate about how / what your looking at and a *LOT* easier to debug.

Quoted 2 lines

P.S. I'll postpone critique of the patch itself for now. The above issues are much more important.

That's fine. The issues your raising aren't new though, and stem back to before I created gitweb-caching, got rehashed with Lea's patches and not surprisingly are back on the table now. Like I said above, there is no one caching strategy that's perfect in all cases here and that's again why I eventually plan to merge Lea's changes (which uses memcached) in as well, I'm just trying to get code that I'm getting considerable demand for, that's proven, upstream.