[PATCH] Handle If-None-Match HTTP header in plain view
John Keeping
john at keeping.me.uk
Wed Aug 13 21:25:27 CEST 2014
On Tue, Aug 12, 2014 at 06:53:01PM -0300, Damián Nohales wrote:
> 2014-08-12 16:15 GMT-03:00 John Keeping <john at keeping.me.uk>:
> > If we have sufficient infrastructure to handle HEAD requests then it
> > should be trivial to add proper Etag handling on top, but I don't think
> > it's trivial to add that.
>
> With this patch, the Etag verification works on both GET and HEAD
> requests, CGit only disable cache and force the exit after
> cgit_print_http_headers when the request is HEAD, that doesn't
> interfere with the Etag calculation.
>
> What interfere with this is the server's cache. To do it right with
> interoperability with server cache, we need to save to the cache the
> "200 OK" response independently on Etag matches so we can retrieve
> Etag by parsing the cache instead of calculating it in future request.
> Then we need an extra step after getting the content from the cache
> that decides between actually sends the content or sends a "304 Not
> Modified".
I think the only sensible approach would be to put a handler between
cache_process() and stdout when If-None-Match is present in the request
so that we can return 304 if the Etag matches, then it doesn't matter
whether we're streaming from the cache or not and we just have to leave
the original request running after returning the response so that it
streams the body to the cache.
I was assuming that if we handled HEAD requests we would want to do
something similar and not just bypass the cache, but I had not actually
looked at the code.
> >> 2014-08-12 6:00 GMT-03:00 John Keeping <john at keeping.me.uk>:
> >> > On Mon, Aug 11, 2014 at 07:45:53PM -0300, Damián Nohales wrote:
> >> >> (Sorry, I forgot to include the list address in my response.)
> >> >>
> >> >> This does not interact with CGit's cache at all, this interacts with
> >> >> client side cache.
> >> >>
> >> >> If the client sends an Etag it means "Hey, I have the content cached
> >> >> for this URL in the client and its Etag is abc123", so, for a given
> >> >> URL, if server calculates the Etag and results to the same as the Etag
> >> >> sent by client, it means "Ok, client has the same Etag than me, so
> >> >> client's cached content is up-to-date".
> >> >>
> >> >> The Etag is calculated from the requested object hash. I'm not a Git
> >> >> expert, but I know that the hash will be modified when the file is
> >> >> modified by a commit.
> >> >>
> >> >> So, if we request a file, then, one year later, we request the same
> >> >> file and we still have it cached with its Etag, CGit will respond 304
> >> >> Not Modified if the file was not modified by newer commits (CGit
> >> >> doesn't check its internal cache, it checks the client's cache).
> >> >>
> >> >> Actually, we should avoid send "Expires" and "Last-Modified" when
> >> >> "Etag" is available, so the caching information is consistent (and
> >> >> also, using Etag we have a more precise and smart caching).
> >> >
> >> > Yes, but I think the code you've changed comes after CGit has duplicated
> >> > stdout into the cache file. So if a request comes in with an
> >> > If-None-Match header, then CGit will cache the 304 response and send it
> >> > in response to all future request for the same page (until the cached
> >> > copy times out).
> >> >
> >> >> 2014-08-11 18:32 GMT-03:00 John Keeping <john at keeping.me.uk>:
> >> >> > On Mon, Aug 11, 2014 at 05:53:23PM -0300, Damián Nohales wrote:
> >> >> >> We are sending Etag to clients but this header is basically unusefulness
> >> >> >> if the server doesn't tell the client if the content has been changed or
> >> >> >> not for a given Path/Etag pair.
> >> >> >>
> >> >> >> Signed-off-by: Damián Nohales <damiannohales at gmail.com>
> >> >> >> ---
> >> >> >
> >> >> > How does this interact with CGit's cache? What happens if the original
> >> >> > page expires from the cache and then a request comes in with a matching
> >> >> > Etag?
> >> >> >
> >> >> >> cgit.c | 1 +
> >> >> >> cgit.h | 1 +
> >> >> >> ui-plain.c | 41 +++++++++++++++++++++--------------------
> >> >> >> ui-shared.c | 20 ++++++++++++++++++++
> >> >> >> ui-shared.h | 1 +
> >> >> >> 5 files changed, 44 insertions(+), 20 deletions(-)
> >> >> >>
> >> >> >> diff --git a/cgit.c b/cgit.c
> >> >> >> index 8c4517d..7af7712 100644
> >> >> >> --- a/cgit.c
> >> >> >> +++ b/cgit.c
> >> >> >> @@ -385,6 +385,7 @@ static void prepare_context(void)
> >> >> >> ctx.env.server_port = getenv("SERVER_PORT");
> >> >> >> ctx.env.http_cookie = getenv("HTTP_COOKIE");
> >> >> >> ctx.env.http_referer = getenv("HTTP_REFERER");
> >> >> >> + ctx.env.if_none_match = getenv("HTTP_IF_NONE_MATCH");
> >> >> >> ctx.env.content_length = getenv("CONTENT_LENGTH") ? strtoul(getenv("CONTENT_LENGTH"), NULL, 10) : 0;
> >> >> >> ctx.env.authenticated = 0;
> >> >> >> ctx.page.mimetype = "text/html";
> >> >> >> diff --git a/cgit.h b/cgit.h
> >> >> >> index 0badc64..eddd4c7 100644
> >> >> >> --- a/cgit.h
> >> >> >> +++ b/cgit.h
> >> >> >> @@ -282,6 +282,7 @@ struct cgit_environment {
> >> >> >> const char *server_port;
> >> >> >> const char *http_cookie;
> >> >> >> const char *http_referer;
> >> >> >> + const char *if_none_match;
> >> >> >> unsigned int content_length;
> >> >> >> int authenticated;
> >> >> >> };
> >> >> >> diff --git a/ui-plain.c b/ui-plain.c
> >> >> >> index 30fff89..a08dc5b 100644
> >> >> >> --- a/ui-plain.c
> >> >> >> +++ b/ui-plain.c
> >> >> >> @@ -103,8 +103,8 @@ static int print_object(const unsigned char *sha1, const char *path)
> >> >> >> ctx.page.filename = path;
> >> >> >> ctx.page.size = size;
> >> >> >> ctx.page.etag = sha1_to_hex(sha1);
> >> >> >> - cgit_print_http_headers();
> >> >> >> - html_raw(buf, size);
> >> >> >> + if (!cgit_print_http_headers_matching_etag())
> >> >> >> + html_raw(buf, size);
> >> >> >> /* If we allocated this, then casting away const is safe. */
> >> >> >> if (freemime)
> >> >> >> free((char*) ctx.page.mimetype);
> >> >> >> @@ -128,24 +128,25 @@ static void print_dir(const unsigned char *sha1, const char *base,
> >> >> >> fullpath = buildpath(base, baselen, path);
> >> >> >> slash = (fullpath[0] == '/' ? "" : "/");
> >> >> >> ctx.page.etag = sha1_to_hex(sha1);
> >> >> >> - cgit_print_http_headers();
> >> >> >> - htmlf("<html><head><title>%s", slash);
> >> >> >> - html_txt(fullpath);
> >> >> >> - htmlf("</title></head>\n<body>\n<h2>%s", slash);
> >> >> >> - html_txt(fullpath);
> >> >> >> - html("</h2>\n<ul>\n");
> >> >> >> - len = strlen(fullpath);
> >> >> >> - if (len > 1) {
> >> >> >> - fullpath[len - 1] = 0;
> >> >> >> - slash = strrchr(fullpath, '/');
> >> >> >> - if (slash)
> >> >> >> - *(slash + 1) = 0;
> >> >> >> - else
> >> >> >> - fullpath = NULL;
> >> >> >> - html("<li>");
> >> >> >> - cgit_plain_link("../", NULL, NULL, ctx.qry.head, ctx.qry.sha1,
> >> >> >> - fullpath);
> >> >> >> - html("</li>\n");
> >> >> >> + if (!cgit_print_http_headers_matching_etag()) {
> >> >> >> + htmlf("<html><head><title>%s", slash);
> >> >> >> + html_txt(fullpath);
> >> >> >> + htmlf("</title></head>\n<body>\n<h2>%s", slash);
> >> >> >> + html_txt(fullpath);
> >> >> >> + html("</h2>\n<ul>\n");
> >> >> >> + len = strlen(fullpath);
> >> >> >> + if (len > 1) {
> >> >> >> + fullpath[len - 1] = 0;
> >> >> >> + slash = strrchr(fullpath, '/');
> >> >> >> + if (slash)
> >> >> >> + *(slash + 1) = 0;
> >> >> >> + else
> >> >> >> + fullpath = NULL;
> >> >> >> + html("<li>");
> >> >> >> + cgit_plain_link("../", NULL, NULL, ctx.qry.head, ctx.qry.sha1,
> >> >> >> + fullpath);
> >> >> >> + html("</li>\n");
> >> >> >> + }
> >> >> >> }
> >> >> >> free(fullpath);
> >> >> >> }
> >> >> >> diff --git a/ui-shared.c b/ui-shared.c
> >> >> >> index 9dde0a3..84c7efd 100644
> >> >> >> --- a/ui-shared.c
> >> >> >> +++ b/ui-shared.c
> >> >> >> @@ -661,6 +661,26 @@ void cgit_print_http_headers(void)
> >> >> >> exit(0);
> >> >> >> }
> >> >> >>
> >> >> >> +int cgit_print_http_headers_matching_etag(void)
> >> >> >> +{
> >> >> >> + int match = 0;
> >> >> >> + char *etag;
> >> >> >> + if (ctx.page.etag && ctx.env.if_none_match) {
> >> >> >> + etag = fmtalloc("\"%s\"", ctx.page.etag);
> >> >> >> + if (!strcmp(etag, ctx.env.if_none_match)) {
> >> >> >> + ctx.page.status = 304;
> >> >> >> + ctx.page.statusmsg = "Not Modified";
> >> >> >> + ctx.page.mimetype = NULL;
> >> >> >> + ctx.page.size = 0;
> >> >> >> + ctx.page.filename = NULL;
> >> >> >> + match = 1;
> >> >> >> + }
> >> >> >> + free(etag);
> >> >> >> + }
> >> >> >> + cgit_print_http_headers();
> >> >> >> + return match;
> >> >> >> +}
> >> >> >> +
> >> >> >> void cgit_print_docstart(void)
> >> >> >> {
> >> >> >> if (ctx.cfg.embedded) {
> >> >> >> diff --git a/ui-shared.h b/ui-shared.h
> >> >> >> index 3e7a91b..e279f42 100644
> >> >> >> --- a/ui-shared.h
> >> >> >> +++ b/ui-shared.h
> >> >> >> @@ -60,6 +60,7 @@ extern void cgit_vprint_error(const char *fmt, va_list ap);
> >> >> >> extern void cgit_print_date(time_t secs, const char *format, int local_time);
> >> >> >> extern void cgit_print_age(time_t t, time_t max_relative, const char *format);
> >> >> >> extern void cgit_print_http_headers(void);
> >> >> >> +extern int cgit_print_http_headers_matching_etag(void);
> >> >> >> extern void cgit_print_docstart(void);
> >> >> >> extern void cgit_print_docend();
> >> >> >> extern void cgit_print_pageheader(void);
> >> >> >> --
> >> >> >> 2.0.4
> >> _______________________________________________
> >> CGit mailing list
> >> CGit at lists.zx2c4.com
> >> http://lists.zx2c4.com/mailman/listinfo/cgit
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit
More information about the CGit
mailing list