git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Jakub Narebski <jnareb@gmail.com>
Date
Mar 27, 2012, 22:24 UTC
Message-ID
<201203280024.20805.jnareb@gmail.com>
In-Reply-To
<20120326191242.GA9041@odin.tremily.us>
On Mon, 26 Mar 2012, W. Trevor King wrote:
Show 5 quoted lines
> 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.

Nice description, and I think quite good name for a subroutine. Well, good enough; any more we would drown in bikeshed-ding.

> 
> 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.
Good.
Show 25 quoted lines
>
> Signed-off-by: W Trevor King <wking@drexel.edu>
> ---
> Patch v4 1/3 is unchanged.  Should I mail it back in with a
> [PATCH v5 1/3] tag?
> 
> Changes since v4:
> * die_if_unmodified() -> exit_if_unmodified_since()
> * Added missing `&&` to tests for feed-last-modified (patch 2/3) and
>   snapshot-last-modified (patch 3/3).
> 
>  gitweb/gitweb.perl                       |   40 +++++++++++++++++------------
>  t/t9501-gitweb-standalone-http-status.sh |   27 +++++++++++++++++++-
>  2 files changed, 49 insertions(+), 18 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 041da17..229f3da 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) = @_;
We could have made \%latest_date an optional parameter...
  +	my ($latest_epoch, $latest_date) = @_;
Show 12 quoted lines
> +	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);
...and use it if provided
  +			my %latest_date = ref($latest_date) eq 'HASH' 
  +				? %$latest_date : parse_date($latest_epoch);

but something is to be said for simplicity. We need epoch for comparison anyway.

> +			print $cgi->header(
> +				-last_modified => $latest_date{'rfc2822'},
> +				-status => '304 Not Modified');

O.K. we lose some unnecessary headers; never mind that. What is important is here.

Show 59 quoted lines
> +			goto DONE_GITWEB;
> +		}
> +	}
> +}
> +
>  sub git_snapshot {
>  	my $format = $input_params{'snapshot_format'};
>  	if (!@snapshot_fmts) {
> @@ -7820,24 +7842,8 @@ sub git_feed {
>  	if (defined($commitlist[0])) {
>  		%latest_commit = %{$commitlist[0]};
>  		my $latest_epoch = $latest_commit{'committer_epoch'};
> +		exit_if_unmodified_since($latest_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',
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index 31076ed..0e49f29 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
> +'
All right.

What's that date from? Wouldn't it be better to read it from commit object with `git show -s --pretty=%cD HEAD` or postprocessed from '%ct' timestamp?

Show 8 quoted lines
> +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" &&
> +	gitweb_run "p=.git;a=atom;h=master" &&
> +	unset HTTP_IF_MODIFIED_SINCE &&
> +	grep "Status: 200 OK" gitweb.output
> +'
I think it *might* be better solution to use test_when_finished:
  +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
  +'
I don't think we need sane_unset here.
Show 8 quoted lines
> +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" &&
> +	gitweb_run "p=.git;a=atom;h=master" &&
> +	unset HTTP_IF_MODIFIED_SINCE &&
> +	grep "Status: 304 Not Modified" gitweb.output
> +'
Same here.
Show 7 quoted lines
> +test_debug 'cat gitweb.headers'
>  
>  # ----------------------------------------------------------------------
>  # load checking
> -- 
> 1.7.3.4
> 
-- 
Jakub Narebski
Poland
Previous: W. Trevor KingNext: W. Trevor King
Message 26 of 43 in “What's cooking in git.git (Mar 2012, #07; Mon, 19)”
  1. Junio C HamanoMar 20, 2012
  2. W. Trevor KingMar 20, 2012
  3. Junio C HamanoMar 20, 2012
  4. Incremental updates to What's cookingJunio C Hamano, Mar 20, 2012
  5. Isolate If-Modified-Since handling in gitwebW. Trevor King, Mar 21, 2012
  6. Jakub NarebskiMar 21, 2012
  7. W. Trevor KingMar 21, 2012
  8. Jakub NarebskiMar 21, 2012
  9. W. Trevor KingMar 21, 2012
  10. Junio C HamanoMar 21, 2012
  11. W. Trevor KingMar 21, 2012
  12. Jakub NarebskiMar 21, 2012
  13. Junio C HamanoMar 21, 2012
  14. W. Trevor KingMar 21, 2012
  15. Jakub NarebskiMar 22, 2012
  16. Junio C HamanoMar 21, 2012
  17. Jakub NarebskiMar 22, 2012
  18. Junio C HamanoMar 22, 2012
  19. W. Trevor KingMar 26, 2012
  20. 1/3 gitweb: add `status` headers to git_feed() responses.W. Trevor King, Mar 26, 2012
  21. 2/3 gitweb: refactor If-Modified-Since handlingW. Trevor King, Mar 26, 2012
  22. Junio C HamanoMar 26, 2012
  23. W. Trevor KingMar 26, 2012
  24. Junio C HamanoMar 26, 2012
  25. W. Trevor KingMar 26, 2012
  26. Jakub NarebskiMar 27, 2012
  27. W. Trevor KingMar 28, 2012
  28. Jakub NarebskiMar 28, 2012
  29. 0/3 gitweb: refactor If-Modified-Since handlingwking@tremily.us, Mar 28, 2012
  30. W. Trevor KingMar 28, 2012
  31. 1/3 gitweb: add `status` headers to git_feed() responses.wking@tremily.us, Mar 28, 2012
  32. 2/3 gitweb: refactor If-Modified-Since handlingwking@tremily.us, Mar 28, 2012
  33. 3/3 gitweb: add If-Modified-Since handling to git_snapshot().wking@tremily.us, Mar 28, 2012
  34. Jakub NarebskiMar 28, 2012
  35. W. Trevor KingMar 26, 2012
  36. Jakub NarebskiMar 27, 2012
  37. W. Trevor KingMar 28, 2012
  38. 3/3 gitweb: add If-Modified-Since handling to git_snapshot().W. Trevor King, Mar 26, 2012
  39. Jakub NarebskiMar 27, 2012
  40. W. Trevor KingMar 27, 2012
  41. Jakub NarebskiMar 27, 2012
  42. W. Trevor KingMar 27, 2012
  43. Junio C HamanoMar 27, 2012

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.