* [PATCH] Handle If-None-Match HTTP header in plain view
@ 2014-08-11 20:53
2014-08-11 21:32 ` john
0 siblings, 1 reply; 8+ messages in thread
From: @ 2014-08-11 20:53 UTC (permalink / raw)
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>
---
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-11 20:53 [PATCH] Handle If-None-Match HTTP header in plain view
@ 2014-08-11 21:32 ` john
2014-08-11 22:45 `
0 siblings, 1 reply; 8+ messages in thread
From: john @ 2014-08-11 21:32 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-11 21:32 ` john
@ 2014-08-11 22:45 `
2014-08-12 9:00 ` john
0 siblings, 1 reply; 8+ messages in thread
From: @ 2014-08-11 22:45 UTC (permalink / raw)
(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).
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-11 22:45 `
@ 2014-08-12 9:00 ` john
2014-08-12 15:38 `
0 siblings, 1 reply; 8+ messages in thread
From: john @ 2014-08-12 9:00 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-12 9:00 ` john
@ 2014-08-12 15:38 `
2014-08-12 19:15 ` john
0 siblings, 1 reply; 8+ messages in thread
From: @ 2014-08-12 15:38 UTC (permalink / raw)
Ah... I got it.
We could implement cache disabling per command to disable it for the
plain command. I don't know if there is another way since Etag is
calculated at command execution.
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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-12 15:38 `
@ 2014-08-12 19:15 ` john
2014-08-12 21:53 `
0 siblings, 1 reply; 8+ messages in thread
From: john @ 2014-08-12 19:15 UTC (permalink / raw)
On Tue, Aug 12, 2014 at 12:38:35PM -0300, Dami?n Nohales wrote:
> Ah... I got it.
>
> We could implement cache disabling per command to disable it for the
> plain command. I don't know if there is another way since Etag is
> calculated at command execution.
Another way to do it would be to only handle "If-None-Match" if the
content is already cached and then parse the Etag header from the cached
content.
I think it comes back to what Lars wrote in the commit that added the
current level of Etag support (commit 488a214):
Todo: add support for HEAD requests...
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.
> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-12 19:15 ` john
@ 2014-08-12 21:53 `
2014-08-13 19:25 ` john
0 siblings, 1 reply; 8+ messages in thread
From: @ 2014-08-12 21:53 UTC (permalink / raw)
2014-08-12 16:15 GMT-03:00 John Keeping <john at keeping.me.uk>:
> On Tue, Aug 12, 2014 at 12:38:35PM -0300, Dami?n Nohales wrote:
>> Ah... I got it.
>>
>> We could implement cache disabling per command to disable it for the
>> plain command. I don't know if there is another way since Etag is
>> calculated at command execution.
>
> Another way to do it would be to only handle "If-None-Match" if the
> content is already cached and then parse the Etag header from the cached
> content.
Uhm... I disagree, because this is related to what the client has in
its cache, not server cache. If client requests an Etag and we
calculate the same Etag in the server for the latest content in the
URL, we can assume that the client cache is up-to-date for this URL
independently on what is in the server cache slot for that URL.
>
> I think it comes back to what Lars wrote in the commit that added the
> current level of Etag support (commit 488a214):
>
> Todo: add support for HEAD requests...
>
I don't know what Lars means with that. In the current version
(without this patch), CGit sends Etag's for GET and HEAD request. The
problem is that the support for Etag's is incomplete if we don't
handle If-None-Match or If-Match.
> 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 that is overcomplicated and prevent us to send real time
updates for plain objects (anyways, I don't know if that is a goal)
>
>> 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
^ permalink raw reply [flat|nested] 8+ messages in thread
* [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-12 21:53 `
@ 2014-08-13 19:25 ` john
0 siblings, 0 replies; 8+ messages in thread
From: john @ 2014-08-13 19:25 UTC (permalink / raw)
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
^ permalink raw reply [flat|nested] 8+ messages in thread
end of thread, other threads:[~2014-08-13 19:25 UTC | newest]
Thread overview: 8+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2014-08-11 20:53 [PATCH] Handle If-None-Match HTTP header in plain view
2014-08-11 21:32 ` john
2014-08-11 22:45 `
2014-08-12 9:00 ` john
2014-08-12 15:38 `
2014-08-12 19:15 ` john
2014-08-12 21:53 `
2014-08-13 19:25 ` john
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).