threads / patch / 29955

patchPull gitweb If-Modified-Since handling out into its own function and use for snapshots.

Subject: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

## tl;dr

3 messages between Mar 15, 2012 and Mar 20, 2012. Diffs are folded; open one to read it.

replies: 2people: 2as markdown or json

W. Trevor King· Mar 15, 2012, 07:54 UTC · lore

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 modified_since. This makes the code easy to reuse for other actions where it is appropriate, and I've added the code to do that in git_snapshot.

Signed-off-by: W Trevor King <wking@drexel.edu>

---
 gitweb/gitweb.perl |   54 ++++++++++++++++++++++++++++++++++++---------------
 1 files changed, 38 insertions(+), 16 deletions(-)

Show changes to gitweb/gitweb.perl +38 −16

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..96a13e7 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7003,6 +7003,31 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub modified_since {
+	my ($content_type, $latest_epoch, $timezone) = @_;
+	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, $timezone);
+			print $cgi->header(
+				-type => $content_type,
+				-charset => 'utf-8',
+				-last_modified => $latest_date{'rfc2822'},
+				-status => '304 Not Modified');
+			return 0;
+		}
+	}
+	return 1;
+}
+
 sub git_snapshot {
 	my $format = $input_params{'snapshot_format'};
 	if (!@snapshot_fmts) {
@@ -7029,6 +7054,14 @@ sub git_snapshot {
 
 	my ($name, $prefix) = snapshot_name($project, $hash);
 	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
+
+	my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
+	if (! modified_since($known_snapshot_formats{$format}{'type'},
+	                     $co{'committer_epoch'},
+	                     $co{'committer_tz'})) {
+		return;
+	}
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",
@@ -7038,9 +7071,11 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %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 => $latest_date{'rfc2822'},
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd
@@ -7821,22 +7856,9 @@ sub git_feed {
 		%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;
-			}
+		if (! modified_since($content_type, $latest_epoch,
+		                     $latest_commit{'comitter_tz'})) {
+			return;
 		}
 		print $cgi->header(
 			-type => $content_type,

-- 
1.7.3.4

Jakub Narebski· Mar 20, 2012, 11:55 UTC · re: W. Trevor King · lore

Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

I'm sorry I am only now answering, but unfortunately this patch has fallen thought crack.

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

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 modified_since.  This makes the code easy
> to reuse for other actions where it is appropriate,

That is a very good idea.

Of course adding support for If-Modified-Since has sense only if we can calculate modification time without doing all the work (and calculating modification time separately doesn;t add to work to be done), or at least we are to send large amount of data so even if we cannot save CPU time and I/O hit we can save network bandwidth.

> and I've added the code to do that in git_snapshot.

Nitpick about grammar: here you change from impersonal to personal form in commit message, unnecessarily I think.

Show 16 quoted lines

> Signed-off-by: W Trevor King <wking@drexel.edu>
> 
> ---
>  gitweb/gitweb.perl |   54 ++++++++++++++++++++++++++++++++++++---------------
>  1 files changed, 38 insertions(+), 16 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index a8b5fad..96a13e7 100755
> --- a/gitweb/gitweb.perl
> +++ b/gitweb/gitweb.perl
> @@ -7003,6 +7003,31 @@ sub snapshot_name {
>  	return wantarray ? ($name, $name) : $name;
>  }
>  
> +sub modified_since {
> +	my ($content_type, $latest_epoch, $timezone) = @_;

Passing $content_type to function named modified_since() looks quite strange to me.

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, $timezone);

Shouldn't we pass \%latest_date (or rather \%modification_date to be more generic)? Ah, I see that parse_date() subroutine does not store original epoch not original timezone.

By the way, for $latest_date{'rfc2822'} we don't need $timezone argument, and I think we should not pass it to generic modified_since() function (or whatever it will be called).

> +			print $cgi->header(
> +				-type => $content_type,
> +				-charset => 'utf-8',

Do we need -charset here? There is no HTTP body, and -charset => 'utf-8' is incorrect for snapshots (with e.g. 'application/x-zip' $content_type)

Show 22 quoted lines

> +				-last_modified => $latest_date{'rfc2822'},
> +				-status => '304 Not Modified');
> +			return 0;
> +		}
> +	}
> +	return 1;
> +}
> +
>  sub git_snapshot {
>  	my $format = $input_params{'snapshot_format'};
>  	if (!@snapshot_fmts) {
> @@ -7029,6 +7054,14 @@ sub git_snapshot {
>  
>  	my ($name, $prefix) = snapshot_name($project, $hash);
>  	my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
> +
> +	my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
> +	if (! modified_since($known_snapshot_formats{$format}{'type'},
> +	                     $co{'committer_epoch'},
> +	                     $co{'committer_tz'})) {
> +		return;
> +	}

For a function named modified_since(), it does too many things.

I think a better idea would be to make this function/subroutine (named e.g. if_modified_since() or something like that) to work more like die_error() -- it should simply end request instead of having caller to use complcated calling convention, and care about eraly termination by itself.

This means turning modified_since() into subroutine (and renaming it), and ending it with

  goto DONE_GITWEB;

or

  goto DONE_REQUEST;

depending on the code base you are applying to.

Show 15 quoted lines

> +
>  	my $cmd = quote_command(
>  		git_cmd(), 'archive',
>  		"--format=$known_snapshot_formats{$format}{'format'}",
> @@ -7038,9 +7071,11 @@ sub git_snapshot {
>  	}
>  
>  	$filename =~ s/(["\\])/\\$1/g;
> +	my %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 => $latest_date{'rfc2822'},
>  		-status => '200 OK');
>

Have you run gitweb tests? This simply has no way of working, as %co variable you use here is not defined anywhere in git_snapshot() subroutine.

What if there is no commit, for example if we are rewuesting snapshot of a tree by its SHA-1? We need to be able to deal with sich situation, if only by not handling Last-Modified / If-Modified-Since.

Show 5 quoted lines

>  	open my $fd, "-|", $cmd
> @@ -7821,22 +7856,9 @@ sub git_feed {
>  		%latest_commit = %{$commitlist[0]};
>  		my $latest_epoch = $latest_commit{'committer_epoch'};
>  		%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});

Do we use %latest_date beside handling If-Modified-Since? Is there a need for separate $latest_epoch and %;atest_date variables?

Show 24 quoted lines

> -		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;
> -			}
> +		if (! modified_since($content_type, $latest_epoch,
> +		                     $latest_commit{'comitter_tz'})) {
> +			return;
>  		}
>  		print $cgi->header(
>  			-type => $content_type,
> -- 
> 1.7.3.4

-- 
Jakub Narebski

W. Trevor King· Mar 20, 2012, 16:40 UTC · re: Jakub Narebski · lore

[PATCH v2] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

Signed-off-by: W Trevor King <wking@drexel.edu>                                 
---
Thanks for the feedback, Jakub.  I believe I've addressed your points
in this revised patch.

Changed since v1:
- Fixed impersonal/personal commit message
- Simplified modified_since() -> die_if_unmodified().  Now it only
  takes a $latest_epoch argument and we set fewer headers.  It also
  jump straight to DONE_GITWEB if the page is unmodified.
- Ran tests (which passed this time).  Somehow I missed t/README
  earlier.

I'd add tests for my patch, but I'd have to have commit timestamps from the test repository, and I don't understand the test framework well enough to know how to do that cleanly.

Things that haven't changed:

Show 8 quoted lines

> > @@ -7038,9 +7071,11 @@ sub git_snapshot {
> >  	}
> >  
> >  	$filename =~ s/(["\\])/\\$1/g;
> > +	my %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});
> ...
> This simply has no way of working, as %co variable you use here is
> not defined anywhere in git_snapshot() subroutine.

I defined it earlier:

Show 8 quoted lines

> > @@ -7029,6 +7054,14 @@ sub git_snapshot {
> >
> >   my ($name, $prefix) = snapshot_name($project, $hash);
> >   my $filename = "$name$known_snapshot_formats{$format}{'suffix'}";
> > +
> > + my %co = parse_commit($hash) or die_error(404, "Unknown commit object");
> > + if (! modified_since($known_snapshot_formats{$format}{'type'},
> > +                      $co{'committer_epoch'},

While modified_since() (now die_if_unmodified()) handles the 304 case, we also want to set Last-Modified for 200s. That's what the latter code is trying to do.

> What if there is no commit, for example if we are rewuesting snapshot
> of a tree by its SHA-1?  We need to be able to deal with sich
> situation, if only by not handling Last-Modified / If-Modified-Since.

The parse_commit() call should handle these cases.

Show changes to gitweb/gitweb.perl +29 −18

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index a8b5fad..b944351 100755
--- a/gitweb/gitweb.perl
+++ b/gitweb/gitweb.perl
@@ -7003,6 +7003,28 @@ sub snapshot_name {
 	return wantarray ? ($name, $name) : $name;
 }
 
+sub die_if_unmodified {
+	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) {
@@ -7029,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) or die_error(404, "Unknown commit object");
+	die_if_unmodified($co{'committer_epoch'});
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",
@@ -7038,9 +7064,11 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
+	my %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 => $latest_date{'rfc2822'},
 		-status => '200 OK');
 
 	open my $fd, "-|", $cmd
@@ -7820,24 +7848,8 @@ sub git_feed {
 	if (defined($commitlist[0])) {
 		%latest_commit = %{$commitlist[0]};
 		my $latest_epoch = $latest_commit{'committer_epoch'};
+		die_if_unmodified($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',
--                                                                              
1.7.3.4