my out-of-tree patches for cgit

my out-of-tree patches for cgit

[sebastian]

Hi,

just rebased my patches on top of v0.10. This includes the sendfile patch
and a few others while I was at it.

Sebastian

[PATCH 1/4] snapshots: Don't allow sneaked in snapshots requests

[sebastian]

If the snapshots are not enabled then the frontend won't show a link to it.
The skilled user however may construct the URL on his own and the frontend
will obey the request.
This patch adds a check for this case so the requst won't be served.

Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
---
 ui-snapshot.c | 6 ++++++
 1 file changed, 6 insertions(+)

diff --git a/ui-snapshot.c b/ui-snapshot.c
index 582dc31..b278ddf 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -209,6 +209,12 @@ void cgit_print_snapshot(const char *head, const char *hex,
 		return;
 	}
 
+	if (!(f->bit & snapshots)) {
+		show_error(xstrdup(fmt("Snapshot format %s is not enabled.",
+						f->suffix)));
+		return;
+	}
+
 	if (!hex && dwim) {
 		hex = get_ref_from_filename(ctx.repo->url, filename, f);
 		if (hex == NULL) {
-- 
1.8.5.2

[PATCH 2/4] cache: use sendfile() instead of a pair of read() + write()

[sebastian]

sendfile() does the same job and avoids to copy the content into userland
and back. One has to define NO_SENDFILE in case the OS (kernel / libc)
does not supported. It is disabled by default on non-linux environemnts.
According to the glibc, sendfile64() was added in Linux 2.4 (so it has
been there for a while) but after browsing over the mapage of FreeBSD's I
noticed that the prototype is little different.

Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
---
 Makefile |  1 +
 cache.c  | 26 +++++++++++++++++++++++++-
 cgit.mk  |  8 ++++++++
 3 files changed, 34 insertions(+), 1 deletion(-)

diff --git a/Makefile b/Makefile
index 2dc92df..05b97d7 100644
--- a/Makefile
+++ b/Makefile
@@ -29,6 +29,7 @@ DOC_PDF  = $(patsubst %.txt,%.pdf,$(MAN_TXT))
 # j, z, t. (representing long long int, char, intmax_t, size_t, ptrdiff_t).
 # some C compilers supported these specifiers prior to C99 as an extension.
 #
+# Define HAVE_LINUX_SENDFILE to use sendfile()
 
 #-include config.mak
 
diff --git a/cache.c b/cache.c
index fcd461f..9e7eeb0 100644
--- a/cache.c
+++ b/cache.c
@@ -13,6 +13,9 @@
  *
  */
 
+#ifdef HAVE_LINUX_SENDFILE
+#include <sys/sendfile.h>
+#endif
 #include "cgit.h"
 #include "cache.h"
 #include "html.h"
@@ -30,7 +33,6 @@ struct cache_slot {
 	const char *lock_name;
 	int match;
 	struct stat cache_st;
-	struct stat lock_st;
 	int bufsize;
 	char buf[CACHE_BUFSIZE];
 };
@@ -81,6 +83,23 @@ static int close_slot(struct cache_slot *slot)
 /* Print the content of the active cache slot (but skip the key). */
 static int print_slot(struct cache_slot *slot)
 {
+#ifdef HAVE_LINUX_SENDFILE
+	off_t start_off;
+	int ret;
+
+	start_off = slot->keylen + 1;
+
+	do {
+		ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off,
+				slot->cache_st.st_size - start_off);
+		if (ret < 0) {
+			if (errno == EAGAIN || errno == EINTR)
+				continue;
+			return errno;
+		}
+		return 0;
+	} while (1);
+#else
 	ssize_t i, j;
 
 	i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
@@ -97,6 +116,7 @@ static int print_slot(struct cache_slot *slot)
 		return errno;
 	else
 		return 0;
+#endif
 }
 
 /* Check if the slot has expired */
@@ -188,6 +208,10 @@ static int fill_slot(struct cache_slot *slot)
 	/* Generate cache content */
 	slot->fn();
 
+	/* update stat info */
+	if (fstat(slot->lock_fd, &slot->cache_st))
+		return errno;
+
 	/* Restore stdout */
 	if (dup2(tmp, STDOUT_FILENO) == -1)
 		return errno;
diff --git a/cgit.mk b/cgit.mk
index 056c3f9..3b8b79a 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -68,6 +68,14 @@ ifeq ($(findstring BSD,$(uname_S)),)
 	CGIT_LIBS += -ldl
 endif
 
+# glibc 2.1+ offers sendfile which the most common C library on Linux
+ifeq ($(uname_S),Linux)
+	HAVE_LINUX_SENDFILE = YesPlease
+endif
+
+ifdef HAVE_LINUX_SENDFILE
+	CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
+endif
 
 CGIT_OBJ_NAMES += cgit.o
 CGIT_OBJ_NAMES += cache.o
-- 
1.8.5.2

[PATCH 3/4] summary: Add tag head line in the dowload section

[sebastian]

If the downloads are disabled one gets only ugly "commit sha1". With
downloads enabled you see the file name with different extensions a few
times.
This patches changes it a little. Instead of printing the hash number it
prints the first line of the tag i.e. the head line / commit subject if
available. With downloads enabled it prints additionally the extension
of the archive type (i.e. .tar, .tar.xz) next to it.

Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
---
 ui-refs.c |  5 +++--
 ui-tag.c  | 31 +++++++++++++++++++++++++++++++
 ui-tag.h  |  1 +
 3 files changed, 35 insertions(+), 2 deletions(-)

diff --git a/ui-refs.c b/ui-refs.c
index 147b665..e0fd120 100644
--- a/ui-refs.c
+++ b/ui-refs.c
@@ -10,6 +10,7 @@
 #include "ui-refs.h"
 #include "html.h"
 #include "ui-shared.h"
+#include "ui-tag.h"
 
 static int cmp_age(int age1, int age2)
 {
@@ -150,10 +151,10 @@ static int print_tag(struct refinfo *ref)
 	html("<tr><td>");
 	cgit_tag_link(name, NULL, NULL, ctx.qry.head, name);
 	html("</td><td>");
+	cgit_print_tag_subject(name);
+	html(" ");
 	if (ctx.repo->snapshots && (obj->type == OBJ_COMMIT))
 		print_tag_downloads(ctx.repo, name);
-	else
-		cgit_object_link(obj);
 	html("</td><td>");
 	if (info) {
 		if (info->tagger) {
diff --git a/ui-tag.c b/ui-tag.c
index c1d1738..ef1d320 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -39,6 +39,37 @@ static void print_download_links(char *revname)
 	html("</td></tr>");
 }
 
+void cgit_print_tag_subject(char *revname)
+{
+	unsigned char sha1[20];
+	struct object *obj;
+	struct taginfo *info;
+	struct tag *tag;
+	char *p;
+	size_t len;
+
+	if (get_sha1(fmt("refs/tags/%s", revname), sha1))
+		return;
+
+	obj = parse_object(sha1);
+	if (!obj)
+		return;
+
+	tag = lookup_tag(sha1);
+	if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag)))
+		return;
+
+	p = strchr(info->msg, '\n');
+	if (p)
+		*p = '\0';
+	len = strlen(info->msg);
+	if (len > 74)
+		info->msg[74] = '\0';
+
+	html_txt(info->msg);
+	free(info);
+}
+
 void cgit_print_tag(char *revname)
 {
 	struct strbuf fullref = STRBUF_INIT;
diff --git a/ui-tag.h b/ui-tag.h
index d295cdc..352e0fd 100644
--- a/ui-tag.h
+++ b/ui-tag.h
@@ -2,5 +2,6 @@
 #define UI_TAG_H
 
 extern void cgit_print_tag(char *revname);
+void cgit_print_tag_subject(char *revname);
 
 #endif /* UI_TAG_H */
-- 
1.8.5.2

[PATCH 4/4] repolist: add a git link on front page

[sebastian]

If links are enabled there will be an additinal "git" links next to
"summary log tree" _if_ the config has clone-prefix set or the
repository has clone_url set. In case the clone-prefix contains multiple
enties then only the first one will be considered.
The intention is to present the git:// link for a given repo early to the user.

Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
---
 ui-repolist.c |  1 +
 ui-shared.c   | 35 +++++++++++++++++++++++++++++++++++
 ui-shared.h   |  1 +
 3 files changed, 37 insertions(+)

diff --git a/ui-repolist.c b/ui-repolist.c
index 477a949..b6d56ed 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -323,6 +323,7 @@ void cgit_print_repolist()
 			cgit_log_link("log", NULL, "button", NULL, NULL, NULL,
 				      0, NULL, NULL, ctx.qry.showmsg);
 			cgit_tree_link("tree", NULL, "button", NULL, NULL, NULL);
+			cgit_git_link();
 			html("</td>");
 		}
 		html("</tr>\n");
diff --git a/ui-shared.c b/ui-shared.c
index 1ede2b0..5f80de9 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -284,6 +284,41 @@ void cgit_tree_link(const char *name, const char *title, const char *class,
 	reporevlink("tree", name, title, class, head, rev, path);
 }
 
+void cgit_git_link(void)
+{
+	char *url = NULL;
+
+	if (ctx.repo->clone_url)
+		url = strdup(ctx.repo->clone_url);
+	else if (ctx.cfg.clone_prefix) {
+		char *second;
+		char *p;
+		int len;
+
+		p = ctx.cfg.clone_prefix;
+		while (*p == ' ')
+			p++;
+		second = strchr(p, ' ');
+		if (second)
+			*second = '\0';
+		len = snprintf(NULL, 0, "%s/%s", p, ctx.repo->url);
+		if (len < 0)
+			return;
+		len++;
+		url = xmalloc(len);
+		snprintf(url, len, "%s/%s", p, ctx.repo->url);
+		if (second)
+			*second = ' ';
+	}
+	if (!url)
+		return;
+	html("<a class='button'");
+	html(" href='");
+	html_url_path(url);
+	html("'>git</a>");
+	free(url);
+}
+
 void cgit_plain_link(const char *name, const char *title, const char *class,
 		     const char *head, const char *rev, const char *path)
 {
diff --git a/ui-shared.h b/ui-shared.h
index 3e7a91b..2d5d9cd 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -21,6 +21,7 @@ extern void cgit_tag_link(const char *name, const char *title,
 extern void cgit_tree_link(const char *name, const char *title,
 			   const char *class, const char *head,
 			   const char *rev, const char *path);
+extern void cgit_git_link(void);
 extern void cgit_plain_link(const char *name, const char *title,
 			    const char *class, const char *head,
 			    const char *rev, const char *path);
-- 
1.8.5.2

[PATCH 2/4] cache: use sendfile() instead of a pair of read() + write()

[jamie.couture]

On Sat, Jan 18, 2014 at 09:24:58PM +0100, Sebastian Andrzej Siewior wrote:
> sendfile() does the same job and avoids to copy the content into userland
> and back. One has to define NO_SENDFILE in case the OS (kernel / libc)
> does not supported. It is disabled by default on non-linux environemnts.
> According to the glibc, sendfile64() was added in Linux 2.4 (so it has
> been there for a while) but after browsing over the mapage of FreeBSD's I
> noticed that the prototype is little different.
> 
> Signed-off-by: Sebastian Andrzej Siewior <sebastian at breakpoint.cc>
> ---
>  Makefile |  1 +
>  cache.c  | 26 +++++++++++++++++++++++++-
>  cgit.mk  |  8 ++++++++
>  3 files changed, 34 insertions(+), 1 deletion(-)
> 
> diff --git a/Makefile b/Makefile
> index 2dc92df..05b97d7 100644
> --- a/Makefile
> +++ b/Makefile
> @@ -29,6 +29,7 @@ DOC_PDF  = $(patsubst %.txt,%.pdf,$(MAN_TXT))
>  # j, z, t. (representing long long int, char, intmax_t, size_t, ptrdiff_t).
>  # some C compilers supported these specifiers prior to C99 as an extension.
>  #
> +# Define HAVE_LINUX_SENDFILE to use sendfile()
>  
>  #-include config.mak
>  
> diff --git a/cache.c b/cache.c
> index fcd461f..9e7eeb0 100644
> --- a/cache.c
> +++ b/cache.c
> @@ -13,6 +13,9 @@
>   *
>   */
>  
> +#ifdef HAVE_LINUX_SENDFILE
> +#include <sys/sendfile.h>
> +#endif
>  #include "cgit.h"
>  #include "cache.h"
>  #include "html.h"
> @@ -30,7 +33,6 @@ struct cache_slot {
>  	const char *lock_name;
>  	int match;
>  	struct stat cache_st;
> -	struct stat lock_st;

Hmm. It looks like struct stat lock_st member, introduced in
939d32fd, was never used anywhere in cgit.  Would it make sense to
separate this line in a differnt commit, explaining the unused
member should be removed?

Not to nit, but I'm wondering if the subtly of removing this from
the cache_slot struct is worth a seperate commit.  Arguably it
doesn't hurt the way it is now, but it did throw me.

>  	int bufsize;
>  	char buf[CACHE_BUFSIZE];
>  };
> @@ -81,6 +83,23 @@ static int close_slot(struct cache_slot *slot)
>  /* Print the content of the active cache slot (but skip the key). */
>  static int print_slot(struct cache_slot *slot)
>  {
> +#ifdef HAVE_LINUX_SENDFILE
> +	off_t start_off;
> +	int ret;
> +
> +	start_off = slot->keylen + 1;
> +
> +	do {
> +		ret = sendfile(STDOUT_FILENO, slot->cache_fd, &start_off,
> +				slot->cache_st.st_size - start_off);
> +		if (ret < 0) {
> +			if (errno == EAGAIN || errno == EINTR)
> +				continue;
> +			return errno;
> +		}
> +		return 0;
> +	} while (1);
> +#else
>  	ssize_t i, j;
>  
>  	i = lseek(slot->cache_fd, slot->keylen + 1, SEEK_SET);
> @@ -97,6 +116,7 @@ static int print_slot(struct cache_slot *slot)
>  		return errno;
>  	else
>  		return 0;
> +#endif
>  }
>  
>  /* Check if the slot has expired */
> @@ -188,6 +208,10 @@ static int fill_slot(struct cache_slot *slot)
>  	/* Generate cache content */
>  	slot->fn();
>  
> +	/* update stat info */
> +	if (fstat(slot->lock_fd, &slot->cache_st))
> +		return errno;
> +
>  	/* Restore stdout */
>  	if (dup2(tmp, STDOUT_FILENO) == -1)
>  		return errno;
> diff --git a/cgit.mk b/cgit.mk
> index 056c3f9..3b8b79a 100644
> --- a/cgit.mk
> +++ b/cgit.mk
> @@ -68,6 +68,14 @@ ifeq ($(findstring BSD,$(uname_S)),)
>  	CGIT_LIBS += -ldl
>  endif
>  
> +# glibc 2.1+ offers sendfile which the most common C library on Linux
> +ifeq ($(uname_S),Linux)
> +	HAVE_LINUX_SENDFILE = YesPlease
> +endif
> +
> +ifdef HAVE_LINUX_SENDFILE
> +	CGIT_CFLAGS += -DHAVE_LINUX_SENDFILE
> +endif
>  
>  CGIT_OBJ_NAMES += cgit.o
>  CGIT_OBJ_NAMES += cache.o
> -- 
> 1.8.5.2
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[PATCH 2/4] cache: use sendfile() instead of a pair of read() + write()

[Jason]

On Sat, Jan 18, 2014 at 10:32 PM, Jamie Couture <jamie.couture at gmail.com> wrote:
> Hmm. It looks like struct stat lock_st member, introduced in
> 939d32fd, was never used anywhere in cgit.  Would it make sense to
> separate this line in a differnt commit, explaining the unused
> member should be removed?

I noticed this too. In the future, it's nice to break these up. For
now, not such a huge deal.

[PATCH 2/4] cache: use sendfile() instead of a pair of read() + write()

[Jason]

Excellent, thanks Sebastian! I've merged this commit.

Would you mind sending another commit where you implement this for the
read() write() situation in authenticate_post() on
http://git.zx2c4.com/cgit/tree/cgit.c#n624 ? Still bounding it to
MAX_AUTHENTICATION_POST_BYTES, but not having to copy it back to
userspace?

[PATCH 3/4] summary: Add tag head line in the dowload section

[Jason]

On Sat, Jan 18, 2014 at 9:24 PM, Sebastian Andrzej Siewior
<sebastian at breakpoint.cc> wrote:
> If the downloads are disabled one gets only ugly "commit sha1". With
> downloads enabled you see the file name with different extensions a few
> times.
> This patches changes it a little. Instead of printing the hash number it
> prints the first line of the tag i.e. the head line / commit subject if
> available. With downloads enabled it prints additionally the extension
> of the archive type (i.e. .tar, .tar.xz) next to it.

This is in fact way better than what we have now. I was thinking about
this the other day as well. Perhaps instead of this, we should just
hide the Download column all together, when snapshots are disabled.

List -- what do you all prefer?

[PATCH 4/4] repolist: add a git link on front page

[Jason]

On Sat, Jan 18, 2014 at 9:25 PM, Sebastian Andrzej Siewior
<sebastian at breakpoint.cc> wrote:
>                         cgit_tree_link("tree", NULL, "button", NULL, NULL, NULL);
> +                       cgit_git_link();
> @@ -284,6 +284,41 @@ void cgit_tree_link(const char *name, const char *title, const char *class,
> +void cgit_git_link(void)

I like this, but it's probably best to refactor it to take the same
arguments as all the other cgit_*_link functions.

[PATCH 2/4] cache: use sendfile() instead of a pair of read() + write()

[Jason]

On Sun, Jan 19, 2014 at 3:13 PM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> Would you mind sending another commit where you implement this for the
> read() write() situation in authenticate_post() on
> http://git.zx2c4.com/cgit/tree/cgit.c#n624 ? Still bounding it to
> MAX_AUTHENTICATION_POST_BYTES, but not having to copy it back to
> userspace?


On second thought, don't. Since we're hooking write() here, it doesn't
make too much sense to do. Silly me.

[PATCH 4/4] repolist: add a git link on front page

[sebastian]

On 19.01.14, Jason A. Donenfeld wrote:
> On Sat, Jan 18, 2014 at 9:25 PM, Sebastian Andrzej Siewior
> <sebastian at breakpoint.cc> wrote:
> >                         cgit_tree_link("tree", NULL, "button", NULL, NULL, NULL);
> > +                       cgit_git_link();
> > @@ -284,6 +284,41 @@ void cgit_tree_link(const char *name, const char *title, const char *class,
> > +void cgit_git_link(void)
> 
> I like this, but it's probably best to refactor it to take the same
> arguments as all the other cgit_*_link functions.

A little hint please. I could change this to
|void cgit_git_link(const char *name, const char *class)
and update the caller.
Is it this what you mean? If you want me to reuse site_link() (and its
arguemnts) I would ne to replace site_url() somehow because it creates
script based arguements.

Sebastian

[PATCH 1/4] snapshots: Don't allow sneaked in snapshots requests

[sebastian]

On 18.01.14, Sebastian Andrzej Siewior wrote:
> If the snapshots are not enabled then the frontend won't show a link to it.
> The skilled user however may construct the URL on his own and the frontend
> will obey the request.
> This patch adds a check for this case so the requst won't be served.

Any comments on this one?

Sebastian

[PATCH 3/4] summary: Add tag head line in the dowload section

[cgit]

On Sun, 19 Jan 2014 at 15:17:02, Jason A. Donenfeld wrote:
> On Sat, Jan 18, 2014 at 9:24 PM, Sebastian Andrzej Siewior
> <sebastian at breakpoint.cc> wrote:
> > If the downloads are disabled one gets only ugly "commit sha1". With
> > downloads enabled you see the file name with different extensions a few
> > times.
> > This patches changes it a little. Instead of printing the hash number it
> > prints the first line of the tag i.e. the head line / commit subject if
> > available. With downloads enabled it prints additionally the extension
> > of the archive type (i.e. .tar, .tar.xz) next to it.
> 
> This is in fact way better than what we have now. I was thinking about
> this the other day as well. Perhaps instead of this, we should just
> hide the Download column all together, when snapshots are disabled.
> 
> List -- what do you all prefer?

Since the column header clearly says "Download", I think it is very
confusing to fill it with other information. I also don't see the point
in showing arbitrary information just to fill the gap. So +1 to removing
the column altogether when snapshots are disabled. The only thing I am
not sure about is whether dropping the column breaks the design since we
use that 4-column layout throughout the whole summary page.

[PATCH 1/4] snapshots: Don't allow sneaked in snapshots requests

[cgit]

On Sat, 01 Feb 2014 at 15:54:22, Sebastian Andrzej Siewior wrote:
> On 18.01.14, Sebastian Andrzej Siewior wrote:
> > If the snapshots are not enabled then the frontend won't show a link to it.
> > The skilled user however may construct the URL on his own and the frontend
> > will obey the request.
> > This patch adds a check for this case so the requst won't be served.
> 
> Any comments on this one?
> 

While I like this idea [1], I think that Jason is reluctant to add this
"fix" [2]. I am putting "fix" in quotes because being able to access
"disabled" snapshots is documented since commit 70546a3 (cgitrc.5.txt:
Fix documentation of the snapshot mask, 2014-01-13) [3].

> Sebastian
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> http://lists.zx2c4.com/mailman/listinfo/cgit

[1] http://lists.zx2c4.com/pipermail/cgit/2014-January/001692.html
[2] http://lists.zx2c4.com/pipermail/cgit/2012-October/000792.html
[2] http://git.zx2c4.com/cgit/commit/?id=70546a34583923a73da6fb89c2efb85801294dc1