List for cgit developers and users
 help / color / mirror / Atom feed
* [RFC PATCH 0/4] Add ui-blame
@ 2017-06-08  2:18 whydoubt
  2017-06-08  2:18 ` [RFC PATCH 1/4] git: update to v2.14 whydoubt
                   ` (6 more replies)
  0 siblings, 7 replies; 63+ messages in thread
From: whydoubt @ 2017-06-08  2:18 UTC (permalink / raw)


I split git blame functionality into libgit, and the changes were
accepted upstream a few days ago.  Now that the git infrastructure is in
place, it is time to get back to the cgit part.

The first patch advances git to current master (which should be future
v2.14), and makes changes made necessary by doing so.  The remaining
patches deal with adding the new blame functionality.

Based on branch ch/git-2-13-1.

Jeff Smith (4):
  git: update to v2.14
  ui-blame: create placeholder and links
  ui-blame: create needed html_ntxt_noellipsis function
  ui-blame: fill in the contents

 cgit.css      |   8 ++
 cgit.mk       |   1 +
 cmd.c         |   8 +-
 git           |   2 +-
 html.c        |  37 +++---
 html.h        |   1 +
 shared.c      |   2 +-
 ui-blame.c    | 370 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h    |   7 ++
 ui-blob.c     |   6 +-
 ui-clone.c    |   2 +-
 ui-commit.c   |   4 +-
 ui-diff.c     |   4 +-
 ui-patch.c    |   4 +-
 ui-plain.c    |   2 +-
 ui-shared.c   |  20 +++-
 ui-shared.h   |   3 +
 ui-snapshot.c |   2 +-
 ui-tag.c      |   4 +-
 ui-tree.c     |  26 +++--
 20 files changed, 462 insertions(+), 51 deletions(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

-- 
2.9.3



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 1/4] git: update to v2.14
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
@ 2017-06-08  2:18 ` whydoubt
  2017-07-22 11:25   ` john
  2017-06-08  2:18 ` [RFC PATCH 2/4] ui-blame: create placeholder and links whydoubt
                   ` (5 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-06-08  2:18 UTC (permalink / raw)


Update to git version v2.14: commit 6b526ce (Merge branch bc/object-id)
merged changes for several functions from using sha1 hashes to using
struct object_id pointers.  The functions that affect cgit are:
parse_object, lookup_commit_reference, lookup_tag, lookup_tree, and
parse_tree_indirect.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 git           |  2 +-
 shared.c      |  2 +-
 ui-blob.c     |  6 +++---
 ui-clone.c    |  2 +-
 ui-commit.c   |  4 ++--
 ui-diff.c     |  4 ++--
 ui-patch.c    |  4 ++--
 ui-plain.c    |  2 +-
 ui-snapshot.c |  2 +-
 ui-tag.c      |  4 ++--
 ui-tree.c     | 18 +++++++++---------
 11 files changed, 25 insertions(+), 25 deletions(-)

diff --git a/git b/git
index 2c04f63..8d1b103 160000
--- a/git
+++ b/git
@@ -1 +1 @@
-Subproject commit 2c04f6340579518c55a554fcac9fe21c01b3d3ea
+Subproject commit 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b
diff --git a/shared.c b/shared.c
index 13a65a9..c93b193 100644
--- a/shared.c
+++ b/shared.c
@@ -160,7 +160,7 @@ static struct refinfo *cgit_mk_refinfo(const char *refname, const struct object_
 
 	ref = xmalloc(sizeof (struct refinfo));
 	ref->refname = xstrdup(refname);
-	ref->object = parse_object(oid->hash);
+	ref->object = parse_object(oid);
 	switch (ref->object->type) {
 	case OBJ_TAG:
 		ref->tag = cgit_parse_tag((struct tag *)ref->object);
diff --git a/ui-blob.c b/ui-blob.c
index 793817f..761e886 100644
--- a/ui-blob.c
+++ b/ui-blob.c
@@ -56,7 +56,7 @@ int cgit_ref_path_exists(const char *path, const char *ref, int file_only)
 		goto done;
 	if (sha1_object_info(oid.hash, &size) != OBJ_COMMIT)
 		goto done;
-	read_tree_recursive(lookup_commit_reference(oid.hash)->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
+	read_tree_recursive(lookup_commit_reference(&oid)->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 
 done:
 	free(path_items.match);
@@ -89,7 +89,7 @@ int cgit_print_file(char *path, const char *head, int file_only)
 		return -1;
 	type = sha1_object_info(oid.hash, &size);
 	if (type == OBJ_COMMIT) {
-		commit = lookup_commit_reference(oid.hash);
+		commit = lookup_commit_reference(&oid);
 		read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 		if (!walk_tree_ctx.found_path)
 			return -1;
@@ -145,7 +145,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head, int file_onl
 	type = sha1_object_info(oid.hash, &size);
 
 	if ((!hex) && type == OBJ_COMMIT && path) {
-		commit = lookup_commit_reference(oid.hash);
+		commit = lookup_commit_reference(&oid);
 		read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
 		type = sha1_object_info(oid.hash, &size);
 	}
diff --git a/ui-clone.c b/ui-clone.c
index 5f6606a..0d11672 100644
--- a/ui-clone.c
+++ b/ui-clone.c
@@ -17,7 +17,7 @@ static int print_ref_info(const char *refname, const struct object_id *oid,
 {
 	struct object *obj;
 
-	if (!(obj = parse_object(oid->hash)))
+	if (!(obj = parse_object(oid)))
 		return 0;
 
 	htmlf("%s\t%s\n", oid_to_hex(oid), refname);
diff --git a/ui-commit.c b/ui-commit.c
index db69d54..e1d4a9b 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -31,7 +31,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 				"Bad object id: %s", hex);
 		return;
 	}
-	commit = lookup_commit_reference(oid.hash);
+	commit = lookup_commit_reference(&oid);
 	if (!commit) {
 		cgit_print_error_page(404, "Not found",
 				"Bad commit reference: %s", hex);
@@ -87,7 +87,7 @@ void cgit_print_commit(char *hex, const char *prefix)
 	free(tmp);
 	html("</td></tr>\n");
 	for (p = commit->parents; p; p = p->next) {
-		parent = lookup_commit_reference(p->item->object.oid.hash);
+		parent = lookup_commit_reference(&p->item->object.oid);
 		if (!parent) {
 			html("<tr><td colspan='3'>");
 			cgit_print_error("Error reading parent commit");
diff --git a/ui-diff.c b/ui-diff.c
index 173d351..3d40876 100644
--- a/ui-diff.c
+++ b/ui-diff.c
@@ -407,7 +407,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 			"Bad object name: %s", new_rev);
 		return;
 	}
-	commit = lookup_commit_reference(new_rev_oid->hash);
+	commit = lookup_commit_reference(new_rev_oid);
 	if (!commit || parse_commit(commit)) {
 		cgit_print_error_page(404, "Not found",
 			"Bad commit: %s", oid_to_hex(new_rev_oid));
@@ -428,7 +428,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
 	}
 
 	if (!is_null_oid(old_rev_oid)) {
-		commit2 = lookup_commit_reference(old_rev_oid->hash);
+		commit2 = lookup_commit_reference(old_rev_oid);
 		if (!commit2 || parse_commit(commit2)) {
 			cgit_print_error_page(404, "Not found",
 				"Bad commit: %s", oid_to_hex(old_rev_oid));
diff --git a/ui-patch.c b/ui-patch.c
index 047e2f9..69aa4a8 100644
--- a/ui-patch.c
+++ b/ui-patch.c
@@ -33,7 +33,7 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
 				"Bad object id: %s", new_rev);
 		return;
 	}
-	commit = lookup_commit_reference(new_rev_oid.hash);
+	commit = lookup_commit_reference(&new_rev_oid);
 	if (!commit) {
 		cgit_print_error_page(404, "Not found",
 				"Bad commit reference: %s", new_rev);
@@ -46,7 +46,7 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
 					"Bad object id: %s", old_rev);
 			return;
 		}
-		if (!lookup_commit_reference(old_rev_oid.hash)) {
+		if (!lookup_commit_reference(&old_rev_oid)) {
 			cgit_print_error_page(404, "Not found",
 					"Bad commit reference: %s", old_rev);
 			return;
diff --git a/ui-plain.c b/ui-plain.c
index 8d541e3..e45d553 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -185,7 +185,7 @@ void cgit_print_plain(void)
 		cgit_print_error_page(404, "Not found", "Not found");
 		return;
 	}
-	commit = lookup_commit_reference(oid.hash);
+	commit = lookup_commit_reference(&oid);
 	if (!commit || parse_commit(commit)) {
 		cgit_print_error_page(404, "Not found", "Not found");
 		return;
diff --git a/ui-snapshot.c b/ui-snapshot.c
index 9b8cddd..b2d95f7 100644
--- a/ui-snapshot.c
+++ b/ui-snapshot.c
@@ -116,7 +116,7 @@ static int make_snapshot(const struct cgit_snapshot_format *format,
 				"Bad object id: %s", hex);
 		return 1;
 	}
-	if (!lookup_commit_reference(oid.hash)) {
+	if (!lookup_commit_reference(&oid)) {
 		cgit_print_error_page(400, "Bad request",
 				"Not a commit reference: %s", hex);
 		return 1;
diff --git a/ui-tag.c b/ui-tag.c
index afd7d61..909cde0 100644
--- a/ui-tag.c
+++ b/ui-tag.c
@@ -54,7 +54,7 @@ void cgit_print_tag(char *revname)
 			"Bad tag reference: %s", revname);
 		goto cleanup;
 	}
-	obj = parse_object(oid.hash);
+	obj = parse_object(&oid);
 	if (!obj) {
 		cgit_print_error_page(500, "Internal server error",
 			"Bad object id: %s", oid_to_hex(&oid));
@@ -64,7 +64,7 @@ void cgit_print_tag(char *revname)
 		struct tag *tag;
 		struct taginfo *info;
 
-		tag = lookup_tag(oid.hash);
+		tag = lookup_tag(&oid);
 		if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) {
 			cgit_print_error_page(500, "Internal server error",
 				"Bad tag object: %s", revname);
diff --git a/ui-tree.c b/ui-tree.c
index b310242..ca24a03 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -157,7 +157,7 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
 
 struct single_tree_ctx {
 	struct strbuf *path;
-	unsigned char sha1[GIT_SHA1_RAWSZ];
+	struct object_id oid;
 	char *name;
 	size_t count;
 };
@@ -177,7 +177,7 @@ static int single_tree_cb(const unsigned char *sha1, struct strbuf *base,
 	}
 
 	ctx->name = xstrdup(pathname);
-	hashcpy(ctx->sha1, sha1);
+	hashcpy(ctx->oid.hash, sha1);
 	strbuf_addf(ctx->path, "/%s", pathname);
 	return 0;
 }
@@ -195,13 +195,13 @@ static void write_tree_link(const unsigned char *sha1, char *name,
 		.nr = 0
 	};
 
-	hashcpy(tree_ctx.sha1, sha1);
+	hashcpy(tree_ctx.oid.hash, sha1);
 
 	while (tree_ctx.count == 1) {
 		cgit_tree_link(name, NULL, "ls-dir", ctx.qry.head, rev,
 			       fullpath->buf);
 
-		tree = lookup_tree(tree_ctx.sha1);
+		tree = lookup_tree(&tree_ctx.oid);
 		if (!tree)
 			return;
 
@@ -300,17 +300,17 @@ static void ls_tail(void)
 	cgit_print_layout_end();
 }
 
-static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
+static void ls_tree(const struct object_id *oid, char *path, struct walk_tree_context *walk_tree_ctx)
 {
 	struct tree *tree;
 	struct pathspec paths = {
 		.nr = 0
 	};
 
-	tree = parse_tree_indirect(sha1);
+	tree = parse_tree_indirect(oid);
 	if (!tree) {
 		cgit_print_error_page(404, "Not found",
-			"Not a tree object: %s", sha1_to_hex(sha1));
+			"Not a tree object: %s", sha1_to_hex(oid->hash));
 		return;
 	}
 
@@ -380,7 +380,7 @@ void cgit_print_tree(const char *rev, char *path)
 			"Invalid revision name: %s", rev);
 		return;
 	}
-	commit = lookup_commit_reference(oid.hash);
+	commit = lookup_commit_reference(&oid);
 	if (!commit || parse_commit(commit)) {
 		cgit_print_error_page(404, "Not found",
 			"Invalid commit reference: %s", rev);
@@ -390,7 +390,7 @@ void cgit_print_tree(const char *rev, char *path)
 	walk_tree_ctx.curr_rev = xstrdup(rev);
 
 	if (path == NULL) {
-		ls_tree(commit->tree->object.oid.hash, NULL, &walk_tree_ctx);
+		ls_tree(&commit->tree->object.oid, NULL, &walk_tree_ctx);
 		goto cleanup;
 	}
 
-- 
2.9.3



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 2/4] ui-blame: create placeholder and links
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
  2017-06-08  2:18 ` [RFC PATCH 1/4] git: update to v2.14 whydoubt
@ 2017-06-08  2:18 ` whydoubt
  2017-07-22 11:31   ` john
  2017-06-08  2:18 ` [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function whydoubt
                   ` (4 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-06-08  2:18 UTC (permalink / raw)


Create a placeholder for and links to a page that will contain the
'blame' for a file in the repository.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.mk     |   1 +
 cmd.c       |   8 ++-
 ui-blame.c  | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h  |   7 +++
 ui-shared.c |  20 ++++++--
 ui-shared.h |   3 ++
 ui-tree.c   |   8 ++-
 7 files changed, 202 insertions(+), 5 deletions(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

diff --git a/cgit.mk b/cgit.mk
index 90a2346..3fcc1ca 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -77,6 +77,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/cmd.c b/cmd.c
index d280e95..262e31f 100644
--- a/cmd.c
+++ b/cmd.c
@@ -1,6 +1,6 @@
 /* cmd.c: the cgit command dispatcher
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -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"
@@ -63,6 +64,10 @@ 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);
@@ -164,6 +169,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(HEAD, 1, 0, 1),
 		def_cmd(atom, 1, 0, 0),
 		def_cmd(about, 0, 0, 0),
+		def_cmd(blame, 1, 1, 0),
 		def_cmd(blob, 1, 0, 0),
 		def_cmd(commit, 1, 1, 0),
 		def_cmd(diff, 1, 1, 0),
diff --git a/ui-blame.c b/ui-blame.c
new file mode 100644
index 0000000..901ca89
--- /dev/null
+++ b/ui-blame.c
@@ -0,0 +1,160 @@
+/* ui-blame.c: functions for blame output
+ *
+ * Copyright (C) 2006-2017 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"
+
+struct walk_tree_context {
+	char *curr_rev;
+	int match_baselen;
+	int state;
+};
+
+static void set_title_from_path(const char *path)
+{
+	size_t path_len, path_index, path_last_end;
+	char *new_title;
+
+	if (!path)
+		return;
+
+	path_len = strlen(path);
+	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
+	new_title[0] = '\0';
+
+	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
+		if (path[path_index] == '/') {
+			if (path_index == path_len - 1) {
+				path_last_end = path_index - 1;
+				continue;
+			}
+			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
+			strcat(new_title, "\\");
+			path_last_end = path_index;
+		}
+	}
+	if (path_last_end)
+		strncat(new_title, path, path_last_end);
+
+	strcat(new_title, " - ");
+	strcat(new_title, ctx.page.title);
+	ctx.page.title = new_title;
+}
+static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
+{
+	enum object_type type;
+	unsigned long size;
+
+	type = sha1_object_info(sha1, &size);
+	if (type == OBJ_BAD) {
+		cgit_print_error_page(404, "Not found", "Bad object name: %s",
+				      sha1_to_hex(sha1));
+		return;
+	}
+
+	/* Read in applicable data here */
+
+	set_title_from_path(path);
+
+	cgit_print_layout_start();
+	htmlf("blob: %s (", sha1_to_hex(sha1));
+	cgit_plain_link("plain", NULL, NULL, ctx.qry.head, rev, path);
+	html(") (");
+	cgit_tree_link("tree", NULL, NULL, ctx.qry.head, rev, path);
+	html(")\n");
+
+	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
+		htmlf("<div class='error'>blob size (%ldKB) exceeds display size limit (%dKB).</div>",
+		      size / 1024, ctx.cfg.max_blob_size);
+		return;
+	}
+
+	/* Output data here */
+
+	cgit_print_layout_end();
+	return;
+}
+
+static int walk_tree(const unsigned char *sha1, struct strbuf *base,
+		     const char *pathname, unsigned mode, int stage, void *cbdata)
+{
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (base->len == walk_tree_ctx->match_baselen) {
+		if (S_ISREG(mode)) {
+			struct strbuf buffer = STRBUF_INIT;
+			strbuf_addbuf(&buffer, base);
+			strbuf_addstr(&buffer, pathname);
+			print_object(sha1, buffer.buf, pathname, walk_tree_ctx->curr_rev);
+			strbuf_release(&buffer);
+			walk_tree_ctx->state = 1;
+		} else if (S_ISDIR(mode)) {
+			walk_tree_ctx->state = 2;
+		}
+	} else if (base->len < INT_MAX && (int)base->len > walk_tree_ctx->match_baselen) {
+		walk_tree_ctx->state = 2;
+	} else if (S_ISDIR(mode)) {
+		return READ_TREE_RECURSIVE;
+	}
+	return 0;
+}
+
+static int basedir_len(const char *path)
+{
+	char *p = strrchr(path, '/');
+	if (p)
+		return p - path + 1;
+	return 0;
+}
+
+void cgit_print_blame(void)
+{
+	const char *rev = ctx.qry.sha1;
+	struct object_id oid;
+	struct commit *commit;
+	struct pathspec_item path_items = {
+		.match = ctx.qry.path,
+		.len = ctx.qry.path ? strlen(ctx.qry.path) : 0
+	};
+	struct pathspec paths = {
+		.nr = 1,
+		.items = &path_items
+	};
+	struct walk_tree_context walk_tree_ctx = {
+		.state = 0
+	};
+
+	if (!rev)
+		rev = ctx.qry.head;
+
+	if (get_oid(rev, &oid)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
+		return;
+	}
+	commit = lookup_commit_reference(&oid);
+	if (!commit || parse_commit(commit)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
+		return;
+	}
+
+	walk_tree_ctx.curr_rev = xstrdup(rev);
+	walk_tree_ctx.match_baselen = (path_items.match) ?
+				       basedir_len(path_items.match) : -1;
+
+	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
+	if (!walk_tree_ctx.state)
+		cgit_print_error_page(404, "Not found", "Not found");
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_error_page(404, "No blame for folders", "Blame is not available for folders.");
+
+	free(walk_tree_ctx.curr_rev);
+}
diff --git a/ui-blame.h b/ui-blame.h
new file mode 100644
index 0000000..8a438e9
--- /dev/null
+++ b/ui-blame.h
@@ -0,0 +1,7 @@
+#ifndef UI_BLAME_H
+#define UI_BLAME_H
+
+extern void cgit_print_blame(void);
+
+#endif /* UI_BLAME_H */
+
diff --git a/ui-shared.c b/ui-shared.c
index 2e4fcd9..9b90d62 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1,6 +1,6 @@
 /* ui-shared.c: common web output functions
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -304,6 +304,12 @@ void cgit_plain_link(const char *name, const char *title, const char *class,
 	reporevlink("plain", name, title, class, head, rev, path);
 }
 
+void cgit_blame_link(const char *name, const char *title, const char *class,
+		     const char *head, const char *rev, const char *path)
+{
+	reporevlink("blame", name, title, class, head, rev, path);
+}
+
 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,
@@ -481,6 +487,10 @@ static void cgit_self_link(char *name, const char *title, const char *class)
 		cgit_plain_link(name, title, class, ctx.qry.head,
 				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
 				ctx.qry.path);
+	else if (!strcmp(ctx.qry.page, "blame"))
+		cgit_blame_link(name, title, class, ctx.qry.head,
+				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
+				ctx.qry.path);
 	else if (!strcmp(ctx.qry.page, "log"))
 		cgit_log_link(name, title, class, ctx.qry.head,
 			      ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
@@ -986,8 +996,12 @@ void cgit_print_pageheader(void)
 		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
 			      NULL, ctx.qry.vpath, 0, NULL, NULL,
 			      ctx.qry.showmsg, ctx.qry.follow);
-		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
-			       ctx.qry.sha1, ctx.qry.vpath);
+		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
+			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
+				        ctx.qry.sha1, ctx.qry.vpath);
+		else
+			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
+				       ctx.qry.sha1, ctx.qry.vpath);
 		cgit_commit_link("commit", NULL, hc("commit"),
 				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
 		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
diff --git a/ui-shared.h b/ui-shared.h
index 87799f1..e5992bc 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
 extern void cgit_plain_link(const char *name, const char *title,
 			    const char *class, const char *head,
 			    const char *rev, const char *path);
+extern void cgit_blame_link(const char *name, const char *title,
+			    const char *class, const char *head,
+			    const char *rev, const char *path);
 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,
diff --git a/ui-tree.c b/ui-tree.c
index ca24a03..6e432d7 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -1,6 +1,6 @@
 /* ui-tree.c: functions for tree output
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -141,6 +141,9 @@ 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);
+	html(") (");
+	cgit_blame_link("blame", NULL, NULL, ctx.qry.head,
+		        rev, path);
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -275,6 +278,9 @@ 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_ISDIR(mode))
+		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev, fullpath.buf);
 	html("</td></tr>\n");
 	free(name);
 	strbuf_release(&fullpath);
-- 
2.9.3



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
  2017-06-08  2:18 ` [RFC PATCH 1/4] git: update to v2.14 whydoubt
  2017-06-08  2:18 ` [RFC PATCH 2/4] ui-blame: create placeholder and links whydoubt
@ 2017-06-08  2:18 ` whydoubt
  2017-07-22 11:36   ` john
  2017-06-08  2:18 ` [RFC PATCH 4/4] ui-blame: fill in the contents whydoubt
                   ` (3 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-06-08  2:18 UTC (permalink / raw)


For implementing a ui-blame page, there is need for a function that
outputs a selection from a block of text, transformed for HTML output,
but with no further modifications or additions.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 html.c | 37 ++++++++++++++++---------------------
 html.h |  1 +
 2 files changed, 17 insertions(+), 21 deletions(-)

diff --git a/html.c b/html.c
index e7e6e07..c7d1c64 100644
--- a/html.c
+++ b/html.c
@@ -122,10 +122,10 @@ void html_vtxtf(const char *format, va_list ap)
 	strbuf_release(&buf);
 }
 
-void html_txt(const char *txt)
+static int _html_txt(int len, const char *txt)
 {
 	const char *t = txt;
-	while (t && *t) {
+	while (t && *t && len--) {
 		int c = *t;
 		if (c == '<' || c == '>' || c == '&') {
 			html_raw(txt, t - txt);
@@ -140,32 +140,27 @@ void html_txt(const char *txt)
 		t++;
 	}
 	if (t != txt)
-		html(txt);
+		html_raw(txt, t - txt);
+	return len;
+}
+
+void html_txt(const char *txt)
+{
+	if (txt)
+		_html_txt(strlen(txt), txt);
 }
 
 void html_ntxt(int len, const char *txt)
 {
-	const char *t = txt;
-	while (t && *t && len--) {
-		int c = *t;
-		if (c == '<' || c == '>' || c == '&') {
-			html_raw(txt, t - txt);
-			if (c == '>')
-				html("&gt;");
-			else if (c == '<')
-				html("&lt;");
-			else if (c == '&')
-				html("&amp;");
-			txt = t + 1;
-		}
-		t++;
-	}
-	if (t != txt)
-		html_raw(txt, t - txt);
-	if (len < 0)
+	if (_html_txt(len, txt) < 0)
 		html("...");
 }
 
+void html_ntxt_noellipsis(int len, const char *txt)
+{
+	_html_txt(len, txt);
+}
+
 void html_attrf(const char *fmt, ...)
 {
 	va_list ap;
diff --git a/html.h b/html.h
index 1b24e55..e1b85b3 100644
--- a/html.h
+++ b/html.h
@@ -20,6 +20,7 @@ extern void html_attrf(const char *format,...);
 
 extern void html_txt(const char *txt);
 extern void html_ntxt(int len, const char *txt);
+extern void html_ntxt_noellipsis(int len, const char *txt);
 extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
 extern void html_url_arg(const char *txt);
-- 
2.9.3



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 4/4] ui-blame: fill in the contents
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
                   ` (2 preceding siblings ...)
  2017-06-08  2:18 ` [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function whydoubt
@ 2017-06-08  2:18 ` whydoubt
  2017-07-05  8:32   ` list
  2017-07-22 11:47   ` john
  2017-06-08  9:00 ` [RFC PATCH 0/4] Add ui-blame list
                   ` (2 subsequent siblings)
  6 siblings, 2 replies; 63+ messages in thread
From: whydoubt @ 2017-06-08  2:18 UTC (permalink / raw)


Use the blame interface added in libgit to output the blame information
of a file in the repository.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.css   |   8 +++
 ui-blame.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 220 insertions(+), 2 deletions(-)

diff --git a/cgit.css b/cgit.css
index 1dc2c11..258991c 100644
--- a/cgit.css
+++ b/cgit.css
@@ -329,6 +329,14 @@ div#cgit table.ssdiff td.lineno a:hover {
 	color: black;
 }
 
+div#cgit table.blame tr:nth-child(even) {
+	background: #f7f0e7;
+}
+
+div#cgit table.blame tr:nth-child(odd) {
+	background: white;
+}
+
 div#cgit table.bin-blob {
 	margin-top: 0.5em;
 	border: solid 1px black;
diff --git a/ui-blame.c b/ui-blame.c
index 901ca89..6ad0009 100644
--- a/ui-blame.c
+++ b/ui-blame.c
@@ -10,6 +10,187 @@
 #include "ui-blame.h"
 #include "html.h"
 #include "ui-shared.h"
+#include "argv-array.h"
+#include "blame.h"
+
+
+/* Remember to update object flag allocation in object.h */
+#define MORE_THAN_ONE_PATH	(1u<<13)
+
+/*
+ * Information on commits, used for output.
+ */
+struct commit_info {
+	struct strbuf author;
+	struct strbuf author_mail;
+	struct ident_split author_ident;
+
+	/* filled only when asked for details */
+	struct strbuf committer;
+	struct strbuf committer_mail;
+	struct ident_split committer_ident;
+
+	struct strbuf summary;
+};
+
+/*
+ * Parse author/committer line in the commit object buffer
+ */
+static void get_ac_line(const char *inbuf, const char *what,
+			struct strbuf *name, struct strbuf *mail,
+			struct ident_split *ident)
+{
+	size_t len, maillen, namelen;
+	char *tmp, *endp;
+	const char *namebuf, *mailbuf;
+
+	tmp = strstr(inbuf, what);
+	if (!tmp)
+		goto error_out;
+	tmp += strlen(what);
+	endp = strchr(tmp, '\n');
+	if (!endp)
+		len = strlen(tmp);
+	else
+		len = endp - tmp;
+
+	if (split_ident_line(ident, tmp, len)) {
+	error_out:
+		/* Ugh */
+		tmp = "(unknown)";
+		strbuf_addstr(name, tmp);
+		strbuf_addstr(mail, tmp);
+		return;
+	}
+
+	namelen = ident->name_end - ident->name_begin;
+	namebuf = ident->name_begin;
+
+	maillen = ident->mail_end - ident->mail_begin;
+	mailbuf = ident->mail_begin;
+
+	strbuf_addf(mail, "<%.*s>", (int)maillen, mailbuf);
+	strbuf_add(name, namebuf, namelen);
+}
+
+static void commit_info_init(struct commit_info *ci)
+{
+
+	strbuf_init(&ci->author, 0);
+	strbuf_init(&ci->author_mail, 0);
+	strbuf_init(&ci->committer, 0);
+	strbuf_init(&ci->committer_mail, 0);
+	strbuf_init(&ci->summary, 0);
+}
+
+static void commit_info_destroy(struct commit_info *ci)
+{
+
+	strbuf_release(&ci->author);
+	strbuf_release(&ci->author_mail);
+	strbuf_release(&ci->committer);
+	strbuf_release(&ci->committer_mail);
+	strbuf_release(&ci->summary);
+}
+
+static void get_commit_info(struct commit *commit,
+			    struct commit_info *ret,
+			    int detailed)
+{
+	int len;
+	const char *subject, *encoding;
+	const char *message;
+
+	commit_info_init(ret);
+
+	encoding = get_log_output_encoding();
+	message = logmsg_reencode(commit, NULL, encoding);
+	get_ac_line(message, "\nauthor ",
+		    &ret->author, &ret->author_mail,
+		    &ret->author_ident);
+
+	if (!detailed) {
+		unuse_commit_buffer(commit, message);
+		return;
+	}
+
+	get_ac_line(message, "\ncommitter ",
+		    &ret->committer, &ret->committer_mail,
+		    &ret->committer_ident);
+
+	len = find_commit_subject(message, &subject);
+	if (len)
+		strbuf_add(&ret->summary, subject, len);
+	else
+		strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
+
+	unuse_commit_buffer(commit, message);
+}
+
+static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
+{
+	struct commit_info ci;
+	struct strbuf detail = STRBUF_INIT;
+
+	get_commit_info(suspect->commit, &ci, 1);
+
+	strbuf_addf(&detail, "author  %s", ci.author.buf);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", ci.author_mail.buf);
+	strbuf_addf(&detail, "  %s\n",
+		    show_ident_date(&ci.author_ident,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addf(&detail, "committer  %s", ci.committer.buf);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", ci.committer_mail.buf);
+	strbuf_addf(&detail, "  %s\n",
+		    show_ident_date(&ci.committer_ident,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addf(&detail, "commit  %s\n", hex);
+	strbuf_addbuf(&detail, &ci.summary);
+
+	commit_info_destroy(&ci);
+	return strbuf_detach(&detail, NULL);
+}
+
+static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
+{
+	struct blame_origin *suspect = ent->suspect;
+	char hex[GIT_SHA1_HEXSZ + 1];
+	char *detail, *abbrev;
+	unsigned long lineno;
+	const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
+	const char *cp, *cpend;
+
+	oid_to_hex_r(hex, &suspect->commit->object.oid);
+	detail = emit_one_suspect_detail(suspect, hex);
+	abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
+					    DEFAULT_ABBREV));
+
+	html("<tr><td class='lines'>");
+	cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
+	html("</td>\n");
+
+	free(detail);
+	free(abbrev);
+
+	if (ctx.cfg.enable_tree_linenumbers) {
+		html("<td class='linenumbers'><pre>");
+		lineno = ent->lno;
+		while (lineno < ent->lno + ent->num_lines)
+			htmlf(numberfmt, ++lineno);
+		html("</pre></td>\n");
+	}
+
+	cp = blame_nth_line(sb, ent->lno);
+	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
+
+	html("<td class='lines'><pre><code>");
+	html_ntxt_noellipsis(cpend - cp, cp);
+	html("</code></pre></td></tr>\n");
+}
 
 struct walk_tree_context {
 	char *curr_rev;
@@ -47,10 +228,16 @@ static void set_title_from_path(const char *path)
 	strcat(new_title, ctx.page.title);
 	ctx.page.title = new_title;
 }
+
 static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
 {
 	enum object_type type;
 	unsigned long size;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+	struct rev_info revs;
+	struct blame_scoreboard sb;
+	struct blame_origin *o;
+	struct blame_entry *ent = NULL;
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
@@ -59,7 +246,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
 		return;
 	}
 
-	/* Read in applicable data here */
+	argv_array_push(&rev_argv, "blame");
+	argv_array_push(&rev_argv, rev);
+	init_revisions(&revs, NULL);
+	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
+	setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
+	init_scoreboard(&sb);
+	sb.revs = &revs;
+	setup_scoreboard(&sb, path, &o);
+	o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
+	prio_queue_put(&sb.commits, o->commit);
+	blame_origin_decref(o);
+	sb.ent = NULL;
+	sb.path = path;
+	assign_blame(&sb, 0);
+	blame_sort_final(&sb);
+	blame_coalesce(&sb);
 
 	set_title_from_path(path);
 
@@ -76,7 +278,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
 		return;
 	}
 
-	/* Output data here */
+	html("<table class='blame blob'>");
+	for (ent = sb.ent; ent; ) {
+		struct blame_entry *e = ent->next;
+		emit_blame_entry(&sb, ent);
+		free(ent);
+		ent = e;
+	}
+	html("</table>\n");
+	free((void *)sb.final_buf);
 
 	cgit_print_layout_end();
 	return;
-- 
2.9.3



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
                   ` (3 preceding siblings ...)
  2017-06-08  2:18 ` [RFC PATCH 4/4] ui-blame: fill in the contents whydoubt
@ 2017-06-08  9:00 ` list
  2017-07-22 12:02 ` john
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
  6 siblings, 0 replies; 63+ messages in thread
From: list @ 2017-06-08  9:00 UTC (permalink / raw)


Jeff Smith <whydoubt at gmail.com> on Wed, 2017/06/07 21:18:
> I split git blame functionality into libgit, and the changes were
> accepted upstream a few days ago.  Now that the git infrastructure is in
> place, it is time to get back to the cgit part.
> 
> The first patch advances git to current master (which should be future
> v2.14), and makes changes made necessary by doing so.  The remaining
> patches deal with adding the new blame functionality.
> 
> Based on branch ch/git-2-13-1.

I pushed this to ch/ui-blame.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170608/75cc07ee/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 4/4] ui-blame: fill in the contents
  2017-06-08  2:18 ` [RFC PATCH 4/4] ui-blame: fill in the contents whydoubt
@ 2017-07-05  8:32   ` list
  2017-07-22 11:47   ` john
  1 sibling, 0 replies; 63+ messages in thread
From: list @ 2017-07-05  8:32 UTC (permalink / raw)


Jeff Smith <whydoubt at gmail.com> on Wed, 2017/06/07 21:18:
> +div#cgit table.blame tr:nth-child(even) {
> +	background: #f7f0e7;
> +}

We do not use this color for anything, no? Any reason not to use a color that
is used elsewhere, for example #f7f7f7?
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170705/36db9be1/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 1/4] git: update to v2.14
  2017-06-08  2:18 ` [RFC PATCH 1/4] git: update to v2.14 whydoubt
@ 2017-07-22 11:25   ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-07-22 11:25 UTC (permalink / raw)


On Wed, Jun 07, 2017 at 09:18:07PM -0500, Jeff Smith wrote:
> Update to git version v2.14: commit 6b526ce (Merge branch bc/object-id)
> merged changes for several functions from using sha1 hashes to using
> struct object_id pointers.  The functions that affect cgit are:
> parse_object, lookup_commit_reference, lookup_tag, lookup_tree, and
> parse_tree_indirect.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>

Of course, our tests fail with this patch because the submodule has been
updated without the Makefile... other than that, the changes all look
reasonable, so when 2.14 final is out and we can update the Makefile to
a tag:

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  git           |  2 +-
>  shared.c      |  2 +-
>  ui-blob.c     |  6 +++---
>  ui-clone.c    |  2 +-
>  ui-commit.c   |  4 ++--
>  ui-diff.c     |  4 ++--
>  ui-patch.c    |  4 ++--
>  ui-plain.c    |  2 +-
>  ui-snapshot.c |  2 +-
>  ui-tag.c      |  4 ++--
>  ui-tree.c     | 18 +++++++++---------
>  11 files changed, 25 insertions(+), 25 deletions(-)
> 
> diff --git a/git b/git
> index 2c04f63..8d1b103 160000
> --- a/git
> +++ b/git
> @@ -1 +1 @@
> -Subproject commit 2c04f6340579518c55a554fcac9fe21c01b3d3ea
> +Subproject commit 8d1b10321b20bd2a73a5b561cfc3cf2e8051b70b
> diff --git a/shared.c b/shared.c
> index 13a65a9..c93b193 100644
> --- a/shared.c
> +++ b/shared.c
> @@ -160,7 +160,7 @@ static struct refinfo *cgit_mk_refinfo(const char *refname, const struct object_
>  
>  	ref = xmalloc(sizeof (struct refinfo));
>  	ref->refname = xstrdup(refname);
> -	ref->object = parse_object(oid->hash);
> +	ref->object = parse_object(oid);
>  	switch (ref->object->type) {
>  	case OBJ_TAG:
>  		ref->tag = cgit_parse_tag((struct tag *)ref->object);
> diff --git a/ui-blob.c b/ui-blob.c
> index 793817f..761e886 100644
> --- a/ui-blob.c
> +++ b/ui-blob.c
> @@ -56,7 +56,7 @@ int cgit_ref_path_exists(const char *path, const char *ref, int file_only)
>  		goto done;
>  	if (sha1_object_info(oid.hash, &size) != OBJ_COMMIT)
>  		goto done;
> -	read_tree_recursive(lookup_commit_reference(oid.hash)->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
> +	read_tree_recursive(lookup_commit_reference(&oid)->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
>  
>  done:
>  	free(path_items.match);
> @@ -89,7 +89,7 @@ int cgit_print_file(char *path, const char *head, int file_only)
>  		return -1;
>  	type = sha1_object_info(oid.hash, &size);
>  	if (type == OBJ_COMMIT) {
> -		commit = lookup_commit_reference(oid.hash);
> +		commit = lookup_commit_reference(&oid);
>  		read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
>  		if (!walk_tree_ctx.found_path)
>  			return -1;
> @@ -145,7 +145,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head, int file_onl
>  	type = sha1_object_info(oid.hash, &size);
>  
>  	if ((!hex) && type == OBJ_COMMIT && path) {
> -		commit = lookup_commit_reference(oid.hash);
> +		commit = lookup_commit_reference(&oid);
>  		read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
>  		type = sha1_object_info(oid.hash, &size);
>  	}
> diff --git a/ui-clone.c b/ui-clone.c
> index 5f6606a..0d11672 100644
> --- a/ui-clone.c
> +++ b/ui-clone.c
> @@ -17,7 +17,7 @@ static int print_ref_info(const char *refname, const struct object_id *oid,
>  {
>  	struct object *obj;
>  
> -	if (!(obj = parse_object(oid->hash)))
> +	if (!(obj = parse_object(oid)))
>  		return 0;
>  
>  	htmlf("%s\t%s\n", oid_to_hex(oid), refname);
> diff --git a/ui-commit.c b/ui-commit.c
> index db69d54..e1d4a9b 100644
> --- a/ui-commit.c
> +++ b/ui-commit.c
> @@ -31,7 +31,7 @@ void cgit_print_commit(char *hex, const char *prefix)
>  				"Bad object id: %s", hex);
>  		return;
>  	}
> -	commit = lookup_commit_reference(oid.hash);
> +	commit = lookup_commit_reference(&oid);
>  	if (!commit) {
>  		cgit_print_error_page(404, "Not found",
>  				"Bad commit reference: %s", hex);
> @@ -87,7 +87,7 @@ void cgit_print_commit(char *hex, const char *prefix)
>  	free(tmp);
>  	html("</td></tr>\n");
>  	for (p = commit->parents; p; p = p->next) {
> -		parent = lookup_commit_reference(p->item->object.oid.hash);
> +		parent = lookup_commit_reference(&p->item->object.oid);
>  		if (!parent) {
>  			html("<tr><td colspan='3'>");
>  			cgit_print_error("Error reading parent commit");
> diff --git a/ui-diff.c b/ui-diff.c
> index 173d351..3d40876 100644
> --- a/ui-diff.c
> +++ b/ui-diff.c
> @@ -407,7 +407,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
>  			"Bad object name: %s", new_rev);
>  		return;
>  	}
> -	commit = lookup_commit_reference(new_rev_oid->hash);
> +	commit = lookup_commit_reference(new_rev_oid);
>  	if (!commit || parse_commit(commit)) {
>  		cgit_print_error_page(404, "Not found",
>  			"Bad commit: %s", oid_to_hex(new_rev_oid));
> @@ -428,7 +428,7 @@ void cgit_print_diff(const char *new_rev, const char *old_rev,
>  	}
>  
>  	if (!is_null_oid(old_rev_oid)) {
> -		commit2 = lookup_commit_reference(old_rev_oid->hash);
> +		commit2 = lookup_commit_reference(old_rev_oid);
>  		if (!commit2 || parse_commit(commit2)) {
>  			cgit_print_error_page(404, "Not found",
>  				"Bad commit: %s", oid_to_hex(old_rev_oid));
> diff --git a/ui-patch.c b/ui-patch.c
> index 047e2f9..69aa4a8 100644
> --- a/ui-patch.c
> +++ b/ui-patch.c
> @@ -33,7 +33,7 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
>  				"Bad object id: %s", new_rev);
>  		return;
>  	}
> -	commit = lookup_commit_reference(new_rev_oid.hash);
> +	commit = lookup_commit_reference(&new_rev_oid);
>  	if (!commit) {
>  		cgit_print_error_page(404, "Not found",
>  				"Bad commit reference: %s", new_rev);
> @@ -46,7 +46,7 @@ void cgit_print_patch(const char *new_rev, const char *old_rev,
>  					"Bad object id: %s", old_rev);
>  			return;
>  		}
> -		if (!lookup_commit_reference(old_rev_oid.hash)) {
> +		if (!lookup_commit_reference(&old_rev_oid)) {
>  			cgit_print_error_page(404, "Not found",
>  					"Bad commit reference: %s", old_rev);
>  			return;
> diff --git a/ui-plain.c b/ui-plain.c
> index 8d541e3..e45d553 100644
> --- a/ui-plain.c
> +++ b/ui-plain.c
> @@ -185,7 +185,7 @@ void cgit_print_plain(void)
>  		cgit_print_error_page(404, "Not found", "Not found");
>  		return;
>  	}
> -	commit = lookup_commit_reference(oid.hash);
> +	commit = lookup_commit_reference(&oid);
>  	if (!commit || parse_commit(commit)) {
>  		cgit_print_error_page(404, "Not found", "Not found");
>  		return;
> diff --git a/ui-snapshot.c b/ui-snapshot.c
> index 9b8cddd..b2d95f7 100644
> --- a/ui-snapshot.c
> +++ b/ui-snapshot.c
> @@ -116,7 +116,7 @@ static int make_snapshot(const struct cgit_snapshot_format *format,
>  				"Bad object id: %s", hex);
>  		return 1;
>  	}
> -	if (!lookup_commit_reference(oid.hash)) {
> +	if (!lookup_commit_reference(&oid)) {
>  		cgit_print_error_page(400, "Bad request",
>  				"Not a commit reference: %s", hex);
>  		return 1;
> diff --git a/ui-tag.c b/ui-tag.c
> index afd7d61..909cde0 100644
> --- a/ui-tag.c
> +++ b/ui-tag.c
> @@ -54,7 +54,7 @@ void cgit_print_tag(char *revname)
>  			"Bad tag reference: %s", revname);
>  		goto cleanup;
>  	}
> -	obj = parse_object(oid.hash);
> +	obj = parse_object(&oid);
>  	if (!obj) {
>  		cgit_print_error_page(500, "Internal server error",
>  			"Bad object id: %s", oid_to_hex(&oid));
> @@ -64,7 +64,7 @@ void cgit_print_tag(char *revname)
>  		struct tag *tag;
>  		struct taginfo *info;
>  
> -		tag = lookup_tag(oid.hash);
> +		tag = lookup_tag(&oid);
>  		if (!tag || parse_tag(tag) || !(info = cgit_parse_tag(tag))) {
>  			cgit_print_error_page(500, "Internal server error",
>  				"Bad tag object: %s", revname);
> diff --git a/ui-tree.c b/ui-tree.c
> index b310242..ca24a03 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -157,7 +157,7 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
>  
>  struct single_tree_ctx {
>  	struct strbuf *path;
> -	unsigned char sha1[GIT_SHA1_RAWSZ];
> +	struct object_id oid;
>  	char *name;
>  	size_t count;
>  };
> @@ -177,7 +177,7 @@ static int single_tree_cb(const unsigned char *sha1, struct strbuf *base,
>  	}
>  
>  	ctx->name = xstrdup(pathname);
> -	hashcpy(ctx->sha1, sha1);
> +	hashcpy(ctx->oid.hash, sha1);
>  	strbuf_addf(ctx->path, "/%s", pathname);
>  	return 0;
>  }
> @@ -195,13 +195,13 @@ static void write_tree_link(const unsigned char *sha1, char *name,
>  		.nr = 0
>  	};
>  
> -	hashcpy(tree_ctx.sha1, sha1);
> +	hashcpy(tree_ctx.oid.hash, sha1);
>  
>  	while (tree_ctx.count == 1) {
>  		cgit_tree_link(name, NULL, "ls-dir", ctx.qry.head, rev,
>  			       fullpath->buf);
>  
> -		tree = lookup_tree(tree_ctx.sha1);
> +		tree = lookup_tree(&tree_ctx.oid);
>  		if (!tree)
>  			return;
>  
> @@ -300,17 +300,17 @@ static void ls_tail(void)
>  	cgit_print_layout_end();
>  }
>  
> -static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
> +static void ls_tree(const struct object_id *oid, char *path, struct walk_tree_context *walk_tree_ctx)
>  {
>  	struct tree *tree;
>  	struct pathspec paths = {
>  		.nr = 0
>  	};
>  
> -	tree = parse_tree_indirect(sha1);
> +	tree = parse_tree_indirect(oid);
>  	if (!tree) {
>  		cgit_print_error_page(404, "Not found",
> -			"Not a tree object: %s", sha1_to_hex(sha1));
> +			"Not a tree object: %s", sha1_to_hex(oid->hash));
>  		return;
>  	}
>  
> @@ -380,7 +380,7 @@ void cgit_print_tree(const char *rev, char *path)
>  			"Invalid revision name: %s", rev);
>  		return;
>  	}
> -	commit = lookup_commit_reference(oid.hash);
> +	commit = lookup_commit_reference(&oid);
>  	if (!commit || parse_commit(commit)) {
>  		cgit_print_error_page(404, "Not found",
>  			"Invalid commit reference: %s", rev);
> @@ -390,7 +390,7 @@ void cgit_print_tree(const char *rev, char *path)
>  	walk_tree_ctx.curr_rev = xstrdup(rev);
>  
>  	if (path == NULL) {
> -		ls_tree(commit->tree->object.oid.hash, NULL, &walk_tree_ctx);
> +		ls_tree(&commit->tree->object.oid, NULL, &walk_tree_ctx);
>  		goto cleanup;
>  	}
>  
> -- 
> 2.9.3
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 2/4] ui-blame: create placeholder and links
  2017-06-08  2:18 ` [RFC PATCH 2/4] ui-blame: create placeholder and links whydoubt
@ 2017-07-22 11:31   ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-07-22 11:31 UTC (permalink / raw)


On Wed, Jun 07, 2017 at 09:18:08PM -0500, Jeff Smith wrote:
> Create a placeholder for and links to a page that will contain the
> 'blame' for a file in the repository.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  cgit.mk     |   1 +
>  cmd.c       |   8 ++-
>  ui-blame.c  | 160 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ui-blame.h  |   7 +++
>  ui-shared.c |  20 ++++++--
>  ui-shared.h |   3 ++
>  ui-tree.c   |   8 ++-
>  7 files changed, 202 insertions(+), 5 deletions(-)
>  create mode 100644 ui-blame.c
>  create mode 100644 ui-blame.h
> 
> diff --git a/cgit.mk b/cgit.mk
> index 90a2346..3fcc1ca 100644
> --- a/cgit.mk
> +++ b/cgit.mk
> @@ -77,6 +77,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/cmd.c b/cmd.c
> index d280e95..262e31f 100644
> --- a/cmd.c
> +++ b/cmd.c
> @@ -1,6 +1,6 @@
>  /* cmd.c: the cgit command dispatcher
>   *
> - * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
> + * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -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"
> @@ -63,6 +64,10 @@ static void about_fn(void)
>  		cgit_print_site_readme();
>  }
>  
> +static void blame_fn(void)
> +{
> +	cgit_print_blame();
> +}

Missing blank line here.

>  static void blob_fn(void)
>  {
>  	cgit_print_blob(ctx.qry.sha1, ctx.qry.path, ctx.qry.head, 0);

I don't have any comments on the rest of this patch, although I'm not
sure about the order of this series.  I think it would be cleaner to
have:

- a patch adding ui-blame.[ch] in their final form along with the
  Makefile change to enable building but no other changes

- follow-up patch(es) to link the blame UI in to the rest of the system
  (I don't know if this should be one patch, or a series adding the
  command handling, then ui-shared changes followed by links in various
  other ui-*.c files)


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function
  2017-06-08  2:18 ` [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function whydoubt
@ 2017-07-22 11:36   ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-07-22 11:36 UTC (permalink / raw)


On Wed, Jun 07, 2017 at 09:18:09PM -0500, Jeff Smith wrote:
> For implementing a ui-blame page, there is need for a function that
> outputs a selection from a block of text, transformed for HTML output,
> but with no further modifications or additions.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  html.c | 37 ++++++++++++++++---------------------
>  html.h |  1 +
>  2 files changed, 17 insertions(+), 21 deletions(-)

We only use the current html_ntxt() function in one place so I don't
think there's any real need to keep the name as is.

The end result would be nicer if html_ntxt() is your new "no ellipsis"
variant which can return an int to indicate whether the text was
truncated, then ui-repolist can switch to a new html_txt_ellipsis()
which has the same effect as the current html_ntxt().

> diff --git a/html.c b/html.c
> index e7e6e07..c7d1c64 100644
> --- a/html.c
> +++ b/html.c
> @@ -122,10 +122,10 @@ void html_vtxtf(const char *format, va_list ap)
>  	strbuf_release(&buf);
>  }
>  
> -void html_txt(const char *txt)
> +static int _html_txt(int len, const char *txt)
>  {
>  	const char *t = txt;
> -	while (t && *t) {
> +	while (t && *t && len--) {
>  		int c = *t;
>  		if (c == '<' || c == '>' || c == '&') {
>  			html_raw(txt, t - txt);
> @@ -140,32 +140,27 @@ void html_txt(const char *txt)
>  		t++;
>  	}
>  	if (t != txt)
> -		html(txt);
> +		html_raw(txt, t - txt);
> +	return len;
> +}
> +
> +void html_txt(const char *txt)
> +{
> +	if (txt)
> +		_html_txt(strlen(txt), txt);
>  }
>  
>  void html_ntxt(int len, const char *txt)
>  {
> -	const char *t = txt;
> -	while (t && *t && len--) {
> -		int c = *t;
> -		if (c == '<' || c == '>' || c == '&') {
> -			html_raw(txt, t - txt);
> -			if (c == '>')
> -				html("&gt;");
> -			else if (c == '<')
> -				html("&lt;");
> -			else if (c == '&')
> -				html("&amp;");
> -			txt = t + 1;
> -		}
> -		t++;
> -	}
> -	if (t != txt)
> -		html_raw(txt, t - txt);
> -	if (len < 0)
> +	if (_html_txt(len, txt) < 0)
>  		html("...");
>  }
>  
> +void html_ntxt_noellipsis(int len, const char *txt)
> +{
> +	_html_txt(len, txt);
> +}
> +
>  void html_attrf(const char *fmt, ...)
>  {
>  	va_list ap;
> diff --git a/html.h b/html.h
> index 1b24e55..e1b85b3 100644
> --- a/html.h
> +++ b/html.h
> @@ -20,6 +20,7 @@ extern void html_attrf(const char *format,...);
>  
>  extern void html_txt(const char *txt);
>  extern void html_ntxt(int len, const char *txt);
> +extern void html_ntxt_noellipsis(int len, const char *txt);
>  extern void html_attr(const char *txt);
>  extern void html_url_path(const char *txt);
>  extern void html_url_arg(const char *txt);
> -- 
> 2.9.3
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 4/4] ui-blame: fill in the contents
  2017-06-08  2:18 ` [RFC PATCH 4/4] ui-blame: fill in the contents whydoubt
  2017-07-05  8:32   ` list
@ 2017-07-22 11:47   ` john
  1 sibling, 0 replies; 63+ messages in thread
From: john @ 2017-07-22 11:47 UTC (permalink / raw)


On Wed, Jun 07, 2017 at 09:18:10PM -0500, Jeff Smith wrote:
> Use the blame interface added in libgit to output the blame information
> of a file in the repository.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  cgit.css   |   8 +++
>  ui-blame.c | 214 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
>  2 files changed, 220 insertions(+), 2 deletions(-)
> 
> diff --git a/cgit.css b/cgit.css
> index 1dc2c11..258991c 100644
> --- a/cgit.css
> +++ b/cgit.css
> @@ -329,6 +329,14 @@ div#cgit table.ssdiff td.lineno a:hover {
>  	color: black;
>  }
>  
> +div#cgit table.blame tr:nth-child(even) {
> +	background: #f7f0e7;
> +}
> +
> +div#cgit table.blame tr:nth-child(odd) {
> +	background: white;
> +}
> +
>  div#cgit table.bin-blob {
>  	margin-top: 0.5em;
>  	border: solid 1px black;
> diff --git a/ui-blame.c b/ui-blame.c
> index 901ca89..6ad0009 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -10,6 +10,187 @@
>  #include "ui-blame.h"
>  #include "html.h"
>  #include "ui-shared.h"
> +#include "argv-array.h"
> +#include "blame.h"
> +
> +
> +/* Remember to update object flag allocation in object.h */

This comment doesn't really apply in CGit, but it looks like the define
is entirely unused, so we can just delete it.

> +#define MORE_THAN_ONE_PATH	(1u<<13)
> +
> +/*
> + * Information on commits, used for output.
> + */

Can we reuse our existing struct commitinfo here?  I can't see anything
that this function does that our existing cgit_parse_commit() doesn't do
and cgit_parse_commit() deals with encoding properly as well.

> +struct commit_info {
> +	struct strbuf author;
> +	struct strbuf author_mail;
> +	struct ident_split author_ident;
> +
> +	/* filled only when asked for details */
> +	struct strbuf committer;
> +	struct strbuf committer_mail;
> +	struct ident_split committer_ident;
> +
> +	struct strbuf summary;
> +};
> +
[snip parsing details]
> +static void get_commit_info(struct commit *commit,
> +			    struct commit_info *ret,
> +			    int detailed)

This is only ever called with detailed == 1.

> +{
> +	int len;
> +	const char *subject, *encoding;
> +	const char *message;
> +
> +	commit_info_init(ret);
> +
> +	encoding = get_log_output_encoding();
> +	message = logmsg_reencode(commit, NULL, encoding);
> +	get_ac_line(message, "\nauthor ",
> +		    &ret->author, &ret->author_mail,
> +		    &ret->author_ident);
> +
> +	if (!detailed) {
> +		unuse_commit_buffer(commit, message);
> +		return;
> +	}
> +
> +	get_ac_line(message, "\ncommitter ",
> +		    &ret->committer, &ret->committer_mail,
> +		    &ret->committer_ident);
> +
> +	len = find_commit_subject(message, &subject);
> +	if (len)
> +		strbuf_add(&ret->summary, subject, len);
> +	else
> +		strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
> +
> +	unuse_commit_buffer(commit, message);
> +}

[snip most of the code, which all looks entirely reasonable.]

> @@ -76,7 +278,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>  		return;
>  	}
>  
> -	/* Output data here */
> +	html("<table class='blame blob'>");
> +	for (ent = sb.ent; ent; ) {
> +		struct blame_entry *e = ent->next;
> +		emit_blame_entry(&sb, ent);
> +		free(ent);
> +		ent = e;
> +	}
> +	html("</table>\n");
> +	free((void *)sb.final_buf);

There's no need to cast to void* in C, so drop the cast here.

>  
>  	cgit_print_layout_end();
>  	return;
> -- 
> 2.9.3


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
                   ` (4 preceding siblings ...)
  2017-06-08  9:00 ` [RFC PATCH 0/4] Add ui-blame list
@ 2017-07-22 12:02 ` john
  2017-08-05  0:23   ` dlcampbell
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
  6 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-07-22 12:02 UTC (permalink / raw)


On Wed, Jun 07, 2017 at 09:18:06PM -0500, Jeff Smith wrote:
> I split git blame functionality into libgit, and the changes were
> accepted upstream a few days ago.  Now that the git infrastructure is in
> place, it is time to get back to the cgit part.
> 
> The first patch advances git to current master (which should be future
> v2.14), and makes changes made necessary by doing so.  The remaining
> patches deal with adding the new blame functionality.
> 
> Based on branch ch/git-2-13-1.
> 
> Jeff Smith (4):
>   git: update to v2.14
>   ui-blame: create placeholder and links
>   ui-blame: create needed html_ntxt_noellipsis function
>   ui-blame: fill in the contents

This looks like a good start.  I've commented on the individual patches,
but apart from the patch order and struct commit_info question they're
all minor.

I have a general question, possible more for any administrators of CGit
sites who happen to see this: Should we offer a configuration switch to
disable the blame function?  Blame is a comparatively expensive
operation compared to everything else we do to display a page, so is
there a desire to disable this feature for sites worried about resource
usage?


Finally, some general comments on the output formatting in the browser:

- Should the commit SHA be fixed-width?  I'm comparing the output with
  http://repo.or.cz/git.git/blame_incremental/HEAD:/branch.c which I
  think is a bit easier to read.

- The tooltip looks very confusing since the labels and values aren't
  aligned.  I'd simplify it to show either the author's name and date
  (like GitWeb in the link above) or just the commit message.


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-07-22 12:02 ` john
@ 2017-08-05  0:23   ` dlcampbell
  2017-08-05  0:57     ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: dlcampbell @ 2017-08-05  0:23 UTC (permalink / raw)


On 07/22/2017 05:02 AM, John Keeping wrote:
> On Wed, Jun 07, 2017 at 09:18:06PM -0500, Jeff Smith wrote:
>> [snip]
> 
> I have a general question, possible more for any administrators of CGit
> sites who happen to see this: Should we offer a configuration switch to
> disable the blame function?  Blame is a comparatively expensive
> operation compared to everything else we do to display a page, so is
> there a desire to disable this feature for sites worried about resource
> usage?
> 
> [snip]
> 

Hi John,

The option would be great to have, especially for some like me who run
cgit on a Raspberry Pi or another comparatively weaker device.
Personally I don't mind it being a little slower in order to use `git
blame`, but if disabling it improves speed, it seems smart to put it
behind an option so cgit admins can make the right decision for their
environments.

~Daniel


-------------- next part --------------
A non-text attachment was scrubbed...
Name: signature.asc
Type: application/pgp-signature
Size: 833 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170804/6c39a592/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-08-05  0:23   ` dlcampbell
@ 2017-08-05  0:57     ` whydoubt
  2017-08-24 18:14       ` list
  0 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-08-05  0:57 UTC (permalink / raw)


I am working at rebasing my changes onto git 2.14-rc1 and integrating some
of the suggestions made thus far. Blame is certainly a more expensive
operation, and adding a flag for it makes a lot of sense. I will look into
it further.

 - Jeff

On Aug 4, 2017 7:23 PM, "Daniel Campbell" <dlcampbell at gmx.com> wrote:

> On 07/22/2017 05:02 AM, John Keeping wrote:
> > On Wed, Jun 07, 2017 at 09:18:06PM -0500, Jeff Smith wrote:
> >> [snip]
> >
> > I have a general question, possible more for any administrators of CGit
> > sites who happen to see this: Should we offer a configuration switch to
> > disable the blame function?  Blame is a comparatively expensive
> > operation compared to everything else we do to display a page, so is
> > there a desire to disable this feature for sites worried about resource
> > usage?
> >
> > [snip]
> >
>
> Hi John,
>
> The option would be great to have, especially for some like me who run
> cgit on a Raspberry Pi or another comparatively weaker device.
> Personally I don't mind it being a little slower in order to use `git
> blame`, but if disabling it improves speed, it seems smart to put it
> behind an option so cgit admins can make the right decision for their
> environments.
>
> ~Daniel
>
>
>
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
>
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170804/63751552/attachment.html>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-08-05  0:57     ` whydoubt
@ 2017-08-24 18:14       ` list
  2017-08-31 13:05         ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: list @ 2017-08-24 18:14 UTC (permalink / raw)


Jeffrey Smith <whydoubt at gmail.com> on Fri, 2017/08/04 19:57:
> I am working at rebasing my changes onto git 2.14-rc1 and integrating some
> of the suggestions made thus far. Blame is certainly a more expensive
> operation, and adding a flag for it makes a lot of sense. I will look into
> it further.

I rebased the original code on current master.

Any news on an updated set of patch?
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20170824/58b269b5/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFC PATCH 0/4] Add ui-blame
  2017-08-24 18:14       ` list
@ 2017-08-31 13:05         ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-08-31 13:05 UTC (permalink / raw)


I will try to get the latest iteration of it sent out in the next day or two.

On Thu, Aug 24, 2017 at 1:14 PM, Christian Hesse <list at eworm.de> wrote:
> Jeffrey Smith <whydoubt at gmail.com> on Fri, 2017/08/04 19:57:
>> I am working at rebasing my changes onto git 2.14-rc1 and integrating some
>> of the suggestions made thus far. Blame is certainly a more expensive
>> operation, and adding a flag for it makes a lot of sense. I will look into
>> it further.
>
> I rebased the original code on current master.
>
> Any news on an updated set of patch?
> --
> main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
> "CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
> putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 0/7] Add ui-blame
  2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
                   ` (5 preceding siblings ...)
  2017-07-22 12:02 ` john
@ 2017-09-23  3:38 ` whydoubt
  2017-09-23  3:38   ` [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item whydoubt
                     ` (8 more replies)
  6 siblings, 9 replies; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


I split git blame functionality into libgit, and the changes were
accepted upstream and are a part of git 2.14.  Now that the git
infrastructure is in place, here is what is needed for cgit to make use
of it.

Jeff Smith (7):
  ui-blame: create enable-blame config item
  ui-blame: create framework
  ui-blame: create links
  ui-blame: html_ntxt with no ellipsis
  ui-blame: pull blame info from libgit
  ui-blame: begin building
  ui-blame: generate blame page when requested

 cgit.c        |   3 +
 cgit.css      |   8 ++
 cgit.h        |   1 +
 cgit.mk       |   1 +
 cgitrc.5.txt  |   5 +
 cmd.c         |  12 +-
 html.c        |  37 +++---
 html.h        |   1 +
 ui-blame.c    | 366 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h    |   7 ++
 ui-repolist.c |   2 +-
 ui-shared.c   |  20 +++-
 ui-shared.h   |   3 +
 ui-tree.c     |  10 +-
 14 files changed, 449 insertions(+), 27 deletions(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:46     ` john
  2017-09-23  3:38   ` [RFCv2 PATCH 2/7] ui-blame: create framework whydoubt
                     ` (7 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.c       | 3 +++
 cgit.h       | 1 +
 cgitrc.5.txt | 5 +++++
 3 files changed, 9 insertions(+)

diff --git a/cgit.c b/cgit.c
index 1dae4b8..c03f69c 100644
--- a/cgit.c
+++ b/cgit.c
@@ -183,6 +183,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.enable_tree_linenumbers = atoi(value);
 	else if (!strcmp(name, "enable-git-config"))
 		ctx.cfg.enable_git_config = atoi(value);
+	else if (!strcmp(name, "enable-blame"))
+		ctx.cfg.enable_blame = atoi(value);
 	else if (!strcmp(name, "max-stats"))
 		ctx.cfg.max_stats = cgit_find_stats_period(value, NULL);
 	else if (!strcmp(name, "cache-size"))
@@ -373,6 +375,7 @@ static void prepare_context(void)
 	ctx.cfg.enable_index_owner = 1;
 	ctx.cfg.enable_tree_linenumbers = 1;
 	ctx.cfg.enable_git_config = 0;
+	ctx.cfg.enable_blame = 1;
 	ctx.cfg.max_repo_count = 50;
 	ctx.cfg.max_commit_count = 50;
 	ctx.cfg.max_lock_attempts = 5;
diff --git a/cgit.h b/cgit.h
index fbc6c6a..a48e622 100644
--- a/cgit.h
+++ b/cgit.h
@@ -236,6 +236,7 @@ struct cgit_config {
 	int enable_html_serving;
 	int enable_tree_linenumbers;
 	int enable_git_config;
+	int enable_blame;
 	int local_time;
 	int max_atom_items;
 	int max_repo_count;
diff --git a/cgitrc.5.txt b/cgitrc.5.txt
index 9fcf445..b236bf2 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -211,6 +211,11 @@ enable-git-config::
 	with "cgit." will be mapped to the corresponding "repo." key in cgit.
 	Default value: "0". See also: scan-path, section-from-path.
 
+enable-blame::
+	Flag which, when set to "1", will allow cgit to provide a "blame" page
+	for files, and will make it generate links to that page in appropriate
+	places. Default value: "1".
+
 favicon::
 	Url used as link to a shortcut icon for cgit. It is suggested to use
 	the value "/favicon.ico" since certain browsers will ignore other
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 2/7] ui-blame: create framework
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
  2017-09-23  3:38   ` [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:47     ` john
  2017-09-23  3:38   ` [RFCv2 PATCH 3/7] ui-blame: create links whydoubt
                     ` (6 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Create framework for a page that will contain the 'blame' for a file
in the repository.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-blame.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h |   7 +++
 2 files changed, 167 insertions(+)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

diff --git a/ui-blame.c b/ui-blame.c
new file mode 100644
index 0000000..901ca89
--- /dev/null
+++ b/ui-blame.c
@@ -0,0 +1,160 @@
+/* ui-blame.c: functions for blame output
+ *
+ * Copyright (C) 2006-2017 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"
+
+struct walk_tree_context {
+	char *curr_rev;
+	int match_baselen;
+	int state;
+};
+
+static void set_title_from_path(const char *path)
+{
+	size_t path_len, path_index, path_last_end;
+	char *new_title;
+
+	if (!path)
+		return;
+
+	path_len = strlen(path);
+	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
+	new_title[0] = '\0';
+
+	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
+		if (path[path_index] == '/') {
+			if (path_index == path_len - 1) {
+				path_last_end = path_index - 1;
+				continue;
+			}
+			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
+			strcat(new_title, "\\");
+			path_last_end = path_index;
+		}
+	}
+	if (path_last_end)
+		strncat(new_title, path, path_last_end);
+
+	strcat(new_title, " - ");
+	strcat(new_title, ctx.page.title);
+	ctx.page.title = new_title;
+}
+static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
+{
+	enum object_type type;
+	unsigned long size;
+
+	type = sha1_object_info(sha1, &size);
+	if (type == OBJ_BAD) {
+		cgit_print_error_page(404, "Not found", "Bad object name: %s",
+				      sha1_to_hex(sha1));
+		return;
+	}
+
+	/* Read in applicable data here */
+
+	set_title_from_path(path);
+
+	cgit_print_layout_start();
+	htmlf("blob: %s (", sha1_to_hex(sha1));
+	cgit_plain_link("plain", NULL, NULL, ctx.qry.head, rev, path);
+	html(") (");
+	cgit_tree_link("tree", NULL, NULL, ctx.qry.head, rev, path);
+	html(")\n");
+
+	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
+		htmlf("<div class='error'>blob size (%ldKB) exceeds display size limit (%dKB).</div>",
+		      size / 1024, ctx.cfg.max_blob_size);
+		return;
+	}
+
+	/* Output data here */
+
+	cgit_print_layout_end();
+	return;
+}
+
+static int walk_tree(const unsigned char *sha1, struct strbuf *base,
+		     const char *pathname, unsigned mode, int stage, void *cbdata)
+{
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (base->len == walk_tree_ctx->match_baselen) {
+		if (S_ISREG(mode)) {
+			struct strbuf buffer = STRBUF_INIT;
+			strbuf_addbuf(&buffer, base);
+			strbuf_addstr(&buffer, pathname);
+			print_object(sha1, buffer.buf, pathname, walk_tree_ctx->curr_rev);
+			strbuf_release(&buffer);
+			walk_tree_ctx->state = 1;
+		} else if (S_ISDIR(mode)) {
+			walk_tree_ctx->state = 2;
+		}
+	} else if (base->len < INT_MAX && (int)base->len > walk_tree_ctx->match_baselen) {
+		walk_tree_ctx->state = 2;
+	} else if (S_ISDIR(mode)) {
+		return READ_TREE_RECURSIVE;
+	}
+	return 0;
+}
+
+static int basedir_len(const char *path)
+{
+	char *p = strrchr(path, '/');
+	if (p)
+		return p - path + 1;
+	return 0;
+}
+
+void cgit_print_blame(void)
+{
+	const char *rev = ctx.qry.sha1;
+	struct object_id oid;
+	struct commit *commit;
+	struct pathspec_item path_items = {
+		.match = ctx.qry.path,
+		.len = ctx.qry.path ? strlen(ctx.qry.path) : 0
+	};
+	struct pathspec paths = {
+		.nr = 1,
+		.items = &path_items
+	};
+	struct walk_tree_context walk_tree_ctx = {
+		.state = 0
+	};
+
+	if (!rev)
+		rev = ctx.qry.head;
+
+	if (get_oid(rev, &oid)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
+		return;
+	}
+	commit = lookup_commit_reference(&oid);
+	if (!commit || parse_commit(commit)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
+		return;
+	}
+
+	walk_tree_ctx.curr_rev = xstrdup(rev);
+	walk_tree_ctx.match_baselen = (path_items.match) ?
+				       basedir_len(path_items.match) : -1;
+
+	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
+	if (!walk_tree_ctx.state)
+		cgit_print_error_page(404, "Not found", "Not found");
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_error_page(404, "No blame for folders", "Blame is not available for folders.");
+
+	free(walk_tree_ctx.curr_rev);
+}
diff --git a/ui-blame.h b/ui-blame.h
new file mode 100644
index 0000000..8a438e9
--- /dev/null
+++ b/ui-blame.h
@@ -0,0 +1,7 @@
+#ifndef UI_BLAME_H
+#define UI_BLAME_H
+
+extern void cgit_print_blame(void);
+
+#endif /* UI_BLAME_H */
+
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 3/7] ui-blame: create links
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
  2017-09-23  3:38   ` [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item whydoubt
  2017-09-23  3:38   ` [RFCv2 PATCH 2/7] ui-blame: create framework whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:47     ` john
  2017-09-23  3:38   ` [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis whydoubt
                     ` (5 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Create links to the blame page.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 20 +++++++++++++++++---
 ui-shared.h |  3 +++
 ui-tree.c   | 10 +++++++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index e5c9a02..faa0d6a 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1,6 +1,6 @@
 /* ui-shared.c: common web output functions
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -304,6 +304,12 @@ void cgit_plain_link(const char *name, const char *title, const char *class,
 	reporevlink("plain", name, title, class, head, rev, path);
 }
 
+void cgit_blame_link(const char *name, const char *title, const char *class,
+		     const char *head, const char *rev, const char *path)
+{
+	reporevlink("blame", name, title, class, head, rev, path);
+}
+
 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,
@@ -481,6 +487,10 @@ static void cgit_self_link(char *name, const char *title, const char *class)
 		cgit_plain_link(name, title, class, ctx.qry.head,
 				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
 				ctx.qry.path);
+	else if (!strcmp(ctx.qry.page, "blame"))
+		cgit_blame_link(name, title, class, ctx.qry.head,
+				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
+				ctx.qry.path);
 	else if (!strcmp(ctx.qry.page, "log"))
 		cgit_log_link(name, title, class, ctx.qry.head,
 			      ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
@@ -986,8 +996,12 @@ void cgit_print_pageheader(void)
 		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
 			      NULL, ctx.qry.vpath, 0, NULL, NULL,
 			      ctx.qry.showmsg, ctx.qry.follow);
-		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
-			       ctx.qry.sha1, ctx.qry.vpath);
+		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
+			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
+				        ctx.qry.sha1, ctx.qry.vpath);
+		else
+			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
+				       ctx.qry.sha1, ctx.qry.vpath);
 		cgit_commit_link("commit", NULL, hc("commit"),
 				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
 		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
diff --git a/ui-shared.h b/ui-shared.h
index 87799f1..e5992bc 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
 extern void cgit_plain_link(const char *name, const char *title,
 			    const char *class, const char *head,
 			    const char *rev, const char *path);
+extern void cgit_blame_link(const char *name, const char *title,
+			    const char *class, const char *head,
+			    const char *rev, const char *path);
 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,
diff --git a/ui-tree.c b/ui-tree.c
index ca24a03..3513d5e 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -1,6 +1,6 @@
 /* ui-tree.c: functions for tree output
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -141,6 +141,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);
+	}
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -275,6 +280,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
+		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev, fullpath.buf);
 	html("</td></tr>\n");
 	free(name);
 	strbuf_release(&fullpath);
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (2 preceding siblings ...)
  2017-09-23  3:38   ` [RFCv2 PATCH 3/7] ui-blame: create links whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:47     ` john
  2017-09-23  3:38   ` [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit whydoubt
                     ` (4 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


For implementing a ui-blame page, there is need for a function that
outputs a selection from a block of text, transformed for HTML output,
but with no further modifications or additions.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 html.c        | 37 ++++++++++++++++---------------------
 html.h        |  1 +
 ui-repolist.c |  2 +-
 3 files changed, 18 insertions(+), 22 deletions(-)

diff --git a/html.c b/html.c
index e7e6e07..345300e 100644
--- a/html.c
+++ b/html.c
@@ -122,10 +122,10 @@ void html_vtxtf(const char *format, va_list ap)
 	strbuf_release(&buf);
 }
 
-void html_txt(const char *txt)
+static int _html_txt(int len, const char *txt)
 {
 	const char *t = txt;
-	while (t && *t) {
+	while (t && *t && len--) {
 		int c = *t;
 		if (c == '<' || c == '>' || c == '&') {
 			html_raw(txt, t - txt);
@@ -140,29 +140,24 @@ void html_txt(const char *txt)
 		t++;
 	}
 	if (t != txt)
-		html(txt);
+		html_raw(txt, t - txt);
+	return len;
+}
+
+void html_txt(const char *txt)
+{
+	if (txt)
+		_html_txt(strlen(txt), txt);
 }
 
 void html_ntxt(int len, const char *txt)
 {
-	const char *t = txt;
-	while (t && *t && len--) {
-		int c = *t;
-		if (c == '<' || c == '>' || c == '&') {
-			html_raw(txt, t - txt);
-			if (c == '>')
-				html("&gt;");
-			else if (c == '<')
-				html("&lt;");
-			else if (c == '&')
-				html("&amp;");
-			txt = t + 1;
-		}
-		t++;
-	}
-	if (t != txt)
-		html_raw(txt, t - txt);
-	if (len < 0)
+	_html_txt(len, txt);
+}
+
+void html_ntxt_ellipsis(int len, const char *txt)
+{
+	if (_html_txt(len, txt) < 0)
 		html("...");
 }
 
diff --git a/html.h b/html.h
index 1b24e55..de53430 100644
--- a/html.h
+++ b/html.h
@@ -20,6 +20,7 @@ extern void html_attrf(const char *format,...);
 
 extern void html_txt(const char *txt);
 extern void html_ntxt(int len, const char *txt);
+extern void html_ntxt_ellipsis(int len, const char *txt);
 extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
 extern void html_url_arg(const char *txt);
diff --git a/ui-repolist.c b/ui-repolist.c
index 7272e87..77c33fd 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -329,7 +329,7 @@ void cgit_print_repolist(void)
 		repourl = cgit_repourl(ctx.repo->url);
 		html_link_open(repourl, NULL, NULL);
 		free(repourl);
-		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
+		html_ntxt_ellipsis(ctx.cfg.max_repodesc_len, ctx.repo->desc);
 		html_link_close();
 		html("</td><td>");
 		if (ctx.cfg.enable_index_owner) {
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (3 preceding siblings ...)
  2017-09-23  3:38   ` [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:47     ` john
  2017-09-23  3:38   ` [RFCv2 PATCH 6/7] ui-blame: begin building whydoubt
                     ` (3 subsequent siblings)
  8 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Use the blame interface added in libgit to output the blame information
of a file in the repository.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.css   |   8 +++
 ui-blame.c | 210 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++-
 2 files changed, 216 insertions(+), 2 deletions(-)

diff --git a/cgit.css b/cgit.css
index 1dc2c11..836f8ae 100644
--- a/cgit.css
+++ b/cgit.css
@@ -329,6 +329,14 @@ div#cgit table.ssdiff td.lineno a:hover {
 	color: black;
 }
 
+div#cgit table.blame tr:nth-child(even) {
+	background: #eee;
+}
+
+div#cgit table.blame tr:nth-child(odd) {
+	background: white;
+}
+
 div#cgit table.bin-blob {
 	margin-top: 0.5em;
 	border: solid 1px black;
diff --git a/ui-blame.c b/ui-blame.c
index 901ca89..cc4457a 100644
--- a/ui-blame.c
+++ b/ui-blame.c
@@ -10,6 +10,183 @@
 #include "ui-blame.h"
 #include "html.h"
 #include "ui-shared.h"
+#include "argv-array.h"
+#include "blame.h"
+
+
+/*
+ * Information on commits, used for output.
+ */
+struct commit_info {
+	struct strbuf author;
+	struct strbuf author_mail;
+	struct ident_split author_ident;
+
+	/* filled only when asked for details */
+	struct strbuf committer;
+	struct strbuf committer_mail;
+	struct ident_split committer_ident;
+
+	struct strbuf summary;
+};
+
+/*
+ * Parse author/committer line in the commit object buffer
+ */
+static void get_ac_line(const char *inbuf, const char *what,
+			struct strbuf *name, struct strbuf *mail,
+			struct ident_split *ident)
+{
+	size_t len, maillen, namelen;
+	char *tmp, *endp;
+	const char *namebuf, *mailbuf;
+
+	tmp = strstr(inbuf, what);
+	if (!tmp)
+		goto error_out;
+	tmp += strlen(what);
+	endp = strchr(tmp, '\n');
+	if (!endp)
+		len = strlen(tmp);
+	else
+		len = endp - tmp;
+
+	if (split_ident_line(ident, tmp, len)) {
+	error_out:
+		/* Ugh */
+		tmp = "(unknown)";
+		strbuf_addstr(name, tmp);
+		strbuf_addstr(mail, tmp);
+		return;
+	}
+
+	namelen = ident->name_end - ident->name_begin;
+	namebuf = ident->name_begin;
+
+	maillen = ident->mail_end - ident->mail_begin;
+	mailbuf = ident->mail_begin;
+
+	strbuf_addf(mail, "<%.*s>", (int)maillen, mailbuf);
+	strbuf_add(name, namebuf, namelen);
+}
+
+static void commit_info_init(struct commit_info *ci)
+{
+
+	strbuf_init(&ci->author, 0);
+	strbuf_init(&ci->author_mail, 0);
+	strbuf_init(&ci->committer, 0);
+	strbuf_init(&ci->committer_mail, 0);
+	strbuf_init(&ci->summary, 0);
+}
+
+static void commit_info_destroy(struct commit_info *ci)
+{
+
+	strbuf_release(&ci->author);
+	strbuf_release(&ci->author_mail);
+	strbuf_release(&ci->committer);
+	strbuf_release(&ci->committer_mail);
+	strbuf_release(&ci->summary);
+}
+
+static void get_commit_info(struct commit *commit,
+			    struct commit_info *ret,
+			    int detailed)
+{
+	int len;
+	const char *subject, *encoding;
+	const char *message;
+
+	commit_info_init(ret);
+
+	encoding = get_log_output_encoding();
+	message = logmsg_reencode(commit, NULL, encoding);
+	get_ac_line(message, "\nauthor ",
+		    &ret->author, &ret->author_mail,
+		    &ret->author_ident);
+
+	if (!detailed) {
+		unuse_commit_buffer(commit, message);
+		return;
+	}
+
+	get_ac_line(message, "\ncommitter ",
+		    &ret->committer, &ret->committer_mail,
+		    &ret->committer_ident);
+
+	len = find_commit_subject(message, &subject);
+	if (len)
+		strbuf_add(&ret->summary, subject, len);
+	else
+		strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
+
+	unuse_commit_buffer(commit, message);
+}
+
+static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
+{
+	struct commit_info ci;
+	struct strbuf detail = STRBUF_INIT;
+
+	get_commit_info(suspect->commit, &ci, 1);
+
+	strbuf_addf(&detail, "author  %s", ci.author.buf);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", ci.author_mail.buf);
+	strbuf_addf(&detail, "  %s\n",
+		    show_ident_date(&ci.author_ident,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addf(&detail, "committer  %s", ci.committer.buf);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", ci.committer_mail.buf);
+	strbuf_addf(&detail, "  %s\n\n",
+		    show_ident_date(&ci.committer_ident,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addbuf(&detail, &ci.summary);
+
+	commit_info_destroy(&ci);
+	return strbuf_detach(&detail, NULL);
+}
+
+static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
+{
+	struct blame_origin *suspect = ent->suspect;
+	char hex[GIT_SHA1_HEXSZ + 1];
+	char *detail, *abbrev;
+	unsigned long lineno;
+	const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
+	const char *cp, *cpend;
+
+	oid_to_hex_r(hex, &suspect->commit->object.oid);
+	detail = emit_one_suspect_detail(suspect, hex);
+	abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
+					    DEFAULT_ABBREV));
+
+	html("<tr><td class='lines'><pre>");
+	cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
+	html("</pre></td>\n");
+
+	free(detail);
+	free(abbrev);
+
+	if (ctx.cfg.enable_tree_linenumbers) {
+		html("<td class='linenumbers'><pre>");
+		lineno = ent->lno;
+		while (lineno < ent->lno + ent->num_lines)
+			htmlf(numberfmt, ++lineno);
+		html("</pre></td>\n");
+	}
+
+	cp = blame_nth_line(sb, ent->lno);
+	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
+
+	html("<td class='lines'><pre><code>");
+	html_ntxt(cpend - cp, cp);
+	html("</code></pre></td></tr>\n");
+}
 
 struct walk_tree_context {
 	char *curr_rev;
@@ -47,10 +224,16 @@ static void set_title_from_path(const char *path)
 	strcat(new_title, ctx.page.title);
 	ctx.page.title = new_title;
 }
+
 static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
 {
 	enum object_type type;
 	unsigned long size;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+	struct rev_info revs;
+	struct blame_scoreboard sb;
+	struct blame_origin *o;
+	struct blame_entry *ent = NULL;
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
@@ -59,7 +242,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
 		return;
 	}
 
-	/* Read in applicable data here */
+	argv_array_push(&rev_argv, "blame");
+	argv_array_push(&rev_argv, rev);
+	init_revisions(&revs, NULL);
+	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
+	setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
+	init_scoreboard(&sb);
+	sb.revs = &revs;
+	setup_scoreboard(&sb, path, &o);
+	o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
+	prio_queue_put(&sb.commits, o->commit);
+	blame_origin_decref(o);
+	sb.ent = NULL;
+	sb.path = path;
+	assign_blame(&sb, 0);
+	blame_sort_final(&sb);
+	blame_coalesce(&sb);
 
 	set_title_from_path(path);
 
@@ -76,7 +274,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
 		return;
 	}
 
-	/* Output data here */
+	html("<table class='blame blob'>");
+	for (ent = sb.ent; ent; ) {
+		struct blame_entry *e = ent->next;
+		emit_blame_entry(&sb, ent);
+		free(ent);
+		ent = e;
+	}
+	html("</table>\n");
+	free((void *)sb.final_buf);
 
 	cgit_print_layout_end();
 	return;
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 6/7] ui-blame: begin building
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (4 preceding siblings ...)
  2017-09-23  3:38   ` [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23  3:38   ` [RFCv2 PATCH 7/7] ui-blame: generate blame page when requested whydoubt
                     ` (2 subsequent siblings)
  8 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.mk | 1 +
 1 file changed, 1 insertion(+)

diff --git a/cgit.mk b/cgit.mk
index 90a2346..3fcc1ca 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -77,6 +77,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
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 7/7] ui-blame: generate blame page when requested
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (5 preceding siblings ...)
  2017-09-23  3:38   ` [RFCv2 PATCH 6/7] ui-blame: begin building whydoubt
@ 2017-09-23  3:38   ` whydoubt
  2017-09-23 15:53   ` [RFCv2 PATCH 0/7] Add ui-blame john
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
  8 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-23  3:38 UTC (permalink / raw)


Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cmd.c | 12 +++++++++++-
 1 file changed, 11 insertions(+), 1 deletion(-)

diff --git a/cmd.c b/cmd.c
index d280e95..63f0ae5 100644
--- a/cmd.c
+++ b/cmd.c
@@ -1,6 +1,6 @@
 /* cmd.c: the cgit command dispatcher
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -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"
@@ -63,6 +64,14 @@ static void about_fn(void)
 		cgit_print_site_readme();
 }
 
+static void blame_fn(void)
+{
+	if (ctx.cfg.enable_blame)
+		cgit_print_blame();
+	else
+		cgit_print_error_page(403, "Forbidden", "Blame is disabled");
+}
+
 static void blob_fn(void)
 {
 	cgit_print_blob(ctx.qry.sha1, ctx.qry.path, ctx.qry.head, 0);
@@ -164,6 +173,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(HEAD, 1, 0, 1),
 		def_cmd(atom, 1, 0, 0),
 		def_cmd(about, 0, 0, 0),
+		def_cmd(blame, 1, 1, 0),
 		def_cmd(blob, 1, 0, 0),
 		def_cmd(commit, 1, 1, 0),
 		def_cmd(diff, 1, 1, 0),
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item
  2017-09-23  3:38   ` [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item whydoubt
@ 2017-09-23 15:46     ` john
  2017-09-24  3:12       ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-23 15:46 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:42PM -0500, Jeff Smith wrote:
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  cgit.c       | 3 +++
>  cgit.h       | 1 +
>  cgitrc.5.txt | 5 +++++
>  3 files changed, 9 insertions(+)
> 
> diff --git a/cgit.c b/cgit.c
> index 1dae4b8..c03f69c 100644
> --- a/cgit.c
> +++ b/cgit.c
> @@ -183,6 +183,8 @@ static void config_cb(const char *name, const char *value)
>  		ctx.cfg.enable_tree_linenumbers = atoi(value);
>  	else if (!strcmp(name, "enable-git-config"))
>  		ctx.cfg.enable_git_config = atoi(value);
> +	else if (!strcmp(name, "enable-blame"))
> +		ctx.cfg.enable_blame = atoi(value);
>  	else if (!strcmp(name, "max-stats"))
>  		ctx.cfg.max_stats = cgit_find_stats_period(value, NULL);
>  	else if (!strcmp(name, "cache-size"))
> @@ -373,6 +375,7 @@ static void prepare_context(void)
>  	ctx.cfg.enable_index_owner = 1;
>  	ctx.cfg.enable_tree_linenumbers = 1;
>  	ctx.cfg.enable_git_config = 0;
> +	ctx.cfg.enable_blame = 1;

Should this default to false?  Either way, I think the commit message
should have some discussion of what the default is any why it has been
chosen.

>  	ctx.cfg.max_repo_count = 50;
>  	ctx.cfg.max_commit_count = 50;
>  	ctx.cfg.max_lock_attempts = 5;
> diff --git a/cgit.h b/cgit.h
> index fbc6c6a..a48e622 100644
> --- a/cgit.h
> +++ b/cgit.h
> @@ -236,6 +236,7 @@ struct cgit_config {
>  	int enable_html_serving;
>  	int enable_tree_linenumbers;
>  	int enable_git_config;
> +	int enable_blame;
>  	int local_time;
>  	int max_atom_items;
>  	int max_repo_count;
> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
> index 9fcf445..b236bf2 100644
> --- a/cgitrc.5.txt
> +++ b/cgitrc.5.txt
> @@ -211,6 +211,11 @@ enable-git-config::
>  	with "cgit." will be mapped to the corresponding "repo." key in cgit.
>  	Default value: "0". See also: scan-path, section-from-path.
>  
> +enable-blame::
> +	Flag which, when set to "1", will allow cgit to provide a "blame" page
> +	for files, and will make it generate links to that page in appropriate
> +	places. Default value: "1".
> +

Although the handful of enable-* options above this are not well sorted,
most of this file is, so this should move to above enable-commit-graph.

>  favicon::
>  	Url used as link to a shortcut icon for cgit. It is suggested to use
>  	the value "/favicon.ico" since certain browsers will ignore other


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 2/7] ui-blame: create framework
  2017-09-23  3:38   ` [RFCv2 PATCH 2/7] ui-blame: create framework whydoubt
@ 2017-09-23 15:47     ` john
  2017-09-24  3:24       ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-23 15:47 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:43PM -0500, Jeff Smith wrote:
> Create framework for a page that will contain the 'blame' for a file
> in the repository.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  ui-blame.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>  ui-blame.h |   7 +++
>  2 files changed, 167 insertions(+)
>  create mode 100644 ui-blame.c
>  create mode 100644 ui-blame.h
> 
> diff --git a/ui-blame.c b/ui-blame.c
> new file mode 100644
> index 0000000..901ca89
> --- /dev/null
> +++ b/ui-blame.c
> @@ -0,0 +1,160 @@
> +/* ui-blame.c: functions for blame output
> + *
> + * Copyright (C) 2006-2017 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"
> +
> +struct walk_tree_context {
> +	char *curr_rev;
> +	int match_baselen;
> +	int state;
> +};
> +
> +static void set_title_from_path(const char *path)

This looks exactly the same as the function in ui-tree.c, so can't we
extract it to ui-shared.c?

> +{
> +	size_t path_len, path_index, path_last_end;
> +	char *new_title;
> +
> +	if (!path)
> +		return;
> +
> +	path_len = strlen(path);
> +	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
> +	new_title[0] = '\0';
> +
> +	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
> +		if (path[path_index] == '/') {
> +			if (path_index == path_len - 1) {
> +				path_last_end = path_index - 1;
> +				continue;
> +			}
> +			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
> +			strcat(new_title, "\\");
> +			path_last_end = path_index;
> +		}
> +	}
> +	if (path_last_end)
> +		strncat(new_title, path, path_last_end);
> +
> +	strcat(new_title, " - ");
> +	strcat(new_title, ctx.page.title);
> +	ctx.page.title = new_title;
> +}


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 3/7] ui-blame: create links
  2017-09-23  3:38   ` [RFCv2 PATCH 3/7] ui-blame: create links whydoubt
@ 2017-09-23 15:47     ` john
  2017-09-24 20:25       ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-23 15:47 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:44PM -0500, Jeff Smith wrote:
> Create links to the blame page.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  ui-shared.c | 20 +++++++++++++++++---
>  ui-shared.h |  3 +++
>  ui-tree.c   | 10 +++++++++-
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index e5c9a02..faa0d6a 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -986,8 +996,12 @@ void cgit_print_pageheader(void)
>  		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
>  			      NULL, ctx.qry.vpath, 0, NULL, NULL,
>  			      ctx.qry.showmsg, ctx.qry.follow);
> -		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
> -			       ctx.qry.sha1, ctx.qry.vpath);
> +		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
> +			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
> +				        ctx.qry.sha1, ctx.qry.vpath);
> +		else
> +			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
> +				       ctx.qry.sha1, ctx.qry.vpath);

I'm curious what other people think of this link, I'm not sure it's
right but I can't think of a better behaviour.  In some sense it feels
like blame is a subtype of tree so we should still show the tree link
here, but that makes the "is it active?" check more complicated, and we
can't add blame unconditionally because it has no meaning if the path is
a tree rather than a blob.

>  		cgit_commit_link("commit", NULL, hc("commit"),
>  				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
>  		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
> diff --git a/ui-shared.h b/ui-shared.h
> index 87799f1..e5992bc 100644
> --- a/ui-shared.h
> +++ b/ui-shared.h
> @@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
>  extern void cgit_plain_link(const char *name, const char *title,
>  			    const char *class, const char *head,
>  			    const char *rev, const char *path);
> +extern void cgit_blame_link(const char *name, const char *title,
> +			    const char *class, const char *head,
> +			    const char *rev, const char *path);
>  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,
> diff --git a/ui-tree.c b/ui-tree.c
> index ca24a03..3513d5e 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -1,6 +1,6 @@
>  /* ui-tree.c: functions for tree output
>   *
> - * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
> + * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -141,6 +141,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);
> +	}
>  	html(")\n");
>  
>  	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
> @@ -275,6 +280,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
> +		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
> +				walk_tree_ctx->curr_rev, fullpath.buf);
>  	html("</td></tr>\n");
>  	free(name);
>  	strbuf_release(&fullpath);


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis
  2017-09-23  3:38   ` [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis whydoubt
@ 2017-09-23 15:47     ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-23 15:47 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:45PM -0500, Jeff Smith wrote:
> For implementing a ui-blame page, there is need for a function that
> outputs a selection from a block of text, transformed for HTML output,
> but with no further modifications or additions.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  html.c        | 37 ++++++++++++++++---------------------
>  html.h        |  1 +
>  ui-repolist.c |  2 +-
>  3 files changed, 18 insertions(+), 22 deletions(-)
> 
> diff --git a/html.c b/html.c
> index e7e6e07..345300e 100644
> --- a/html.c
> +++ b/html.c
> @@ -122,10 +122,10 @@ void html_vtxtf(const char *format, va_list ap)
>  	strbuf_release(&buf);
>  }
>  
> -void html_txt(const char *txt)
> +static int _html_txt(int len, const char *txt)

This should use ssize_t not int.

>  {
>  	const char *t = txt;
> -	while (t && *t) {
> +	while (t && *t && len--) {
>  		int c = *t;
>  		if (c == '<' || c == '>' || c == '&') {
>  			html_raw(txt, t - txt);
> @@ -140,29 +140,24 @@ void html_txt(const char *txt)
>  		t++;
>  	}
>  	if (t != txt)
> -		html(txt);
> +		html_raw(txt, t - txt);
> +	return len;
> +}
> +
> +void html_txt(const char *txt)
> +{
> +	if (txt)
> +		_html_txt(strlen(txt), txt);
>  }
>  
>  void html_ntxt(int len, const char *txt)
>  {
> -	const char *t = txt;
> -	while (t && *t && len--) {
> -		int c = *t;
> -		if (c == '<' || c == '>' || c == '&') {
> -			html_raw(txt, t - txt);
> -			if (c == '>')
> -				html("&gt;");
> -			else if (c == '<')
> -				html("&lt;");
> -			else if (c == '&')
> -				html("&amp;");
> -			txt = t + 1;
> -		}
> -		t++;
> -	}
> -	if (t != txt)
> -		html_raw(txt, t - txt);
> -	if (len < 0)
> +	_html_txt(len, txt);

Why bother with this wrapper function?  Just make html_ntxt be _html_txt
and ignore the result everywhere except html_ntxt_ellipsis.

> +}
> +
> +void html_ntxt_ellipsis(int len, const char *txt)
> +{
> +	if (_html_txt(len, txt) < 0)
>  		html("...");
>  }
>  
> diff --git a/html.h b/html.h
> index 1b24e55..de53430 100644
> --- a/html.h
> +++ b/html.h
> @@ -20,6 +20,7 @@ extern void html_attrf(const char *format,...);
>  
>  extern void html_txt(const char *txt);
>  extern void html_ntxt(int len, const char *txt);
> +extern void html_ntxt_ellipsis(int len, const char *txt);
>  extern void html_attr(const char *txt);
>  extern void html_url_path(const char *txt);
>  extern void html_url_arg(const char *txt);
> diff --git a/ui-repolist.c b/ui-repolist.c
> index 7272e87..77c33fd 100644
> --- a/ui-repolist.c
> +++ b/ui-repolist.c
> @@ -329,7 +329,7 @@ void cgit_print_repolist(void)
>  		repourl = cgit_repourl(ctx.repo->url);
>  		html_link_open(repourl, NULL, NULL);
>  		free(repourl);
> -		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
> +		html_ntxt_ellipsis(ctx.cfg.max_repodesc_len, ctx.repo->desc);
>  		html_link_close();
>  		html("</td><td>");
>  		if (ctx.cfg.enable_index_owner) {


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
  2017-09-23  3:38   ` [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit whydoubt
@ 2017-09-23 15:47     ` john
  2017-09-24 19:06       ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-23 15:47 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:46PM -0500, Jeff Smith wrote:
> Use the blame interface added in libgit to output the blame information
> of a file in the repository.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
> diff --git a/ui-blame.c b/ui-blame.c
> index 901ca89..cc4457a 100644
> --- a/ui-blame.c
> +++ b/ui-blame.c
> @@ -10,6 +10,183 @@
>  #include "ui-blame.h"
>  #include "html.h"
>  #include "ui-shared.h"
> +#include "argv-array.h"
> +#include "blame.h"
> +
> +
> +/*
> + * Information on commits, used for output.
> + */
> +struct commit_info {
> +	struct strbuf author;
> +	struct strbuf author_mail;
> +	struct ident_split author_ident;
> +
> +	/* filled only when asked for details */
> +	struct strbuf committer;
> +	struct strbuf committer_mail;
> +	struct ident_split committer_ident;
> +
> +	struct strbuf summary;
> +};

I think I commented on this last time, but I still don't understand why
this adds a new commit_info structure rather than reusing our existing
commitinfo and cgit_parse_commit(), especially since...

> +static void get_commit_info(struct commit *commit,
> +			    struct commit_info *ret,
> +			    int detailed)
> +{
> +	int len;
> +	const char *subject, *encoding;
> +	const char *message;
> +
> +	commit_info_init(ret);
> +
> +	encoding = get_log_output_encoding();

... this is wrong as our output encoding has nothing to do with the
environment but is specified by PAGE_ENCODING.

> +	message = logmsg_reencode(commit, NULL, encoding);
> +	get_ac_line(message, "\nauthor ",
> +		    &ret->author, &ret->author_mail,
> +		    &ret->author_ident);
> +
> +	if (!detailed) {
> +		unuse_commit_buffer(commit, message);
> +		return;
> +	}
> +
> +	get_ac_line(message, "\ncommitter ",
> +		    &ret->committer, &ret->committer_mail,
> +		    &ret->committer_ident);
> +
> +	len = find_commit_subject(message, &subject);
> +	if (len)
> +		strbuf_add(&ret->summary, subject, len);
> +	else
> +		strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
> +
> +	unuse_commit_buffer(commit, message);
> +}
> +
> +static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
> +{
> +	struct commit_info ci;
> +	struct strbuf detail = STRBUF_INIT;
> +
> +	get_commit_info(suspect->commit, &ci, 1);
> +
> +	strbuf_addf(&detail, "author  %s", ci.author.buf);
> +	if (!ctx.cfg.noplainemail)
> +		strbuf_addf(&detail, " %s", ci.author_mail.buf);
> +	strbuf_addf(&detail, "  %s\n",
> +		    show_ident_date(&ci.author_ident,
> +				    cgit_date_mode(DATE_ISO8601)));
> +
> +	strbuf_addf(&detail, "committer  %s", ci.committer.buf);
> +	if (!ctx.cfg.noplainemail)
> +		strbuf_addf(&detail, " %s", ci.committer_mail.buf);
> +	strbuf_addf(&detail, "  %s\n\n",
> +		    show_ident_date(&ci.committer_ident,
> +				    cgit_date_mode(DATE_ISO8601)));
> +
> +	strbuf_addbuf(&detail, &ci.summary);
> +
> +	commit_info_destroy(&ci);
> +	return strbuf_detach(&detail, NULL);
> +}
> +
> +static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
> +{
> +	struct blame_origin *suspect = ent->suspect;
> +	char hex[GIT_SHA1_HEXSZ + 1];
> +	char *detail, *abbrev;
> +	unsigned long lineno;
> +	const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
> +	const char *cp, *cpend;
> +
> +	oid_to_hex_r(hex, &suspect->commit->object.oid);
> +	detail = emit_one_suspect_detail(suspect, hex);
> +	abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
> +					    DEFAULT_ABBREV));

nit: I don't think there's any need to strdup for abbrev, we use the
result immediately so the static buffer won't get overwritten.

> +	html("<tr><td class='lines'><pre>");
> +	cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
> +	html("</pre></td>\n");
> +
> +	free(detail);
> +	free(abbrev);
> +
> +	if (ctx.cfg.enable_tree_linenumbers) {
> +		html("<td class='linenumbers'><pre>");
> +		lineno = ent->lno;
> +		while (lineno < ent->lno + ent->num_lines)
> +			htmlf(numberfmt, ++lineno);
> +		html("</pre></td>\n");
> +	}
> +
> +	cp = blame_nth_line(sb, ent->lno);
> +	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
> +
> +	html("<td class='lines'><pre><code>");
> +	html_ntxt(cpend - cp, cp);
> +	html("</code></pre></td></tr>\n");
> +}
>  
>  struct walk_tree_context {
>  	char *curr_rev;
> @@ -47,10 +224,16 @@ static void set_title_from_path(const char *path)
>  	strcat(new_title, ctx.page.title);
>  	ctx.page.title = new_title;
>  }
> +
>  static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
>  {
>  	enum object_type type;
>  	unsigned long size;
> +	struct argv_array rev_argv = ARGV_ARRAY_INIT;
> +	struct rev_info revs;
> +	struct blame_scoreboard sb;
> +	struct blame_origin *o;
> +	struct blame_entry *ent = NULL;
>  
>  	type = sha1_object_info(sha1, &size);
>  	if (type == OBJ_BAD) {
> @@ -59,7 +242,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>  		return;
>  	}
>  
> -	/* Read in applicable data here */
> +	argv_array_push(&rev_argv, "blame");
> +	argv_array_push(&rev_argv, rev);
> +	init_revisions(&revs, NULL);
> +	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
> +	setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
> +	init_scoreboard(&sb);
> +	sb.revs = &revs;
> +	setup_scoreboard(&sb, path, &o);
> +	o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
> +	prio_queue_put(&sb.commits, o->commit);
> +	blame_origin_decref(o);
> +	sb.ent = NULL;
> +	sb.path = path;
> +	assign_blame(&sb, 0);
> +	blame_sort_final(&sb);
> +	blame_coalesce(&sb);
>  
>  	set_title_from_path(path);
>  
> @@ -76,7 +274,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>  		return;
>  	}
>  
> -	/* Output data here */
> +	html("<table class='blame blob'>");
> +	for (ent = sb.ent; ent; ) {
> +		struct blame_entry *e = ent->next;
> +		emit_blame_entry(&sb, ent);
> +		free(ent);
> +		ent = e;
> +	}
> +	html("</table>\n");
> +	free((void *)sb.final_buf);
>  
>  	cgit_print_layout_end();
>  	return;


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 0/7] Add ui-blame
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (6 preceding siblings ...)
  2017-09-23  3:38   ` [RFCv2 PATCH 7/7] ui-blame: generate blame page when requested whydoubt
@ 2017-09-23 15:53   ` john
  2017-09-24  3:05     ` whydoubt
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
  8 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-23 15:53 UTC (permalink / raw)


On Fri, Sep 22, 2017 at 10:38:41PM -0500, Jeff Smith wrote:
> I split git blame functionality into libgit, and the changes were
> accepted upstream and are a part of git 2.14.  Now that the git
> infrastructure is in place, here is what is needed for cgit to make use
> of it.
> 
> Jeff Smith (7):
>   ui-blame: create enable-blame config item
>   ui-blame: create framework
>   ui-blame: create links
>   ui-blame: html_ntxt with no ellipsis
>   ui-blame: pull blame info from libgit
>   ui-blame: begin building
>   ui-blame: generate blame page when requested

I still find the arrangement of this patch series a bit strange, I think
it should be more like:

- html: html_ntxt with no ellipsis
- ui-tree: move set_title_from_path to ui-shared
- ui-blame: add blame UI
	A squashed version of your "begin building" and "pull blame info
	from libgit"; splitting them up just makes it harder to review

	I think it also makes sense to squash the config, cmd and
	Makefile changes into this patch so that the feature springs
	into life fully formed, and then the following patches just link
	it into place
- ui-tree: link to blame UI if enabled


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 0/7] Add ui-blame
  2017-09-23 15:53   ` [RFCv2 PATCH 0/7] Add ui-blame john
@ 2017-09-24  3:05     ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-24  3:05 UTC (permalink / raw)


I will start re-arranging the patches to follow your suggestion.

On Sat, Sep 23, 2017 at 10:53 AM, John Keeping <john at keeping.me.uk> wrote:
> On Fri, Sep 22, 2017 at 10:38:41PM -0500, Jeff Smith wrote:
>> I split git blame functionality into libgit, and the changes were
>> accepted upstream and are a part of git 2.14.  Now that the git
>> infrastructure is in place, here is what is needed for cgit to make use
>> of it.
>>
>> Jeff Smith (7):
>>   ui-blame: create enable-blame config item
>>   ui-blame: create framework
>>   ui-blame: create links
>>   ui-blame: html_ntxt with no ellipsis
>>   ui-blame: pull blame info from libgit
>>   ui-blame: begin building
>>   ui-blame: generate blame page when requested
>
> I still find the arrangement of this patch series a bit strange, I think
> it should be more like:
>
> - html: html_ntxt with no ellipsis
> - ui-tree: move set_title_from_path to ui-shared
> - ui-blame: add blame UI
>         A squashed version of your "begin building" and "pull blame info
>         from libgit"; splitting them up just makes it harder to review
>
>         I think it also makes sense to squash the config, cmd and
>         Makefile changes into this patch so that the feature springs
>         into life fully formed, and then the following patches just link
>         it into place
> - ui-tree: link to blame UI if enabled


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item
  2017-09-23 15:46     ` john
@ 2017-09-24  3:12       ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-24  3:12 UTC (permalink / raw)


I don't have a strong opinion one way or the other.
I suppose setting it to false would avoid springing the potential
added resource use on people, as blame can be a relatively
expensive operation.


On Sat, Sep 23, 2017 at 10:46 AM, John Keeping <john at keeping.me.uk> wrote:
> On Fri, Sep 22, 2017 at 10:38:42PM -0500, Jeff Smith wrote:
>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>> ---
>>  cgit.c       | 3 +++
>>  cgit.h       | 1 +
>>  cgitrc.5.txt | 5 +++++
>>  3 files changed, 9 insertions(+)
>>
>> diff --git a/cgit.c b/cgit.c
>> index 1dae4b8..c03f69c 100644
>> --- a/cgit.c
>> +++ b/cgit.c
>> @@ -183,6 +183,8 @@ static void config_cb(const char *name, const char *value)
>>               ctx.cfg.enable_tree_linenumbers = atoi(value);
>>       else if (!strcmp(name, "enable-git-config"))
>>               ctx.cfg.enable_git_config = atoi(value);
>> +     else if (!strcmp(name, "enable-blame"))
>> +             ctx.cfg.enable_blame = atoi(value);
>>       else if (!strcmp(name, "max-stats"))
>>               ctx.cfg.max_stats = cgit_find_stats_period(value, NULL);
>>       else if (!strcmp(name, "cache-size"))
>> @@ -373,6 +375,7 @@ static void prepare_context(void)
>>       ctx.cfg.enable_index_owner = 1;
>>       ctx.cfg.enable_tree_linenumbers = 1;
>>       ctx.cfg.enable_git_config = 0;
>> +     ctx.cfg.enable_blame = 1;
>
> Should this default to false?  Either way, I think the commit message
> should have some discussion of what the default is any why it has been
> chosen.
>
>>       ctx.cfg.max_repo_count = 50;
>>       ctx.cfg.max_commit_count = 50;
>>       ctx.cfg.max_lock_attempts = 5;
>> diff --git a/cgit.h b/cgit.h
>> index fbc6c6a..a48e622 100644
>> --- a/cgit.h
>> +++ b/cgit.h
>> @@ -236,6 +236,7 @@ struct cgit_config {
>>       int enable_html_serving;
>>       int enable_tree_linenumbers;
>>       int enable_git_config;
>> +     int enable_blame;
>>       int local_time;
>>       int max_atom_items;
>>       int max_repo_count;
>> diff --git a/cgitrc.5.txt b/cgitrc.5.txt
>> index 9fcf445..b236bf2 100644
>> --- a/cgitrc.5.txt
>> +++ b/cgitrc.5.txt
>> @@ -211,6 +211,11 @@ enable-git-config::
>>       with "cgit." will be mapped to the corresponding "repo." key in cgit.
>>       Default value: "0". See also: scan-path, section-from-path.
>>
>> +enable-blame::
>> +     Flag which, when set to "1", will allow cgit to provide a "blame" page
>> +     for files, and will make it generate links to that page in appropriate
>> +     places. Default value: "1".
>> +
>
> Although the handful of enable-* options above this are not well sorted,
> most of this file is, so this should move to above enable-commit-graph.
>
>>  favicon::
>>       Url used as link to a shortcut icon for cgit. It is suggested to use
>>       the value "/favicon.ico" since certain browsers will ignore other


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 2/7] ui-blame: create framework
  2017-09-23 15:47     ` john
@ 2017-09-24  3:24       ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-24  3:24 UTC (permalink / raw)


Yes, it is identical.
I will look at extracting to ui-shared.c as you suggest.

On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
> On Fri, Sep 22, 2017 at 10:38:43PM -0500, Jeff Smith wrote:
>> Create framework for a page that will contain the 'blame' for a file
>> in the repository.
>>
>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>> ---
>>  ui-blame.c | 160 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
>>  ui-blame.h |   7 +++
>>  2 files changed, 167 insertions(+)
>>  create mode 100644 ui-blame.c
>>  create mode 100644 ui-blame.h
>>
>> diff --git a/ui-blame.c b/ui-blame.c
>> new file mode 100644
>> index 0000000..901ca89
>> --- /dev/null
>> +++ b/ui-blame.c
>> @@ -0,0 +1,160 @@
>> +/* ui-blame.c: functions for blame output
>> + *
>> + * Copyright (C) 2006-2017 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"
>> +
>> +struct walk_tree_context {
>> +     char *curr_rev;
>> +     int match_baselen;
>> +     int state;
>> +};
>> +
>> +static void set_title_from_path(const char *path)
>
> This looks exactly the same as the function in ui-tree.c, so can't we
> extract it to ui-shared.c?
>
>> +{
>> +     size_t path_len, path_index, path_last_end;
>> +     char *new_title;
>> +
>> +     if (!path)
>> +             return;
>> +
>> +     path_len = strlen(path);
>> +     new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
>> +     new_title[0] = '\0';
>> +
>> +     for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
>> +             if (path[path_index] == '/') {
>> +                     if (path_index == path_len - 1) {
>> +                             path_last_end = path_index - 1;
>> +                             continue;
>> +                     }
>> +                     strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
>> +                     strcat(new_title, "\\");
>> +                     path_last_end = path_index;
>> +             }
>> +     }
>> +     if (path_last_end)
>> +             strncat(new_title, path, path_last_end);
>> +
>> +     strcat(new_title, " - ");
>> +     strcat(new_title, ctx.page.title);
>> +     ctx.page.title = new_title;
>> +}


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
  2017-09-23 15:47     ` john
@ 2017-09-24 19:06       ` whydoubt
  2017-09-24 20:09         ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-24 19:06 UTC (permalink / raw)


A few of the original suggestions apparently got lost in the shuffle.
Anyway, it appears that making your suggested structure change
should clean some things up a lot.

On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
> On Fri, Sep 22, 2017 at 10:38:46PM -0500, Jeff Smith wrote:
>> Use the blame interface added in libgit to output the blame information
>> of a file in the repository.
>>
>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>> ---
>> diff --git a/ui-blame.c b/ui-blame.c
>> index 901ca89..cc4457a 100644
>> --- a/ui-blame.c
>> +++ b/ui-blame.c
>> @@ -10,6 +10,183 @@
>>  #include "ui-blame.h"
>>  #include "html.h"
>>  #include "ui-shared.h"
>> +#include "argv-array.h"
>> +#include "blame.h"
>> +
>> +
>> +/*
>> + * Information on commits, used for output.
>> + */
>> +struct commit_info {
>> +     struct strbuf author;
>> +     struct strbuf author_mail;
>> +     struct ident_split author_ident;
>> +
>> +     /* filled only when asked for details */
>> +     struct strbuf committer;
>> +     struct strbuf committer_mail;
>> +     struct ident_split committer_ident;
>> +
>> +     struct strbuf summary;
>> +};
>
> I think I commented on this last time, but I still don't understand why
> this adds a new commit_info structure rather than reusing our existing
> commitinfo and cgit_parse_commit(), especially since...
>
>> +static void get_commit_info(struct commit *commit,
>> +                         struct commit_info *ret,
>> +                         int detailed)
>> +{
>> +     int len;
>> +     const char *subject, *encoding;
>> +     const char *message;
>> +
>> +     commit_info_init(ret);
>> +
>> +     encoding = get_log_output_encoding();
>
> ... this is wrong as our output encoding has nothing to do with the
> environment but is specified by PAGE_ENCODING.
>
>> +     message = logmsg_reencode(commit, NULL, encoding);
>> +     get_ac_line(message, "\nauthor ",
>> +                 &ret->author, &ret->author_mail,
>> +                 &ret->author_ident);
>> +
>> +     if (!detailed) {
>> +             unuse_commit_buffer(commit, message);
>> +             return;
>> +     }
>> +
>> +     get_ac_line(message, "\ncommitter ",
>> +                 &ret->committer, &ret->committer_mail,
>> +                 &ret->committer_ident);
>> +
>> +     len = find_commit_subject(message, &subject);
>> +     if (len)
>> +             strbuf_add(&ret->summary, subject, len);
>> +     else
>> +             strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
>> +
>> +     unuse_commit_buffer(commit, message);
>> +}
>> +
>> +static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
>> +{
>> +     struct commit_info ci;
>> +     struct strbuf detail = STRBUF_INIT;
>> +
>> +     get_commit_info(suspect->commit, &ci, 1);
>> +
>> +     strbuf_addf(&detail, "author  %s", ci.author.buf);
>> +     if (!ctx.cfg.noplainemail)
>> +             strbuf_addf(&detail, " %s", ci.author_mail.buf);
>> +     strbuf_addf(&detail, "  %s\n",
>> +                 show_ident_date(&ci.author_ident,
>> +                                 cgit_date_mode(DATE_ISO8601)));
>> +
>> +     strbuf_addf(&detail, "committer  %s", ci.committer.buf);
>> +     if (!ctx.cfg.noplainemail)
>> +             strbuf_addf(&detail, " %s", ci.committer_mail.buf);
>> +     strbuf_addf(&detail, "  %s\n\n",
>> +                 show_ident_date(&ci.committer_ident,
>> +                                 cgit_date_mode(DATE_ISO8601)));
>> +
>> +     strbuf_addbuf(&detail, &ci.summary);
>> +
>> +     commit_info_destroy(&ci);
>> +     return strbuf_detach(&detail, NULL);
>> +}
>> +
>> +static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
>> +{
>> +     struct blame_origin *suspect = ent->suspect;
>> +     char hex[GIT_SHA1_HEXSZ + 1];
>> +     char *detail, *abbrev;
>> +     unsigned long lineno;
>> +     const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
>> +     const char *cp, *cpend;
>> +
>> +     oid_to_hex_r(hex, &suspect->commit->object.oid);
>> +     detail = emit_one_suspect_detail(suspect, hex);
>> +     abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
>> +                                         DEFAULT_ABBREV));
>
> nit: I don't think there's any need to strdup for abbrev, we use the
> result immediately so the static buffer won't get overwritten.
>
>> +     html("<tr><td class='lines'><pre>");
>> +     cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
>> +     html("</pre></td>\n");
>> +
>> +     free(detail);
>> +     free(abbrev);
>> +
>> +     if (ctx.cfg.enable_tree_linenumbers) {
>> +             html("<td class='linenumbers'><pre>");
>> +             lineno = ent->lno;
>> +             while (lineno < ent->lno + ent->num_lines)
>> +                     htmlf(numberfmt, ++lineno);
>> +             html("</pre></td>\n");
>> +     }
>> +
>> +     cp = blame_nth_line(sb, ent->lno);
>> +     cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
>> +
>> +     html("<td class='lines'><pre><code>");
>> +     html_ntxt(cpend - cp, cp);
>> +     html("</code></pre></td></tr>\n");
>> +}
>>
>>  struct walk_tree_context {
>>       char *curr_rev;
>> @@ -47,10 +224,16 @@ static void set_title_from_path(const char *path)
>>       strcat(new_title, ctx.page.title);
>>       ctx.page.title = new_title;
>>  }
>> +
>>  static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
>>  {
>>       enum object_type type;
>>       unsigned long size;
>> +     struct argv_array rev_argv = ARGV_ARRAY_INIT;
>> +     struct rev_info revs;
>> +     struct blame_scoreboard sb;
>> +     struct blame_origin *o;
>> +     struct blame_entry *ent = NULL;
>>
>>       type = sha1_object_info(sha1, &size);
>>       if (type == OBJ_BAD) {
>> @@ -59,7 +242,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>               return;
>>       }
>>
>> -     /* Read in applicable data here */
>> +     argv_array_push(&rev_argv, "blame");
>> +     argv_array_push(&rev_argv, rev);
>> +     init_revisions(&revs, NULL);
>> +     DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>> +     setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
>> +     init_scoreboard(&sb);
>> +     sb.revs = &revs;
>> +     setup_scoreboard(&sb, path, &o);
>> +     o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
>> +     prio_queue_put(&sb.commits, o->commit);
>> +     blame_origin_decref(o);
>> +     sb.ent = NULL;
>> +     sb.path = path;
>> +     assign_blame(&sb, 0);
>> +     blame_sort_final(&sb);
>> +     blame_coalesce(&sb);
>>
>>       set_title_from_path(path);
>>
>> @@ -76,7 +274,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>               return;
>>       }
>>
>> -     /* Output data here */
>> +     html("<table class='blame blob'>");
>> +     for (ent = sb.ent; ent; ) {
>> +             struct blame_entry *e = ent->next;
>> +             emit_blame_entry(&sb, ent);
>> +             free(ent);
>> +             ent = e;
>> +     }
>> +     html("</table>\n");
>> +     free((void *)sb.final_buf);
>>
>>       cgit_print_layout_end();
>>       return;


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
  2017-09-24 19:06       ` whydoubt
@ 2017-09-24 20:09         ` whydoubt
  2017-09-24 20:52           ` john
  0 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-24 20:09 UTC (permalink / raw)


On Sun, Sep 24, 2017 at 2:06 PM, Jeffrey Smith <whydoubt at gmail.com> wrote:
> A few of the original suggestions apparently got lost in the shuffle.
> Anyway, it appears that making your suggested structure change
> should clean some things up a lot.
>
> On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
>> On Fri, Sep 22, 2017 at 10:38:46PM -0500, Jeff Smith wrote:
>>> Use the blame interface added in libgit to output the blame information
>>> of a file in the repository.
>>>
>>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>>> ---
>>> diff --git a/ui-blame.c b/ui-blame.c
>>> index 901ca89..cc4457a 100644
>>> --- a/ui-blame.c
>>> +++ b/ui-blame.c
>>> @@ -10,6 +10,183 @@
>>>  #include "ui-blame.h"
>>>  #include "html.h"
>>>  #include "ui-shared.h"
>>> +#include "argv-array.h"
>>> +#include "blame.h"
>>> +
>>> +
>>> +/*
>>> + * Information on commits, used for output.
>>> + */
>>> +struct commit_info {
>>> +     struct strbuf author;
>>> +     struct strbuf author_mail;
>>> +     struct ident_split author_ident;
>>> +
>>> +     /* filled only when asked for details */
>>> +     struct strbuf committer;
>>> +     struct strbuf committer_mail;
>>> +     struct ident_split committer_ident;
>>> +
>>> +     struct strbuf summary;
>>> +};
>>
>> I think I commented on this last time, but I still don't understand why
>> this adds a new commit_info structure rather than reusing our existing
>> commitinfo and cgit_parse_commit(), especially since...
>>
>>> +static void get_commit_info(struct commit *commit,
>>> +                         struct commit_info *ret,
>>> +                         int detailed)
>>> +{
>>> +     int len;
>>> +     const char *subject, *encoding;
>>> +     const char *message;
>>> +
>>> +     commit_info_init(ret);
>>> +
>>> +     encoding = get_log_output_encoding();
>>
>> ... this is wrong as our output encoding has nothing to do with the
>> environment but is specified by PAGE_ENCODING.
>>
>>> +     message = logmsg_reencode(commit, NULL, encoding);
>>> +     get_ac_line(message, "\nauthor ",
>>> +                 &ret->author, &ret->author_mail,
>>> +                 &ret->author_ident);
>>> +
>>> +     if (!detailed) {
>>> +             unuse_commit_buffer(commit, message);
>>> +             return;
>>> +     }
>>> +
>>> +     get_ac_line(message, "\ncommitter ",
>>> +                 &ret->committer, &ret->committer_mail,
>>> +                 &ret->committer_ident);
>>> +
>>> +     len = find_commit_subject(message, &subject);
>>> +     if (len)
>>> +             strbuf_add(&ret->summary, subject, len);
>>> +     else
>>> +             strbuf_addf(&ret->summary, "(%s)", oid_to_hex(&commit->object.oid));
>>> +
>>> +     unuse_commit_buffer(commit, message);
>>> +}
>>> +
>>> +static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
>>> +{
>>> +     struct commit_info ci;
>>> +     struct strbuf detail = STRBUF_INIT;
>>> +
>>> +     get_commit_info(suspect->commit, &ci, 1);
>>> +
>>> +     strbuf_addf(&detail, "author  %s", ci.author.buf);
>>> +     if (!ctx.cfg.noplainemail)
>>> +             strbuf_addf(&detail, " %s", ci.author_mail.buf);
>>> +     strbuf_addf(&detail, "  %s\n",
>>> +                 show_ident_date(&ci.author_ident,
>>> +                                 cgit_date_mode(DATE_ISO8601)));
>>> +
>>> +     strbuf_addf(&detail, "committer  %s", ci.committer.buf);
>>> +     if (!ctx.cfg.noplainemail)
>>> +             strbuf_addf(&detail, " %s", ci.committer_mail.buf);
>>> +     strbuf_addf(&detail, "  %s\n\n",
>>> +                 show_ident_date(&ci.committer_ident,
>>> +                                 cgit_date_mode(DATE_ISO8601)));
>>> +
>>> +     strbuf_addbuf(&detail, &ci.summary);
>>> +
>>> +     commit_info_destroy(&ci);
>>> +     return strbuf_detach(&detail, NULL);
>>> +}
>>> +
>>> +static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
>>> +{
>>> +     struct blame_origin *suspect = ent->suspect;
>>> +     char hex[GIT_SHA1_HEXSZ + 1];
>>> +     char *detail, *abbrev;
>>> +     unsigned long lineno;
>>> +     const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
>>> +     const char *cp, *cpend;
>>> +
>>> +     oid_to_hex_r(hex, &suspect->commit->object.oid);
>>> +     detail = emit_one_suspect_detail(suspect, hex);
>>> +     abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
>>> +                                         DEFAULT_ABBREV));
>>
>> nit: I don't think there's any need to strdup for abbrev, we use the
>> result immediately so the static buffer won't get overwritten.

But find_unique_abbrev() returns a const char*, and abbrev is passed
as the first parameter
to cgit_commit_link(), which is char* (and the function can in fact
change the contents).

I will add a patch to the series that avoids altering the first
parameter of cgit_commit_link
so that it will be const char*.

>>
>>> +     html("<tr><td class='lines'><pre>");
>>> +     cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
>>> +     html("</pre></td>\n");
>>> +
>>> +     free(detail);
>>> +     free(abbrev);
>>> +
>>> +     if (ctx.cfg.enable_tree_linenumbers) {
>>> +             html("<td class='linenumbers'><pre>");
>>> +             lineno = ent->lno;
>>> +             while (lineno < ent->lno + ent->num_lines)
>>> +                     htmlf(numberfmt, ++lineno);
>>> +             html("</pre></td>\n");
>>> +     }
>>> +
>>> +     cp = blame_nth_line(sb, ent->lno);
>>> +     cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
>>> +
>>> +     html("<td class='lines'><pre><code>");
>>> +     html_ntxt(cpend - cp, cp);
>>> +     html("</code></pre></td></tr>\n");
>>> +}
>>>
>>>  struct walk_tree_context {
>>>       char *curr_rev;
>>> @@ -47,10 +224,16 @@ static void set_title_from_path(const char *path)
>>>       strcat(new_title, ctx.page.title);
>>>       ctx.page.title = new_title;
>>>  }
>>> +
>>>  static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
>>>  {
>>>       enum object_type type;
>>>       unsigned long size;
>>> +     struct argv_array rev_argv = ARGV_ARRAY_INIT;
>>> +     struct rev_info revs;
>>> +     struct blame_scoreboard sb;
>>> +     struct blame_origin *o;
>>> +     struct blame_entry *ent = NULL;
>>>
>>>       type = sha1_object_info(sha1, &size);
>>>       if (type == OBJ_BAD) {
>>> @@ -59,7 +242,22 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>>               return;
>>>       }
>>>
>>> -     /* Read in applicable data here */
>>> +     argv_array_push(&rev_argv, "blame");
>>> +     argv_array_push(&rev_argv, rev);
>>> +     init_revisions(&revs, NULL);
>>> +     DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
>>> +     setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
>>> +     init_scoreboard(&sb);
>>> +     sb.revs = &revs;
>>> +     setup_scoreboard(&sb, path, &o);
>>> +     o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
>>> +     prio_queue_put(&sb.commits, o->commit);
>>> +     blame_origin_decref(o);
>>> +     sb.ent = NULL;
>>> +     sb.path = path;
>>> +     assign_blame(&sb, 0);
>>> +     blame_sort_final(&sb);
>>> +     blame_coalesce(&sb);
>>>
>>>       set_title_from_path(path);
>>>
>>> @@ -76,7 +274,15 @@ static void print_object(const unsigned char *sha1, const char *path, const char
>>>               return;
>>>       }
>>>
>>> -     /* Output data here */
>>> +     html("<table class='blame blob'>");
>>> +     for (ent = sb.ent; ent; ) {
>>> +             struct blame_entry *e = ent->next;
>>> +             emit_blame_entry(&sb, ent);
>>> +             free(ent);
>>> +             ent = e;
>>> +     }
>>> +     html("</table>\n");
>>> +     free((void *)sb.final_buf);
>>>
>>>       cgit_print_layout_end();
>>>       return;


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 3/7] ui-blame: create links
  2017-09-23 15:47     ` john
@ 2017-09-24 20:25       ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-09-24 20:25 UTC (permalink / raw)


That pretty well sums up the dilemma I had in coming up with this solution.
It's not great, but it was the best compromise that I've found.
I would be as glad as anyone for a cleaner design.

On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
> On Fri, Sep 22, 2017 at 10:38:44PM -0500, Jeff Smith wrote:
>> Create links to the blame page.
>>
>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
>> ---
>>  ui-shared.c | 20 +++++++++++++++++---
>>  ui-shared.h |  3 +++
>>  ui-tree.c   | 10 +++++++++-
>>  3 files changed, 29 insertions(+), 4 deletions(-)
>>
>> diff --git a/ui-shared.c b/ui-shared.c
>> index e5c9a02..faa0d6a 100644
>> --- a/ui-shared.c
>> +++ b/ui-shared.c
>> @@ -986,8 +996,12 @@ void cgit_print_pageheader(void)
>>               cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
>>                             NULL, ctx.qry.vpath, 0, NULL, NULL,
>>                             ctx.qry.showmsg, ctx.qry.follow);
>> -             cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
>> -                            ctx.qry.sha1, ctx.qry.vpath);
>> +             if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
>> +                     cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
>> +                                     ctx.qry.sha1, ctx.qry.vpath);
>> +             else
>> +                     cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
>> +                                    ctx.qry.sha1, ctx.qry.vpath);
>
> I'm curious what other people think of this link, I'm not sure it's
> right but I can't think of a better behaviour.  In some sense it feels
> like blame is a subtype of tree so we should still show the tree link
> here, but that makes the "is it active?" check more complicated, and we
> can't add blame unconditionally because it has no meaning if the path is
> a tree rather than a blob.
>
>>               cgit_commit_link("commit", NULL, hc("commit"),
>>                                ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
>>               cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
>> diff --git a/ui-shared.h b/ui-shared.h
>> index 87799f1..e5992bc 100644
>> --- a/ui-shared.h
>> +++ b/ui-shared.h
>> @@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
>>  extern void cgit_plain_link(const char *name, const char *title,
>>                           const char *class, const char *head,
>>                           const char *rev, const char *path);
>> +extern void cgit_blame_link(const char *name, const char *title,
>> +                         const char *class, const char *head,
>> +                         const char *rev, const char *path);
>>  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,
>> diff --git a/ui-tree.c b/ui-tree.c
>> index ca24a03..3513d5e 100644
>> --- a/ui-tree.c
>> +++ b/ui-tree.c
>> @@ -1,6 +1,6 @@
>>  /* ui-tree.c: functions for tree output
>>   *
>> - * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
>> + * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
>>   *
>>   * Licensed under GNU General Public License v2
>>   *   (see COPYING for full license text)
>> @@ -141,6 +141,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);
>> +     }
>>       html(")\n");
>>
>>       if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
>> @@ -275,6 +280,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
>> +             cgit_blame_link("blame", NULL, "button", ctx.qry.head,
>> +                             walk_tree_ctx->curr_rev, fullpath.buf);
>>       html("</td></tr>\n");
>>       free(name);
>>       strbuf_release(&fullpath);


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit
  2017-09-24 20:09         ` whydoubt
@ 2017-09-24 20:52           ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-24 20:52 UTC (permalink / raw)


On Sun, Sep 24, 2017 at 03:09:53PM -0500, Jeffrey Smith wrote:
> On Sun, Sep 24, 2017 at 2:06 PM, Jeffrey Smith <whydoubt at gmail.com> wrote:
> > A few of the original suggestions apparently got lost in the shuffle.
> > Anyway, it appears that making your suggested structure change
> > should clean some things up a lot.
> >
> > On Sat, Sep 23, 2017 at 10:47 AM, John Keeping <john at keeping.me.uk> wrote:
> >> On Fri, Sep 22, 2017 at 10:38:46PM -0500, Jeff Smith wrote:
> >>> Use the blame interface added in libgit to output the blame information
> >>> of a file in the repository.
> >>>
> >>> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> >>> ---
> >>> diff --git a/ui-blame.c b/ui-blame.c
> >>> index 901ca89..cc4457a 100644
> >>> --- a/ui-blame.c
> >>> +++ b/ui-blame.c
> >>> +static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
> >>> +{
> >>> +     struct blame_origin *suspect = ent->suspect;
> >>> +     char hex[GIT_SHA1_HEXSZ + 1];
> >>> +     char *detail, *abbrev;
> >>> +     unsigned long lineno;
> >>> +     const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
> >>> +     const char *cp, *cpend;
> >>> +
> >>> +     oid_to_hex_r(hex, &suspect->commit->object.oid);
> >>> +     detail = emit_one_suspect_detail(suspect, hex);
> >>> +     abbrev = xstrdup(find_unique_abbrev(suspect->commit->object.oid.hash,
> >>> +                                         DEFAULT_ABBREV));
> >>
> >> nit: I don't think there's any need to strdup for abbrev, we use the
> >> result immediately so the static buffer won't get overwritten.
> 
> But find_unique_abbrev() returns a const char*, and abbrev is passed
> as the first parameter
> to cgit_commit_link(), which is char* (and the function can in fact
> change the contents).
> 
> I will add a patch to the series that avoids altering the first
> parameter of cgit_commit_link
> so that it will be const char*.

The strdup approach is probably better than requiring cgit_commit_link()
to make a copy of its argument.  I hadn't spotted that
find_unique_abbrev() returned a const char*.


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
                     ` (7 preceding siblings ...)
  2017-09-23 15:53   ` [RFCv2 PATCH 0/7] Add ui-blame john
@ 2017-09-27 22:43   ` whydoubt
  2017-09-27 22:43     ` [PATCH 1/5] html: html_ntxt with no ellipsis whydoubt
                       ` (6 more replies)
  8 siblings, 7 replies; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


I split git blame functionality into libgit, and the changes were
accepted upstream and are a part of git 2.14.  Now that the git
infrastructure is in place, here is what is needed for cgit to make use
of it.

Since RFCv2:
. Re-ordered and squashed some of the patches
. Used cgit's commitinfo structure
. Factored set_title_from_path out to ui-shared
. Made cgit_commit_link's first parameter const char*
. Disabled the feature by default
. Simplified changes to html_ntxt / html_txt

Jeff Smith (5):
  html: html_ntxt with no ellipsis
  ui-tree: move set_title_from_path to ui-shared
  ui-shared: make a char* parameter const
  ui-blame: add blame UI
  ui-tree: link to blame UI if enabled

 cgit.c        |   2 +
 cgit.css      |   8 +++
 cgit.h        |   1 +
 cgit.mk       |   1 +
 cgitrc.5.txt  |   9 +++
 cmd.c         |  12 +++-
 html.c        |  24 ++-----
 html.h        |   2 +-
 ui-blame.c    | 224 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h    |   7 ++
 ui-repolist.c |   3 +-
 ui-shared.c   |  70 ++++++++++++++----
 ui-shared.h   |   7 +-
 ui-tree.c     |  41 +++--------
 14 files changed, 341 insertions(+), 70 deletions(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 1/5] html: html_ntxt with no ellipsis
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
@ 2017-09-27 22:43     ` whydoubt
  2017-09-30 11:55       ` john
  2017-09-27 22:43     ` [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
                       ` (5 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


For implementing a ui-blame page, there is need for a function that
outputs a selection from a block of text, transformed for HTML output,
but with no further modifications or additions.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 html.c        | 24 ++++--------------------
 html.h        |  2 +-
 ui-repolist.c |  3 ++-
 3 files changed, 7 insertions(+), 22 deletions(-)

diff --git a/html.c b/html.c
index e7e6e07..87ef5aa 100644
--- a/html.c
+++ b/html.c
@@ -124,26 +124,11 @@ void html_vtxtf(const char *format, va_list ap)
 
 void html_txt(const char *txt)
 {
-	const char *t = txt;
-	while (t && *t) {
-		int c = *t;
-		if (c == '<' || c == '>' || c == '&') {
-			html_raw(txt, t - txt);
-			if (c == '>')
-				html("&gt;");
-			else if (c == '<')
-				html("&lt;");
-			else if (c == '&')
-				html("&amp;");
-			txt = t + 1;
-		}
-		t++;
-	}
-	if (t != txt)
-		html(txt);
+	if (txt)
+		html_ntxt(strlen(txt), txt);
 }
 
-void html_ntxt(int len, const char *txt)
+int html_ntxt(int len, const char *txt)
 {
 	const char *t = txt;
 	while (t && *t && len--) {
@@ -162,8 +147,7 @@ void html_ntxt(int len, const char *txt)
 	}
 	if (t != txt)
 		html_raw(txt, t - txt);
-	if (len < 0)
-		html("...");
+	return len;
 }
 
 void html_attrf(const char *fmt, ...)
diff --git a/html.h b/html.h
index 1b24e55..18a71d1 100644
--- a/html.h
+++ b/html.h
@@ -19,7 +19,7 @@ __attribute__((format (printf,1,2)))
 extern void html_attrf(const char *format,...);
 
 extern void html_txt(const char *txt);
-extern void html_ntxt(int len, const char *txt);
+extern int html_ntxt(int len, const char *txt);
 extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
 extern void html_url_arg(const char *txt);
diff --git a/ui-repolist.c b/ui-repolist.c
index 7272e87..089113d 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -329,7 +329,8 @@ void cgit_print_repolist(void)
 		repourl = cgit_repourl(ctx.repo->url);
 		html_link_open(repourl, NULL, NULL);
 		free(repourl);
-		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
+		if (html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc) < 0)
+			html("...");
 		html_link_close();
 		html("</td><td>");
 		if (ctx.cfg.enable_index_owner) {
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
  2017-09-27 22:43     ` [PATCH 1/5] html: html_ntxt with no ellipsis whydoubt
@ 2017-09-27 22:43     ` whydoubt
  2017-09-30 11:56       ` john
  2017-09-27 22:43     ` [PATCH 3/5] ui-shared: make a char* parameter const whydoubt
                       ` (4 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


The ui-blame code will also need to call set_title_from_path, so go
ahead and move it to ui-shared.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 31 +++++++++++++++++++++++++++++++
 ui-shared.h |  2 ++
 ui-tree.c   | 31 -------------------------------
 3 files changed, 33 insertions(+), 31 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index e5c9a02..4ace20c 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1111,3 +1111,34 @@ void cgit_print_snapshot_links(const char *repo, const char *head,
 	}
 	strbuf_release(&filename);
 }
+
+void set_title_from_path(const char *path)
+{
+	size_t path_len, path_index, path_last_end;
+	char *new_title;
+
+	if (!path)
+		return;
+
+	path_len = strlen(path);
+	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
+	new_title[0] = '\0';
+
+	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
+		if (path[path_index] == '/') {
+			if (path_index == path_len - 1) {
+				path_last_end = path_index - 1;
+				continue;
+			}
+			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
+			strcat(new_title, "\\");
+			path_last_end = path_index;
+		}
+	}
+	if (path_last_end)
+		strncat(new_title, path, path_last_end);
+
+	strcat(new_title, " - ");
+	strcat(new_title, ctx.page.title);
+	ctx.page.title = new_title;
+}
diff --git a/ui-shared.h b/ui-shared.h
index 87799f1..7948867 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -77,4 +77,6 @@ extern void cgit_print_snapshot_links(const char *repo, const char *head,
 				      const char *hex, int snapshots);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
+
+extern void set_title_from_path(const char *path);
 #endif /* UI_SHARED_H */
diff --git a/ui-tree.c b/ui-tree.c
index ca24a03..12eaaf0 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -84,37 +84,6 @@ static void print_binary_buffer(char *buf, unsigned long size)
 	html("</table>\n");
 }
 
-static void set_title_from_path(const char *path)
-{
-	size_t path_len, path_index, path_last_end;
-	char *new_title;
-
-	if (!path)
-		return;
-
-	path_len = strlen(path);
-	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
-	new_title[0] = '\0';
-
-	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
-		if (path[path_index] == '/') {
-			if (path_index == path_len - 1) {
-				path_last_end = path_index - 1;
-				continue;
-			}
-			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
-			strcat(new_title, "\\");
-			path_last_end = path_index;
-		}
-	}
-	if (path_last_end)
-		strncat(new_title, path, path_last_end);
-
-	strcat(new_title, " - ");
-	strcat(new_title, ctx.page.title);
-	ctx.page.title = new_title;
-}
-
 static void print_object(const unsigned char *sha1, char *path, const char *basename, const char *rev)
 {
 	enum object_type type;
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 3/5] ui-shared: make a char* parameter const
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
  2017-09-27 22:43     ` [PATCH 1/5] html: html_ntxt with no ellipsis whydoubt
  2017-09-27 22:43     ` [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
@ 2017-09-27 22:43     ` whydoubt
  2017-09-30 12:00       ` john
  2017-09-27 22:43     ` [PATCH 4/5] ui-blame: add blame UI whydoubt
                       ` (3 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


All cgit_xxx_link functions take const char* for the 'name' parameter,
except for cgit_commit_link, which takes a char* and subsequently
modifies the contents.  Avoiding the content changes, and making it
const char* will avoid the need to make copies of const char* strings
being passed to cgit_commit_link.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 19 ++++++++-----------
 ui-shared.h |  2 +-
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 4ace20c..ee96755 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -347,18 +347,11 @@ void cgit_log_link(const char *name, const char *title, const char *class,
 	html("</a>");
 }
 
-void cgit_commit_link(char *name, const char *title, const char *class,
+void cgit_commit_link(const char *name, const char *title, const char *class,
 		      const char *head, const char *rev, const char *path)
 {
 	char *delim;
 
-	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] = '.';
-	}
-
 	delim = repolink(title, class, "commit", head, path);
 	if (rev && ctx.qry.head && strcmp(rev, ctx.qry.head)) {
 		html(delim);
@@ -387,9 +380,13 @@ void cgit_commit_link(char *name, const char *title, const char *class,
 		html("follow=1");
 	}
 	html("'>");
-	if (name[0] != '\0')
-		html_txt(name);
-	else
+	if (name[0] != '\0') {
+		if (strlen(name) > ctx.cfg.max_msg_len && ctx.cfg.max_msg_len >= 15) {
+			html_ntxt(ctx.cfg.max_msg_len - 3, name);
+			html("...");
+		} else
+			html_txt(name);
+	} else
 		html_txt("(no commit message)");
 	html("</a>");
 }
diff --git a/ui-shared.h b/ui-shared.h
index 7948867..18e3994 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -30,7 +30,7 @@ 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, int follow);
-extern void cgit_commit_link(char *name, const char *title,
+extern void cgit_commit_link(const char *name, const char *title,
 			     const char *class, const char *head,
 			     const char *rev, const char *path);
 extern void cgit_patch_link(const char *name, const char *title,
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 4/5] ui-blame: add blame UI
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
                       ` (2 preceding siblings ...)
  2017-09-27 22:43     ` [PATCH 3/5] ui-shared: make a char* parameter const whydoubt
@ 2017-09-27 22:43     ` whydoubt
  2017-09-30 12:07       ` john
  2017-09-27 22:43     ` [PATCH 5/5] ui-tree: link to blame UI if enabled whydoubt
                       ` (2 subsequent siblings)
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


Implement a page which provides the blame view of a specified file.

This feature is controlled by a new config variable, "enable-blame",
which is disabled by default.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.c       |   2 +
 cgit.css     |   8 +++
 cgit.h       |   1 +
 cgit.mk      |   1 +
 cgitrc.5.txt |   9 +++
 cmd.c        |  12 +++-
 ui-blame.c   | 224 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h   |   7 ++
 8 files changed, 263 insertions(+), 1 deletion(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

diff --git a/cgit.c b/cgit.c
index 1dae4b8..972a67e 100644
--- a/cgit.c
+++ b/cgit.c
@@ -167,6 +167,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.enable_index_links = atoi(value);
 	else if (!strcmp(name, "enable-index-owner"))
 		ctx.cfg.enable_index_owner = atoi(value);
+	else if (!strcmp(name, "enable-blame"))
+		ctx.cfg.enable_blame = atoi(value);
 	else if (!strcmp(name, "enable-commit-graph"))
 		ctx.cfg.enable_commit_graph = atoi(value);
 	else if (!strcmp(name, "enable-log-filecount"))
diff --git a/cgit.css b/cgit.css
index 1dc2c11..836f8ae 100644
--- a/cgit.css
+++ b/cgit.css
@@ -329,6 +329,14 @@ div#cgit table.ssdiff td.lineno a:hover {
 	color: black;
 }
 
+div#cgit table.blame tr:nth-child(even) {
+	background: #eee;
+}
+
+div#cgit table.blame tr:nth-child(odd) {
+	background: white;
+}
+
 div#cgit table.bin-blob {
 	margin-top: 0.5em;
 	border: solid 1px black;
diff --git a/cgit.h b/cgit.h
index fbc6c6a..0b88dcd 100644
--- a/cgit.h
+++ b/cgit.h
@@ -228,6 +228,7 @@ struct cgit_config {
 	int enable_http_clone;
 	int enable_index_links;
 	int enable_index_owner;
+	int enable_blame;
 	int enable_commit_graph;
 	int enable_log_filecount;
 	int enable_log_linecount;
diff --git a/cgit.mk b/cgit.mk
index 90a2346..3fcc1ca 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -77,6 +77,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 9fcf445..4da166c 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -141,6 +141,11 @@ embedded::
 	suitable for embedding in other html pages. Default value: none. See
 	also: "noheader".
 
+enable-blame::
+	Flag which, when set to "1", will allow cgit to provide a "blame" page
+	for files, and will make it generate links to that page in appropriate
+	places. Default value: "0".
+
 enable-commit-graph::
 	Flag which, when set to "1", will make cgit print an ASCII-art commit
 	history graph to the left of the commit messages in the repository
@@ -799,6 +804,10 @@ enable-http-clone=1
 enable-index-links=1
 
 
+# Enable blame page and create links to it from tree page
+enable-blame=1
+
+
 # Enable ASCII art commit history graph on the log pages
 enable-commit-graph=1
 
diff --git a/cmd.c b/cmd.c
index d280e95..63f0ae5 100644
--- a/cmd.c
+++ b/cmd.c
@@ -1,6 +1,6 @@
 /* cmd.c: the cgit command dispatcher
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -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"
@@ -63,6 +64,14 @@ static void about_fn(void)
 		cgit_print_site_readme();
 }
 
+static void blame_fn(void)
+{
+	if (ctx.cfg.enable_blame)
+		cgit_print_blame();
+	else
+		cgit_print_error_page(403, "Forbidden", "Blame is disabled");
+}
+
 static void blob_fn(void)
 {
 	cgit_print_blob(ctx.qry.sha1, ctx.qry.path, ctx.qry.head, 0);
@@ -164,6 +173,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(HEAD, 1, 0, 1),
 		def_cmd(atom, 1, 0, 0),
 		def_cmd(about, 0, 0, 0),
+		def_cmd(blame, 1, 1, 0),
 		def_cmd(blob, 1, 0, 0),
 		def_cmd(commit, 1, 1, 0),
 		def_cmd(diff, 1, 1, 0),
diff --git a/ui-blame.c b/ui-blame.c
new file mode 100644
index 0000000..40f695a
--- /dev/null
+++ b/ui-blame.c
@@ -0,0 +1,224 @@
+/* ui-blame.c: functions for blame output
+ *
+ * Copyright (C) 2006-2017 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"
+#include "argv-array.h"
+#include "blame.h"
+
+
+static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)
+{
+	struct commitinfo *info;
+	struct strbuf detail = STRBUF_INIT;
+
+	info = cgit_parse_commit(suspect->commit);
+
+	strbuf_addf(&detail, "author  %s", info->author);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", info->author_email);
+	strbuf_addf(&detail, "  %s\n",
+		    show_date(info->author_date, info->author_tz,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addf(&detail, "committer  %s", info->committer);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", info->committer_email);
+	strbuf_addf(&detail, "  %s\n\n",
+		    show_date(info->committer_date, info->committer_tz,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addstr(&detail, info->subject);
+
+	cgit_free_commitinfo(info);
+	return strbuf_detach(&detail, NULL);
+}
+
+static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)
+{
+	struct blame_origin *suspect = ent->suspect;
+	char hex[GIT_SHA1_HEXSZ + 1];
+	char *detail;
+	const char *abbrev;
+	unsigned long lineno;
+	const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
+	const char *cp, *cpend;
+
+	oid_to_hex_r(hex, &suspect->commit->object.oid);
+	detail = emit_one_suspect_detail(suspect, hex);
+	abbrev = find_unique_abbrev(suspect->commit->object.oid.hash, DEFAULT_ABBREV);
+
+	html("<tr><td class='sha1 lines'>");
+	cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);
+	html("</td>\n");
+
+	free(detail);
+
+	if (ctx.cfg.enable_tree_linenumbers) {
+		html("<td class='linenumbers'><pre>");
+		lineno = ent->lno;
+		while (lineno < ent->lno + ent->num_lines)
+			htmlf(numberfmt, ++lineno);
+		html("</pre></td>\n");
+	}
+
+	cp = blame_nth_line(sb, ent->lno);
+	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
+
+	html("<td class='lines'><pre><code>");
+	html_ntxt(cpend - cp, cp);
+	html("</code></pre></td></tr>\n");
+}
+
+struct walk_tree_context {
+	char *curr_rev;
+	int match_baselen;
+	int state;
+};
+
+static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)
+{
+	enum object_type type;
+	unsigned long size;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+	struct rev_info revs;
+	struct blame_scoreboard sb;
+	struct blame_origin *o;
+	struct blame_entry *ent = NULL;
+
+	type = sha1_object_info(sha1, &size);
+	if (type == OBJ_BAD) {
+		cgit_print_error_page(404, "Not found", "Bad object name: %s",
+				      sha1_to_hex(sha1));
+		return;
+	}
+
+	argv_array_push(&rev_argv, "blame");
+	argv_array_push(&rev_argv, rev);
+	init_revisions(&revs, NULL);
+	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
+	setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
+	init_scoreboard(&sb);
+	sb.revs = &revs;
+	setup_scoreboard(&sb, path, &o);
+	o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
+	prio_queue_put(&sb.commits, o->commit);
+	blame_origin_decref(o);
+	sb.ent = NULL;
+	sb.path = path;
+	assign_blame(&sb, 0);
+	blame_sort_final(&sb);
+	blame_coalesce(&sb);
+
+	set_title_from_path(path);
+
+	cgit_print_layout_start();
+	htmlf("blob: %s (", sha1_to_hex(sha1));
+	cgit_plain_link("plain", NULL, NULL, ctx.qry.head, rev, path);
+	html(") (");
+	cgit_tree_link("tree", NULL, NULL, ctx.qry.head, rev, path);
+	html(")\n");
+
+	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
+		htmlf("<div class='error'>blob size (%ldKB) exceeds display size limit (%dKB).</div>",
+		      size / 1024, ctx.cfg.max_blob_size);
+		return;
+	}
+
+	html("<table class='blame blob'>");
+	for (ent = sb.ent; ent; ) {
+		struct blame_entry *e = ent->next;
+		emit_blame_entry(&sb, ent);
+		free(ent);
+		ent = e;
+	}
+	html("</table>\n");
+	free((void *)sb.final_buf);
+
+	cgit_print_layout_end();
+	return;
+}
+
+static int walk_tree(const unsigned char *sha1, struct strbuf *base,
+		     const char *pathname, unsigned mode, int stage, void *cbdata)
+{
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (base->len == walk_tree_ctx->match_baselen) {
+		if (S_ISREG(mode)) {
+			struct strbuf buffer = STRBUF_INIT;
+			strbuf_addbuf(&buffer, base);
+			strbuf_addstr(&buffer, pathname);
+			print_object(sha1, buffer.buf, pathname, walk_tree_ctx->curr_rev);
+			strbuf_release(&buffer);
+			walk_tree_ctx->state = 1;
+		} else if (S_ISDIR(mode)) {
+			walk_tree_ctx->state = 2;
+		}
+	} else if (base->len < INT_MAX && (int)base->len > walk_tree_ctx->match_baselen) {
+		walk_tree_ctx->state = 2;
+	} else if (S_ISDIR(mode)) {
+		return READ_TREE_RECURSIVE;
+	}
+	return 0;
+}
+
+static int basedir_len(const char *path)
+{
+	char *p = strrchr(path, '/');
+	if (p)
+		return p - path + 1;
+	return 0;
+}
+
+void cgit_print_blame(void)
+{
+	const char *rev = ctx.qry.sha1;
+	struct object_id oid;
+	struct commit *commit;
+	struct pathspec_item path_items = {
+		.match = ctx.qry.path,
+		.len = ctx.qry.path ? strlen(ctx.qry.path) : 0
+	};
+	struct pathspec paths = {
+		.nr = 1,
+		.items = &path_items
+	};
+	struct walk_tree_context walk_tree_ctx = {
+		.state = 0
+	};
+
+	if (!rev)
+		rev = ctx.qry.head;
+
+	if (get_oid(rev, &oid)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
+		return;
+	}
+	commit = lookup_commit_reference(&oid);
+	if (!commit || parse_commit(commit)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
+		return;
+	}
+
+	walk_tree_ctx.curr_rev = xstrdup(rev);
+	walk_tree_ctx.match_baselen = (path_items.match) ?
+				       basedir_len(path_items.match) : -1;
+
+	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);
+	if (!walk_tree_ctx.state)
+		cgit_print_error_page(404, "Not found", "Not found");
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_error_page(404, "No blame for folders", "Blame is not available for folders.");
+
+	free(walk_tree_ctx.curr_rev);
+}
diff --git a/ui-blame.h b/ui-blame.h
new file mode 100644
index 0000000..8a438e9
--- /dev/null
+++ b/ui-blame.h
@@ -0,0 +1,7 @@
+#ifndef UI_BLAME_H
+#define UI_BLAME_H
+
+extern void cgit_print_blame(void);
+
+#endif /* UI_BLAME_H */
+
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 5/5] ui-tree: link to blame UI if enabled
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
                       ` (3 preceding siblings ...)
  2017-09-27 22:43     ` [PATCH 4/5] ui-blame: add blame UI whydoubt
@ 2017-09-27 22:43     ` whydoubt
  2017-09-30 12:08       ` john
  2017-09-30 12:10     ` [PATCH 0/5] Add ui-blame john
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
  6 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-09-27 22:43 UTC (permalink / raw)


Create links to the blame page.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 20 +++++++++++++++++---
 ui-shared.h |  3 +++
 ui-tree.c   | 10 +++++++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index ee96755..f75338a 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1,6 +1,6 @@
 /* ui-shared.c: common web output functions
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -304,6 +304,12 @@ void cgit_plain_link(const char *name, const char *title, const char *class,
 	reporevlink("plain", name, title, class, head, rev, path);
 }
 
+void cgit_blame_link(const char *name, const char *title, const char *class,
+		     const char *head, const char *rev, const char *path)
+{
+	reporevlink("blame", name, title, class, head, rev, path);
+}
+
 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,
@@ -478,6 +484,10 @@ static void cgit_self_link(char *name, const char *title, const char *class)
 		cgit_plain_link(name, title, class, ctx.qry.head,
 				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
 				ctx.qry.path);
+	else if (!strcmp(ctx.qry.page, "blame"))
+		cgit_blame_link(name, title, class, ctx.qry.head,
+				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
+				ctx.qry.path);
 	else if (!strcmp(ctx.qry.page, "log"))
 		cgit_log_link(name, title, class, ctx.qry.head,
 			      ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
@@ -983,8 +993,12 @@ void cgit_print_pageheader(void)
 		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
 			      NULL, ctx.qry.vpath, 0, NULL, NULL,
 			      ctx.qry.showmsg, ctx.qry.follow);
-		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
-			       ctx.qry.sha1, ctx.qry.vpath);
+		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
+			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
+				        ctx.qry.sha1, ctx.qry.vpath);
+		else
+			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
+				       ctx.qry.sha1, ctx.qry.vpath);
 		cgit_commit_link("commit", NULL, hc("commit"),
 				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
 		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
diff --git a/ui-shared.h b/ui-shared.h
index 18e3994..cc9b4c6 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
 extern void cgit_plain_link(const char *name, const char *title,
 			    const char *class, const char *head,
 			    const char *rev, const char *path);
+extern void cgit_blame_link(const char *name, const char *title,
+			    const char *class, const char *head,
+			    const char *rev, const char *path);
 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,
diff --git a/ui-tree.c b/ui-tree.c
index 12eaaf0..27c9003 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -1,6 +1,6 @@
 /* ui-tree.c: functions for tree output
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -110,6 +110,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);
+	}
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -244,6 +249,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
+		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev, fullpath.buf);
 	html("</td></tr>\n");
 	free(name);
 	strbuf_release(&fullpath);
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 1/5] html: html_ntxt with no ellipsis
  2017-09-27 22:43     ` [PATCH 1/5] html: html_ntxt with no ellipsis whydoubt
@ 2017-09-30 11:55       ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-30 11:55 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:27PM -0500, Jeff Smith wrote:
> For implementing a ui-blame page, there is need for a function that
> outputs a selection from a block of text, transformed for HTML output,
> but with no further modifications or additions.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  html.c        | 24 ++++--------------------
>  html.h        |  2 +-
>  ui-repolist.c |  3 ++-
>  3 files changed, 7 insertions(+), 22 deletions(-)
> 
> diff --git a/html.c b/html.c
> index e7e6e07..87ef5aa 100644
> --- a/html.c
> +++ b/html.c
> @@ -124,26 +124,11 @@ void html_vtxtf(const char *format, va_list ap)
[snip]
> -void html_ntxt(int len, const char *txt)
> +int html_ntxt(int len, const char *txt)

Should this be:

	ssize_t html_ntxt(size_t len, const char *txt)

?  Maybe with a check to die if len > SSIZE_MAX.

>  {
>  	const char *t = txt;
>  	while (t && *t && len--) {
> @@ -162,8 +147,7 @@ void html_ntxt(int len, const char *txt)
>  	}
>  	if (t != txt)
>  		html_raw(txt, t - txt);
> -	if (len < 0)
> -		html("...");
> +	return len;
>  }


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared
  2017-09-27 22:43     ` [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
@ 2017-09-30 11:56       ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-30 11:56 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:28PM -0500, Jeff Smith wrote:
> The ui-blame code will also need to call set_title_from_path, so go
> ahead and move it to ui-shared.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>
> ---
>  ui-shared.c | 31 +++++++++++++++++++++++++++++++
>  ui-shared.h |  2 ++
>  ui-tree.c   | 31 -------------------------------
>  3 files changed, 33 insertions(+), 31 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index e5c9a02..4ace20c 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -1111,3 +1111,34 @@ void cgit_print_snapshot_links(const char *repo, const char *head,
>  	}
>  	strbuf_release(&filename);
>  }
> +
> +void set_title_from_path(const char *path)

Should this be renamed to cgit_set_title_from_path now that it's
exported?


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 3/5] ui-shared: make a char* parameter const
  2017-09-27 22:43     ` [PATCH 3/5] ui-shared: make a char* parameter const whydoubt
@ 2017-09-30 12:00       ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-30 12:00 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:29PM -0500, Jeff Smith wrote:
> All cgit_xxx_link functions take const char* for the 'name' parameter,
> except for cgit_commit_link, which takes a char* and subsequently
> modifies the contents.  Avoiding the content changes, and making it
> const char* will avoid the need to make copies of const char* strings
> being passed to cgit_commit_link.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  ui-shared.c | 19 ++++++++-----------
>  ui-shared.h |  2 +-
>  2 files changed, 9 insertions(+), 12 deletions(-)


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 4/5] ui-blame: add blame UI
  2017-09-27 22:43     ` [PATCH 4/5] ui-blame: add blame UI whydoubt
@ 2017-09-30 12:07       ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-30 12:07 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:30PM -0500, Jeff Smith wrote:
> Implement a page which provides the blame view of a specified file.
> 
> This feature is controlled by a new config variable, "enable-blame",
> which is disabled by default.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>

Reviewed-by: John Keeping <john at keeping.me.uk>

Linux's checkpatch.pl has a few minor complaints if you're re-rolling,
but I don't think these need to block merging:

WARNING: line over 80 characters
#174: FILE: ui-blame.c:17:
+static char *emit_one_suspect_detail(struct blame_origin *suspect, const char *hex)

WARNING: line over 80 characters
#201: FILE: ui-blame.c:44:
+static void emit_blame_entry(struct blame_scoreboard *sb, struct blame_entry *ent)

WARNING: line over 80 characters
#213: FILE: ui-blame.c:56:
+       abbrev = find_unique_abbrev(suspect->commit->object.oid.hash, DEFAULT_ABBREV);

WARNING: line over 80 characters
#216: FILE: ui-blame.c:59:
+       cgit_commit_link(abbrev, detail, NULL, ctx.qry.head, hex, suspect->path);

WARNING: line over 80 characters
#243: FILE: ui-blame.c:86:
+static void print_object(const unsigned char *sha1, const char *path, const char *basename, const char *rev)

WARNING: line over 80 characters
#287: FILE: ui-blame.c:130:
+               htmlf("<div class='error'>blob size (%ldKB) exceeds display size limit (%dKB).</div>",

WARNING: void function return statements are not generally useful
#304: FILE: ui-blame.c:147:
+       return;
+}

WARNING: line over 80 characters
#307: FILE: ui-blame.c:150:
+                    const char *pathname, unsigned mode, int stage, void *cbdata)

WARNING: line over 80 characters
#316: FILE: ui-blame.c:159:
+                       print_object(sha1, buffer.buf, pathname, walk_tree_ctx->curr_rev);

WARNING: line over 80 characters
#322: FILE: ui-blame.c:165:
+       } else if (base->len < INT_MAX && (int)base->len > walk_tree_ctx->match_baselen) {

WARNING: line over 80 characters
#374: FILE: ui-blame.c:217:
+       read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree, &walk_tree_ctx);

WARNING: line over 80 characters
#378: FILE: ui-blame.c:221:
+               cgit_print_error_page(404, "No blame for folders", "Blame is not available for folders.");




^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 5/5] ui-tree: link to blame UI if enabled
  2017-09-27 22:43     ` [PATCH 5/5] ui-tree: link to blame UI if enabled whydoubt
@ 2017-09-30 12:08       ` john
  0 siblings, 0 replies; 63+ messages in thread
From: john @ 2017-09-30 12:08 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:31PM -0500, Jeff Smith wrote:
> Create links to the blame page.
> 
> Signed-off-by: Jeff Smith <whydoubt at gmail.com>

Reviewed-by: John Keeping <john at keeping.me.uk>

> ---
>  ui-shared.c | 20 +++++++++++++++++---
>  ui-shared.h |  3 +++
>  ui-tree.c   | 10 +++++++++-
>  3 files changed, 29 insertions(+), 4 deletions(-)
> 
> diff --git a/ui-shared.c b/ui-shared.c
> index ee96755..f75338a 100644
> --- a/ui-shared.c
> +++ b/ui-shared.c
> @@ -1,6 +1,6 @@
>  /* ui-shared.c: common web output functions
>   *
> - * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
> + * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -304,6 +304,12 @@ void cgit_plain_link(const char *name, const char *title, const char *class,
>  	reporevlink("plain", name, title, class, head, rev, path);
>  }
>  
> +void cgit_blame_link(const char *name, const char *title, const char *class,
> +		     const char *head, const char *rev, const char *path)
> +{
> +	reporevlink("blame", name, title, class, head, rev, path);
> +}
> +
>  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,
> @@ -478,6 +484,10 @@ static void cgit_self_link(char *name, const char *title, const char *class)
>  		cgit_plain_link(name, title, class, ctx.qry.head,
>  				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
>  				ctx.qry.path);
> +	else if (!strcmp(ctx.qry.page, "blame"))
> +		cgit_blame_link(name, title, class, ctx.qry.head,
> +				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
> +				ctx.qry.path);
>  	else if (!strcmp(ctx.qry.page, "log"))
>  		cgit_log_link(name, title, class, ctx.qry.head,
>  			      ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
> @@ -983,8 +993,12 @@ void cgit_print_pageheader(void)
>  		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
>  			      NULL, ctx.qry.vpath, 0, NULL, NULL,
>  			      ctx.qry.showmsg, ctx.qry.follow);
> -		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
> -			       ctx.qry.sha1, ctx.qry.vpath);
> +		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
> +			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
> +				        ctx.qry.sha1, ctx.qry.vpath);
> +		else
> +			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
> +				       ctx.qry.sha1, ctx.qry.vpath);
>  		cgit_commit_link("commit", NULL, hc("commit"),
>  				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
>  		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
> diff --git a/ui-shared.h b/ui-shared.h
> index 18e3994..cc9b4c6 100644
> --- a/ui-shared.h
> +++ b/ui-shared.h
> @@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
>  extern void cgit_plain_link(const char *name, const char *title,
>  			    const char *class, const char *head,
>  			    const char *rev, const char *path);
> +extern void cgit_blame_link(const char *name, const char *title,
> +			    const char *class, const char *head,
> +			    const char *rev, const char *path);
>  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,
> diff --git a/ui-tree.c b/ui-tree.c
> index 12eaaf0..27c9003 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -1,6 +1,6 @@
>  /* ui-tree.c: functions for tree output
>   *
> - * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
> + * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
>   *
>   * Licensed under GNU General Public License v2
>   *   (see COPYING for full license text)
> @@ -110,6 +110,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);
> +	}
>  	html(")\n");
>  
>  	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
> @@ -244,6 +249,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
> +		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
> +				walk_tree_ctx->curr_rev, fullpath.buf);
>  	html("</td></tr>\n");
>  	free(name);
>  	strbuf_release(&fullpath);
> -- 
> 2.9.4
> 
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
                       ` (4 preceding siblings ...)
  2017-09-27 22:43     ` [PATCH 5/5] ui-tree: link to blame UI if enabled whydoubt
@ 2017-09-30 12:10     ` john
  2017-10-02  1:17       ` Jason
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
  6 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-09-30 12:10 UTC (permalink / raw)


On Wed, Sep 27, 2017 at 05:43:26PM -0500, Jeff Smith wrote:
> I split git blame functionality into libgit, and the changes were
> accepted upstream and are a part of git 2.14.  Now that the git
> infrastructure is in place, here is what is needed for cgit to make use
> of it.
> 
> Since RFCv2:
> . Re-ordered and squashed some of the patches
> . Used cgit's commitinfo structure
> . Factored set_title_from_path out to ui-shared
> . Made cgit_commit_link's first parameter const char*
> . Disabled the feature by default
> . Simplified changes to html_ntxt / html_txt
> 
> Jeff Smith (5):
>   html: html_ntxt with no ellipsis
>   ui-tree: move set_title_from_path to ui-shared
>   ui-shared: make a char* parameter const
>   ui-blame: add blame UI
>   ui-tree: link to blame UI if enabled

This looks pretty good now.  I've sent some comments on a couple of
patches, but the only one I think is important is using (s)size_t for
html_nxt in patch 1.


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-09-30 12:10     ` [PATCH 0/5] Add ui-blame john
@ 2017-10-02  1:17       ` Jason
  2017-10-02  5:34         ` list
  0 siblings, 1 reply; 63+ messages in thread
From: Jason @ 2017-10-02  1:17 UTC (permalink / raw)


Jeff -- thanks for your hard work on this.

John -- thanks for the review. If you'd like to put this in a
for-jason branch, I'll get to merging it soon.

Jason


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 0/5] Add ui-blame
  2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
                       ` (5 preceding siblings ...)
  2017-09-30 12:10     ` [PATCH 0/5] Add ui-blame john
@ 2017-10-02  4:39     ` whydoubt
  2017-10-02  4:39       ` [PATCHv2 1/5] html: html_ntxt with no ellipsis whydoubt
                         ` (4 more replies)
  6 siblings, 5 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


I split git blame functionality into libgit, and the changes were
accepted upstream and are a part of git 2.14.  Now that the git
infrastructure is in place, here is what is needed for cgit to make use
of it.

Since previous set:
. Updated html_ntxt signature
. set_title_from_path -> cgit_set_title_from_path
. A bit more tidying up ui-blame.c

Jeff Smith (5):
  html: html_ntxt with no ellipsis
  ui-tree: move set_title_from_path to ui-shared
  ui-shared: make a char* parameter const
  ui-blame: add blame UI
  ui-tree: link to blame UI if enabled

 cgit.c        |   2 +
 cgit.css      |   8 +++
 cgit.h        |   1 +
 cgit.mk       |   1 +
 cgitrc.5.txt  |   9 +++
 cmd.c         |  12 +++-
 html.c        |  29 +++-----
 html.h        |   2 +-
 ui-blame.c    | 227 ++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h    |   7 ++
 ui-repolist.c |   3 +-
 ui-shared.c   |  70 ++++++++++++++----
 ui-shared.h   |   7 +-
 ui-tree.c     |  45 +++---------
 14 files changed, 350 insertions(+), 73 deletions(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 1/5] html: html_ntxt with no ellipsis
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
@ 2017-10-02  4:39       ` whydoubt
  2017-10-02  4:39       ` [PATCHv2 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
                         ` (3 subsequent siblings)
  4 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


For implementing a ui-blame page, there is need for a function that
outputs a selection from a block of text, transformed for HTML output,
but with no further modifications or additions.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 html.c        | 29 ++++++++---------------------
 html.h        |  2 +-
 ui-repolist.c |  3 ++-
 3 files changed, 11 insertions(+), 23 deletions(-)

diff --git a/html.c b/html.c
index e7e6e07..f009624 100644
--- a/html.c
+++ b/html.c
@@ -124,29 +124,17 @@ void html_vtxtf(const char *format, va_list ap)
 
 void html_txt(const char *txt)
 {
-	const char *t = txt;
-	while (t && *t) {
-		int c = *t;
-		if (c == '<' || c == '>' || c == '&') {
-			html_raw(txt, t - txt);
-			if (c == '>')
-				html("&gt;");
-			else if (c == '<')
-				html("&lt;");
-			else if (c == '&')
-				html("&amp;");
-			txt = t + 1;
-		}
-		t++;
-	}
-	if (t != txt)
-		html(txt);
+	if (txt)
+		html_ntxt(txt, strlen(txt));
 }
 
-void html_ntxt(int len, const char *txt)
+ssize_t html_ntxt(const char *txt, size_t len)
 {
 	const char *t = txt;
-	while (t && *t && len--) {
+	if (len > SSIZE_MAX)
+		return -1;
+	ssize_t slen = (size_t) len;
+	while (t && *t && slen--) {
 		int c = *t;
 		if (c == '<' || c == '>' || c == '&') {
 			html_raw(txt, t - txt);
@@ -162,8 +150,7 @@ void html_ntxt(int len, const char *txt)
 	}
 	if (t != txt)
 		html_raw(txt, t - txt);
-	if (len < 0)
-		html("...");
+	return slen;
 }
 
 void html_attrf(const char *fmt, ...)
diff --git a/html.h b/html.h
index 1b24e55..fa4de77 100644
--- a/html.h
+++ b/html.h
@@ -19,7 +19,7 @@ __attribute__((format (printf,1,2)))
 extern void html_attrf(const char *format,...);
 
 extern void html_txt(const char *txt);
-extern void html_ntxt(int len, const char *txt);
+extern ssize_t html_ntxt(const char *txt, size_t len);
 extern void html_attr(const char *txt);
 extern void html_url_path(const char *txt);
 extern void html_url_arg(const char *txt);
diff --git a/ui-repolist.c b/ui-repolist.c
index 7272e87..af52f9b 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -329,7 +329,8 @@ void cgit_print_repolist(void)
 		repourl = cgit_repourl(ctx.repo->url);
 		html_link_open(repourl, NULL, NULL);
 		free(repourl);
-		html_ntxt(ctx.cfg.max_repodesc_len, ctx.repo->desc);
+		if (html_ntxt(ctx.repo->desc, ctx.cfg.max_repodesc_len) < 0)
+			html("...");
 		html_link_close();
 		html("</td><td>");
 		if (ctx.cfg.enable_index_owner) {
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 2/5] ui-tree: move set_title_from_path to ui-shared
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
  2017-10-02  4:39       ` [PATCHv2 1/5] html: html_ntxt with no ellipsis whydoubt
@ 2017-10-02  4:39       ` whydoubt
  2017-10-02  4:39       ` [PATCHv2 3/5] ui-shared: make a char* parameter const whydoubt
                         ` (2 subsequent siblings)
  4 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


The ui-blame code will also need to call set_title_from_path, so go
ahead and move it to ui-shared.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 31 +++++++++++++++++++++++++++++++
 ui-shared.h |  2 ++
 ui-tree.c   | 35 ++---------------------------------
 3 files changed, 35 insertions(+), 33 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index e5c9a02..2547e43 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1111,3 +1111,34 @@ void cgit_print_snapshot_links(const char *repo, const char *head,
 	}
 	strbuf_release(&filename);
 }
+
+void cgit_set_title_from_path(const char *path)
+{
+	size_t path_len, path_index, path_last_end;
+	char *new_title;
+
+	if (!path)
+		return;
+
+	path_len = strlen(path);
+	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
+	new_title[0] = '\0';
+
+	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
+		if (path[path_index] == '/') {
+			if (path_index == path_len - 1) {
+				path_last_end = path_index - 1;
+				continue;
+			}
+			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
+			strcat(new_title, "\\");
+			path_last_end = path_index;
+		}
+	}
+	if (path_last_end)
+		strncat(new_title, path, path_last_end);
+
+	strcat(new_title, " - ");
+	strcat(new_title, ctx.page.title);
+	ctx.page.title = new_title;
+}
diff --git a/ui-shared.h b/ui-shared.h
index 87799f1..40f6207 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -77,4 +77,6 @@ extern void cgit_print_snapshot_links(const char *repo, const char *head,
 				      const char *hex, int snapshots);
 extern void cgit_add_hidden_formfields(int incl_head, int incl_search,
 				       const char *page);
+
+extern void cgit_set_title_from_path(const char *path);
 #endif /* UI_SHARED_H */
diff --git a/ui-tree.c b/ui-tree.c
index ca24a03..3925809 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -84,37 +84,6 @@ static void print_binary_buffer(char *buf, unsigned long size)
 	html("</table>\n");
 }
 
-static void set_title_from_path(const char *path)
-{
-	size_t path_len, path_index, path_last_end;
-	char *new_title;
-
-	if (!path)
-		return;
-
-	path_len = strlen(path);
-	new_title = xmalloc(path_len + 3 + strlen(ctx.page.title) + 1);
-	new_title[0] = '\0';
-
-	for (path_index = path_len, path_last_end = path_len; path_index-- > 0;) {
-		if (path[path_index] == '/') {
-			if (path_index == path_len - 1) {
-				path_last_end = path_index - 1;
-				continue;
-			}
-			strncat(new_title, &path[path_index + 1], path_last_end - path_index - 1);
-			strcat(new_title, "\\");
-			path_last_end = path_index;
-		}
-	}
-	if (path_last_end)
-		strncat(new_title, path, path_last_end);
-
-	strcat(new_title, " - ");
-	strcat(new_title, ctx.page.title);
-	ctx.page.title = new_title;
-}
-
 static void print_object(const unsigned char *sha1, char *path, const char *basename, const char *rev)
 {
 	enum object_type type;
@@ -135,7 +104,7 @@ static void print_object(const unsigned char *sha1, char *path, const char *base
 		return;
 	}
 
-	set_title_from_path(path);
+	cgit_set_title_from_path(path);
 
 	cgit_print_layout_start();
 	htmlf("blob: %s (", sha1_to_hex(sha1));
@@ -335,7 +304,7 @@ static int walk_tree(const unsigned char *sha1, struct strbuf *base,
 
 		if (S_ISDIR(mode)) {
 			walk_tree_ctx->state = 1;
-			set_title_from_path(buffer.buf);
+			cgit_set_title_from_path(buffer.buf);
 			strbuf_release(&buffer);
 			ls_head();
 			return READ_TREE_RECURSIVE;
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 3/5] ui-shared: make a char* parameter const
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
  2017-10-02  4:39       ` [PATCHv2 1/5] html: html_ntxt with no ellipsis whydoubt
  2017-10-02  4:39       ` [PATCHv2 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
@ 2017-10-02  4:39       ` whydoubt
  2017-10-02  4:39       ` [PATCHv2 4/5] ui-blame: add blame UI whydoubt
  2017-10-02  4:39       ` [PATCHv2 5/5] ui-tree: link to blame UI if enabled whydoubt
  4 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


All cgit_xxx_link functions take const char* for the 'name' parameter,
except for cgit_commit_link, which takes a char* and subsequently
modifies the contents.  Avoiding the content changes, and making it
const char* will avoid the need to make copies of const char* strings
being passed to cgit_commit_link.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 19 ++++++++-----------
 ui-shared.h |  2 +-
 2 files changed, 9 insertions(+), 12 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 2547e43..315dedb 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -347,18 +347,11 @@ void cgit_log_link(const char *name, const char *title, const char *class,
 	html("</a>");
 }
 
-void cgit_commit_link(char *name, const char *title, const char *class,
+void cgit_commit_link(const char *name, const char *title, const char *class,
 		      const char *head, const char *rev, const char *path)
 {
 	char *delim;
 
-	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] = '.';
-	}
-
 	delim = repolink(title, class, "commit", head, path);
 	if (rev && ctx.qry.head && strcmp(rev, ctx.qry.head)) {
 		html(delim);
@@ -387,9 +380,13 @@ void cgit_commit_link(char *name, const char *title, const char *class,
 		html("follow=1");
 	}
 	html("'>");
-	if (name[0] != '\0')
-		html_txt(name);
-	else
+	if (name[0] != '\0') {
+		if (strlen(name) > ctx.cfg.max_msg_len && ctx.cfg.max_msg_len >= 15) {
+			html_ntxt(name, ctx.cfg.max_msg_len - 3);
+			html("...");
+		} else
+			html_txt(name);
+	} else
 		html_txt("(no commit message)");
 	html("</a>");
 }
diff --git a/ui-shared.h b/ui-shared.h
index 40f6207..2cd7ac9 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -30,7 +30,7 @@ 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, int follow);
-extern void cgit_commit_link(char *name, const char *title,
+extern void cgit_commit_link(const char *name, const char *title,
 			     const char *class, const char *head,
 			     const char *rev, const char *path);
 extern void cgit_patch_link(const char *name, const char *title,
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 4/5] ui-blame: add blame UI
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
                         ` (2 preceding siblings ...)
  2017-10-02  4:39       ` [PATCHv2 3/5] ui-shared: make a char* parameter const whydoubt
@ 2017-10-02  4:39       ` whydoubt
  2017-10-02  4:39       ` [PATCHv2 5/5] ui-tree: link to blame UI if enabled whydoubt
  4 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


Implement a page which provides the blame view of a specified file.

This feature is controlled by a new config variable, "enable-blame",
which is disabled by default.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 cgit.c       |   2 +
 cgit.css     |   8 +++
 cgit.h       |   1 +
 cgit.mk      |   1 +
 cgitrc.5.txt |   9 +++
 cmd.c        |  12 +++-
 ui-blame.c   | 227 +++++++++++++++++++++++++++++++++++++++++++++++++++++++++++
 ui-blame.h   |   7 ++
 8 files changed, 266 insertions(+), 1 deletion(-)
 create mode 100644 ui-blame.c
 create mode 100644 ui-blame.h

diff --git a/cgit.c b/cgit.c
index 1dae4b8..972a67e 100644
--- a/cgit.c
+++ b/cgit.c
@@ -167,6 +167,8 @@ static void config_cb(const char *name, const char *value)
 		ctx.cfg.enable_index_links = atoi(value);
 	else if (!strcmp(name, "enable-index-owner"))
 		ctx.cfg.enable_index_owner = atoi(value);
+	else if (!strcmp(name, "enable-blame"))
+		ctx.cfg.enable_blame = atoi(value);
 	else if (!strcmp(name, "enable-commit-graph"))
 		ctx.cfg.enable_commit_graph = atoi(value);
 	else if (!strcmp(name, "enable-log-filecount"))
diff --git a/cgit.css b/cgit.css
index 1dc2c11..836f8ae 100644
--- a/cgit.css
+++ b/cgit.css
@@ -329,6 +329,14 @@ div#cgit table.ssdiff td.lineno a:hover {
 	color: black;
 }
 
+div#cgit table.blame tr:nth-child(even) {
+	background: #eee;
+}
+
+div#cgit table.blame tr:nth-child(odd) {
+	background: white;
+}
+
 div#cgit table.bin-blob {
 	margin-top: 0.5em;
 	border: solid 1px black;
diff --git a/cgit.h b/cgit.h
index fbc6c6a..0b88dcd 100644
--- a/cgit.h
+++ b/cgit.h
@@ -228,6 +228,7 @@ struct cgit_config {
 	int enable_http_clone;
 	int enable_index_links;
 	int enable_index_owner;
+	int enable_blame;
 	int enable_commit_graph;
 	int enable_log_filecount;
 	int enable_log_linecount;
diff --git a/cgit.mk b/cgit.mk
index 90a2346..3fcc1ca 100644
--- a/cgit.mk
+++ b/cgit.mk
@@ -77,6 +77,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 9fcf445..4da166c 100644
--- a/cgitrc.5.txt
+++ b/cgitrc.5.txt
@@ -141,6 +141,11 @@ embedded::
 	suitable for embedding in other html pages. Default value: none. See
 	also: "noheader".
 
+enable-blame::
+	Flag which, when set to "1", will allow cgit to provide a "blame" page
+	for files, and will make it generate links to that page in appropriate
+	places. Default value: "0".
+
 enable-commit-graph::
 	Flag which, when set to "1", will make cgit print an ASCII-art commit
 	history graph to the left of the commit messages in the repository
@@ -799,6 +804,10 @@ enable-http-clone=1
 enable-index-links=1
 
 
+# Enable blame page and create links to it from tree page
+enable-blame=1
+
+
 # Enable ASCII art commit history graph on the log pages
 enable-commit-graph=1
 
diff --git a/cmd.c b/cmd.c
index d280e95..63f0ae5 100644
--- a/cmd.c
+++ b/cmd.c
@@ -1,6 +1,6 @@
 /* cmd.c: the cgit command dispatcher
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -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"
@@ -63,6 +64,14 @@ static void about_fn(void)
 		cgit_print_site_readme();
 }
 
+static void blame_fn(void)
+{
+	if (ctx.cfg.enable_blame)
+		cgit_print_blame();
+	else
+		cgit_print_error_page(403, "Forbidden", "Blame is disabled");
+}
+
 static void blob_fn(void)
 {
 	cgit_print_blob(ctx.qry.sha1, ctx.qry.path, ctx.qry.head, 0);
@@ -164,6 +173,7 @@ struct cgit_cmd *cgit_get_cmd(void)
 		def_cmd(HEAD, 1, 0, 1),
 		def_cmd(atom, 1, 0, 0),
 		def_cmd(about, 0, 0, 0),
+		def_cmd(blame, 1, 1, 0),
 		def_cmd(blob, 1, 0, 0),
 		def_cmd(commit, 1, 1, 0),
 		def_cmd(diff, 1, 1, 0),
diff --git a/ui-blame.c b/ui-blame.c
new file mode 100644
index 0000000..62cf431
--- /dev/null
+++ b/ui-blame.c
@@ -0,0 +1,227 @@
+/* ui-blame.c: functions for blame output
+ *
+ * Copyright (C) 2006-2017 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"
+#include "argv-array.h"
+#include "blame.h"
+
+
+static char *emit_suspect_detail(struct blame_origin *suspect)
+{
+	struct commitinfo *info;
+	struct strbuf detail = STRBUF_INIT;
+
+	info = cgit_parse_commit(suspect->commit);
+
+	strbuf_addf(&detail, "author  %s", info->author);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", info->author_email);
+	strbuf_addf(&detail, "  %s\n",
+		    show_date(info->author_date, info->author_tz,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addf(&detail, "committer  %s", info->committer);
+	if (!ctx.cfg.noplainemail)
+		strbuf_addf(&detail, " %s", info->committer_email);
+	strbuf_addf(&detail, "  %s\n\n",
+		    show_date(info->committer_date, info->committer_tz,
+				    cgit_date_mode(DATE_ISO8601)));
+
+	strbuf_addstr(&detail, info->subject);
+
+	cgit_free_commitinfo(info);
+	return strbuf_detach(&detail, NULL);
+}
+
+static void emit_blame_entry(struct blame_scoreboard *sb,
+			     struct blame_entry *ent)
+{
+	struct blame_origin *suspect = ent->suspect;
+	struct object_id *oid = &suspect->commit->object.oid;
+	const char *numberfmt = "<a id='n%1$d' href='#n%1$d'>%1$d</a>\n";
+	const char *cp, *cpend;
+
+	char *detail = emit_suspect_detail(suspect);
+
+	html("<tr><td class='sha1 lines'>");
+	cgit_commit_link(find_unique_abbrev(oid->hash, DEFAULT_ABBREV), detail,
+			 NULL, ctx.qry.head, oid_to_hex(oid), suspect->path);
+	html("</td>\n");
+
+	free(detail);
+
+	if (ctx.cfg.enable_tree_linenumbers) {
+		unsigned long lineno = ent->lno;
+		html("<td class='linenumbers'><pre>");
+		while (lineno < ent->lno + ent->num_lines)
+			htmlf(numberfmt, ++lineno);
+		html("</pre></td>\n");
+	}
+
+	cp = blame_nth_line(sb, ent->lno);
+	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
+
+	html("<td class='lines'><pre><code>");
+	html_ntxt(cp, cpend - cp);
+	html("</code></pre></td></tr>\n");
+}
+
+struct walk_tree_context {
+	char *curr_rev;
+	int match_baselen;
+	int state;
+};
+
+static void print_object(const unsigned char *sha1, const char *path,
+			 const char *basename, const char *rev)
+{
+	enum object_type type;
+	unsigned long size;
+	struct argv_array rev_argv = ARGV_ARRAY_INIT;
+	struct rev_info revs;
+	struct blame_scoreboard sb;
+	struct blame_origin *o;
+	struct blame_entry *ent = NULL;
+
+	type = sha1_object_info(sha1, &size);
+	if (type == OBJ_BAD) {
+		cgit_print_error_page(404, "Not found", "Bad object name: %s",
+				      sha1_to_hex(sha1));
+		return;
+	}
+
+	argv_array_push(&rev_argv, "blame");
+	argv_array_push(&rev_argv, rev);
+	init_revisions(&revs, NULL);
+	DIFF_OPT_SET(&revs.diffopt, ALLOW_TEXTCONV);
+	setup_revisions(rev_argv.argc, rev_argv.argv, &revs, NULL);
+	init_scoreboard(&sb);
+	sb.revs = &revs;
+	setup_scoreboard(&sb, path, &o);
+	o->suspects = blame_entry_prepend(NULL, 0, sb.num_lines, o);
+	prio_queue_put(&sb.commits, o->commit);
+	blame_origin_decref(o);
+	sb.ent = NULL;
+	sb.path = path;
+	assign_blame(&sb, 0);
+	blame_sort_final(&sb);
+	blame_coalesce(&sb);
+
+	cgit_set_title_from_path(path);
+
+	cgit_print_layout_start();
+	htmlf("blob: %s (", sha1_to_hex(sha1));
+	cgit_plain_link("plain", NULL, NULL, ctx.qry.head, rev, path);
+	html(") (");
+	cgit_tree_link("tree", NULL, NULL, ctx.qry.head, rev, path);
+	html(")\n");
+
+	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
+		htmlf("<div class='error'>blob size (%ldKB)"
+		      " exceeds display size limit (%dKB).</div>",
+		      size / 1024, ctx.cfg.max_blob_size);
+		return;
+	}
+
+	html("<table class='blame blob'>");
+	for (ent = sb.ent; ent; ) {
+		struct blame_entry *e = ent->next;
+		emit_blame_entry(&sb, ent);
+		free(ent);
+		ent = e;
+	}
+	html("</table>\n");
+	free((void *)sb.final_buf);
+
+	cgit_print_layout_end();
+}
+
+static int walk_tree(const unsigned char *sha1, struct strbuf *base,
+		     const char *pathname, unsigned mode, int stage,
+		     void *cbdata)
+{
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (base->len == walk_tree_ctx->match_baselen) {
+		if (S_ISREG(mode)) {
+			struct strbuf buffer = STRBUF_INIT;
+			strbuf_addbuf(&buffer, base);
+			strbuf_addstr(&buffer, pathname);
+			print_object(sha1, buffer.buf, pathname,
+				     walk_tree_ctx->curr_rev);
+			strbuf_release(&buffer);
+			walk_tree_ctx->state = 1;
+		} else if (S_ISDIR(mode)) {
+			walk_tree_ctx->state = 2;
+		}
+	} else if (base->len < INT_MAX
+			&& (int)base->len > walk_tree_ctx->match_baselen) {
+		walk_tree_ctx->state = 2;
+	} else if (S_ISDIR(mode)) {
+		return READ_TREE_RECURSIVE;
+	}
+	return 0;
+}
+
+static int basedir_len(const char *path)
+{
+	char *p = strrchr(path, '/');
+	if (p)
+		return p - path + 1;
+	return 0;
+}
+
+void cgit_print_blame(void)
+{
+	const char *rev = ctx.qry.sha1;
+	struct object_id oid;
+	struct commit *commit;
+	struct pathspec_item path_items = {
+		.match = ctx.qry.path,
+		.len = ctx.qry.path ? strlen(ctx.qry.path) : 0
+	};
+	struct pathspec paths = {
+		.nr = 1,
+		.items = &path_items
+	};
+	struct walk_tree_context walk_tree_ctx = {
+		.state = 0
+	};
+
+	if (!rev)
+		rev = ctx.qry.head;
+
+	if (get_oid(rev, &oid)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid revision name: %s", rev);
+		return;
+	}
+	commit = lookup_commit_reference(&oid);
+	if (!commit || parse_commit(commit)) {
+		cgit_print_error_page(404, "Not found",
+			"Invalid commit reference: %s", rev);
+		return;
+	}
+
+	walk_tree_ctx.curr_rev = xstrdup(rev);
+	walk_tree_ctx.match_baselen = (path_items.match) ?
+				       basedir_len(path_items.match) : -1;
+
+	read_tree_recursive(commit->tree, "", 0, 0, &paths, walk_tree,
+		&walk_tree_ctx);
+	if (!walk_tree_ctx.state)
+		cgit_print_error_page(404, "Not found", "Not found");
+	else if (walk_tree_ctx.state == 2)
+		cgit_print_error_page(404, "No blame for folders",
+			"Blame is not available for folders.");
+
+	free(walk_tree_ctx.curr_rev);
+}
diff --git a/ui-blame.h b/ui-blame.h
new file mode 100644
index 0000000..8a438e9
--- /dev/null
+++ b/ui-blame.h
@@ -0,0 +1,7 @@
+#ifndef UI_BLAME_H
+#define UI_BLAME_H
+
+extern void cgit_print_blame(void);
+
+#endif /* UI_BLAME_H */
+
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCHv2 5/5] ui-tree: link to blame UI if enabled
  2017-10-02  4:39     ` [PATCHv2 " whydoubt
                         ` (3 preceding siblings ...)
  2017-10-02  4:39       ` [PATCHv2 4/5] ui-blame: add blame UI whydoubt
@ 2017-10-02  4:39       ` whydoubt
  4 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-02  4:39 UTC (permalink / raw)


Create links to the blame page.

Signed-off-by: Jeff Smith <whydoubt at gmail.com>
---
 ui-shared.c | 20 +++++++++++++++++---
 ui-shared.h |  3 +++
 ui-tree.c   | 10 +++++++++-
 3 files changed, 29 insertions(+), 4 deletions(-)

diff --git a/ui-shared.c b/ui-shared.c
index 315dedb..07c78a5 100644
--- a/ui-shared.c
+++ b/ui-shared.c
@@ -1,6 +1,6 @@
 /* ui-shared.c: common web output functions
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -304,6 +304,12 @@ void cgit_plain_link(const char *name, const char *title, const char *class,
 	reporevlink("plain", name, title, class, head, rev, path);
 }
 
+void cgit_blame_link(const char *name, const char *title, const char *class,
+		     const char *head, const char *rev, const char *path)
+{
+	reporevlink("blame", name, title, class, head, rev, path);
+}
+
 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,
@@ -478,6 +484,10 @@ static void cgit_self_link(char *name, const char *title, const char *class)
 		cgit_plain_link(name, title, class, ctx.qry.head,
 				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
 				ctx.qry.path);
+	else if (!strcmp(ctx.qry.page, "blame"))
+		cgit_blame_link(name, title, class, ctx.qry.head,
+				ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
+				ctx.qry.path);
 	else if (!strcmp(ctx.qry.page, "log"))
 		cgit_log_link(name, title, class, ctx.qry.head,
 			      ctx.qry.has_sha1 ? ctx.qry.sha1 : NULL,
@@ -983,8 +993,12 @@ void cgit_print_pageheader(void)
 		cgit_log_link("log", NULL, hc("log"), ctx.qry.head,
 			      NULL, ctx.qry.vpath, 0, NULL, NULL,
 			      ctx.qry.showmsg, ctx.qry.follow);
-		cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
-			       ctx.qry.sha1, ctx.qry.vpath);
+		if (ctx.qry.page && !strcmp(ctx.qry.page, "blame"))
+			cgit_blame_link("blame", NULL, hc("blame"), ctx.qry.head,
+				        ctx.qry.sha1, ctx.qry.vpath);
+		else
+			cgit_tree_link("tree", NULL, hc("tree"), ctx.qry.head,
+				       ctx.qry.sha1, ctx.qry.vpath);
 		cgit_commit_link("commit", NULL, hc("commit"),
 				 ctx.qry.head, ctx.qry.sha1, ctx.qry.vpath);
 		cgit_diff_link("diff", NULL, hc("diff"), ctx.qry.head,
diff --git a/ui-shared.h b/ui-shared.h
index 2cd7ac9..b760a17 100644
--- a/ui-shared.h
+++ b/ui-shared.h
@@ -26,6 +26,9 @@ extern void cgit_tree_link(const char *name, const char *title,
 extern void cgit_plain_link(const char *name, const char *title,
 			    const char *class, const char *head,
 			    const char *rev, const char *path);
+extern void cgit_blame_link(const char *name, const char *title,
+			    const char *class, const char *head,
+			    const char *rev, const char *path);
 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,
diff --git a/ui-tree.c b/ui-tree.c
index 3925809..67fd1bc 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -1,6 +1,6 @@
 /* ui-tree.c: functions for tree output
  *
- * Copyright (C) 2006-2014 cgit Development Team <cgit at lists.zx2c4.com>
+ * Copyright (C) 2006-2017 cgit Development Team <cgit at lists.zx2c4.com>
  *
  * Licensed under GNU General Public License v2
  *   (see COPYING for full license text)
@@ -110,6 +110,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);
+	}
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -244,6 +249,9 @@ 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_ISDIR(mode) && ctx.cfg.enable_blame)
+		cgit_blame_link("blame", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev, fullpath.buf);
 	html("</td></tr>\n");
 	free(name);
 	strbuf_release(&fullpath);
-- 
2.9.4



^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-02  1:17       ` Jason
@ 2017-10-02  5:34         ` list
  2017-10-02 22:35           ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: list @ 2017-10-02  5:34 UTC (permalink / raw)


"Jason A. Donenfeld" <Jason at zx2c4.com> on Mon, 2017/10/02 03:17:
> Jeff -- thanks for your hard work on this.

Another thanks from me!

> John -- thanks for the review.

Same here... You beat me doing the review.

> If you'd like to put this in a
> for-jason branch, I'll get to merging it soon.

I pushed the latest version to ch/ui-blame.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20171002/82ba29a4/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-02  5:34         ` list
@ 2017-10-02 22:35           ` whydoubt
  2017-10-02 23:29             ` list
  0 siblings, 1 reply; 63+ messages in thread
From: whydoubt @ 2017-10-02 22:35 UTC (permalink / raw)


From what I can see, it appears that it is in ch/for-jason.

Also, I noticed a typo in my html_ntxt patch:
ssize_t slen = (size_t) len;
   should be
ssize_t slen = (ssize_t) len;

Do I need to resend, or can someone fix that one up for me?

 -- Jeff


On Mon, Oct 2, 2017 at 12:34 AM, Christian Hesse <list at eworm.de> wrote:
> "Jason A. Donenfeld" <Jason at zx2c4.com> on Mon, 2017/10/02 03:17:
>> Jeff -- thanks for your hard work on this.
>
> Another thanks from me!
>
>> John -- thanks for the review.
>
> Same here... You beat me doing the review.
>
>> If you'd like to put this in a
>> for-jason branch, I'll get to merging it soon.
>
> I pushed the latest version to ch/ui-blame.
> --
> main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
> "CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
> putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
>
> _______________________________________________
> CGit mailing list
> CGit at lists.zx2c4.com
> https://lists.zx2c4.com/mailman/listinfo/cgit
>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-02 22:35           ` whydoubt
@ 2017-10-02 23:29             ` list
  2017-10-03 18:22               ` john
  0 siblings, 1 reply; 63+ messages in thread
From: list @ 2017-10-02 23:29 UTC (permalink / raw)


Jeffrey Smith <whydoubt at gmail.com> on Mon, 2017/10/02 17:35:
> From what I can see, it appears that it is in ch/for-jason.
> 
> Also, I noticed a typo in my html_ntxt patch:
> ssize_t slen = (size_t) len;
>    should be
> ssize_t slen = (ssize_t) len;
> 
> Do I need to resend, or can someone fix that one up for me?

Updated. Also added proper Reviewed-by lines.
-- 
main(a){char*c=/*    Schoene Gruesse                         */"B?IJj;MEH"
"CX:;",b;for(a/*    Best regards             my address:    */=0;b=c[a++];)
putchar(b-1/(/*    Chris            cc -ox -xc - && ./x    */b/42*2-3)*42);}
-------------- next part --------------
A non-text attachment was scrubbed...
Name: not available
Type: application/pgp-signature
Size: 488 bytes
Desc: OpenPGP digital signature
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20171003/963e27df/attachment.asc>


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-02 23:29             ` list
@ 2017-10-03 18:22               ` john
  2017-10-03 18:23                 ` Jason
  0 siblings, 1 reply; 63+ messages in thread
From: john @ 2017-10-03 18:22 UTC (permalink / raw)


On Tue, Oct 03, 2017 at 01:29:59AM +0200, Christian Hesse wrote:
> Jeffrey Smith <whydoubt at gmail.com> on Mon, 2017/10/02 17:35:
> > From what I can see, it appears that it is in ch/for-jason.
> > 
> > Also, I noticed a typo in my html_ntxt patch:
> > ssize_t slen = (size_t) len;
> >    should be
> > ssize_t slen = (ssize_t) len;
> > 
> > Do I need to resend, or can someone fix that one up for me?
> 
> Updated. Also added proper Reviewed-by lines.

That also added decl-after-statement, so I've pulled the patches over to
my jk/for-jason branch and fixed that, as well as a trailing blank line
in ui-blame.h that git-am warned about.


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-03 18:22               ` john
@ 2017-10-03 18:23                 ` Jason
  2017-10-03 18:36                   ` Jason
  0 siblings, 1 reply; 63+ messages in thread
From: Jason @ 2017-10-03 18:23 UTC (permalink / raw)


Hi John,

Alright, I'll pull from your branch rather than Christian's then.

Thanks to all three of you in any case!

Jason


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-03 18:23                 ` Jason
@ 2017-10-03 18:36                   ` Jason
  2017-10-03 19:06                     ` whydoubt
  0 siblings, 1 reply; 63+ messages in thread
From: Jason @ 2017-10-03 18:36 UTC (permalink / raw)


Merged to master. Working quite nicely:

https://git.zx2c4.com/cgit/blame/ui-shared.c

Jeff -- do we want to be passing this through the source code filter
to get highlighted lines?


^ permalink raw reply	[flat|nested] 63+ messages in thread

* [PATCH 0/5] Add ui-blame
  2017-10-03 18:36                   ` Jason
@ 2017-10-03 19:06                     ` whydoubt
  0 siblings, 0 replies; 63+ messages in thread
From: whydoubt @ 2017-10-03 19:06 UTC (permalink / raw)


Jason,

I looked at that at one point, and as I recall, it was not going to
work because, as things currently stand, the source code gets
broken up into chunks.

I will take a look at it again, but the only way that comes to
mind will require some creativity with style-sheets and such.

On Tue, Oct 3, 2017 at 1:36 PM, Jason A. Donenfeld <Jason at zx2c4.com> wrote:
> Merged to master. Working quite nicely:
>
> https://git.zx2c4.com/cgit/blame/ui-shared.c
>
> Jeff -- do we want to be passing this through the source code filter
> to get highlighted lines?


^ permalink raw reply	[flat|nested] 63+ messages in thread

end of thread, other threads:[~2017-10-03 19:06 UTC | newest]

Thread overview: 63+ messages (download: mbox.gz / follow: Atom feed)
-- links below jump to the message on this page --
2017-06-08  2:18 [RFC PATCH 0/4] Add ui-blame whydoubt
2017-06-08  2:18 ` [RFC PATCH 1/4] git: update to v2.14 whydoubt
2017-07-22 11:25   ` john
2017-06-08  2:18 ` [RFC PATCH 2/4] ui-blame: create placeholder and links whydoubt
2017-07-22 11:31   ` john
2017-06-08  2:18 ` [RFC PATCH 3/4] ui-blame: create needed html_ntxt_noellipsis function whydoubt
2017-07-22 11:36   ` john
2017-06-08  2:18 ` [RFC PATCH 4/4] ui-blame: fill in the contents whydoubt
2017-07-05  8:32   ` list
2017-07-22 11:47   ` john
2017-06-08  9:00 ` [RFC PATCH 0/4] Add ui-blame list
2017-07-22 12:02 ` john
2017-08-05  0:23   ` dlcampbell
2017-08-05  0:57     ` whydoubt
2017-08-24 18:14       ` list
2017-08-31 13:05         ` whydoubt
2017-09-23  3:38 ` [RFCv2 PATCH 0/7] " whydoubt
2017-09-23  3:38   ` [RFCv2 PATCH 1/7] ui-blame: create enable-blame config item whydoubt
2017-09-23 15:46     ` john
2017-09-24  3:12       ` whydoubt
2017-09-23  3:38   ` [RFCv2 PATCH 2/7] ui-blame: create framework whydoubt
2017-09-23 15:47     ` john
2017-09-24  3:24       ` whydoubt
2017-09-23  3:38   ` [RFCv2 PATCH 3/7] ui-blame: create links whydoubt
2017-09-23 15:47     ` john
2017-09-24 20:25       ` whydoubt
2017-09-23  3:38   ` [RFCv2 PATCH 4/7] ui-blame: html_ntxt with no ellipsis whydoubt
2017-09-23 15:47     ` john
2017-09-23  3:38   ` [RFCv2 PATCH 5/7] ui-blame: pull blame info from libgit whydoubt
2017-09-23 15:47     ` john
2017-09-24 19:06       ` whydoubt
2017-09-24 20:09         ` whydoubt
2017-09-24 20:52           ` john
2017-09-23  3:38   ` [RFCv2 PATCH 6/7] ui-blame: begin building whydoubt
2017-09-23  3:38   ` [RFCv2 PATCH 7/7] ui-blame: generate blame page when requested whydoubt
2017-09-23 15:53   ` [RFCv2 PATCH 0/7] Add ui-blame john
2017-09-24  3:05     ` whydoubt
2017-09-27 22:43   ` [PATCH 0/5] " whydoubt
2017-09-27 22:43     ` [PATCH 1/5] html: html_ntxt with no ellipsis whydoubt
2017-09-30 11:55       ` john
2017-09-27 22:43     ` [PATCH 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
2017-09-30 11:56       ` john
2017-09-27 22:43     ` [PATCH 3/5] ui-shared: make a char* parameter const whydoubt
2017-09-30 12:00       ` john
2017-09-27 22:43     ` [PATCH 4/5] ui-blame: add blame UI whydoubt
2017-09-30 12:07       ` john
2017-09-27 22:43     ` [PATCH 5/5] ui-tree: link to blame UI if enabled whydoubt
2017-09-30 12:08       ` john
2017-09-30 12:10     ` [PATCH 0/5] Add ui-blame john
2017-10-02  1:17       ` Jason
2017-10-02  5:34         ` list
2017-10-02 22:35           ` whydoubt
2017-10-02 23:29             ` list
2017-10-03 18:22               ` john
2017-10-03 18:23                 ` Jason
2017-10-03 18:36                   ` Jason
2017-10-03 19:06                     ` whydoubt
2017-10-02  4:39     ` [PATCHv2 " whydoubt
2017-10-02  4:39       ` [PATCHv2 1/5] html: html_ntxt with no ellipsis whydoubt
2017-10-02  4:39       ` [PATCHv2 2/5] ui-tree: move set_title_from_path to ui-shared whydoubt
2017-10-02  4:39       ` [PATCHv2 3/5] ui-shared: make a char* parameter const whydoubt
2017-10-02  4:39       ` [PATCHv2 4/5] ui-blame: add blame UI whydoubt
2017-10-02  4:39       ` [PATCHv2 5/5] ui-tree: link to blame UI if enabled whydoubt

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).