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

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

[nobody]

On 10/28/2012 09:40 PM, Jason A. Donenfeld wrote:
> "Disabling snapshots" as a security "feature" isn't really so valid either.

With snapshots, few requests can easily create a tremendous amount of
system load, rendering the system unusable. (Imagine you have a kernel
repository and someone repeatedly requests a tar.xz for it)

In any case, your use-case seems valid as well.

Maybe one could have a setting for enabling/disabling snapshots
altogether and another setting for the snapshot formats which should be
listed? (we already have the latter)

-Christian

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

[Jason]

On Sun, Oct 28, 2012 at 2:18 PM, Sebastian Andrzej Siewior
<sebastian at breakpoint.cc> 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.

What's the purpose of this? I kind of like just having tar.xz and zip
enabled on mine, and then for folks who need tar.gz (like for bsd pkg
managers), they can have the other link. That way UI clutter is
minimized while the functionality stays in tact.

"Disabling snapshots" as a security "feature" isn't really so valid either.