From: Jakub Narebski <jnareb@gmail.com>
To: "W. Trevor King" <wking@drexel.edu>
Cc: git@vger.kernel.org, Junio C Hamano <gitster@pobox.com>
Subject: Re: [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot().
Date: Wed, 28 Mar 2012 22:28:08 +0100 [thread overview]
Message-ID: <201203282328.08876.jnareb@gmail.com> (raw)
In-Reply-To: <20120328183712.GA5992@odin.tremily.us>
On Wed, 28 Mar 2012, W. Trevor King wrote:
> On Wed, Mar 28, 2012 at 07:11:31PM +0100, Jakub Narebski wrote:
[...]
> > And it was not caught by test because CGI.pm can output the last modified
> > header as "Last-modified" (RFC 2616, sec 4.2 states "Field names are
> > case-insensitive"), so the last check should be
> >
> > + ! grep -i "Last-Modified" gitweb.output
> >
> > Hmmm... why we use gitweb.output and not gitweb.headers? Is it consistency
> > with earlier tests?
>
> Yes, but I can switch to `gitweb.headers` if you'd like. Should I
> adjust all the header tests in t9501 to use `gitweb.headers` and `grep
> -i`? It should probably be a separate patch for the tests that
> existed before my i-m-s additions.
Eh, don't worry about this. First, I think we can assume that HTTP
headers from CGI.pm will all start with capital letter.
Second, for positive match being overly strict is safe - if assumption
doesn't hold we would get false failure. The problem is for negative
match - being overly strict means that we won't catch the breakage.
I think that the gitweb.output vs gitweb.headers (and gitweb.body) is
because those tests predate gitweb_run producing gitweb.headers file.
Be consistent if you want, or use new feature in new test; you don't
need to modernize t9501.
--
Jakub Narebski
Poland
next prev parent reply other threads:[~2012-03-28 21:28 UTC|newest]
Thread overview: 18+ messages / expand[flat|nested] mbox.gz Atom feed top
[not found] <20120328164513.GA4389@odin.tremily.us>
2012-03-28 17:44 ` [PATCH v7 0/3] gitweb: refactor If-Modified-Since handling W. Trevor King
2012-03-28 17:44 ` [PATCH v7 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King
2012-03-28 17:44 ` [PATCH v7 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King
2012-03-28 17:44 ` [PATCH v7 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King
2012-03-28 18:11 ` Jakub Narebski
2012-03-28 18:37 ` W. Trevor King
2012-03-28 19:11 ` Junio C Hamano
2012-03-28 19:27 ` W. Trevor King
2012-03-28 21:45 ` Jakub Narebski
2012-03-28 21:28 ` Jakub Narebski [this message]
2012-03-29 12:45 ` [PATCH v8 0/3] gitweb: refactor If-Modified-Since handling W. Trevor King
2012-03-29 12:45 ` [PATCH v8 1/3] gitweb: add `status` headers to git_feed() responses W. Trevor King
2012-03-30 15:21 ` Jakub Narebski
2012-03-29 12:45 ` [PATCH v8 2/3] gitweb: refactor If-Modified-Since handling W. Trevor King
2012-03-30 15:30 ` Jakub Narebski
2012-03-29 12:45 ` [PATCH v8 3/3] gitweb: add If-Modified-Since handling to git_snapshot() W. Trevor King
2012-03-30 16:07 ` Jakub Narebski
2012-03-30 16:11 ` [PATCH v8 0/3] gitweb: refactor If-Modified-Since handling Junio C Hamano
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=201203282328.08876.jnareb@gmail.com \
--to=jnareb@gmail.com \
--cc=git@vger.kernel.org \
--cc=gitster@pobox.com \
--cc=wking@drexel.edu \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
* If your mail client supports setting the In-Reply-To header
via mailto: links, try the mailto: link
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox;
as well as URLs for NNTP newsgroup(s).