{"thread":{"id":"29955","subject":"[PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","startedAt":"2012-03-15T07:54:07Z","lastAt":"2012-03-20T16:40:56Z","messageCount":3,"participants":["W. Trevor King","Jakub Narebski"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"187024","messageId":"20120315075407.GA18246@odin.tremily.us","threadId":"29955","inReplyTo":null,"subject":"[PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-15T07:54:07Z","receivedAt":"2012-03-15T07:54:07Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles\nIf-Modified-Since headers for the git_feed action.  This patch breaks\nthe Last-Modified and If-Modified-Since handling code out from\ngit_feed into a new function modified_since.  This makes the code easy\nto reuse for other actions where it is appropriate, and I've added the\ncode to do that in git_snapshot.\n\nSigned-off-by: W Trevor King <wking@drexel.edu>\n\n---\n gitweb/gitweb.perl |   54 ++++++++++++++++++++++++++++++++++++---------------\n 1 files changed, 38 insertions(+), 16 deletions(-)\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..96a13e7 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,31 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub modified_since {\n+\tmy ($content_type, $latest_epoch, $timezone) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch, $timezone);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-type => $content_type,\n+\t\t\t\t-charset => 'utf-8',\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\treturn 0;\n+\t\t}\n+\t}\n+\treturn 1;\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7029,6 +7054,14 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n+\tif (! modified_since($known_snapshot_formats{$format}{'type'},\n+\t                     $co{'committer_epoch'},\n+\t                     $co{'committer_tz'})) {\n+\t\treturn;\n+\t}\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7038,9 +7071,11 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n+\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \tprint $cgi->header(\n \t\t-type => $known_snapshot_formats{$format}{'type'},\n \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t-last_modified => $latest_date{'rfc2822'},\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n@@ -7821,22 +7856,9 @@ sub git_feed {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n+\t\tif (! modified_since($content_type, $latest_epoch,\n+\t\t                     $latest_commit{'comitter_tz'})) {\n+\t\t\treturn;\n \t\t}\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n-- \n1.7.3.4\n"},{"id":"187314","messageId":"m37gyf4hc3.fsf@localhost.localdomain","threadId":"29955","inReplyTo":"20120315075407.GA18246@odin.tremily.us","subject":"Re: [PATCH] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","fromName":"Jakub Narebski","fromEmail":"jnareb@gmail.com","sentAt":"2012-03-20T11:55:45Z","receivedAt":"2012-03-20T11:55:45Z","isPatch":true,"sender":{"key":"jnareb@gmail.com","avatar":"https://avatars.githubusercontent.com/u/2706?v=4"},"body":"I'm sorry I am only now answering, but unfortunately this patch has\nfallen thought crack.\n\n\"W. Trevor King\" <wking@drexel.edu> writes:\n\n> The current gitweb only generates Last-Modified and handles\n> If-Modified-Since headers for the git_feed action.  This patch breaks\n> the Last-Modified and If-Modified-Since handling code out from\n> git_feed into a new function modified_since.  This makes the code easy\n> to reuse for other actions where it is appropriate,\n\nThat is a very good idea.\n\nOf course adding support for If-Modified-Since has sense only if we\ncan calculate modification time without doing all the work (and\ncalculating modification time separately doesn;t add to work to be\ndone), or at least we are to send large amount of data so even if we\ncannot save CPU time and I/O hit we can save network bandwidth.\n\n> and I've added the code to do that in git_snapshot.\n\nNitpick about grammar: here you change from impersonal to personal\nform in commit message, unnecessarily I think.\n\n> Signed-off-by: W Trevor King <wking@drexel.edu>\n> \n> ---\n>  gitweb/gitweb.perl |   54 ++++++++++++++++++++++++++++++++++++---------------\n>  1 files changed, 38 insertions(+), 16 deletions(-)\n> \n> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\n> index a8b5fad..96a13e7 100755\n> --- a/gitweb/gitweb.perl\n> +++ b/gitweb/gitweb.perl\n> @@ -7003,6 +7003,31 @@ sub snapshot_name {\n>  \treturn wantarray ? ($name, $name) : $name;\n>  }\n>  \n> +sub modified_since {\n> +\tmy ($content_type, $latest_epoch, $timezone) = @_;\n\nPassing $content_type to function named modified_since() looks quite\nstrange to me.\n\n> +\tour $cgi;\n> +\n> +\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> +\tif (defined $if_modified) {\n> +\t\tmy $since;\n> +\t\tif (eval { require HTTP::Date; 1; }) {\n> +\t\t\t$since = HTTP::Date::str2time($if_modified);\n> +\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> +\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> +\t\t}\n> +\t\tif (defined $since && $latest_epoch <= $since) {\n> +\t\t\tmy %latest_date = parse_date($latest_epoch, $timezone);\n\nShouldn't we pass \\%latest_date (or rather \\%modification_date to be\nmore generic)?  Ah, I see that parse_date() subroutine does not store\noriginal epoch not original timezone.\n\nBy the way, for $latest_date{'rfc2822'} we don't need $timezone\nargument, and I think we should not pass it to generic\nmodified_since() function (or whatever it will be called).\n\n> +\t\t\tprint $cgi->header(\n> +\t\t\t\t-type => $content_type,\n> +\t\t\t\t-charset => 'utf-8',\n\nDo we need -charset here?  There is no HTTP body, and -charset => 'utf-8'\nis incorrect for snapshots (with e.g. 'application/x-zip' $content_type)\n\n> +\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> +\t\t\t\t-status => '304 Not Modified');\n> +\t\t\treturn 0;\n> +\t\t}\n> +\t}\n> +\treturn 1;\n> +}\n> +\n>  sub git_snapshot {\n>  \tmy $format = $input_params{'snapshot_format'};\n>  \tif (!@snapshot_fmts) {\n> @@ -7029,6 +7054,14 @@ sub git_snapshot {\n>  \n>  \tmy ($name, $prefix) = snapshot_name($project, $hash);\n>  \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> +\n> +\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n> +\tif (! modified_since($known_snapshot_formats{$format}{'type'},\n> +\t                     $co{'committer_epoch'},\n> +\t                     $co{'committer_tz'})) {\n> +\t\treturn;\n> +\t}\n\nFor a function named modified_since(), it does too many things.\n\nI think a better idea would be to make this function/subroutine (named\ne.g. if_modified_since() or something like that) to work more like\ndie_error() -- it should simply end request instead of having caller\nto use complcated calling convention, and care about eraly termination\nby itself.\n\nThis means turning modified_since() into subroutine (and renaming it),\nand ending it with\n\n  goto DONE_GITWEB;\n\nor\n\n  goto DONE_REQUEST;\n\ndepending on the code base you are applying to.\n\n> +\n>  \tmy $cmd = quote_command(\n>  \t\tgit_cmd(), 'archive',\n>  \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n> @@ -7038,9 +7071,11 @@ sub git_snapshot {\n>  \t}\n>  \n>  \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n> +\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n>  \tprint $cgi->header(\n>  \t\t-type => $known_snapshot_formats{$format}{'type'},\n>  \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n> +\t\t-last_modified => $latest_date{'rfc2822'},\n>  \t\t-status => '200 OK');\n>  \n\nHave you run gitweb tests?  This simply has no way of working, as %co\nvariable you use here is not defined anywhere in git_snapshot()\nsubroutine.\n\nWhat if there is no commit, for example if we are rewuesting snapshot\nof a tree by its SHA-1?  We need to be able to deal with sich\nsituation, if only by not handling Last-Modified / If-Modified-Since.\n\n>  \topen my $fd, \"-|\", $cmd\n> @@ -7821,22 +7856,9 @@ sub git_feed {\n>  \t\t%latest_commit = %{$commitlist[0]};\n>  \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n>  \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n\nDo we use %latest_date beside handling If-Modified-Since?  Is there a\nneed for separate $latest_epoch and %;atest_date variables?\n\n> -\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n> -\t\tif (defined $if_modified) {\n> -\t\t\tmy $since;\n> -\t\t\tif (eval { require HTTP::Date; 1; }) {\n> -\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n> -\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n> -\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n> -\t\t\t}\n> -\t\t\tif (defined $since && $latest_epoch <= $since) {\n> -\t\t\t\tprint $cgi->header(\n> -\t\t\t\t\t-type => $content_type,\n> -\t\t\t\t\t-charset => 'utf-8',\n> -\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n> -\t\t\t\t\t-status => '304 Not Modified');\n> -\t\t\t\treturn;\n> -\t\t\t}\n> +\t\tif (! modified_since($content_type, $latest_epoch,\n> +\t\t                     $latest_commit{'comitter_tz'})) {\n> +\t\t\treturn;\n>  \t\t}\n>  \t\tprint $cgi->header(\n>  \t\t\t-type => $content_type,\n> -- \n> 1.7.3.4\n\n-- \nJakub Narebski\n"},{"id":"187336","messageId":"20120320164056.GA23093@odin.tremily.us","threadId":"29955","inReplyTo":"m37gyf4hc3.fsf@localhost.localdomain","subject":"[PATCH v2] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.","fromName":"W. Trevor King","fromEmail":"wking@drexel.edu","sentAt":"2012-03-20T16:40:56Z","receivedAt":"2012-03-20T16:40:56Z","isPatch":true,"sender":{"key":"wking@tremily.us","avatar":"https://avatars.githubusercontent.com/u/209920?v=4"},"body":"The current gitweb only generates Last-Modified and handles                     \nIf-Modified-Since headers for the git_feed action.  This patch breaks           \nthe Last-Modified and If-Modified-Since handling code out from                  \ngit_feed into a new function modified_since.  This makes the code easy          \nto reuse for other actions where it is appropriate and adds the code            \nto do that to git_snapshot.                                                     \n\nSigned-off-by: W Trevor King <wking@drexel.edu>                                 \n---\nThanks for the feedback, Jakub.  I believe I've addressed your points\nin this revised patch.\n\nChanged since v1:\n- Fixed impersonal/personal commit message\n- Simplified modified_since() -> die_if_unmodified().  Now it only\n  takes a $latest_epoch argument and we set fewer headers.  It also\n  jump straight to DONE_GITWEB if the page is unmodified.\n- Ran tests (which passed this time).  Somehow I missed t/README\n  earlier.\n\nI'd add tests for my patch, but I'd have to have commit timestamps\nfrom the test repository, and I don't understand the test framework\nwell enough to know how to do that cleanly.\n\nThings that haven't changed:\n\n> > @@ -7038,9 +7071,11 @@ sub git_snapshot {\n> >  \t}\n> >  \n> >  \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n> > +\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n> ...\n> This simply has no way of working, as %co variable you use here is\n> not defined anywhere in git_snapshot() subroutine.\n\nI defined it earlier:\n\n> > @@ -7029,6 +7054,14 @@ sub git_snapshot {\n> >\n> >   my ($name, $prefix) = snapshot_name($project, $hash);\n> >   my $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n> > +\n> > + my %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n> > + if (! modified_since($known_snapshot_formats{$format}{'type'},\n> > +                      $co{'committer_epoch'},\n\nWhile modified_since() (now die_if_unmodified()) handles the 304 case,\nwe also want to set Last-Modified for 200s.  That's what the latter\ncode is trying to do.\n\n> What if there is no commit, for example if we are rewuesting snapshot\n> of a tree by its SHA-1?  We need to be able to deal with sich\n> situation, if only by not handling Last-Modified / If-Modified-Since.\n\nThe parse_commit() call should handle these cases.\n\ndiff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl\nindex a8b5fad..b944351 100755\n--- a/gitweb/gitweb.perl\n+++ b/gitweb/gitweb.perl\n@@ -7003,6 +7003,28 @@ sub snapshot_name {\n \treturn wantarray ? ($name, $name) : $name;\n }\n \n+sub die_if_unmodified {\n+\tmy ($latest_epoch) = @_;\n+\tour $cgi;\n+\n+\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n+\tif (defined $if_modified) {\n+\t\tmy $since;\n+\t\tif (eval { require HTTP::Date; 1; }) {\n+\t\t\t$since = HTTP::Date::str2time($if_modified);\n+\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n+\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n+\t\t}\n+\t\tif (defined $since && $latest_epoch <= $since) {\n+\t\t\tmy %latest_date = parse_date($latest_epoch);\n+\t\t\tprint $cgi->header(\n+\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n+\t\t\t\t-status => '304 Not Modified');\n+\t\t\tgoto DONE_GITWEB;\n+\t\t}\n+\t}\n+}\n+\n sub git_snapshot {\n \tmy $format = $input_params{'snapshot_format'};\n \tif (!@snapshot_fmts) {\n@@ -7029,6 +7051,10 @@ sub git_snapshot {\n \n \tmy ($name, $prefix) = snapshot_name($project, $hash);\n \tmy $filename = \"$name$known_snapshot_formats{$format}{'suffix'}\";\n+\n+\tmy %co = parse_commit($hash) or die_error(404, \"Unknown commit object\");\n+\tdie_if_unmodified($co{'committer_epoch'});\n+\n \tmy $cmd = quote_command(\n \t\tgit_cmd(), 'archive',\n \t\t\"--format=$known_snapshot_formats{$format}{'format'}\",\n@@ -7038,9 +7064,11 @@ sub git_snapshot {\n \t}\n \n \t$filename =~ s/([\"\\\\])/\\\\$1/g;\n+\tmy %latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'});\n \tprint $cgi->header(\n \t\t-type => $known_snapshot_formats{$format}{'type'},\n \t\t-content_disposition => 'inline; filename=\"' . $filename . '\"',\n+\t\t-last_modified => $latest_date{'rfc2822'},\n \t\t-status => '200 OK');\n \n \topen my $fd, \"-|\", $cmd\n@@ -7820,24 +7848,8 @@ sub git_feed {\n \tif (defined($commitlist[0])) {\n \t\t%latest_commit = %{$commitlist[0]};\n \t\tmy $latest_epoch = $latest_commit{'committer_epoch'};\n+\t\tdie_if_unmodified($latest_epoch);\n \t\t%latest_date   = parse_date($latest_epoch, $latest_commit{'comitter_tz'});\n-\t\tmy $if_modified = $cgi->http('IF_MODIFIED_SINCE');\n-\t\tif (defined $if_modified) {\n-\t\t\tmy $since;\n-\t\t\tif (eval { require HTTP::Date; 1; }) {\n-\t\t\t\t$since = HTTP::Date::str2time($if_modified);\n-\t\t\t} elsif (eval { require Time::ParseDate; 1; }) {\n-\t\t\t\t$since = Time::ParseDate::parsedate($if_modified, GMT => 1);\n-\t\t\t}\n-\t\t\tif (defined $since && $latest_epoch <= $since) {\n-\t\t\t\tprint $cgi->header(\n-\t\t\t\t\t-type => $content_type,\n-\t\t\t\t\t-charset => 'utf-8',\n-\t\t\t\t\t-last_modified => $latest_date{'rfc2822'},\n-\t\t\t\t\t-status => '304 Not Modified');\n-\t\t\t\treturn;\n-\t\t\t}\n-\t\t}\n \t\tprint $cgi->header(\n \t\t\t-type => $content_type,\n \t\t\t-charset => 'utf-8',\n--                                                                              \n1.7.3.4                                                                         \n"}]}