[PATCH 0/4] adding color to ui-blame

[PATCH 0/4] adding color to ui-blame

[Jason]

This patch set is currently broken, because in the exec filter,
processes like to buffer their output. The result is that the text winds
up at the bottom:

	https://git.zx2c4.com/cgit/blame/cache.c

If anybody has some ideas on how we might enact a flush operation, please
pipe up (hah).

Jason A. Donenfeld (4):
  filter: add generic scafolding for temporarily disabling filter
  filter: wire up exec_filter to enable function
  filter: wire up lua_filter to enable function
  ui-blame: put source lines through filter

 cgit.h     |  3 +++
 filter.c   | 46 ++++++++++++++++++++++++++++++++++++++--------
 ui-blame.c | 15 ++++++++++++++-
 3 files changed, 55 insertions(+), 9 deletions(-)

-- 
2.14.2

[PATCH 1/4] filter: add generic scafolding for temporarily disabling filter

[Jason]

Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
---
 cgit.h   | 2 ++
 filter.c | 9 +++++++--
 2 files changed, 9 insertions(+), 2 deletions(-)

diff --git a/cgit.h b/cgit.h
index 005ae63..8da69e7 100644
--- a/cgit.h
+++ b/cgit.h
@@ -61,6 +61,7 @@ typedef enum {
 struct cgit_filter {
 	int (*open)(struct cgit_filter *, va_list ap);
 	int (*close)(struct cgit_filter *);
+	int (*enable)(struct cgit_filter *, bool on);
 	void (*fprintf)(struct cgit_filter *, FILE *, const char *prefix);
 	void (*cleanup)(struct cgit_filter *);
 	int argument_count;
@@ -376,6 +377,7 @@ extern int cgit_parse_snapshots_mask(const char *str);
 
 extern int cgit_open_filter(struct cgit_filter *filter, ...);
 extern int cgit_close_filter(struct cgit_filter *filter);
+extern int cgit_enable_filter(struct cgit_filter *filter, bool on);
 extern void cgit_fprintf_filter(struct cgit_filter *filter, FILE *f, const char *prefix);
 extern void cgit_exec_filter_init(struct cgit_exec_filter *filter, char *cmd, char **argv);
 extern struct cgit_filter *cgit_new_filter(const char *cmd, filter_type filtertype);
diff --git a/filter.c b/filter.c
index 70f5b74..aa0027a 100644
--- a/filter.c
+++ b/filter.c
@@ -383,13 +383,18 @@ int cgit_close_filter(struct cgit_filter *filter)
 	return filter->close(filter);
 }
 
+int cgit_enable_filter(struct cgit_filter *filter, bool on)
+{
+	if (!filter)
+		return 0;
+	return filter->enable(filter, on);
+}
+
 void cgit_fprintf_filter(struct cgit_filter *filter, FILE *f, const char *prefix)
 {
 	filter->fprintf(filter, f, prefix);
 }
 
-
-
 static const struct {
 	const char *prefix;
 	struct cgit_filter *(*ctor)(const char *cmd, int argument_count);
-- 
2.14.2

[PATCH 2/4] filter: wire up exec_filter to enable function

[Jason]

We also move pipe_fh to be local, since it's not needed any place else.

Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
---
 cgit.h   |  1 +
 filter.c | 27 +++++++++++++++++++++------
 2 files changed, 22 insertions(+), 6 deletions(-)

diff --git a/cgit.h b/cgit.h
index 8da69e7..f9949a7 100644
--- a/cgit.h
+++ b/cgit.h
@@ -72,6 +72,7 @@ struct cgit_exec_filter {
 	char *cmd;
 	char **argv;
 	int old_stdout;
+	int new_stdout;
 	int pid;
 };
 
diff --git a/filter.c b/filter.c
index aa0027a..4deb4de 100644
--- a/filter.c
+++ b/filter.c
@@ -60,9 +60,8 @@ static int open_exec_filter(struct cgit_filter *base, va_list ap)
 		die_errno("Unable to exec subprocess %s", filter->cmd);
 	}
 	close(pipe_fh[0]);
-	chk_non_negative(dup2(pipe_fh[1], STDOUT_FILENO),
-		"Unable to use pipe as STDOUT");
-	close(pipe_fh[1]);
+	filter->new_stdout = pipe_fh[1];
+	chk_non_negative(dup2(filter->new_stdout, STDOUT_FILENO), "Unable to assign new fd to STDOUT");
 	return 0;
 }
 
@@ -71,9 +70,10 @@ static int close_exec_filter(struct cgit_filter *base)
 	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
 	int i, exit_status = 0;
 
-	chk_non_negative(dup2(filter->old_stdout, STDOUT_FILENO),
-		"Unable to restore STDOUT");
+	chk_non_negative(dup2(filter->old_stdout, STDOUT_FILENO), "Unable to assign old fd to STDOUT");
 	close(filter->old_stdout);
+	close(filter->new_stdout);
+
 	if (filter->pid < 0)
 		goto done;
 	waitpid(filter->pid, &exit_status, 0);
@@ -82,10 +82,24 @@ static int close_exec_filter(struct cgit_filter *base)
 	die("Subprocess %s exited abnormally", filter->cmd);
 
 done:
+	close(filter->old_stdout);
+	close(filter->new_stdout);
 	for (i = 0; i < filter->base.argument_count; i++)
 		filter->argv[i + 1] = NULL;
-	return WEXITSTATUS(exit_status);
+	return WIFEXITED(exit_status);
+}
 
+static int enable_exec_filter(struct cgit_filter *base, bool on)
+{
+	struct cgit_exec_filter *filter = (struct cgit_exec_filter *)base;
+	int ret;
+
+	if (on)
+		ret = dup2(filter->new_stdout, STDOUT_FILENO);
+	else
+		ret = dup2(filter->old_stdout, STDOUT_FILENO);
+	chk_non_negative(ret, "Unable to assign fd to STDOUT");
+	return 0;
 }
 
 static void fprintf_exec_filter(struct cgit_filter *base, FILE *f, const char *prefix)
@@ -128,6 +142,7 @@ void cgit_exec_filter_init(struct cgit_exec_filter *filter, char *cmd, char **ar
 	memset(filter, 0, sizeof(*filter));
 	filter->base.open = open_exec_filter;
 	filter->base.close = close_exec_filter;
+	filter->base.enable = enable_exec_filter;
 	filter->base.fprintf = fprintf_exec_filter;
 	filter->base.cleanup = cleanup_exec_filter;
 	filter->cmd = cmd;
-- 
2.14.2

[PATCH 3/4] filter: wire up lua_filter to enable function

[Jason]

Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
---
 filter.c | 10 ++++++++++
 1 file changed, 10 insertions(+)

diff --git a/filter.c b/filter.c
index 4deb4de..7b5bef5 100644
--- a/filter.c
+++ b/filter.c
@@ -353,6 +353,15 @@ static int close_lua_filter(struct cgit_filter *base)
 	return ret;
 }
 
+static int enable_lua_filter(struct cgit_filter *base, bool on)
+{
+	if (on)
+		hook_write(base, write_lua_filter);
+	else
+		unhook_write();
+	return 0;
+}
+
 static void fprintf_lua_filter(struct cgit_filter *base, FILE *f, const char *prefix)
 {
 	struct lua_filter *filter = (struct lua_filter *)base;
@@ -368,6 +377,7 @@ static struct cgit_filter *new_lua_filter(const char *cmd, int argument_count)
 	memset(filter, 0, sizeof(*filter));
 	filter->base.open = open_lua_filter;
 	filter->base.close = close_lua_filter;
+	filter->base.enable = enable_lua_filter;
 	filter->base.fprintf = fprintf_lua_filter;
 	filter->base.cleanup = cleanup_lua_filter;
 	filter->base.argument_count = argument_count;
-- 
2.14.2

[PATCH 4/4] ui-blame: put source lines through filter

[Jason]

We toggle the filter on and off so that the control table can avoid
going through the filter.

Signed-off-by: Jason A. Donenfeld <Jason at zx2c4.com>
---
 ui-blame.c | 15 ++++++++++++++-
 1 file changed, 14 insertions(+), 1 deletion(-)

diff --git a/ui-blame.c b/ui-blame.c
index 62cf431..71aec65 100644
--- a/ui-blame.c
+++ b/ui-blame.c
@@ -70,7 +70,12 @@ static void emit_blame_entry(struct blame_scoreboard *sb,
 	cpend = blame_nth_line(sb, ent->lno + ent->num_lines);
 
 	html("<td class='lines'><pre><code>");
-	html_ntxt(cp, cpend - cp);
+	if (ctx.repo->source_filter) {
+		cgit_enable_filter(ctx.repo->source_filter, true);
+		html_raw(cp, cpend - cp);
+		cgit_enable_filter(ctx.repo->source_filter, false);
+	} else
+		html_ntxt(cp, cpend - cp);
 	html("</code></pre></td></tr>\n");
 }
 
@@ -90,6 +95,7 @@ static void print_object(const unsigned char *sha1, const char *path,
 	struct blame_scoreboard sb;
 	struct blame_origin *o;
 	struct blame_entry *ent = NULL;
+	char *filter_arg;
 
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
@@ -131,6 +137,11 @@ static void print_object(const unsigned char *sha1, const char *path,
 		return;
 	}
 
+	if (ctx.repo->source_filter) {
+		filter_arg = xstrdup(path);
+		cgit_open_filter(ctx.repo->source_filter, filter_arg);
+		cgit_enable_filter(ctx.repo->source_filter, false);
+	}
 	html("<table class='blame blob'>");
 	for (ent = sb.ent; ent; ) {
 		struct blame_entry *e = ent->next;
@@ -139,6 +150,8 @@ static void print_object(const unsigned char *sha1, const char *path,
 		ent = e;
 	}
 	html("</table>\n");
+	if (ctx.repo->source_filter)
+		cgit_close_filter(ctx.repo->source_filter);
 	free((void *)sb.final_buf);
 
 	cgit_print_layout_end();
-- 
2.14.2

[PATCH 0/4] adding color to ui-blame

[john]

On Sat, Oct 14, 2017 at 04:17:46PM +0200, Jason A. Donenfeld wrote:
> This patch set is currently broken, because in the exec filter,
> processes like to buffer their output. The result is that the text winds
> up at the bottom:
> 
> 	https://git.zx2c4.com/cgit/blame/cache.c
> 
> If anybody has some ideas on how we might enact a flush operation, please
> pipe up (hah).

The only thing I can think of is stdbuf, but that uses LD_PRELOAD
internally and only works if the filter only uses the default stdio
buffering and processing the input incrementally; our
syntax-highlighting.py slurps the entire input stream before outputting
anything.

I wonder if it would be more reliable to split the output of the filter
on the assumption that there is a one-to-one mapping from input lines to
output lines.  Both of the filters we ship do this, although the Python
version dumps out a stylesheet before starting the document.

Having thought about this a bit, I can't see any way to make this work
reliably with our current filter protocol.  Maybe we should consider
introducing a more capable protocol for source filters that is designed
to be usable in this scenario.

[PATCH 0/4] adding color to ui-blame

[Jason]

Right, we may very well need a more capable model.

One approach would be to get rid of all the exec stuff, and instead
have plugins, that can register themselves in all sorts of places. A
plugin would define a series of hooks and filters, which would be
directly called at the right time. We'd expand the Lua stuff to
support that, and then add Python support and all the other various
languages one at a time.

Would indeed be a big project, but potentially useful.

Though I _do_ like the simplicity of the current situation quite a bit.

[PATCH 0/4] adding color to ui-blame

[whydoubt]

That seems like it could be valuable work, but FWIW I have been working on
a different solution to adding syntax highlighting to the blame page. I
hope to have it ready (at least a prototype) in the next day or two.

On Oct 15, 2017 11:47 AM, "Jason A. Donenfeld" <Jason at zx2c4.com> wrote:

> Right, we may very well need a more capable model.
>
> One approach would be to get rid of all the exec stuff, and instead
> have plugins, that can register themselves in all sorts of places. A
> plugin would define a series of hooks and filters, which would be
> directly called at the right time. We'd expand the Lua stuff to
> support that, and then add Python support and all the other various
> languages one at a time.
>
> Would indeed be a big project, but potentially useful.
>
> Though I _do_ like the simplicity of the current situation quite a bit.
>
-------------- next part --------------
An HTML attachment was scrubbed...
URL: <http://lists.zx2c4.com/pipermail/cgit/attachments/20171015/cf28205f/attachment.html>

[PATCH 0/4] adding color to ui-blame

[Jason]

Hi Jeff,

Oh, cool. What's your idea for accomplishing that? Planning on putting
it through the filter, buffering it, and then spitting it back out in
equal sized chunks, hoping the filter doesn't add extra new lines?

Jason