[PATCH 0/3] Blame UI

[PATCH 0/3] Blame UI

[john]

This is an attempt at adding "blame" support to CGit.  Because git.git
doesn't include the blame implementation in libgit.a, I decided to
simply invoke git-blame(1) and parse its porcelain output.  I don't
think the result is too hideous (although I do perhaps need to extract
some helper functions from process_blame()) and given how expensive
blame is as an operation I can live with the overhead of an extra
process.

The UI is inspired by Gitweb's blame output (e.g. [1]).  I did want to
find a way to add a "blame parent" link, the implementation of which is
relatively straightforward, but I failed to invent a format I was happy
with (concise enough to keep the column narrow and also avoiding
confusion with Git's existing ref format).  You can still manage to
blame the parent by following links to the commit's parent and then
blaming the file there.


[1] http://repo.or.cz/w/cgit.git/blame/HEAD:/cgit.c

John Keeping (3):
  ui-blame: add blame UI
  ui-shared: add cgit_blame_link()
  ui-tree: generate blame links

 cgit.c       |   4 +-
 cgit.css     |   4 ++
 cgit.h       |   3 +
 cgit.mk      |   1 +
 cgitrc.5.txt |   4 ++
 cmd.c        |   7 +++
 ui-blame.c   | 202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h   |   6 ++
 ui-shared.c  |  40 ++++++++++++
 ui-shared.h  |   3 +
 ui-tree.c    |   9 +++
 11 files changed, 282 insertions(+), 1 deletion(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

-- 
2.5.0.466.g9af26fa

[PATCH 1/3] ui-blame: add blame UI

[john]

This is disabled by default and needs to be turned on in the config
file.

Because libgit.a does not include the blame implementation (which lives
in git/builtin/blame.c), this is implemented by executing a git-blame
subprocess and parsing its output.  Given how expensive the blame
operation itself is, the overhead of using a separate process should not
be noticeable.
---
 cgit.c       |   4 +-
 cgit.css     |   4 ++
 cgit.h       |   3 +
 cgit.mk      |   1 +
 cgitrc.5.txt |   4 ++
 cmd.c        |   7 +++
 ui-blame.c   | 202 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h   |   6 ++
 8 files changed, 230 insertions(+), 1 deletion(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

diff --git a/cgit.c b/cgit.c
index ae413c6..fe952d4 100644
--- a/cgit.c
+++ b/cgit.c
@@ -150,6 +150,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.noheader = atoi(value);
 	else if (!strcmp(name, "snapshots"))
 		ctx.cfg.snapshots = cgit_parse_snapshots_mask(value);
+	else if (!strcmp(name, "enable-blame"))
+		ctx.cfg.enable_blame = atoi(value);
 	else if (!strcmp(name, "enable-filter-overrides"))
 		ctx.cfg.enable_filter_overrides = atoi(value);
 	else if (!strcmp(name, "enable-http-clone"))
@@ -707,7 +709,7 @@ static void process_request(void)
 	}
 
 	cmd = cgit_get_cmd();
-	if (!cmd) {
+	if (!cmd || (!ctx.cfg.enable_blame && !strcmp(cmd->name, "blame"))) {
 		ctx.page.title = "cgit error";
 		ctx.page.status = 404;
 		ctx.page.statusmsg = "Not found";
diff --git a/cgit.css b/cgit.css
index 82c755c..39b8277 100644
--- a/cgit.css
+++ b/cgit.css
@@ -280,6 +280,10 @@ div#cgit table.blob td.lines {
 	color: black;
 }
 
+div#cgit table.blob td.noprevious {
+	font-weight: bold;
+}
+
 div#cgit table.blob td.linenumbers {
 	margin: 0; padding: 0 0.5em 0 0.5em;
 	vertical-align: top;
diff --git a/cgit.h b/cgit.h
index 16f8092..4347046 100644
--- a/cgit.h
+++ b/cgit.h
@@ -16,12 +16,14 @@
 #include <revision.h>
 #include <log-tree.h>
 #include <archive.h>
+#include <run-command.h>
 #include <string-list.h>
 #include <xdiff-interface.h>
 #include <xdiff/xdiff.h>
 #include <utf8.h>
 #include <notes.h>
 #include <graph.h>
+#include <quote.h>
 
 
 /*
@@ -220,6 +222,7 @@ struct cgit_config {
 	int cache_snapshot_ttl;
 	int case_sensitive_sort;
 	int embedded;
+	int enable_blame;
 	int enable_filter_overrides;
 	int enable_http_clone;
 	int enable_index_links;
diff --git a/cgit.mk b/cgit.mk
index 1b50307..c427229 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -75,6 +75,7 @@ CGIT_OBJ_NAMES += parsing.o
 CGIT_OBJ_NAMES += scan-tree.o
 CGIT_OBJ_NAMES += shared.o
 CGIT_OBJ_NAMES += ui-atom.o
+CGIT_OBJ_NAMES += ui-blame.o
 CGIT_OBJ_NAMES += ui-blob.o
 CGIT_OBJ_NAMES += ui-clone.o
 CGIT_OBJ_NAMES += ui-commit.o
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index e21ece9..a5a54ec 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -146,6 +146,10 @@ enable-commit-graph::
 	history graph to the left of the commit messages in the repository
 	log page. Default value: "0".
 
+enable-blame::
+	Flag which, when set to "1", enables the blame UI.  Default value:
+	"0".
+
 enable-filter-overrides::
 	Flag which, when set to "1", allows all filter settings to be
 	overridden in repository-specific cgitrc files. Default value: none.
diff --git a/cmd.c b/cmd.c
index 188cd56..ca27e8d 100644
--- a/cmd.c
+++ b/cmd.c
@@ -11,6 +11,7 @@
 #include "cache.h"
 #include "ui-shared.h"
 #include "ui-atom.h"
+#include "ui-blame.h"
 #include "ui-blob.h"
 #include "ui-clone.h"
 #include "ui-commit.h"
@@ -44,6 +45,11 @@ static void about_fn(void)
 		cgit_print_site_readme();
 }
 
+static void blame_fn(void)
+{
+	cgit_print_blame();
+}
+
 static void blob_fn(void)
 {
 	cgit_print_blob(ctx.qry.sha1, ctx.qry.path, ctx.qry.head, 0);
@@ -145,6 +151,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(HEAD, 1, 0, 0, 1),
 		def_cmd(atom, 1, 0, 0, 0),
 		def_cmd(about, 0, 1, 0, 0),
+		def_cmd(blame, 1, 1, 0, 0),
 		def_cmd(blob, 1, 0, 0, 0),
 		def_cmd(commit, 1, 1, 1, 0),
 		def_cmd(diff, 1, 1, 1, 0),
diff --git a/ui-blame.c b/ui-blame.c
new file mode 100644
index 0000000..34c836a
--- /dev/null
+++ b/ui-blame.c
@@ -0,0 +1,202 @@
+/* ui-blame.c: functions for showing blame output
+ *
+ * Copyright (C) 2015 cgit Development Team <cgit at lists.zx2c4.com>
+ *
+ * Licensed under GNU General Public License v2
+ *   (see COPYING for full license text)
+ */
+
+#include "cgit.h"
+#include "ui-blame.h"
+#include "html.h"
+#include "ui-shared.h"
+
+static void start_output(void)
+{
+	html("<table class='blob'>");
+}
+
+static void end_output(void)
+{
+	html("</table>");
+}
+
+static int parse_init_line(const char *line, unsigned char sha1[20],
+		unsigned long *orig_line, unsigned long *final_line,
+		unsigned long *count)
+{
+	char *end;
+	if (get_sha1_hex(line, sha1) < 0)
+		return -1;
+
+	*orig_line = strtoul(line + 41, &end, 10);
+	/* We already have a count, so there isn't one on this line. */
+	if (*count)
+		return 0;
+
+	end++;
+	*final_line = strtoul(end, &end, 10);
+
+	if (!*end)
+		return -1;
+	end++;
+	*count = strtoul(end, NULL, 10);
+
+	return 0;
+}
+
+static const char *author_abbrev(const char *author)
+{
+	static char buf[4];
+	size_t idx = 0;
+	const char *c = strrchr(author, ' ');
+	buf[idx++] = *author;
+	if (c && c != author)
+		buf[idx++] = c[1];
+	buf[idx] = '\0';
+	return buf;
+}
+
+static int process_blame(FILE *out)
+{
+	/*
+	 * The format is:
+	 *	<sha1> <orig-line> <cur-line> [<count>]
+	 *	<header> <value>
+	 *	...
+	 *	<TAB> <content>
+	 *
+	 * STATE_INIT means we expect a SHA1, otherwise it's a header or
+	 * content depending on whether or not the line starts with <TAB>.
+	 */
+	enum {
+		STATE_INIT,
+		STATE_HEADER,
+	} state = STATE_INIT;
+	int output_started = 0;
+	int first = 0;
+	int previous = 0;
+	unsigned long line = 1;
+	unsigned long orig_line = 0;
+	unsigned long final_line = 0;
+	unsigned long count = 0;
+	unsigned char sha1[20];
+	struct strbuf buf = STRBUF_INIT;
+	struct strbuf author = STRBUF_INIT;
+	time_t author_time = 0;
+	int author_tz;
+
+	while (strbuf_getline(&buf, out, '\n') != EOF) {
+		const char *value;
+		if (ferror(out))
+			break;
+
+		switch (state) {
+		case STATE_INIT:
+			first = !count;
+			previous = 0;
+
+			if (parse_init_line(buf.buf, sha1, &orig_line, &final_line, &count) < 0)
+				goto err;
+
+			if (!output_started) {
+				start_output();
+				output_started = 1;
+			}
+			html("<tr>");
+			state++;
+			break;
+
+		case STATE_HEADER:
+			if (buf.buf[0] == '\t') {
+				if (first) {
+					char date_buf[64];
+					strftime(date_buf, sizeof(date_buf) - 1, FMT_LONGDATE, gmtime(&author_time));
+					htmlf("<td rowspan='%lu' class='lines%s' title='",
+							count, previous ? "" : " noprevious");
+					html_attr(fmt("%s, %s", author.buf, date_buf));
+					htmlf("'><pre>");
+					cgit_commit_link(fmt("%s", find_unique_abbrev(sha1, DEFAULT_ABBREV)),
+						NULL, NULL, NULL, sha1_to_hex(sha1), ctx.qry.path);
+					if (count > 1) {
+						html("<br>");
+						html_txt(author_abbrev(author.buf));
+					}
+					html("</pre></td>");
+					first = 0;
+				}
+				htmlf("<td class='linenumbers'><a name='l%lu' href='#l%lu'>%lu</a></td><td class='lines'><pre>",
+						line, line, line);
+				html_txt(buf.buf + 1);
+				html("</pre></td></tr>");
+
+				line++;
+				count--;
+				state = STATE_INIT;
+			} else if (skip_prefix(buf.buf, "author ", &value)) {
+				strbuf_reset(&author);
+				strbuf_addstr(&author, value);
+			} else if (skip_prefix(buf.buf, "author-time ", &value)) {
+				author_time = (time_t) strtoul(value, NULL, 10);
+			} else if (skip_prefix(buf.buf, "author-tz ", &value)) {
+				strtol_i(value, 10, &author_tz);
+			} else if (skip_prefix(buf.buf, "previous ", &value)) {
+				previous++;
+			}
+			break;
+		}
+	}
+
+	return output_started;
+err:
+	return -1;
+}
+
+void cgit_print_blame(void)
+{
+	FILE *out;
+	int output_started = 0;
+	struct child_process proc = CHILD_PROCESS_INIT;
+
+	if (!ctx.qry.path) {
+		cgit_print_error("Bad request");
+		return;
+	}
+
+	argv_array_push(&proc.args, "blame");
+	argv_array_push(&proc.args, "--line-porcelain");
+	if (ctx.qry.ignorews)
+		argv_array_push(&proc.args, "-w");
+	argv_array_push(&proc.args, ctx.qry.sha1 ? ctx.qry.sha1 : ctx.qry.head);
+	argv_array_push(&proc.args, "--");
+	argv_array_push(&proc.args, ctx.qry.path);
+
+	proc.out = -1;
+	proc.no_stdin = 1;
+	proc.no_stderr = 1;
+	proc.git_cmd = 1;
+
+	if (start_command(&proc) < 0)
+		goto err;
+
+	out = fdopen(proc.out, "r");
+	if (!out)
+		goto err;
+
+	output_started = process_blame(out);
+
+	if (finish_command(&proc) != 0) {
+		if (!output_started) {
+			cgit_print_error("Not found");
+			return;
+		}
+		/* What to do? - we started getting data then something went wrong. */
+	}
+
+	if (output_started)
+		end_output();
+
+	return;
+err:
+	cgit_print_error("Internal server error");
+}
diff --git a/ui-blame.h b/ui-blame.h
new file mode 100644
index 0000000..5b97e03
--- /dev/null
+++ b/ui-blame.h
@@ -0,0 +1,6 @@
+#ifndef UI_BLAME_H
+#define UI_BLAME_H
+
+extern void cgit_print_blame(void);
+
+#endif /* UI_BLAME_H */
-- 
2.5.0.466.g9af26fa

[PATCH 2/3] ui-shared: add cgit_blame_link()

[john]

---
 ui-shared.c | 40 ++++++++++++++++++++++++++++++++++++++++
 ui-shared.h |  3 +++
 2 files changed, 43 insertions(+)

diff --git a/ui-shared.c b/ui-shared.c
index ac5a287..da9c3d0 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -338,6 +338,46 @@ void cgit_log_link(const char *name, const char *title, const char *class,
 	html("</a>");
 }
 
+void cgit_blame_link(char *name, const char *title, const char *class,
+		     const char *head, const char *rev, const char *path,
+		     long line)
+{
+	if (strlen(name) > ctx.cfg.max_msg_len && ctx.cfg.max_msg_len >= 15) {
+		name[ctx.cfg.max_msg_len] = '\0';
+		name[ctx.cfg.max_msg_len - 1] = '.';
+		name[ctx.cfg.max_msg_len - 2] = '.';
+		name[ctx.cfg.max_msg_len - 3] = '.';
+	}
+
+	char *delim;
+
+	delim = repolink(title, class, "blame", head, path);
+	if (rev && ctx.qry.head && strcmp(rev, ctx.qry.head)) {
+		html(delim);
+		html("id=");
+		html_url_arg(rev);
+		delim = "&amp;";
+	}
+	if (ctx.qry.difftype) {
+		html(delim);
+		htmlf("dt=%d", ctx.qry.difftype);
+		delim = "&amp;";
+	}
+	if (ctx.qry.ignorews) {
+		html(delim);
+		html("ignorews=1");
+		delim = "&amp;";
+	}
+	if (line >= 0)
+		htmlf("#l%ld", line);
+	html("'>");
+	if (name[0] != '\0')
+		html_txt(name);
+	else
+		html_txt("(no commit message)");
+	html("</a>");
+}
+
 void cgit_commit_link(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 1b8ecb5..9a406d0 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -30,6 +30,9 @@ extern void cgit_log_link(const char *name, const char *title,
 			  const char *class, const char *head, const char *rev,
 			  const char *path, int ofs, const char *grep,
 			  const char *pattern, int showmsg);
+extern void cgit_blame_link(char *name, const char *title,
+			    const char *class, const char *head,
+			    const char *rev, const char *path, long line);
 extern void cgit_commit_link(char *name, const char *title,
 			     const char *class, const char *head,
 			     const char *rev, const char *path);
-- 
2.5.0.466.g9af26fa

[PATCH 3/3] ui-tree: generate blame links

[john]

---
 ui-tree.c | 9 +++++++++
 1 file changed, 9 insertions(+)

diff --git a/ui-tree.c b/ui-tree.c
index bbc468e..add0344 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -106,6 +106,11 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
 	htmlf("blob: %s (", sha1_to_hex(sha1));
 	cgit_plain_link("plain", NULL, NULL, ctx.qry.head,
 		        rev, path);
+	if (ctx.cfg.enable_blame) {
+		html(" | ");
+		cgit_blame_link("blame", NULL, NULL, ctx.qry.head,
+				rev, path, -1);
+	}
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -173,6 +178,10 @@ static int ls_item(const unsigned char *sha1, struct strbuf *base,
 	if (!S_ISGITLINK(mode))
 		cgit_plain_link("plain", NULL, "button", ctx.qry.head,
 				walk_tree_ctx->curr_rev, fullpath.buf);
+	if (S_ISREG(mode) && ctx.cfg.enable_blame)
+		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev,
+				fullpath.buf, -1);
 	html("</td></tr>\n");
 	free(name);
 	strbuf_release(&fullpath);
-- 
2.5.0.466.g9af26fa

[PATCH 0/3] Blame UI

[Jason]

On Wed, Aug 12, 2015 at 3:03 PM, John Keeping <john at keeping.me.uk> wrote:
> This is an attempt at adding "blame" support to CGit.  Because git.git
> doesn't include the blame implementation in libgit.a, I decided to
> simply invoke git-blame(1) and parse its porcelain output.

Could you possibly just do:

#include "git/builtin/blame.c"

And then call the static functions?

[PATCH 0/3] Blame UI

[john]

On Wed, Aug 12, 2015 at 03:44:19PM +0200, Jason A. Donenfeld wrote:
> On Wed, Aug 12, 2015 at 3:03 PM, John Keeping <john at keeping.me.uk> wrote:
> > This is an attempt at adding "blame" support to CGit.  Because git.git
> > doesn't include the blame implementation in libgit.a, I decided to
> > simply invoke git-blame(1) and parse its porcelain output.
> 
> Could you possibly just do:
> 
> #include "git/builtin/blame.c"
> 
> And then call the static functions?

I was trying to stay to a well defined interface.  It looks like we'd
have to copy reasonable chunks of cmd_blame() and emit_other() in order
to drive the static functions.

I'll give it a go and see what it looks like, but I expect it to result
in more pain during future maintenance than using a separate process.

[PATCH 0/3] Blame UI

[Jason]

Relying on an installed git does impose an additional deployment
requirement we didn't have before. Previously git was just required
for running the tests (developers), but not for deployment.

I'll wait to receive some other opinions on the topic of relying on
having the executables around.

Alternatively - any chance of working with upstream to make it a more
generic interface?

[PATCH 0/3] Blame UI

[john]

On Wed, Aug 12, 2015 at 05:07:12PM +0200, Jason A. Donenfeld wrote:
> Relying on an installed git does impose an additional deployment
> requirement we didn't have before. Previously git was just required
> for running the tests (developers), but not for deployment.

I'd be surprised if most people don't already have Git on their system,
given that they certainly have Git repositories!  Although I suppose
it's possible to have CGit running in a container that can see the
repositories but doesn't have git available.

> I'll wait to receive some other opinions on the topic of relying on
> having the executables around.
> 
> Alternatively - any chance of working with upstream to make it a more
> generic interface?

There's enough code to work through and understand (including quite a
lot that isn't used in our scenario) that it's outside the scope of my
CGit budget at the moment.