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

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

[W. Trevor King]

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

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(-)

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

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

What's cooking in git.git (Mar 2012, #07; Mon, 19)

[Junio C Hamano]

What's cooking in git.git (Mar 2012, #07; Mon, 19)
--------------------------------------------------

Here are the topics that have been cooking.  Commits prefixed with '-' are
only in 'pu' (proposed updates) while commits prefixed with '+' are in 'next'.

The "git push" preparation steps (there are two topics) have enough time
to be polished to graduate to 'master' soon after 1.7.10 final.

You can find the changes described here in the integration branches of the
repositories listed at

    http://git-blame.blogspot.com/p/git-public-repositories.html

--------------------------------------------------
[Graduated to "master"]

* ab/perl-i18n (2012-03-10) 3 commits
 + perl/Makefile: install Git::I18N under NO_PERL_MAKEMAKER
 + Git::I18N: compatibility with perl <5.8.3

Attempts to help installations with ancient Perl and/or without
MakeMaker.

* jc/maint-verify-objects-remove-pessimism (2012-03-15) 1 commit
  (merged to 'next' on 2012-03-15 at f824530)
 + fetch/receive: remove over-pessimistic connectivity check

The code to validate the history connectivity between old refs and new
refs used by fetch and receive-pack, introduced in 1.7.8, was grossly
inefficient and unnecessarily tried to re-validate integrity of individual
objects. This essentially reverts the patch that introduced the
performance regression.

* jn/maint-fast-import-empty-ls (2012-03-09) 2 commits
  (merged to 'next' on 2012-03-15 at d531079)
 + fast-import: don't allow 'ls' of path with empty components
 + fast-import: leakfix for 'ls' of dirty trees

fast-import did not diagnose "ls ''" that asks an empty path
as an error.

* sl/customize-sane-tool-path (2012-03-09) 1 commit
  (merged to 'next' on 2012-03-15 at a838844)
 + configure: allow user to prevent $PATH "sanitization" on Solaris

"configure" script learned to take "--sane-tool-path" from the command
line to record SANE_TOOL_PATH (used to avoid broken platform tools in
/usr/bin) in config.mak-autogen.  This may be useful for people on Solaris
who have saner tools outside /usr/xpg[46]/bin.

* th/doc-diff-submodule-option (2012-03-14) 1 commit
  (merged to 'next' on 2012-03-14 at 0e1d755)
 + Documentation/diff-options: reword description of --submodule option

Update "diff --submodule" documentation.

* th/git-diffall (2012-03-14) 5 commits
  (merged to 'next' on 2012-03-14 at 38e1251)
 + contrib/diffall: fix cleanup trap on Windows
 + contrib/diffall: eliminate duplicate while loops
 + contrib/diffall: eliminate use of tar
 + contrib/diffall: create tmp dirs without mktemp
 + contrib/diffall: comment actual reason for 'cdup'

Update sample "diffall" script.

* th/mergetools-deltawalker (2012-03-15) 1 commit
  (merged to 'next' on 2012-03-15 at 1a62faf)
 + Documentation/difftool: add deltawalker to list of valid diff tools

--------------------------------------------------
[New Topics]

* jc/maint-clean-nested-worktree-in-subdir (2012-03-15) 2 commits
 - clean: preserve nested git worktree in subdirectories
 - remove_dir_recursively(): Add flag for skipping removal of toplevel dir
 (this branch is tangled with jh/notes-merge-in-git-dir-worktree.)

"git clean -d -f" (not "-d -f -f") is supposed to protect nested working
trees of independent git repositories that exist in the current project
working tree from getting removed, but the protection applied only to such
working trees that are at the top-level of the current project by mistake.

Not urgent.

* ct/advise-push-default (2012-03-19) 1 commit
 - push: Provide situational hints for non-fast-forward errors

Breaks down the cases in which "git push" fails due to non-ff into three
categories, and gives separate advise messages.  I think there should be
three advise confvars, not just two.

* nl/rebase-i-cheat-sheet (2012-03-16) 1 commit
 - [Do not merge] rebase -i: remind that the lines are top-to-bottom

Not urgent.

* da/difftool-test (2012-03-19) 1 commit
 - t7800: Test difftool passing arguments to diff

Makes sure "difftool" options can be given in any order.

* th/difftool-diffall (2012-03-19) 9 commits
 - difftool: print list of valid tools with '--tool-help'
 - difftool: teach dir-diff to copy modified files back to working tree
 - difftool: teach difftool to handle directory diffs
 - difftool: replace system call with Git::command_noisy
 - difftool: eliminate setup_environment function
 - difftool: stop appending '.exe' to git
 - difftool: remove explicit change of PATH
 - difftool: exit(0) when usage is printed
 - difftool: parse options using Getopt::Long

Reworks the "diffall" contrib script into "difftool" framework.  Use of
Getopt::Long obviously contradicts with da/difftool-test and breaks 'pu'.

--------------------------------------------------
[Stalled]

* nd/columns (2012-03-13) 12 commits
 - column: support grouping entries
 - column: support "denser" mode
 - ls-files: support --column
 - tag: add --column
 - column: support piping stdout to external git-column process
 - status: add --column
 - branch: add --column
 - help: reuse print_columns() for help -a
 - column: add dense layout support
 - column: add columnar layout
 - Stop starting pager recursively
 - Add column layout skeleton and git-column

Rerolled again.  Modulo minor nits, looked nicer than the previous round.

* nd/threaded-index-pack (2012-03-11) 2 commits
 - index-pack: support multithreaded delta resolving
 - index-pack: split second pass obj handling into own function

Another reroll after a bugreport on pthread usage discovered by Ramsey,
but it seems the topic is cooking between Ramsay and Duy out of tree.
Waiting for resolution.

* dg/test-from-elsewhere (2012-03-04) 2 commits
 - Support out-of-tree Valgrind tests
 - Allow overriding GIT_BUILD_DIR

Better support for out-of-tree test scripts, but it appears that the
approach needs to be rethought.  By repointing TEST_DIRECTORY to a
directory other than $(pwd)/.., an out of place test script can reach
test helpers and freshly built Git relative to it (GIT_BUILD_DIR is
a mere short-hand for $TEST_DIRECTORY/..).

* cn/apply-fix-ws-can-lengthen-lines (2012-03-11) 1 commit
 . apply: reallocate the postimage buffer when needed

Attempts to address an ancient bug that dates back to the addition
of an oddball "tab-in-indent" whitespace breakage class that wants
to have longer lines than the original when fixing things up.

Needs more work; results in double-frees.

* hv/submodule-recurse-push (2012-02-13) 3 commits
 - push: teach --recurse-submodules the on-demand option
 - Refactor submodule push check to use string list instead of integer
 - Teach revision walking machinery to walk multiple times sequencially

The bottom one was not clearly explained and needs a reroll.

* ss/git-svn-prompt-sans-terminal (2012-01-04) 3 commits
 - fixup! 15eaaf4
 - git-svn, perl/Git.pm: extend Git::prompt helper for querying users
 - perl/Git.pm: "prompt" helper to honor GIT_ASKPASS and SSH_ASKPASS

The bottom one has been replaced with a rewrite based on comments
from Ævar. The second one needs more work, both in perl/Git.pm and
prompt.c, to give precedence to tty over SSH_ASKPASS when terminal
is available.

* jc/split-blob (2012-03-16) 6 commits
 - chunked-object: streaming checkout
 - chunked-object: fallback checkout codepaths
 - bulk-checkin: support chunked-object encoding
 - bulk-checkin: allow the same data to be multiply hashed
 - new representation types in the packstream
 - varint-in-pack: refactor varint encoding/decoding

Not ready.

I finished the streaming checkout codepath, but as explained in
127b177 (bulk-checkin: support chunked-object encoding, 2011-11-30),
these are still early steps of a long and painful journey. At least
pack-objects and fsck need to learn the new encoding for the series
to be usable locally, and then index-pack/unpack-objects needs to
learn it to be used remotely.

Given that I heard a lot of noise that people want large files, and
that I was asked by somebody at GitTogether'11 privately for an
advice on how to pay developers (not me) to help adding necessary
support, I am somewhat dissapointed that the original patch series
that was sent almost two months ago still remains here without much
comments and updates from the developer community. I even made the
interface to the logic that decides where to split chunks easily
replaceable, and I deliberately made the logic in the original patch
extremely stupid to entice others, especially the "bup" fanboys, to
come up with a better logic, thinking that giving people an easy
target to shoot for, they may be encouraged to help out. The plan is
not working :-(.

--------------------------------------------------
[Cooking]

* jh/notes-merge-in-git-dir-worktree (2012-03-15) 4 commits
 - notes-merge: Don't remove .git/NOTES_MERGE_WORKTREE; it may be the user's cwd
 - notes-merge: use opendir/readdir instead of using read_directory()
 - t3310: illustrate failure to "notes merge --commit" inside $GIT_DIR/
 - remove_dir_recursively(): Add flag for skipping removal of toplevel dir
 (this branch is tangled with jc/maint-clean-nested-worktree-in-subdir.)

Running "notes merge --commit" failed to perform correctly when run
from any directory inside $GIT_DIR/.  When "notes merge" stops with
conflicts, $GIT_DIR/NOTES_MERGE_WORKTREE is the place a user edits
to resolve it.

Not urgent.

* jn/diffstat-tests (2012-03-13) 7 commits
 - diffstat summary line varies by locale: miscellany
 - test: use numstat instead of diffstat in binary-diff test
 - test: use --numstat instead of --stat in "git stash show" tests
 - test: test cherry-pick functionality and output separately
 - test: modernize funny-names test style
 - test: use numstat instead of diffstat in funny-names test
 - test: use test_i18ncmp when checking --stat output

Some tests checked the "diff --stat" output when they do not have to,
which unnecessarily made things harder to verify under GETTEXT_POISON.

Not urgent.

* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits
 - diff: tweak a _copy_ of diff_options with word-diff
 - diff: refactor the word-diff setup from builtin_diff_cmd
 - t4034: diff.*.wordregex should not be "sticky" in --word-diff

The regexp configured with wordregex was incorrectly reused across
files.
Not urgent.

* zj/test-cred-helper-nicer-prove (2012-03-15) 2 commits
 - t0303: resurrect commit message as test documentation
 - t0303: immediately bail out w/o GIT_TEST_CREDENTIAL_HELPER

Minor improvement to t0303.
Not urgent.

* jc/commit-hook-authorship (2012-03-11) 3 commits
  (merged to 'next' on 2012-03-12 at 05ca7f8)
 + commit: pass author/committer info to hooks
 + t7503: does pre-commit-hook learn authorship?
 + ident.c: add split_ident_line() to parse formatted ident line
 (this branch is tangled with jc/run-hook-env-1.)

"git commit --author=$name" did not tell the name that was being
recorded in the resulting commit to hooks, even though it does do so
when the end user overrode the authorship via the "GIT_AUTHOR_NAME"
environment variable.  This is a simpler of the two approaches.

Will defer til 1.7.10.

* jc/run-hook-env-1 (2012-03-11) 3 commits
 - run_hook(): enhance the interface to pass arbitrary environment
 + t7503: does pre-commit-hook learn authorship?
 + ident.c: add split_ident_line() to parse formatted ident line
 (this branch is tangled with jc/commit-hook-authorship.)

Not urgent.

Updates run_hook() API to be much less specific to "commit".  It would
only be useful if people start doing more interesting things with hooks.

* jc/diff-algo-cleanup (2012-02-19) 2 commits
  (merged to 'next' on 2012-03-15 at cca0032)
 + xdiff: PATIENCE/HISTOGRAM are not independent option bits
 + xdiff: remove XDL_PATCH_* macros
 (this branch is used by jc/diff-ignore-case.)

Resurrects the preparatory clean-up patches from another topic that was
discarded, as this would give a saner foundation to build on diff.algo
configuration option series.

Not urgent.

* jh/apply-free-patch (2012-03-07) 1 commit
 - apply: do not leak patches and fragments

Will defer til 1.7.10.

* rs/unpack-trees-leakfix (2012-03-06) 1 commit
  (merged to 'next' on 2012-03-07 at 69a69cd)
 + unpack-trees: plug minor memory leak

Will defer til 1.7.10.

* tb/maint-remove-irrelevant-i18n-test (2012-03-06) 1 commit
  (merged to 'next' on 2012-03-07 at 23f2dd1)
 + t0204: remove a test that checks undefined behaviour

I tentatively parked this in 'next' but later reverted the merge.
Will discard.

* mm/push-default-switch-warning (2012-03-09) 1 commit
 - push: start warning upcoming default change for push.default

Not urgent.

This resurrects an ancient patch I wrote during a discussion we had in the
1.6.3-1.6.4 era.  This should probably come after ct/advise-push-default
topic and at that point the advise messages need to be rephrased, taking
the future default change into account.

* jc/fmt-merge-msg-people (2012-03-13) 1 commit
 - fmt-merge-msg: show those involved in a merged series

The "fmt-merge-msg" command learns to list the primary contributors
involved in the side topic you are merging.

Will defer til 1.7.10.

* nl/http-proxy-more (2012-03-15) 5 commits
 - http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
 - http: Avoid limit of retrying request only twice
 - http: handle proxy authentication failure (error 407)
 - http: handle proxy proactive authentication
 - http: try http_proxy env var when http.proxy config option is not set

The code to talk to http proxies learn to use the same credential
API used to talk to the final http destinations.

Will defer til 1.7.10.

* nd/stream-more (2012-03-07) 7 commits
  (merged to 'next' on 2012-03-07 at 7325922)
 + update-server-info: respect core.bigfilethreshold
 + fsck: use streaming API for writing lost-found blobs
 + show: use streaming API for showing blobs
 + parse_object: avoid putting whole blob in core
 + cat-file: use streaming API to print blobs
 + Add more large blob test cases
 + streaming: make streaming-write-entry to be more reusable

Use API to read blob data in smaller chunks in more places to
reduce the memory footprint.  In general, looked fairly good.

Will defer til 1.7.10.

--------------------------------------------------
[Discarded]

* jc/diff-ignore-mode (2012-03-01) 1 commit
 . diff --ignore-mode-change

 * jc/diff-ignore-case (2012-02-28) 6 commits
 . diff: -i is "--ignore-case" but means a bit more in "log"
 . diff: --ignore-case
 . xdiff: introduce XDF_IGNORE_CASE
 . xdiff: introduce XDF_INEXACT_MATCH
 - xdiff: PATIENCE/HISTOGRAM are not independent option bits
 - xdiff: remove XDL_PATCH_* macros

* jh/trace-use-startup-info (2012-03-02) 1 commit
 . Use startup_info->prefix rather than prefix.

I tend to agree with the doubt of the author of this patch expressed.

* nd/optim-connected (2012-03-15) 1 commit
 . {fetch,receive}-pack: skip sha-1 integrity test on objects from new pack

jc/maint-verify-objects-remove-pessimism replaces this.

* jc/advise-push-default (2011-12-18) 1 commit
 . push: hint to use push.default=upstream when appropriate

A rework by Christopher Tiwald replaces this.

* th/mergetools-tool-help (2012-03-15) 1 commit
 . difftool: print list of valid tools with '--tool-help'

Reworked as part of th/difftool-diffall topic.

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

[W. Trevor King]

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

On Thu, Mar 15, 2012 at 03:54:07AM -0400, W. Trevor King wrote:
> The current gitweb only generates Last-Modified and handles
> If-Modified-Since headers for the git_feed action.  This patch…

On Mon, Mar 19, 2012 at 05:23:07PM -0700, Junio C Hamano wrote:
> What's cooking in git.git (Mar 2012, #07; Mon, 19)
> …

My If-Modified-Since patch seems to have fallen through the cracks
here.  Is that because nobody is interested, or did I mess up
something in the submission?

Cheers,
Trevor

-- 
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] Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

[Jakub Narebski]

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

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

> 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.

> 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.

> +	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)

> +				-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.

> +
>  	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.

>  	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?

> -		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

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

[W. Trevor King]

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

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 adds the code            
to do that to git_snapshot.                                                     

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:

> > @@ -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:

> > @@ -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.

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                                                                         

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

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

[Junio C Hamano]

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

> My If-Modified-Since patch seems to have fallen through the cracks
> here.  Is that because nobody is interested, or did I mess up
> something in the submission?

Probably "just fell through the cracks"; if somebody found what it wants
to do very interesting but the implementation or submission lacking, you
would have heard loud noises.

Incremental updates to What's cooking

[Junio C Hamano]

Again, just the highlights of the notable topics.

Not much has happened on the 'master' front.

[New Topics]

* am/completion-zsh-fix (2012-03-20) 1 commit
 - contrib/completion: "local var=()" is misinterpreted as function decl by zsh

Needs a bit better explanation.

* lp/maint-diff-three-dash-with-graph (2012-03-20) 3 commits
 - t4202: add test for "log --graph --stat -p" separator lines
 - log --graph: fix break in graph lines
 - log --graph --stat: three-dash separator should come after graph lines

The combination of two options "log --graph --stat" was an obscure corner
case nobody cared about, and did not correctly show the ancestry graph
lines.

I've split the original patch into three pieces, one for fixes to two
different issues and a test.  Also the test is adjusted so that the series
can be back-merged to older codebase that did not have 7f81463 (Use
correct grammar in diffstat summary line, 2012-02-01) that first appeared
in v1.7.9.2

* wk/gitweb-snapshot-use-if-modified-since (2012-03-20) 1 commit
 - Pull gitweb If-Modified-Since handling out into its own function and use for snapshots.

Unreviewed; the title looks way too long and does not sit well in the
shortlog output.

* jc/maint-merge-autoedit (2012-03-20) 1 commit
 - merge: backport GIT_MERGE_AUTOEDIT support

In 1.7.10, we added GIT_MERGE_AUTOEDIT=no environment variable to help
older scripts to let them refuse giving users a chance to explain the
merge, but forgot that 1.7.9 automatically opens an editor when merging an
annotated tag, and there is no equivalent escape hatch.  A merge of this
topic to 1.7.10 track becomes a no-op, but we may want to apply this to
the 1.7.9.x series.

[Cooking]

* ct/advise-push-default (2012-03-19) 1 commit
 - push: Provide situational hints for non-fast-forward error

Breaks down the cases in which "git push" fails due to non-ff into three
categories, and gives separate advise messages.  This should be a good
change regardless of mm/push-default-switch-warning topic.

* tr/maint-word-diff-regex-sticky (2012-03-14) 3 commits
  (merged to 'next' on 2012-03-20 at b3f67cd)
 + diff: tweak a _copy_ of diff_options with word-diff
 + diff: refactor the word-diff setup from builtin_diff_cmd
 + t4034: diff.*.wordregex should not be "sticky" in --word-diff

It would be nice to have this fix in 1.7.10 but perhaps we can wait until
1.7.10.1.

* nl/http-proxy-more (2012-03-15) 5 commits
  (merged to 'next' on 2012-03-20 at c004001)
 + http: rename HTTP_REAUTH to HTTP_AUTH_RETRY
 + http: Avoid limit of retrying request only twice
 + http: handle proxy authentication failure (error 407)
 + http: handle proxy proactive authentication
 + http: try http_proxy env var when http.proxy config option is not set

The code to talk to http proxies learn to use the same credential
API used to talk to the final http destinations.

[Stalled]

* th/difftool-diffall (2012-03-19) 9 commits
 . difftool: print list of valid tools with '--tool-help'
 . difftool: teach dir-diff to copy modified files back to working tree
 . difftool: teach difftool to handle directory diffs
 . difftool: replace system call with Git::command_noisy
 . difftool: eliminate setup_environment function
 . difftool: stop appending '.exe' to git
 . difftool: remove explicit change of PATH
 . difftool: exit(0) when usage is printed
 . difftool: parse options using Getopt::Long

Reworks the "diffall" contrib script into "difftool" framework.
Breaks tests when merged to 'pu'.

* jh/apply-free-patch (2012-03-20) 2 commits
 - apply: free patch->result
 - apply: do not leak patches and fragments

Valgrind reports quite a lot of discarded memory inside apply. I have a
slight suspicion that we should first clarify the ownership rules of
pieces of memory in this standalone program that was designed to be "run
once and let exit take care of the memory" before proceeding further.

* dg/test-from-elsewhere (2012-03-04) 2 commits
 - Support out-of-tree Valgrind tests
 - Allow overriding GIT_BUILD_DIR

Better support for out-of-tree test scripts, but it appears that the
approach needs to be rethought.  By repointing TEST_DIRECTORY to a
directory other than $(pwd)/.., an out of place test script can reach
test helpers and freshly built Git relative to it (GIT_BUILD_DIR is
a mere short-hand for $TEST_DIRECTORY/..).

Discussion stalled.

[PATCH v3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

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 die_if_unmodified.  This makes the code
easy to reuse for other actions where it is appropriate and adds the
code to do that to git_snapshot.

Signed-off-by: W Trevor King <wking@drexel.edu>                                 
---
Changed since v2:
  - Shorter title.
  - Fixed modified_since -> die_if_unmodified in commit message.

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                                                                         

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

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

By the way, it is custom on this mailing list to usually Cc (send a
copy) to all people participating in discussion, and not only to git
mailing list.

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

> Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb

Perhaps a better title would be:

  gitweb: Refactor If-Modified-Since handling, support in snapshot

to mention all that thispatch does.  Though trouble with coming up
with a short but fairly complete one-line summary might mean that this
patch would be better split in two: refactoring and adding support for
If-Modified-Since to snapshots.

> 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 die_if_unmodified.  This makes the code
> easy to reuse for other actions

O.K.

>                                  where it is appropriate
                                   ^^^^^^^^^^^^^^^^^^^^^^^

This doesn't add any information.  I think it the commit message would
be better if you either remove this, or expand (in a separate
paragraph) where support for If-Modified-Since might make sense, and
where it could not.

>                                                         and adds the
> code to do that to git_snapshot.

This would read better as a separate paragraph.
 
> Signed-off-by: W Trevor King <wking@drexel.edu>                                 
> ---
> Changed since v2:
>   - Shorter title.
>   - Fixed modified_since -> die_if_unmodified in commit message.

Nice.

BTW. what happened to diffstat?

Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?
We could use test_tick() and $test_tick for that (or extract formatted
committer date from a commit).
 
> 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;
> +		}
> +	}
> +}

O.K.

die_if_unmodified() is good enough name, though I wonder if we could
come up with a better name....

> +
>  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'});
> +

That unexplainably changes behavior of 'snapshot' action.  Before we
would accept snapshot of a tree given by its SHA-1, now we do not.

This might be a good idea from a security point of view wrt. leaking
information (c.f. "git archive --remote" behavior), but it at least
needs to be explained in commit message, and perhaps even in a comment
above this line.

Alternative solution would be to skip If-Modified-Since handling if we
request snapshot by tree id:

  +
  +	my %co = parse_commit($hash);
  +	die_if_unmodified($co{'committer_epoch'}) if %co;
  +

> @@ -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');

Of course this would need to be updated too.
  
>  	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',

Nice.

-- 
Jakub Narebski

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Wed, Mar 21, 2012 at 06:19:51AM -0700, Jakub Narebski wrote:
> By the way, it is custom on this mailing list to usually Cc (send a
> copy) to all people participating in discussion, and not only to git
> mailing list.

Ah, sorry.  I figured that if you got the original email to the list,
I'd just be doubling up in your inbox by Cc-ing you directly.

> Though trouble with coming up with a short but fairly complete
> one-line summary might mean that this patch would be better split in
> two: refactoring and adding support for If-Modified-Since to
> snapshots.

Agreed.

> >                                  where it is appropriate
>                                    ^^^^^^^^^^^^^^^^^^^^^^^
> This doesn't add any information.  I think it the commit message would
> be better if you either remove this, or expand (in a separate
> paragraph) where support for If-Modified-Since might make sense, and
> where it could not.

I'll expand it in the refactoring patch.

> BTW. what happened to diffstat?

My local branch has sequential commits for each patch version (e.g.,
commits for v1, v2, ...).  When it's time to email the list, I'm
supposed to send logical patches against the trunk (e.g., patches for
refactoring, git_snapshot, ...).  For v2 I just used `git diff
origin/master` to generate the patch, and it doesn't include the
diffstat.  Now that I'm splitting into two patches, I'll probably use
`git rebase -i origin/master` and just keep track of the changes by
hand ;).  If there's a better way that I'm overlooking, feel free to
point me in the right direction.

> Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?
> We could use test_tick() and $test_tick for that (or extract formatted
> committer date from a commit).

I'll try to set that up.  Should it be bundled into the git_snapshot
patch, or should there be three patches:

1: gitweb: Refactor If-Modified-Since handling
2: gitweb: Add If-Modified-Since tests for git_snapshot
3: gitweb: Add If-Modified-Since support for git_snapshot

> > @@ -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'});
> > +
> 
> That unexplainably changes behavior of 'snapshot' action.  Before we
> would accept snapshot of a tree given by its SHA-1, now we do not.
> 
> This might be a good idea from a security point of view wrt. leaking
> information (c.f. "git archive --remote" behavior), but it at least
> needs to be explained in commit message, and perhaps even in a comment
> above this line.
> 
> Alternative solution would be to skip If-Modified-Since handling if we
> request snapshot by tree id:
> 
>   +
>   +	my %co = parse_commit($hash);
>   +	die_if_unmodified($co{'committer_epoch'}) if %co;
>   +

I'm still not understanding the problem here.  The following all work
in my testing:

  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz
  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz
  http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz

Can you give me an example of a hash that you expect to not work?

Cheers,
Trevor

-- 
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 v3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

On Wed, Mar 21, 2012, W. Trevor King wrote:
> On Wed, Mar 21, 2012 at 06:19:51AM -0700, Jakub Narebski wrote:

> > By the way, it is custom on this mailing list to usually Cc (send a
> > copy) to all people participating in discussion, and not only to git
> > mailing list.
> 
> Ah, sorry.  I figured that if you got the original email to the list,
> I'd just be doubling up in your inbox by Cc-ing you directly.

I am not subscribed to git mailing list, and I am reading it and
responding (to those emails that I'm not Cc-ed) via GMane's NNTP
(Usenet) interface:

  nntp://news.gmane.org/gmane.comp.version-control.git

And I think I am not the only one.  But I guess that even for those
subscribed, emails addressed directly to them take priority, which
is important considering volume of email on git mailing list.
 
[..]
> > BTW. what happened to diffstat?
> 
> My local branch has sequential commits for each patch version (e.g.,
> commits for v1, v2, ...).  When it's time to email the list, I'm
> supposed to send logical patches against the trunk (e.g., patches for
> refactoring, git_snapshot, ...).  For v2 I just used `git diff
> origin/master` to generate the patch, and it doesn't include the
> diffstat.  Now that I'm splitting into two patches, I'll probably use
> `git rebase -i origin/master` and just keep track of the changes by
> hand ;).  If there's a better way that I'm overlooking, feel free to
> point me in the right direction.

There is a tool to create patches to send: git-format-patch.  Myself I
usually create a new directory for a patch series, e.g. mdir.if_mod.v3,
and use git-format-patch to populate it, e.g.

  $ git format-patch origin/master.. -o mdir.if_mod.3/ \
                     --cover-letter --subject-prefix="PATCH v4"

Note that you need to edit at least cover letter.

BTW. "git diff" has '--stat -p' and '--patch-with-stat' options :-)

> > Tests (to be put, I think, in t/t9501-gitweb-standalone-http-status.sh)?
> > We could use test_tick() and $test_tick for that (or extract formatted
> > committer date from a commit).
> 
> I'll try to set that up.  Should it be bundled into the git_snapshot
> patch, or should there be three patches:
> 
> 1: gitweb: Refactor If-Modified-Since handling
> 2: gitweb: Add If-Modified-Since tests for git_snapshot
> 3: gitweb: Add If-Modified-Since support for git_snapshot

I think it would be better to add initial tests with refactoring, and
snapshot specific tests with snapshot support, e.g.:

  1/2: gitweb: Refactor If-Modified-Since handling and add tests
  2/2: gitweb: Add If-Modified-Since support for snapshots

> > > @@ -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'});
> > > +
> > 
> > That unexplainably changes behavior of 'snapshot' action.  Before we
> > would accept snapshot of a tree given by its SHA-1, now we do not.
> > 
> > This might be a good idea from a security point of view wrt. leaking
> > information (c.f. "git archive --remote" behavior), but it at least
> > needs to be explained in commit message, and perhaps even in a comment
> > above this line.
> > 
> > Alternative solution would be to skip If-Modified-Since handling if we
> > request snapshot by tree id:
> > 
> >   +
> >   +	my %co = parse_commit($hash);
> >   +	die_if_unmodified($co{'committer_epoch'}) if %co;
> >   +
> 
> I'm still not understanding the problem here.  The following all work
> in my testing:

But wouldn't work in my clone... ;-(

>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d545cab4a8dc894fae2c2634a74993ea62b054d;sf=tgz
>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=1d5;sf=tgz
>   http://.../gitweb.cgi?p=a/.git;a=snapshot;h=HEAD;sf=tgz
> 
> Can you give me an example of a hash that you expect to not work?

Currently all of those work

    http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz
    http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz"
    http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz"

v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree
(v1.7.6^{tree}).

The last URL would stop working after your change with 404
"Unknown commit object".

I'm not sure but I think that currently 'snapshot' link in the navbar
of the 'tree' view uses that kind of link, with 'h' parameter set to
SHA-1 of a tree.

-- 
Jakub Narebski
Poland

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> By the way, it is custom on this mailing list to usually Cc (send a
> copy) to all people participating in discussion, and not only to git
> mailing list.
>
> "W. Trevor King" <wking@drexel.edu> writes:
>
>> Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb
>
> Perhaps a better title would be:
>
>   gitweb: Refactor If-Modified-Since handling, support in snapshot

With "gitweb: " prefix to denote what area it affects, that is certainly
better.  Given the primary objective and effect is that the snapshot
feature starts honoring i-m-s,

	gitweb: honor If-Modified-Since request header in snapshot

would be sufficient.

> to mention all that thispatch does.  Though trouble with coming up
> with a short but fairly complete one-line summary might mean that this
> patch would be better split in two: refactoring and adding support for
> If-Modified-Since to snapshots.

If many existing callsites had duplicated code to handle i-m-s, we may
want two patch series, the first of which consolidates them into a single
helper function without changing anything else (most importantly, without
regression) and the second that uses the helper to add support in the
snapshot feature.  But if that is not the case, I think we can go either
way.

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote:
> There is a tool to create patches to send: git-format-patch.  Myself I
> usually create a new directory for a patch series, e.g. mdir.if_mod.v3,
> and use git-format-patch to populate it, e.g.

Ah, I like that.  Then I can rebase away ;).

> I think it would be better to add initial tests with refactoring, and
> snapshot specific tests with snapshot support, e.g.:
> 
>   1/2: gitweb: Refactor If-Modified-Since handling and add tests
>   2/2: gitweb: Add If-Modified-Since support for snapshots

But the new tests would be for the new functionality (i.e. snapshot
support), so they wouldn't belong in the general refactoring commit.

> Currently all of those work
> 
>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz
>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz"
>     http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz"
> 
> v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree
> (v1.7.6^{tree}).
> 
> The last URL would stop working after your change with 404
> "Unknown commit object".

Indeed it does.  I'll add you're suggested skipping for these cases.

There should be a way to determine the oldest commit refering to a
tree, which could be used for timestamping tree-ishes, but that can be
a project for another day ;).

-- 
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 v3] Isolate If-Modified-Since handling in gitweb

[Junio C Hamano]

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

>> I think it would be better to add initial tests with refactoring, and
>> snapshot specific tests with snapshot support, e.g.:
>> 
>>   1/2: gitweb: Refactor If-Modified-Since handling and add tests
>>   2/2: gitweb: Add If-Modified-Since support for snapshots
>
> But the new tests would be for the new functionality (i.e. snapshot
> support), so they wouldn't belong in the general refactoring commit.

Then you are planning to split it in a wrong way.

As I said, I do not think it matters that much for a small patch like
this, but if the plan is to make the part to create i-m-s helper function
as a standalone "refactoring" patch, then what Jakub outlined is the right
way to go about it.

In the first patch, you create i-m-s helper and update the existing code
that can use the helper, without changing anything else. Do not touch
snapshot code in this patch, if the current code does not support i-m-s in
snapshot.  And in the same patch, add tests for codepaths that use i-m-s
to make sure your refactoring did not break them.  In other words, if you
remove the change to gitweb/ from the first patch and apply only the
changes to the tests, the resulting new tests should pass with the current
code that has i-m-s support inline without the i-m-s helper.  And if you
add back your change to gitweb/ for the refactoring, the test should still
pass.

And then in the second patch, you update the snapshot code and whatever
else that can use i-m-s helper to support i-m-s.  You can add tests to
protect the new feature from future breakages in this patch.

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:
> "W. Trevor King" <wking@drexel.edu> writes:
> >> I think it would be better to add initial tests with refactoring, and
> >> snapshot specific tests with snapshot support, e.g.:
> >> 
> >>   1/2: gitweb: Refactor If-Modified-Since handling and add tests
> >>   2/2: gitweb: Add If-Modified-Since support for snapshots
> >
> > But the new tests would be for the new functionality (i.e. snapshot
> > support), so they wouldn't belong in the general refactoring commit.
> 
> Then you are planning to split it in a wrong way.
> 
> ... add tests for codepaths that use i-m-s to make sure your
> refactoring did not break them...

Ah, I was assuming that some current tests might be checking the
current behavior, and that my new tests would be testing my new
snapshot behavior.  If the old i-m-s handling also needs tests, that
should happen before any of my previously proposed patches:

1: tests for i-m-s and git_feed
2: refactor i-m-s handling
3: tests for i-m-s and git_snapshot (which fail until 4)
4: add i-m-s to git_snapshot

How does that sound?

-- 
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 v3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

W. Trevor King wrote:
> On Wed, Mar 21, 2012 at 12:22:44PM -0700, Junio C Hamano wrote:
> > "W. Trevor King" <wking@drexel.edu> writes:

> > > > I think it would be better to add initial tests with refactoring, and
> > > > snapshot specific tests with snapshot support, e.g.:
> > > > 
> > > >   1/2: gitweb: Refactor If-Modified-Since handling and add tests
> > > >   2/2: gitweb: Add If-Modified-Since support for snapshots
> > >
> > > But the new tests would be for the new functionality (i.e. snapshot
> > > support), so they wouldn't belong in the general refactoring commit.
> > 
> > Then you are planning to split it in a wrong way.
> > 
> > ... add tests for codepaths that use i-m-s to make sure your
> > refactoring did not break them...
> 
> Ah, I was assuming that some current tests might be checking the
> current behavior, and that my new tests would be testing my new
> snapshot behavior.  If the old i-m-s handling also needs tests, that
> should happen before any of my previously proposed patches:
> 
> 1: tests for i-m-s and git_feed
> 2: refactor i-m-s handling

Those two can be in single commit.  Tests added need only test i-m-s
using git_feed ('atom' or 'rss' action), as it is the only user,
and you touch only i-m-s handling.

> 3: tests for i-m-s and git_snapshot (which fail until 4)
> 4: add i-m-s to git_snapshot

We usually put tests together with feature.  Tests before feature means
that you would need to mark them as test_expect_failure, as they would
not pass before feature is added, isn't it?

-- 
Jakub Narebski
Poland

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

>> 1: tests for i-m-s and git_feed
>> 2: refactor i-m-s handling
>
> Those two can be in single commit.  Tests added need only test i-m-s
> using git_feed ('atom' or 'rss' action), as it is the only user,
> and you touch only i-m-s handling.

Correct.

>> 3: tests for i-m-s and git_snapshot (which fail until 4)
>> 4: add i-m-s to git_snapshot
>
> We usually put tests together with feature.  Tests before feature means
> that you would need to mark them as test_expect_failure, as they would
> not pass before feature is added, isn't it?

Correct.

In a more elaborate series, you would first add a lot of tests to nail the
behaviour of the existing code down, then refactor or reimplement them in
a different way, add tests that expect failure until the next phase, and
then add features and turn expect_failure in tests to expect_success.

But I do not htink this single patch deserves that complexity.  Does it
even have to split into three?

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Wed, Mar 21, 2012 at 01:09:24PM -0700, Junio C Hamano wrote:
> In a more elaborate series, you would first add a lot of tests to nail the
> behaviour of the existing code down, then refactor or reimplement them in
> a different way, add tests that expect failure until the next phase, and
> then add features and turn expect_failure in tests to expect_success.
> 
> But I do not htink this single patch deserves that complexity.  Does it
> even have to split into three?

Sorry, testing makes me think of test-driven development ;).  Two
patches it is (refactor + git_feed tests, git_snapshot + git_snapshot
tests).

-- 
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 v3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

On Wed, 21 Mar 2012, Junio C Hamano wrote:
> Jakub Narebski <jnareb@gmail.com> writes:
> 
> > By the way, it is custom on this mailing list to usually Cc (send a
> > copy) to all people participating in discussion, and not only to git
> > mailing list.
> >
> > "W. Trevor King" <wking@drexel.edu> writes:
> >
> > > Subject: [PATCH v3] Isolate If-Modified-Since handling in gitweb
> >
> > Perhaps a better title would be:
> >
> >   gitweb: Refactor If-Modified-Since handling, support in snapshot
> 
> With "gitweb: " prefix to denote what area it affects, that is certainly
> better.  Given the primary objective and effect is that the snapshot
> feature starts honoring i-m-s,
> 
> 	gitweb: honor If-Modified-Since request header in snapshot
> 
> would be sufficient.

That is a very good title... and if all changes were to be put in single
commit (see below), that is what we should concentrate on.  Refactoring
would be just a detail.
 
> > to mention all that thispatch does.  Though trouble with coming up
> > with a short but fairly complete one-line summary might mean that this
> > patch would be better split in two: refactoring and adding support for
> > If-Modified-Since to snapshots.
> 
> If many existing callsites had duplicated code to handle i-m-s, we may
> want two patch series, the first of which consolidates them into a single
> helper function without changing anything else (most importantly, without
> regression) and the second that uses the helper to add support in the
> snapshot feature.  But if that is not the case, I think we can go either
> way.

There is only one callsite, so theoretically we could do it in single
commit, refactoring to add support in new callsite...

...if not for the fact that control flow changes from using conditional
and early return to [longjump] "exception" based one.  That is why
I think it would be better to put tests and refactoring in a commit
separate from adding If-Modified-Since handling to 'snapshot' action.

tl;dr.  Two commits.
-- 
Jakub Narebski
Poland

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

On Wed, 21 Mar 2012, W. Trevor King wrote:
> On Wed, Mar 21, 2012 at 05:55:06PM +0100, Jakub Narebski wrote:

> > There is a tool to create patches to send: git-format-patch.  Myself I
> > usually create a new directory for a patch series, e.g. mdir.if_mod.v3,
> > and use git-format-patch to populate it, e.g.
> 
> Ah, I like that.  Then I can rebase away ;).

Yep.  Or use some patch / mailqueue management interface like StGit.
 
> > I think it would be better to add initial tests with refactoring, and
> > snapshot specific tests with snapshot support, e.g.:
> > 
> >   1/2: gitweb: Refactor If-Modified-Since handling and add tests
> >   2/2: gitweb: Add If-Modified-Since support for snapshots
> 
> But the new tests would be for the new functionality (i.e. snapshot
> support), so they wouldn't belong in the general refactoring commit.

I was thinking that "and add tests" would be about 'feed' action test
and i-m-s.

> > Currently all of those work
> > 
> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=v1.7.6;sf=tgz
> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=f696543d;sf=tgz"
> >     http://.../gitweb.cgi?p=git.git;a=snapshot;h=b1485af8;sf=tgz"
> > 
> > v1.7.6 is a tag, f696543d is a commit (v1.7.6^{}), b1485af8 is a tree
> > (v1.7.6^{tree}).
> > 
> > The last URL would stop working after your change with 404
> > "Unknown commit object".
> 
> Indeed it does.  I'll add you're suggested skipping for these cases.

If by "skipping" you mean here turning off handling of If-Modified-Since
and Last-Modified headers, then I agree.
 
> There should be a way to determine the oldest commit refering to a
> tree, which could be used for timestamping tree-ishes, but that can be
> a project for another day ;).

That also can be ambiguous.

-- 
Jakub Narebski
Poland

Re: [PATCH v3] Isolate If-Modified-Since handling in gitweb

[Junio C Hamano]

Jakub Narebski <jnareb@gmail.com> writes:

> tl;dr.  Two commits.

OK, thanks.

Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

Sorry for the delay since my last message, it's been a busy week ;).

On Thu, Mar 22, 2012 at 01:46:34PM +0100, Jakub Narebski wrote:
> ...if not for the fact that control flow changes from using conditional
> and early return to [longjump] "exception" based one.  That is why
> I think it would be better to put tests and refactoring in a commit
> separate from adding If-Modified-Since handling to 'snapshot' action.

I'll be sending along three patches.  The second and third are the
ones you discuss above.  The first is a teensy patch to add `Status`
output to non-304 calls to git_feed().  Without it you'd have to get a
bit more creative in the test suite.  If the status lines were left
out intentionally, let me know, and I'll come up with another
condition for those tests.

The second patch refactors and tests git_feed(), and the third patch
adds i-m-s support to git_snapshot() with associated tests.

Changes since v3:
* Patch 1/3 is completely new.
* Split previous patch into 2/3 and 3/3.
* Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}.
* Added testing to both 2/3 and 3/3.
* Reworked commit messages.

Cheers,
Trevor

-- 
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 --]

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

[W. Trevor King]

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

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

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

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

[W. Trevor King]

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

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 die_if_unmodified.  This makes the code
easy to reuse for other actions.

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

Signed-off-by: W Trevor King <wking@drexel.edu>
---
 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 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) {
@@ -7820,24 +7842,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',
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
+'
+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
+'
+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
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking
-- 
1.7.3.4

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

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

[W. Trevor King]

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

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 requested hash is a commit.
Requests for snapshots of tree-ishes, which lack well defined
timestamps, are still handled as they were before.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 229f3da..be9ad5d 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);
+	die_if_unmodified($co{'committer_epoch'}) if %co;
+
 	my $cmd = quote_command(
 		git_cmd(), 'archive',
 		"--format=$known_snapshot_formats{$format}{'format'}",
@@ -7060,10 +7064,19 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
-	print $cgi->header(
-		-type => $known_snapshot_formats{$format}{'type'},
-		-content_disposition => 'inline; filename="' . $filename . '"',
-		-status => '200 OK');
+	if (%co) {
+		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');
+	} else {
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-status => '200 OK');
+	}
 
 	open my $fd, "-|", $cmd
 		or die_error(500, "Execute git-archive failed");
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 0e49f29..38e90bd 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" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	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" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree-ish snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 
-- 
1.7.3.4

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

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

[Junio C Hamano]

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

> +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) {
> @@ -7820,24 +7842,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'});

It might make more sense to call the helper *after* stuffing %latest_date
and pass the $latest_date{'rfc2822'} as parameter to the new helper
function, so that the helper function do not have to call parse_date().

The verb "die" everywhere else in the codebase of Git not just means an
immediate exit, but means an imediate _error_ exit.  The new helper
function is not about failure, but merely is an early return.  Perhaps
rename it to exit_if_unmodified_since() or something?

> 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

Missing " &&" at the end (same in 3/3).

> +	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" &&
> +	gitweb_run "p=.git;a=atom;h=master" &&
> +	unset HTTP_IF_MODIFIED_SINCE &&
> +	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" &&
> +	gitweb_run "p=.git;a=atom;h=master" &&
> +	unset HTTP_IF_MODIFIED_SINCE &&
> +	grep "Status: 304 Not Modified" gitweb.output
> +'
> +test_debug 'cat gitweb.headers'

Other than that, the patch looks cleanly done.

Thanks.

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

[W. Trevor King]

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

On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote:
> It might make more sense to call the helper *after* stuffing %latest_date
> and pass the $latest_date{'rfc2822'} as parameter to the new helper
> function, so that the helper function do not have to call parse_date().

I did something closer to your suggestion in my v1 patch, but

On Tue, Mar 20, 2012 at 04:55:45AM -0700, Jakub Narebski wrote:
> 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).
> 
> …
> 
> 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.

Only passing in the epoch.  We could reduce computation at the expense
of complication by passing in both the epoch and a formatted time
string, but after Jakub's suggestions, I felt like a simpler interface
was the better approach.  I don't feel particularly committed to
either way, so just tell me which you'd like best ;).

> The verb "die" everywhere else in the codebase of Git not just means an
> immediate exit, but means an imediate _error_ exit.  The new helper
> function is not about failure, but merely is an early return.  Perhaps
> rename it to exit_if_unmodified_since() or something?

Will fix in v5.

> Missing " &&" at the end (same in 3/3).

Oops.  I wonder why my tests still passed :p.  Will fix in v5.

> Other than that, the patch looks cleanly done.
> 
> Thanks.

You're welcome.  Thanks for shepherding me through it ;).

-- 
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 v4 2/3] gitweb: refactor If-Modified-Since handling

[Junio C Hamano]

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

> On Mon, Mar 26, 2012 at 10:12:36AM -0700, Junio C Hamano wrote:
> ...
> Only passing in the epoch.  We could reduce computation at the expense
> of complication by passing in both the epoch and a formatted time
> string, but after Jakub's suggestions, I felt like a simpler interface
> was the better approach.  I don't feel particularly committed to
> either way, so just tell me which you'd like best ;).

The existing codepath already had the call to parse_date() nearby and that
was the only reason I made the suggestion.  The caller in the snapshot
codepath would need to call parse_date() much earlier and it would have to
be conditional to the availability of %co, so the simpler interface would
probably be overall win.

>> Missing " &&" at the end (same in 3/3).
>
> Oops.  I wonder why my tests still passed :p.  Will fix in v5.

The test will pass if there is no breakage.  The offence of missing " &&"
after your "grep 200" is that it will let the test pass even if you did
not have "200" in the output, failing to catch future breakage.

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

[W. Trevor King]

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

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>
---
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) = @_;
+	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,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
+'
+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
+'
+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
+'
+test_debug 'cat gitweb.headers'
 
 # ----------------------------------------------------------------------
 # load checking
-- 
1.7.3.4

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

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

[W. Trevor King]

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

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 requested hash is a commit.
Requests for snapshots of tree-ishes, which lack well defined
timestamps, are still handled as they were before.

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

diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
index 229f3da..be9ad5d 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,10 +7064,19 @@ sub git_snapshot {
 	}
 
 	$filename =~ s/(["\\])/\\$1/g;
-	print $cgi->header(
-		-type => $known_snapshot_formats{$format}{'type'},
-		-content_disposition => 'inline; filename="' . $filename . '"',
-		-status => '200 OK');
+	if (%co) {
+		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');
+	} else {
+		print $cgi->header(
+			-type => $known_snapshot_formats{$format}{'type'},
+			-content_disposition => 'inline; filename="' . $filename . '"',
+			-status => '200 OK');
+	}
 
 	open my $fd, "-|", $cmd
 		or die_error(500, "Execute git-archive failed");
diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
index 0e49f29..38e90bd 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" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	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" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 304 Not Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
+test_expect_success 'modification: tree-ish snapshot' '
+	ID=`git rev-parse --verify HEAD^{tree}` &&
+	export HTTP_IF_MODIFIED_SINCE="Wed, 6 Apr 2005 22:14:13 +0000" &&
+	gitweb_run "p=.git;a=snapshot;h=master;sf=tgz" &&
+	unset HTTP_IF_MODIFIED_SINCE &&
+	grep "Status: 200 OK" gitweb.output &&
+	! grep "Last-Modified" gitweb.output
+'
+test_debug 'cat gitweb.headers'
+
 # ----------------------------------------------------------------------
 # load checking
 
-- 
1.7.3.4

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

Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

On Mon, 26 Mar 2012, W. Trevor King wrote:

> Sorry for the delay since my last message, it's been a busy week ;).

Thanks for working on this topic.  

BTW. we are in feature freeze currently, before release of v1.7.10.
 
> I'll be sending along three patches.  [...]
> [...] The first is a teensy patch to add `Status` 
> output to non-304 calls to git_feed().  Without it you'd have to get a
> bit more creative in the test suite.  If the status lines were left
> out intentionally, let me know, and I'll come up with another
> condition for those tests.

No, the status line was omitted unintentionally, thanks for fixing this.
I guess that web server (at least Apache) adds it if it is missing.

Though I am not sure how lack of "Status:" header is different from
incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'...
 
> The second patch refactors and tests git_feed(), and the third patch
> adds i-m-s support to git_snapshot() with associated tests.
> 
> Changes since v3:
> * Patch 1/3 is completely new.
> * Split previous patch into 2/3 and 3/3.
> * Conditionals in 3/3 to avoid 404-ing on non-commits like v1.7.6^{tree}.
> * Added testing to both 2/3 and 3/3.
> * Reworked commit messages.

Good.

-- 
Jakub Narebski
Poland

Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:
> BTW. we are in feature freeze currently, before release of v1.7.10.

I'm not in a big hurry or anything, just trying to save some
bandwidth…

> Though I am not sure how lack of "Status:" header is different from
> incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'...

It's not for the 304 tests.  I thought it would be a good idea to grep
for a 200 status before looking for Last-Modified times, to ensure you
actually get to the successful response stage.

-- 
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 v4 0/3] Isolate If-Modified-Since handling in gitweb

[W. Trevor King]

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

On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:
> No, the status line was omitted unintentionally, thanks for fixing this.
> I guess that web server (at least Apache) adds it if it is missing.

From the CGI 1.1 specs [1], section 9.2:

  If the script does not return a Status header, then "200 OK" should
  be assumed.

So, perhaps it is better to skip the Status header on success (less
for the server to parse).

[1]: http://tools.ietf.org/html/draft-robinson-www-interface-00

-- 
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 v4 0/3] Isolate If-Modified-Since handling in gitweb

[Jakub Narebski]

W. Trevor King wrote:
> On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:

> > Though I am not sure how lack of "Status:" header is different from
> > incorrect "Status:" header for 'grep "Status: 304" gitweb.headers'...
> 
> It's not for the 304 tests.  I thought it would be a good idea to grep
> for a 200 status before looking for Last-Modified times, to ensure you
> actually get to the successful response stage.

Right.

Testing both sides of a new feature (i.e. modified and unmodified here)
is a good testing practice.  Thanks.

-- 
Jakub Narebski
Poland

Re: [PATCH v4 0/3] Isolate If-Modified-Since handling in gitweb

[Junio C Hamano]

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

> On Tue, Mar 27, 2012 at 08:24:22PM +0100, Jakub Narebski wrote:
>> No, the status line was omitted unintentionally, thanks for fixing this.
>> I guess that web server (at least Apache) adds it if it is missing.
>
> From the CGI 1.1 specs [1], section 9.2:
>
>   If the script does not return a Status header, then "200 OK" should
>   be assumed.
>
> So, perhaps it is better to skip the Status header on success (less
> for the server to parse).

I do not think it matters either way.  It is just a single header line, no?

The code is certainly more explicit with your patch.

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

[Jakub Narebski]

On Mon, 26 Mar 2012, 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.

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.
>
> 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) = @_;

> +	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.

> +			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?

> +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.

> +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.

> +test_debug 'cat gitweb.headers'
>  
>  # ----------------------------------------------------------------------
>  # load checking
> -- 
> 1.7.3.4
> 

-- 
Jakub Narebski
Poland

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

[Jakub Narebski]

On Mon, 26 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 requested hash is a commit.

"if the requested hash is a commit" means here "if we request snapshot
of a commit", isn't it?

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

s/tree-ishes/trees/.  Commit is tree-ish but not a tree; it is tree
that lacks timestamp that commit has.

> 
> Signed-off-by: W Trevor King <wking@drexel.edu>
> ---
>  gitweb/gitweb.perl                       |   21 +++++++++++++++---
>  t/t9501-gitweb-standalone-http-status.sh |   33 ++++++++++++++++++++++++++++++
>  2 files changed, 50 insertions(+), 4 deletions(-)
> 
> diff --git a/gitweb/gitweb.perl b/gitweb/gitweb.perl
> index 229f3da..be9ad5d 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;
> +

O.K.  Nice.

>  	my $cmd = quote_command(
>  		git_cmd(), 'archive',
>  		"--format=$known_snapshot_formats{$format}{'format'}",
> @@ -7060,10 +7064,19 @@ sub git_snapshot {
>  	}
>  
>  	$filename =~ s/(["\\])/\\$1/g;
> -	print $cgi->header(
> -		-type => $known_snapshot_formats{$format}{'type'},
> -		-content_disposition => 'inline; filename="' . $filename . '"',
> -		-status => '200 OK');
> +	if (%co) {
> +		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');
> +	} else {
> +		print $cgi->header(
> +			-type => $known_snapshot_formats{$format}{'type'},
> +			-content_disposition => 'inline; filename="' . $filename . '"',
> +			-status => '200 OK');
> +	}

This can be written shorter, and with less code repetition using ?: 
conditional operator:

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

...or something like that.

>  
>  	open my $fd, "-|", $cmd
>  		or die_error(500, "Execute git-archive failed");
> diff --git a/t/t9501-gitweb-standalone-http-status.sh b/t/t9501-gitweb-standalone-http-status.sh
> index 0e49f29..38e90bd 100755
> --- a/t/t9501-gitweb-standalone-http-status.sh
> +++ b/t/t9501-gitweb-standalone-http-status.sh

Same comments as for 2/3.

-- 
Jakub Narebski
Poland

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

[W. Trevor King]

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

On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote:
> On Mon, 26 Mar 2012, W. Trevor King wrote:
> > +# ----------------------------------------------------------------------
> > +# 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?

That's the date set by the first `test_tick`, which is hardcoded in
`test-lib-functions.sh`.  Extracting the date dynamically seems
unnecessary, since I can't imagine anyone changing the `test_tick`
date.  It's easy enough to do if you think it is appropriate though…

> > +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.

Good point.  Sloppy me not reading `t/README` thoroughly enough ;).

-- 
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 v5 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[W. Trevor King]

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

On Tue, Mar 27, 2012 at 11:31:37PM +0100, Jakub Narebski wrote:
> On Mon, 26 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 requested hash is a commit.
> 
> "if the requested hash is a commit" means here "if we request snapshot
> of a commit", isn't it?
> 
> > Requests for snapshots of tree-ishes, which lack well defined
> > timestamps, are still handled as they were before.
> 
> s/tree-ishes/trees/.  Commit is tree-ish but not a tree; it is tree
> that lacks timestamp that commit has.

Right.  I'll fix those in v6 ;).

> > +	if (%co) {
> > +		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');
> > +	} else {
> > +		print $cgi->header(
> > +			-type => $known_snapshot_formats{$format}{'type'},
> > +			-content_disposition => 'inline; filename="' . $filename . '"',
> > +			-status => '200 OK');
> > +	}
> 
> This can be written shorter, and with less code repetition using ?: 
> conditional operator:
> 
> > +	print $cgi->header(
> > +		-type => $known_snapshot_formats{$format}{'type'},
> > +		-content_disposition => 'inline; filename="' . $filename . '"',
> > +		 (%co ? -last_modified => $latest_date{'rfc2822'} : ()),
> > +		-status => '200 OK');
> 
> ...or something like that.

Ah, that's much better.  I'm not particulary familar with Perl, sorry ;).

I'll wait until I here back on Last-Modified timestamp generation, and
then submit v6.

-- 
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 v5 2/3] gitweb: refactor If-Modified-Since handling

[Jakub Narebski]

On Wed, 28 Mar 2012, W. Trevor King wrote:
> On Tue, Mar 27, 2012 at 11:24:20PM +0100, Jakub Narebski wrote:
> > On Mon, 26 Mar 2012, W. Trevor King wrote:
> > > +# ----------------------------------------------------------------------
> > > +# 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?
> 
> That's the date set by the first `test_tick`, which is hardcoded in
> `test-lib-functions.sh`.  Extracting the date dynamically seems
> unnecessary, since I can't imagine anyone changing the `test_tick`
> date.  

Ah, it's all right then.  I should have checked the test_tick function.

That of course assuming that nobody would add test_tick earlier, but
if he/she does, he/she can deal with fallout...

> It's easy enough to do if you think it is appropriate though… 

No, it is not needed.

> > > +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.
> 
> Good point.  Sloppy me not reading `t/README` thoroughly enough ;).

-- 
Jakub Narebski
Poland

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

[wking]

Changes since v5:
* Use `test_when_finished` to unset HTTP_IF_MODIFIED_SINCE.
* Actually use the tree ID in the `modification: tree snapshot` test.
* Conditional last_modified argument for `$cgi->header` git_snapshot
  and git_feed.
* tree-ish -> trees in git_snapshot commit message and test name.

Unchanged since v5:
* Patch 1/3, but I'm resending it anyway with `git send-email`.  I
  could skip resending it with:

    git send-email … HEAD^^

  but that would mess up the patch numbering.


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 v6 1/3] gitweb: add `status` headers to git_feed() responses.

[wking]

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 v6 2/3] gitweb: refactor If-Modified-Since handling

[wking]

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..ba106c1 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',
+		-last_modified => (%latest_date ? $latest_date{'rfc2822'} : undef),
+		-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 v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[wking]

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 ba106c1..591c793 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'} : undef),
 		-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 v6 0/3] gitweb: refactor If-Modified-Since handling

[W. Trevor King]

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

On Wed, Mar 28, 2012 at 11:46:58AM -0400, wking@tremily.us wrote:
> ...

Oops, I set `sendemail.from`, but it doesn't look like it stuck, and I
forgot to check before OKing the mails.  Please consider the v6
patches to be from `W. Trevor King <wking@drexel.edu>`, and adjust any
replies accordingly.

Sorry,
Trevor

-- 
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 v6 3/3] gitweb: add If-Modified-Since handling to git_snapshot().

[Jakub Narebski]

On Wed, 28 Mar 2012, wking@tremily.us 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.
> 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 ba106c1..591c793 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;
> +

Nice.

>  	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'});
> +	}
> +

That is probably more readable than

  +	%latest_date = parse_date($co{'committer_epoch'}, $co{'committer_tz'})
  +		if %co;

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

Same issue as with previous patch:

   	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 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'

Good!

-- 
Jakub Narebski
Poland