[PATCH v7 0/3] gitweb: refactor If-Modified-Since handling

[PATCH v7 0/3] gitweb: refactor If-Modified-Since handling

[W. Trevor King]

Changed since v6:
* Don't set a Last-Modified header when it has no value.


p.s. the `From` issues with v6 were due to a missing

  FromLineOverride=YES

in my sSMTP config ;).


W. Trevor King (3):
  gitweb: add `status` headers to git_feed() responses.
  gitweb: refactor If-Modified-Since handling
  gitweb: add If-Modified-Since handling to git_snapshot().

 gitweb/gitweb.perl                       |   65 ++++++++++++++++++------------
 t/t9501-gitweb-standalone-http-status.sh |   60 +++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 27 deletions(-)

-- 
1.7.3.4

[PATCH v7 1/3] gitweb: add `status` headers to git_feed() responses.

[W. Trevor King]

The git_feed() method was not setting a `Status` header unless it was
responding to an If-Modified-Since request with `304 Not Modified`.
Now, when it is serving successful responses, it sets status to `200
OK`.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..041da17 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7841,11 +7841,13 @@ sub git_feed {
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'});
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
 	} else {
 		print $cgi->header(
 			-type => $content_type,
-			-charset => 'utf-8');
+			-charset => 'utf-8',
+			-status => '200 OK');
 	}
 
 	# Optimization: skip generating the body if client asks only
-- 
1.7.3.4

[PATCH v7 2/3] gitweb: refactor If-Modified-Since handling

[W. Trevor King]

The current gitweb only generates Last-Modified and handles
If-Modified-Since headers for the git_feed action.  This patch breaks
the Last-Modified and If-Modified-Since handling code out from
git_feed into a new function exit_if_unmodified_since.  This makes the
code easy to reuse for other actions.

Only gitweb actions which can easily calculate a modification time
should use exit_if_unmodified_since, as the goal is to balance local
processing time vs. upload bandwidth.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl                       |   57 +++++++++++++++--------------
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++-
 2 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 041da17..6d3f9c0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub exit_if_unmodified_since {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {
@@ -7820,35 +7842,14 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'},
-			-status => '200 OK');
-	} else {
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-status => '200 OK');
+		exit_if_unmodified_since($latest_epoch);
+		%latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
 	}
+	print $cgi->header(
+		-type => $content_type,
+		-charset => 'utf-8',
+		%latest_date ? (-last_modified => $latest_date{'rfc2822'}) : (),
+		-status => '200 OK');
 
 	# Optimization: skip generating the body if client asks only
 	# for Last-Modified date.
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 31076ed..afa6bd4 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'
 test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
 	echo object > tag-object &&
 	git add tag-object &&
-	git commit -m "Object to be tagged" &&
+	test_tick && git commit -m "Object to be tagged" &&
 	git tag tagged-object `git hash-object tag-object` &&
 	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
 	grep "400 - Object is not a tree-ish" gitweb.output
@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '
 '
 test_debug 'cat gitweb.output'
 
+# ----------------------------------------------------------------------
+# modification times (Last-Modified and If-Modified-Since)
+
+test_expect_success 'modification: feed last-modified' '
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking
-- 
1.7.3.4

[PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[W. Trevor King]

Because snapshots can be large, you can save some bandwidth by
supporting caching via If-Modified-Since.  This patch adds support for
the i-m-s request to git_snapshot() if the request is a commit.
Requests for snapshots of trees, which lack well defined timestamps,
are still handled as they were before.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl                       |   10 +++++++++
 t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6d3f9c0..b649f9e 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7051,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash);
+	exit_if_unmodified_since($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",
@@ -7060,9 +7064,15 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %latest_date;
+	if (%co) {
+		%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+	}
+
 	print $cgi->header(
 		-type => $known_snapshot_formats{$format}{'type'},
 		-content_disposition => 'inline; filename="' . $filename . '"',
+		-last_modified => (%co ? $latest_date{'rfc2822'} : ()),
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index afa6bd4..1487820 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'modification: snapshot last-modified' '
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 
-- 
1.7.3.4

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski]

W. Trevor King wrote:

> @@ -7060,9 +7064,15 @@ sub git_snapshot {
>  	}
>  
>  	$filename =~ s/(["\\])/\\$1/g;
> +	my %latest_date;
> +	if (%co) {
> +		%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
> +	}
> +
>  	print $cgi->header(
>  		-type => $known_snapshot_formats{$format}{'type'},
>  		-content_disposition => 'inline; filename="' . $filename . '"',
> +		-last_modified => (%co ? $latest_date{'rfc2822'} : ()),
>  		-status => '200 OK');

I'm sorry to be bearer of bad news, but this is still incorrect.
It should be:

   	print $cgi->header(
   		-type => $known_snapshot_formats{$format}{'type'},
   		-content_disposition => 'inline; filename="' . $filename . '"',
  +		%co ? (-last_modified => $latest_date{'rfc2822'}) : (),
   		-status => '200 OK');

(The "fat comma" => operator has relatively low priority, lower than
ternary conditional operator ?:)
  
> +test_expect_success 'modification: tree snapshot' '
> +	ID=`git rev-parse --verify HEAD^{tree}` &&
> +	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
> +	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
> +	grep "Status: 200 OK" gitweb.output &&
> +	! grep "Last-Modified" gitweb.output
> +'
> +test_debug 'cat gitweb.headers'

And it was not caught by test because CGI.pm can output the last modified
header as "Last-modified" (RFC 2616, sec 4.2 states "Field names are
case-insensitive"), so the last check should be

  +	! grep -i "Last-Modified" gitweb.output

Hmmm... why we use gitweb.output and not gitweb.headers?  Is it consistency
with earlier tests?

-- 
Jakub Narebski
Poland

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1552 bytes --]

On Wed, Mar 28, 2012 at 07:11:31PM +0100, Jakub Narebski wrote:
> W. Trevor King wrote:
> >  	print $cgi->header(
> >  		-type => $known_snapshot_formats{$format}{'type'},
> >  		-content_disposition => 'inline; filename="' . $filename . '"',
> > +		-last_modified => (%co ? $latest_date{'rfc2822'} : ()),
> >  		-status => '200 OK');
> 
> I'm sorry to be bearer of bad news, but this is still incorrect.
> It should be:
> 
>    	print $cgi->header(
>    		-type => $known_snapshot_formats{$format}{'type'},
>    		-content_disposition => 'inline; filename="' . $filename . '"',
>   +		%co ? (-last_modified => $latest_date{'rfc2822'}) : (),
>    		-status => '200 OK');

Grr.  Thanks.  I'm getting lots of rebase practice on this patch set,
but I'm still missing things…

> And it was not caught by test because CGI.pm can output the last modified
> header as "Last-modified" (RFC 2616, sec 4.2 states "Field names are
> case-insensitive"), so the last check should be
> 
>   +	! grep -i "Last-Modified" gitweb.output
> 
> Hmmm... why we use gitweb.output and not gitweb.headers?  Is it consistency
> with earlier tests?

Yes, but I can switch to `gitweb.headers` if you'd like.  Should I
adjust all the header tests in t9501 to use `gitweb.headers` and `grep
-i`?  It should probably be a separate patch for the tests that
existed before my i-m-s additions.

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Junio C Hamano]

"W. Trevor King" <wking@drexel.edu> writes:

> Grr.  Thanks.  I'm getting lots of rebase practice on this patch set,
> but I'm still missing things…

How do you "rebase"?

It often is the easiest to check out the tip of the previous iteration,
fix all issues that were brought up in the working tree, eyeball the
output from "git diff HEAD" to make sure you addressed all the comments,o
and then make separate commits, using "add -p" to sift the fix-ups
according to which commit in the previous round they need to update.

And then finally you run "rebase -i" to squash these fix-ups in.

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[W. Trevor King]

[-- Attachment #1: Type: text/plain, Size: 1228 bytes --]

On Wed, Mar 28, 2012 at 12:11:27PM -0700, Junio C Hamano wrote:
> "W. Trevor King" <wking@drexel.edu> writes:
> 
> > Grr.  Thanks.  I'm getting lots of rebase practice on this patch set,
> > but I'm still missing things…
> 
> How do you "rebase"?
> 
> It often is the easiest to check out the tip of the previous iteration,
> fix all issues that were brought up in the working tree, eyeball the
> output from "git diff HEAD" to make sure you addressed all the comments,o
> and then make separate commits, using "add -p" to sift the fix-ups
> according to which commit in the previous round they need to update.
> 
> And then finally you run "rebase -i" to squash these fix-ups in.

Ah, that makes a lot of sense.  I had been running `rebase -i`,
editing the earlier commits, and using `commit -a --amend` to squash
them on.  The problem with that approach is that you need to check the
changes vs the previous release before each amend, while with your
suggestion there's a single diff to look through.

Hopefully patches v8+ will be cleaner ;).

-- 
This email may be signed or encrypted with GnuPG (http://www.gnupg.org).
For more information, see http://en.wikipedia.org/wiki/Pretty_Good_Privacy

[-- Attachment #2: OpenPGP digital signature --]
[-- Type: application/pgp-signature, Size: 490 bytes --]

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski]

On Wed, 28 Mar 2012, W. Trevor King wrote:
> On Wed, Mar 28, 2012 at 12:11:27PM -0700, Junio C Hamano wrote:
> > "W. Trevor King" <wking@drexel.edu> writes:
> > 
> > > Grr.  Thanks.  I'm getting lots of rebase practice on this patch set,
> > > but I'm still missing things…
> > 
> > How do you "rebase"?
> > 
> > It often is the easiest to check out the tip of the previous iteration,
> > fix all issues that were brought up in the working tree, eyeball the
> > output from "git diff HEAD" to make sure you addressed all the comments,o
> > and then make separate commits, using "add -p" to sift the fix-ups
> > according to which commit in the previous round they need to update.
> > 
> > And then finally you run "rebase -i" to squash these fix-ups in.
> 
> Ah, that makes a lot of sense.  I had been running `rebase -i`,
> editing the earlier commits, and using `commit -a --amend` to squash
> them on.  The problem with that approach is that you need to check the
> changes vs the previous release before each amend, while with your
> suggestion there's a single diff to look through.

BTW. I personally use StGit (a patch management interface on top of git)
instead of interactive rebase.  Just in case, and to be able to write
differences to previous version, I use git-format-patch to generate
patches to a subdirectory, e.g. mdir.gitweb.v7/, and compare with previous
version.

-- 
Jakub Narebski
Poland

Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski]

On Wed, 28 Mar 2012, W. Trevor King wrote:
> On Wed, Mar 28, 2012 at 07:11:31PM +0100, Jakub Narebski wrote:

[...]
> > And it was not caught by test because CGI.pm can output the last modified
> > header as "Last-modified" (RFC 2616, sec 4.2 states "Field names are
> > case-insensitive"), so the last check should be
> > 
> >   +	! grep -i "Last-Modified" gitweb.output
> > 
> > Hmmm... why we use gitweb.output and not gitweb.headers?  Is it consistency
> > with earlier tests?
> 
> Yes, but I can switch to `gitweb.headers` if you'd like.  Should I
> adjust all the header tests in t9501 to use `gitweb.headers` and `grep
> -i`?  It should probably be a separate patch for the tests that
> existed before my i-m-s additions.

Eh, don't worry about this.  First, I think we can assume that HTTP
headers from CGI.pm will all start with capital letter.

Second, for positive match being overly strict is safe - if assumption
doesn't hold we would get false failure.  The problem is for negative
match - being overly strict means that we won't catch the breakage.

I think that the gitweb.output vs gitweb.headers (and gitweb.body) is
because those tests predate gitweb_run producing gitweb.headers file.
Be consistent if you want, or use new feature in new test; you don't
need to modernize t9501.

-- 
Jakub Narebski
Poland

[PATCH v8 0/3] gitweb: refactor If-Modified-Since handling

[W. Trevor King]

Changes since v7:
* Fix git_snapshot `-last_modified =>` switching.
* Case insensitive Last-Modified match for tree snapshot test.
* gitweb.output -> gitweb.headers in the new tests.


W. Trevor King (3):
  gitweb: add `status` headers to git_feed() responses.
  gitweb: refactor If-Modified-Since handling
  gitweb: add If-Modified-Since handling to git_snapshot().

 gitweb/gitweb.perl                       |   65 ++++++++++++++++++------------
 t/t9501-gitweb-standalone-http-status.sh |   60 +++++++++++++++++++++++++++-
 2 files changed, 98 insertions(+), 27 deletions(-)

-- 
1.7.3.4

[PATCH v8 1/3] gitweb: add `status` headers to git_feed() responses.

[W. Trevor King]

The git_feed() method was not setting a `Status` header unless it was
responding to an If-Modified-Since request with `304 Not Modified`.
Now, when it is serving successful responses, it sets status to `200
OK`.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl |    6 ++++--
 1 files changed, 4 insertions(+), 2 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..041da17 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7841,11 +7841,13 @@ sub git_feed {
 		print $cgi->header(
 			-type => $content_type,
 			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'});
+			-last_modified => $latest_date{'rfc2822'},
+			-status => '200 OK');
 	} else {
 		print $cgi->header(
 			-type => $content_type,
-			-charset => 'utf-8');
+			-charset => 'utf-8',
+			-status => '200 OK');
 	}
 
 	# Optimization: skip generating the body if client asks only
-- 
1.7.3.4

Re: [PATCH v8 1/3] gitweb: add `status` headers to git_feed() responses.

[Jakub Narebski]

On Thu, 29 Mar 2012, W. Trevor King wrote:

> The git_feed() method was not setting a `Status` header unless it was
> responding to an If-Modified-Since request with `304 Not Modified`.
> Now, when it is serving successful responses, it sets status to
> `200 OK`.

Nice.  This change is IMHO worth applying even without the rest of series.
So

  Acked-by: Jakub Narebski <jnareb@gmail.com>

You _might_ also add that this change would allow robust testing of
If-Modified-Since request handling in gitweb, but it is not really
necessary.

> 
> Signed-off-by: W Trevor King <wking@drexel.edu>
> ---
>  gitweb/gitweb.perl |    6 ++++--
>  1 files changed, 4 insertions(+), 2 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..041da17 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -7841,11 +7841,13 @@ sub git_feed {
>  		print $cgi->header(
>  			-type => $content_type,
>  			-charset => 'utf-8',
> -			-last_modified => $latest_date{'rfc2822'});
> +			-last_modified => $latest_date{'rfc2822'},
> +			-status => '200 OK');
>  	} else {
>  		print $cgi->header(
>  			-type => $content_type,
> -			-charset => 'utf-8');
> +			-charset => 'utf-8',
> +			-status => '200 OK');
>  	}
>  
>  	# Optimization: skip generating the body if client asks only
> -- 
> 1.7.3.4

-- 
Jakub Narebski
Poland

[PATCH v8 2/3] gitweb: refactor If-Modified-Since handling

[W. Trevor King]

The current gitweb only generates Last-Modified and handles
If-Modified-Since headers for the git_feed action.  This patch breaks
the Last-Modified and If-Modified-Since handling code out from
git_feed into a new function exit_if_unmodified_since.  This makes the
code easy to reuse for other actions.

Only gitweb actions which can easily calculate a modification time
should use exit_if_unmodified_since, as the goal is to balance local
processing time vs. upload bandwidth.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl                       |   57 +++++++++++++++--------------
 t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++-
 2 files changed, 55 insertions(+), 29 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 041da17..6d3f9c0 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub exit_if_unmodified_since {
+	my ($latest_epoch) = @_;
+	our $cgi;
+
+	my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
+	if (defined $if_modified) {
+		my $since;
+		if (eval { require HTTP::Date; 1; }) {
+			$since = HTTP::Date::str2time($if_modified);
+		} elsif (eval { require Time::ParseDate; 1; }) {
+			$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
+		}
+		if (defined $since && $latest_epoch <= $since) {
+			my %latest_date = parse_date($latest_epoch);
+			print $cgi->header(
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			goto DONE_GITWEB;
+		}
+	}
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {
@@ -7820,35 +7842,14 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
-		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
-		my $if_modified = $cgi->http('IF_MODIFIED_SINCE');
-		if (defined $if_modified) {
-			my $since;
-			if (eval { require HTTP::Date; 1; }) {
-				$since = HTTP::Date::str2time($if_modified);
-			} elsif (eval { require Time::ParseDate; 1; }) {
-				$since = Time::ParseDate::parsedate($if_modified, GMT => 1);
-			}
-			if (defined $since && $latest_epoch <= $since) {
-				print $cgi->header(
-					-type => $content_type,
-					-charset => 'utf-8',
-					-last_modified => $latest_date{'rfc2822'},
-					-status => '304 Not Modified');
-				return;
-			}
-		}
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-last_modified => $latest_date{'rfc2822'},
-			-status => '200 OK');
-	} else {
-		print $cgi->header(
-			-type => $content_type,
-			-charset => 'utf-8',
-			-status => '200 OK');
+		exit_if_unmodified_since($latest_epoch);
+		%latest_date = parse_date($latest_epoch, $latest_commit{'comitter_tz'});
 	}
+	print $cgi->header(
+		-type => $content_type,
+		-charset => 'utf-8',
+		%latest_date ? (-last_modified => $latest_date{'rfc2822'}) : (),
+		-status => '200 OK');
 
 	# Optimization: skip generating the body if client asks only
 	# for Last-Modified date.
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 31076ed..3580103 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -92,7 +92,7 @@ test_debug 'cat gitweb.output'
 test_expect_success 'snapshots: bad tree-ish id (tagged object)' '
 	echo object > tag-object &&
 	git add tag-object &&
-	git commit -m "Object to be tagged" &&
+	test_tick && git commit -m "Object to be tagged" &&
 	git tag tagged-object `git hash-object tag-object` &&
 	gitweb_run "p=.git;a=snapshot;h=tagged-object;sf=tgz" &&
 	grep "400 - Object is not a tree-ish" gitweb.output
@@ -112,6 +112,31 @@ test_expect_success 'snapshots: bad object id' '
 '
 test_debug 'cat gitweb.output'
 
+# ----------------------------------------------------------------------
+# modification times (Last-Modified and If-Modified-Since)
+
+test_expect_success 'modification: feed last-modified' '
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.headers &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 200 OK" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: feed if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=atom;h=master" &&
+	grep "Status: 304 Not Modified" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking
-- 
1.7.3.4

Re: [PATCH v8 2/3] gitweb: refactor If-Modified-Since handling

[Jakub Narebski]

W. Trevor King wrote:

> The current gitweb only generates Last-Modified and handles
> If-Modified-Since headers for the git_feed action.  This patch breaks
> the Last-Modified and If-Modified-Since handling code out from
> git_feed into a new function exit_if_unmodified_since.  This makes the
> code easy to reuse for other actions.
> 
> Only gitweb actions which can easily calculate a modification time
> should use exit_if_unmodified_since, as the goal is to balance local
> processing time vs. upload bandwidth.
> 
> Signed-off-by: W Trevor King <wking@drexel.edu>

Nice and clear.

  Acked-by: Jakub Narebski <jnareb@gmail.com>

Perhaps the second paragraph of the commit message would read better if
it started with "Note that only ..." or something like that, but this
is not that important, and not worth another round.

-- 
Jakub Narebski
Poland

[PATCH v8 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[W. Trevor King]

Because snapshots can be large, you can save some bandwidth by
supporting caching via If-Modified-Since.  This patch adds support for
the i-m-s request to git_snapshot() if the request is a commit.
Requests for snapshots of trees, which lack well defined timestamps,
are still handled as they were before.

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 gitweb/gitweb.perl                       |   10 +++++++++
 t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
 2 files changed, 43 insertions(+), 0 deletions(-)

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 6d3f9c0..ede804a 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7051,6 +7051,10 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash);
+	exit_if_unmodified_since($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",
@@ -7060,9 +7064,15 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %latest_date;
+	if (%co) {
+		%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
+	}
+
 	print $cgi->header(
 		-type => $known_snapshot_formats{$format}{'type'},
 		-content_disposition => 'inline; filename="' . $filename . '"',
+		%co ? (-last_modified => $latest_date{'rfc2822'}) : (),
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 3580103..fa2f65f 100755
--- a/t/t9501-gitweb-standalone-http-status.sh
+++ b/t/t9501-gitweb-standalone-http-status.sh
@@ -138,6 +138,39 @@ test_expect_success 'modification: feed if-modified-since (unmodified)' '
 '
 test_debug 'cat gitweb.headers'
 
+test_expect_success 'modification: snapshot last-modified' '
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.headers &&
+	grep "Last-modified: Thu, 7 Apr 2005 22:14:13 +0000" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (modified)' '
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: snapshot if-modified-since (unmodified)' '
+	export HTTP_IF_MODIFIED_SINCE="Thu, 7 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	grep "Status: 304 Not Modified" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
+	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
+	grep "Status: 200 OK" gitweb.headers &&
+	! grep -i "last-modified" gitweb.headers
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 
-- 
1.7.3.4

Re: [PATCH v8 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski]

On Thu, 29 Mar 2012, W. Trevor King wrote:

> Because snapshots can be large, you can save some bandwidth by
> supporting caching via If-Modified-Since.  This patch adds support for
> the i-m-s request to git_snapshot() if the request is a commit.
>
Perhaps it is worth clarifying that "caching" here means external HTTP
caching, either by web browser, or by web accelerator / reverse proxy.
Supporting Last-Modified and If-Modified-Since helps that[1][2].

[1]: http://www.mnot.net/cache_docs/#VALIDATE
[2]: http://www.mnot.net/cache_docs/#SCRIPT

> Requests for snapshots of trees, which lack well defined timestamps,
> are still handled as they were before.

"still handled as they were before" means "do not support l-m / i-m-s",
isn't it?  Wouldn't it be better to write it explicitely?
> 
> Signed-off-by: W Trevor King <wking@drexel.edu>

Anyway, I like those changes:

  Acked-by: Jakub Narebski <jnareb@gmail.com>

[...]
> +test_expect_success 'modification: tree snapshot' '
> +	ID=`git rev-parse --verify HEAD^{tree}` &&
> +	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
> +	test_when_finished "unset HTTP_IF_MODIFIED_SINCE" &&
> +	gitweb_run "p=.git;a=snapshot;h=$ID;sf=tgz" &&
> +	grep "Status: 200 OK" gitweb.headers &&
> +	! grep -i "last-modified" gitweb.headers

If we ignore case, we can write

  +	! grep -i "Last-Modified:" gitweb.headers

which is IMVVVHO slightly more readable.

Not that it matters much.  Just nitpicking.

> +'
> +test_debug 'cat gitweb.headers'

-- 
Jakub Narebski
Poland

Re: [PATCH v8 0/3] gitweb: refactor If-Modified-Since handling

[Junio C Hamano]

Thanks, both.  Looking good.  Will queue.