[CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

This patch series fixes two bugs and communicates the repo name
to the filter scripts.

I have a server setup in which each repo has a trac instance and
for the commit filter I really need to known with which repo I'm
dealing in order to be able to resolve the #123 ticket numbers
into hyperlinks into the correct trac instance.

Patch 0001 fixes a memory leak and can be applied regardless of the other
           patches
Patch 0002 makes sure that all arguments for the filter are correctly initialised
           so that the argv[] list is always terminated with a NULL pointer,
           which is currently not the case for the source filter.
The other patches implement my desired functionality.


Ferry Huberts (6):
  source_filter: fix a memory leak
  new_filter: correctly initialise all arguments for a new filter
  new_filter: determine extra_args from filter type
  source_filter: also communicate the repo name to the filter
  commit_filter: also communicate the repo name to the filter
  about_filter: also communicate the repo name to the filter

 cgit.c                         |   41 +++++++++++++++++++++++++++++++--------
 filters/commit-links.sh        |    2 +
 filters/syntax-highlighting.sh |    1 +
 ui-commit.c                    |    5 +++-
 ui-repolist.c                  |    5 +++-
 ui-summary.c                   |    5 +++-
 ui-tree.c                      |    4 +++
 7 files changed, 51 insertions(+), 12 deletions(-)

-- 
1.7.4

[CGit] [PATCH 1/6] source_filter: fix a memory leak

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 ui-tree.c |    2 ++
 1 files changed, 2 insertions(+), 0 deletions(-)

diff --git a/ui-tree.c b/ui-tree.c
index 0b1b531..442b6be 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -48,6 +48,8 @@ static void print_text_buffer(const char *name, char *buf, unsigned long size)
 		cgit_open_filter(ctx.repo->source_filter);
 		html_raw(buf, size);
 		cgit_close_filter(ctx.repo->source_filter);
+		free(ctx.repo->source_filter->argv[1]);
+		ctx.repo->source_filter->argv[1] = NULL;
 		html("</code></pre></td></tr></table>\n");
 		return;
 	}
-- 
1.7.4

[CGit] [PATCH 2/6] new_filter: correctly initialise all arguments for a new filter

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 cgit.c |    7 +++++--
 1 files changed, 5 insertions(+), 2 deletions(-)

diff --git a/cgit.c b/cgit.c
index 412fbf0..4440feb 100644
--- a/cgit.c
+++ b/cgit.c
@@ -28,6 +28,7 @@ void add_mimetype(const char *name, const char *value)
 
 struct cgit_filter *new_filter(const char *cmd, int extra_args)
 {
+	int i = 0;
 	struct cgit_filter *f;
 
 	if (!cmd || !cmd[0])
@@ -36,8 +37,10 @@ struct cgit_filter *new_filter(const char *cmd, int extra_args)
 	f = xmalloc(sizeof(struct cgit_filter));
 	f->cmd = xstrdup(cmd);
 	f->argv = xmalloc((2 + extra_args) * sizeof(char *));
-	f->argv[0] = f->cmd;
-	f->argv[1] = NULL;
+	f->argv[i++] = f->cmd;
+	while (i < (2 + extra_args)) {
+	  f->argv[i++] = NULL;
+	}
 	return f;
 }
 
-- 
1.7.4

[CGit] [PATCH 3/6] new_filter: determine extra_args from filter type

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 cgit.c |   34 +++++++++++++++++++++++++++-------
 1 files changed, 27 insertions(+), 7 deletions(-)

diff --git a/cgit.c b/cgit.c
index 4440feb..6fa8f60 100644
--- a/cgit.c
+++ b/cgit.c
@@ -18,6 +18,10 @@
 
 const char *cgit_version = CGIT_VERSION;
 
+typedef enum {
+	about, commit, source
+} filter_type;
+
 void add_mimetype(const char *name, const char *value)
 {
 	struct string_list_item *item;
@@ -26,14 +30,30 @@ void add_mimetype(const char *name, const char *value)
 	item->util = xstrdup(value);
 }
 
-struct cgit_filter *new_filter(const char *cmd, int extra_args)
+struct cgit_filter *new_filter(const char *cmd, filter_type filtertype)
 {
 	int i = 0;
 	struct cgit_filter *f;
+	int extra_args;
 
 	if (!cmd || !cmd[0])
 		return NULL;
 
+	switch (filtertype) {
+		case about:
+		case commit:
+			extra_args = 0;
+			break;
+
+		case source:
+			extra_args = 1;
+			break;
+
+		default:
+			extra_args = 0;
+			break;
+	}
+
 	f = xmalloc(sizeof(struct cgit_filter));
 	f->cmd = xstrdup(cmd);
 	f->argv = xmalloc((2 + extra_args) * sizeof(char *));
@@ -78,11 +98,11 @@ void repo_config(struct cgit_repo *repo, const char *name, const char *value)
 		repo->readme = xstrdup(value);
 	} else if (ctx.cfg.enable_filter_overrides) {
 		if (!strcmp(name, "about-filter"))
-			repo->about_filter = new_filter(value, 0);
+			repo->about_filter = new_filter(value, about);
 		else if (!strcmp(name, "commit-filter"))
-			repo->commit_filter = new_filter(value, 0);
+			repo->commit_filter = new_filter(value, commit);
 		else if (!strcmp(name, "source-filter"))
-			repo->source_filter = new_filter(value, 1);
+			repo->source_filter = new_filter(value, source);
 	}
 }
 
@@ -171,9 +191,9 @@ void config_cb(const char *name, const char *value)
 	else if (!strcmp(name, "cache-dynamic-ttl"))
 		ctx.cfg.cache_dynamic_ttl = atoi(value);
 	else if (!strcmp(name, "about-filter"))
-		ctx.cfg.about_filter = new_filter(value, 0);
+		ctx.cfg.about_filter = new_filter(value, about);
 	else if (!strcmp(name, "commit-filter"))
-		ctx.cfg.commit_filter = new_filter(value, 0);
+		ctx.cfg.commit_filter = new_filter(value, commit);
 	else if (!strcmp(name, "embedded"))
 		ctx.cfg.embedded = atoi(value);
 	else if (!strcmp(name, "max-atom-items"))
@@ -201,7 +221,7 @@ void config_cb(const char *name, const char *value)
 	else if (!strcmp(name, "section-from-path"))
 		ctx.cfg.section_from_path = atoi(value);
 	else if (!strcmp(name, "source-filter"))
-		ctx.cfg.source_filter = new_filter(value, 1);
+		ctx.cfg.source_filter = new_filter(value, source);
 	else if (!strcmp(name, "summary-log"))
 		ctx.cfg.summary_log = atoi(value);
 	else if (!strcmp(name, "summary-branches"))
-- 
1.7.4

[CGit] [PATCH 4/6] source_filter: also communicate the repo name to the filter

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 cgit.c                         |    2 +-
 filters/syntax-highlighting.sh |    1 +
 ui-tree.c                      |    2 ++
 3 files changed, 4 insertions(+), 1 deletions(-)

diff --git a/cgit.c b/cgit.c
index 6fa8f60..65c892c 100644
--- a/cgit.c
+++ b/cgit.c
@@ -46,7 +46,7 @@ struct cgit_filter *new_filter(const char *cmd, filter_type filtertype)
 			break;
 
 		case source:
-			extra_args = 1;
+			extra_args = 2;
 			break;
 
 		default:
diff --git a/filters/syntax-highlighting.sh b/filters/syntax-highlighting.sh
index 6b1c576..80d670f 100755
--- a/filters/syntax-highlighting.sh
+++ b/filters/syntax-highlighting.sh
@@ -26,6 +26,7 @@
 
 # store filename and extension in local vars
 BASENAME="$1"
+REPONAME="$2"
 EXTENSION="${BASENAME##*.}"
 
 # map Makefile and Makefile.* to .mk
diff --git a/ui-tree.c b/ui-tree.c
index 442b6be..2c7298c 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -45,6 +45,8 @@ static void print_text_buffer(const char *name, char *buf, unsigned long size)
 	if (ctx.repo->source_filter) {
 		html("<td class='lines'><pre><code>");
 		ctx.repo->source_filter->argv[1] = xstrdup(name);
+		if (!ctx.repo->source_filter->argv[2])
+			ctx.repo->source_filter->argv[2] = xstrdup(ctx.repo->name);
 		cgit_open_filter(ctx.repo->source_filter);
 		html_raw(buf, size);
 		cgit_close_filter(ctx.repo->source_filter);
-- 
1.7.4

[CGit] [PATCH 5/6] commit_filter: also communicate the repo name to the filter

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 cgit.c                  |    5 ++++-
 filters/commit-links.sh |    2 ++
 ui-commit.c             |    5 ++++-
 3 files changed, 10 insertions(+), 2 deletions(-)

diff --git a/cgit.c b/cgit.c
index 65c892c..9e8c4c4 100644
--- a/cgit.c
+++ b/cgit.c
@@ -41,10 +41,13 @@ struct cgit_filter *new_filter(const char *cmd, filter_type filtertype)
 
 	switch (filtertype) {
 		case about:
-		case commit:
 			extra_args = 0;
 			break;
 
+		case commit:
+			extra_args = 1;
+			break;
+
 		case source:
 			extra_args = 2;
 			break;
diff --git a/filters/commit-links.sh b/filters/commit-links.sh
index 110c609..d098cd5 100755
--- a/filters/commit-links.sh
+++ b/filters/commit-links.sh
@@ -3,6 +3,8 @@
 #
 # To use this script, refer to this file with either the commit-filter or the
 # repo.commit-filter options in cgitrc.
+#
+# 1st argument is the repo name
 
 # This expression generates links to commits referenced by their SHA1.
 regex=$regex'
diff --git a/ui-commit.c b/ui-commit.c
index 2b4f677..b0607d0 100644
--- a/ui-commit.c
+++ b/ui-commit.c
@@ -109,8 +109,11 @@ void cgit_print_commit(char *hex, const char *prefix)
 	}
 	html("</table>\n");
 	html("<div class='commit-subject'>");
-	if (ctx.repo->commit_filter)
+	if (ctx.repo->commit_filter) {
+		if (!ctx.repo->commit_filter->argv[1])
+			ctx.repo->commit_filter->argv[1] = xstrdup(ctx.repo->name);
 		cgit_open_filter(ctx.repo->commit_filter);
+	}
 	html_txt(info->subject);
 	if (ctx.repo->commit_filter)
 		cgit_close_filter(ctx.repo->commit_filter);
-- 
1.7.4

[CGit] [PATCH 6/6] about_filter: also communicate the repo name to the filter

[Ferry Huberts]

From: Ferry Huberts <ferry.huberts@pelagic.nl>

Signed-off-by: Ferry Huberts <ferry.huberts@pelagic.nl>
---
 cgit.c        |    3 ---
 ui-repolist.c |    5 ++++-
 ui-summary.c  |    5 ++++-
 3 files changed, 8 insertions(+), 5 deletions(-)

diff --git a/cgit.c b/cgit.c
index 9e8c4c4..653b099 100644
--- a/cgit.c
+++ b/cgit.c
@@ -41,9 +41,6 @@ struct cgit_filter *new_filter(const char *cmd, filter_type filtertype)
 
 	switch (filtertype) {
 		case about:
-			extra_args = 0;
-			break;
-
 		case commit:
 			extra_args = 1;
 			break;
diff --git a/ui-repolist.c b/ui-repolist.c
index 2c98668..a0c2235 100644
--- a/ui-repolist.c
+++ b/ui-repolist.c
@@ -290,8 +290,11 @@ void cgit_print_site_readme()
 {
 	if (!ctx.cfg.root_readme)
 		return;
-	if (ctx.cfg.about_filter)
+	if (ctx.cfg.about_filter) {
+		if (!ctx.repo->about_filter->argv[1])
+			ctx.repo->about_filter->argv[1] = xstrdup(ctx.repo->name);
 		cgit_open_filter(ctx.cfg.about_filter);
+	}
 	html_include(ctx.cfg.root_readme);
 	if (ctx.cfg.about_filter)
 		cgit_close_filter(ctx.cfg.about_filter);
diff --git a/ui-summary.c b/ui-summary.c
index b203bcc..f8c85a2 100644
--- a/ui-summary.c
+++ b/ui-summary.c
@@ -112,8 +112,11 @@ void cgit_print_repo_readme(char *path)
 	 * filesystem, while applying the about-filter.
 	 */
 	html("<div id='summary'>");
-	if (ctx.repo->about_filter)
+	if (ctx.repo->about_filter) {
+		if (!ctx.repo->about_filter->argv[1])
+			ctx.repo->about_filter->argv[1] = xstrdup(ctx.repo->name);
 		cgit_open_filter(ctx.repo->about_filter);
+	}
 	if (ref)
 		cgit_print_file(tmp, ref);
 	else
-- 
1.7.4

Re: [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[Ferry Huberts]

PS. this patch series is (re)based on the current master
17596459fe9a43428a261e66f65b227d15bf7ee5

grtz

-- 
Ferry Huberts

Re: [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[Lars Hjemli]

On Thu, Feb 17, 2011 at 22:11, Ferry Huberts <mailings@hupie.com> wrote:
> This patch series fixes two bugs and communicates the repo name
> to the filter scripts.

Thanks, but I think the current filter invocations with unnamed,
positional command arguments was a mistake. We should probably fix it
instead of extending it, taking care not to break backwards
compatibility.

The easiest fix would be to add some environment variables:
* GIT_DIR
* CGIT_REPO_NAME
* CGIT_REPO_REL_URL
* CGIT_REPO_ABS_URL
* CGIT_BRANCH_NAME
* CGIT_COMMIT_ID
* CGIT_PATH_FILTER

What do you think?

--
larsh

Re: [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[Ferry Huberts]

On 02/19/2011 09:46 AM, Lars Hjemli wrote:
> On Thu, Feb 17, 2011 at 22:11, Ferry Huberts <mailings@hupie.com> wrote:
>> This patch series fixes two bugs and communicates the repo name
>> to the filter scripts.
> 
> Thanks, but I think the current filter invocations with unnamed,
> positional command arguments was a mistake. We should probably fix it
> instead of extending it, taking care not to break backwards
> compatibility.
> 
> The easiest fix would be to add some environment variables:
> * GIT_DIR
> * CGIT_REPO_NAME
> * CGIT_REPO_REL_URL
> * CGIT_REPO_ABS_URL
> * CGIT_BRANCH_NAME
> * CGIT_COMMIT_ID
> * CGIT_PATH_FILTER
> 
> What do you think?
> 
> --
> larsh

I've been thinking about this too and did think about this solution but
also about a solution in which these settings are written to a temporary
file. I have no preference as long as the repo settings are communicated
to the filter script. Writing to a file is probably bad for performance
though.

The patches I sent just plainly solve my own problem and I was hoping a
bit for this discussion :-)

Your proposal sounds reasonable and a good approach.

I'm a bit worried about the GIT_DIR env var, about it conflicting with
the real git env var. It's probably best to rename that one to CGIT_GIT_DIR.

Also, we'd have to be sure that when we set these env vars that they're
only propagated to the filter script we're going to run and not to other
envs: running filter scripts in parallel must not be prevented.


Have you looked at the first 2 patches?
These solve bugs and can be applied regardless of this discussion.


please let me know what you think.


grtz
-- 
Ferry Huberts

Re: [CGit] [PATCH 0/6] Communicate the repo name to the filter scripts

[Ferry Huberts]

Lars,

I've put my repo on github: https://github.com/fhuberts/cgit
You can fetch my changes from there

Any idea on how to proceed?


On 02/19/2011 09:46 AM, Lars Hjemli wrote:
> On Thu, Feb 17, 2011 at 22:11, Ferry Huberts <mailings@hupie.com> wrote:
>> This patch series fixes two bugs and communicates the repo name
>> to the filter scripts.
> 
> Thanks, but I think the current filter invocations with unnamed,
> positional command arguments was a mistake. We should probably fix it
> instead of extending it, taking care not to break backwards
> compatibility.
> 
> The easiest fix would be to add some environment variables:
> * GIT_DIR
> * CGIT_REPO_NAME
> * CGIT_REPO_REL_URL
> * CGIT_REPO_ABS_URL
> * CGIT_BRANCH_NAME
> * CGIT_COMMIT_ID
> * CGIT_PATH_FILTER
> 
> What do you think?
> 
> --
> larsh
> --
> To unsubscribe from this list: send the line "unsubscribe git" in
> the body of a message to majordomo@vger.kernel.org
> More majordomo info at  http://vger.kernel.org/majordomo-info.html

grtz

-- 
Ferry Huberts