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

From: Jakub Narebski <jnareb@gmail.com>
Date: Mar 30, 2012, 16:07 UTC
Message-ID: <201203301807.55284.jnareb@gmail.com>
In-Reply-To: <bfbde5354e25dfbf535307a72016a6b5ac3a2c56.1333024238.git.wking@drexel.edu>

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

> Because snapshots can be large, you can save some bandwidth by
> supporting caching via If-Modified-Since.  This patch adds support for
> the i-m-s request to git_snapshot() if the request is a commit.
>

Perhaps it is worth clarifying that "caching" here means external HTTP caching, either by web browser, or by web accelerator / reverse proxy. Supporting Last-Modified and If-Modified-Since helps that[1][2].

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

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

"still handled as they were before" means "do not support l-m / i-m-s", isn't it? Wouldn't it be better to write it explicitely?

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

Anyway, I like those changes:

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

[...]

Show 7 quoted lines

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

If we ignore case, we can write

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

which is IMVVVHO slightly more readable.

Not that it matters much.  Just nitpicking.

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

-- 
Jakub Narebski
Poland