[PATCH 0/7] Use context structures in walk_tree() functions

[PATCH 0/7] Use context structures in walk_tree() functions

[cgit]

This turned out to be a bit harder than I expected. I split my initial
patch (plus additions) into several chunks that are hopefully a bit
easier to understand/follow.

All patches are based on the "Fix several whitespace errors" I submitted
earlier.

Lukas Fleischer (7):
  ui-blob.c: Use a context structure in walk_tree()
  ui-plain.c: Do not access match variable in print_*()
  ui-plain.c: Use a context structure in walk_tree()
  ui-tree.c: Pass current revision to print_object()
  ui-tree.c: Declare the state variable globally
  ui-tree.c: Drop the header variable
  ui-tree.c: Use a context structure in walk_tree()

 ui-blob.c  | 42 ++++++++++++++++++++++++++----------------
 ui-plain.c | 47 +++++++++++++++++++++++++++--------------------
 ui-tree.c  | 60 ++++++++++++++++++++++++++++++++----------------------------
 3 files changed, 85 insertions(+), 64 deletions(-)

-- 
1.8.2.rc0.247.g811e0c0

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[cgit]

Do not misuse global variables to save the context. Instead, use the
context pointer which was designed to share information between a
read_tree_fn and the caller.

This also prevents from potential misuse of the global pointers
match_path and matched_sha1 after the referenced values have been
overwritten on the stack.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-blob.c | 42 ++++++++++++++++++++++++++----------------
 1 file changed, 26 insertions(+), 16 deletions(-)

diff --git a/ui-blob.c b/ui-blob.c
index 3d07ce5..4bcbc82 100644
--- a/ui-blob.c
+++ b/ui-blob.c
@@ -11,17 +11,22 @@
 #include "html.h"
 #include "ui-shared.h"
 
-static char *match_path;
-static unsigned char *matched_sha1;
-static int found_path;
+struct walk_tree_context {
+	char *match_path;
+	unsigned char *matched_sha1;
+	int found_path;
+};
 
 static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
-	const char *pathname, unsigned mode, int stage, void *cbdata) {
-	if(strncmp(base, match_path, baselen)
-		|| strcmp(match_path + baselen, pathname))
+	const char *pathname, unsigned mode, int stage, void *cbdata)
+{
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if(strncmp(base, walk_tree_ctx->match_path, baselen)
+		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
 		return READ_TREE_RECURSIVE;
-	memmove(matched_sha1, sha1, 20);
-	found_path = 1;
+	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
+	walk_tree_ctx->found_path = 1;
 	return 0;
 }
 
@@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
 	unsigned long size;
 	struct commit *commit;
 	const char *paths[] = {path, NULL};
+	struct walk_tree_context walk_tree_ctx = {
+		.match_path = path,
+		.matched_sha1 = sha1,
+		.found_path = 0
+	};
+
 	if (get_sha1(head, sha1))
 		return -1;
 	type = sha1_object_info(sha1, &size);
 	if(type == OBJ_COMMIT && path) {
 		commit = lookup_commit_reference(sha1);
-		match_path = path;
-		matched_sha1 = sha1;
-		found_path = 0;
-		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
-		if (!found_path)
+		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
+		if (!walk_tree_ctx.found_path)
 			return -1;
 		type = sha1_object_info(sha1, &size);
 	}
@@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
 	unsigned long size;
 	struct commit *commit;
 	const char *paths[] = {path, NULL};
+	struct walk_tree_context walk_tree_ctx = {
+		.match_path = path,
+		.matched_sha1 = sha1,
+	};
 
 	if (hex) {
 		if (get_sha1_hex(hex, sha1)){
@@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
 
 	if((!hex) && type == OBJ_COMMIT && path) {
 		commit = lookup_commit_reference(sha1);
-		match_path = path;
-		matched_sha1 = sha1;
-		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
+		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
 		type = sha1_object_info(sha1,&size);
 	}
 
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 2/7] ui-plain.c: Do not access match variable in print_*()

[cgit]

Move all code setting the match variable to walk_tree().

This allows for easily moving this variable into a context structure
without having to pass the context to print_*().

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-plain.c | 26 ++++++++++++++------------
 1 file changed, 14 insertions(+), 12 deletions(-)

diff --git a/ui-plain.c b/ui-plain.c
index 85877d7..684d5ea 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -54,7 +54,7 @@ static char *get_mimetype_from_file(const char *filename, const char *ext)
 	return result;
 }
 
-static void print_object(const unsigned char *sha1, const char *path)
+static int print_object(const unsigned char *sha1, const char *path)
 {
 	enum object_type type;
 	char *buf, *ext;
@@ -65,13 +65,13 @@ static void print_object(const unsigned char *sha1, const char *path)
 	type = sha1_object_info(sha1, &size);
 	if (type == OBJ_BAD) {
 		html_status(404, "Not found", 0);
-		return;
+		return 0;
 	}
 
 	buf = read_sha1_file(sha1, &type, &size);
 	if (!buf) {
 		html_status(404, "Not found", 0);
-		return;
+		return 0;
 	}
 	ctx.page.mimetype = NULL;
 	ext = strrchr(path, '.');
@@ -97,9 +97,9 @@ static void print_object(const unsigned char *sha1, const char *path)
 	ctx.page.etag = sha1_to_hex(sha1);
 	cgit_print_http_headers(&ctx);
 	html_raw(buf, size);
-	match = 1;
 	if (freemime)
 		free(ctx.page.mimetype);
+	return 1;
 }
 
 static char *buildpath(const char *base, int baselen, const char *path)
@@ -138,7 +138,6 @@ static void print_dir(const unsigned char *sha1, const char *base,
 				fullpath);
 		html("</li>\n");
 	}
-	match = 2;
 }
 
 static void print_dir_entry(const unsigned char *sha1, const char *base,
@@ -156,7 +155,6 @@ static void print_dir_entry(const unsigned char *sha1, const char *base,
 		cgit_plain_link(path, NULL, NULL, ctx.qry.head, ctx.qry.sha1,
 				fullpath);
 	html("</li>\n");
-	match = 2;
 }
 
 static void print_dir_tail(void)
@@ -169,17 +167,20 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 		     void *cbdata)
 {
 	if (baselen == match_baselen) {
-		if (S_ISREG(mode))
-			print_object(sha1, pathname);
-		else if (S_ISDIR(mode)) {
+		if (S_ISREG(mode)) {
+			if (print_object(sha1, pathname))
+				match = 1;
+		} else if (S_ISDIR(mode)) {
 			print_dir(sha1, base, baselen, pathname);
+			match = 2;
 			return READ_TREE_RECURSIVE;
 		}
-	}
-	else if (baselen > match_baselen)
+	} else if (baselen > match_baselen) {
 		print_dir_entry(sha1, base, baselen, pathname, mode);
-	else if (S_ISDIR(mode))
+		match = 2;
+	} else if (S_ISDIR(mode)) {
 		return READ_TREE_RECURSIVE;
+	}
 
 	return 0;
 }
@@ -215,6 +216,7 @@ void cgit_print_plain(struct cgit_context *ctx)
 		paths[0] = "";
 		match_baselen = -1;
 		print_dir(commit->tree->object.sha1, "", 0, "");
+		match = 2;
 	}
 	else
 		match_baselen = basedir_len(paths[0]);
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 3/7] ui-plain.c: Use a context structure in walk_tree()

[cgit]

Do not misuse global variables to save the context. Instead, use the
context pointer which was designed to share information between a
read_tree_fn and the caller.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-plain.c | 31 ++++++++++++++++++-------------
 1 file changed, 18 insertions(+), 13 deletions(-)

diff --git a/ui-plain.c b/ui-plain.c
index 684d5ea..e5addb9 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -11,8 +11,10 @@
 #include "html.h"
 #include "ui-shared.h"
 
-int match_baselen;
-int match;
+struct walk_tree_context {
+	int match_baselen;
+	int match;
+};
 
 static char *get_mimetype_from_file(const char *filename, const char *ext)
 {
@@ -166,18 +168,20 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 		     const char *pathname, unsigned mode, int stage,
 		     void *cbdata)
 {
-	if (baselen == match_baselen) {
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (baselen == walk_tree_ctx->match_baselen) {
 		if (S_ISREG(mode)) {
 			if (print_object(sha1, pathname))
-				match = 1;
+				walk_tree_ctx->match = 1;
 		} else if (S_ISDIR(mode)) {
 			print_dir(sha1, base, baselen, pathname);
-			match = 2;
+			walk_tree_ctx->match = 2;
 			return READ_TREE_RECURSIVE;
 		}
-	} else if (baselen > match_baselen) {
+	} else if (baselen > walk_tree_ctx->match_baselen) {
 		print_dir_entry(sha1, base, baselen, pathname, mode);
-		match = 2;
+		walk_tree_ctx->match = 2;
 	} else if (S_ISDIR(mode)) {
 		return READ_TREE_RECURSIVE;
 	}
@@ -199,6 +203,7 @@ void cgit_print_plain(struct cgit_context *ctx)
 	unsigned char sha1[20];
 	struct commit *commit;
 	const char *paths[] = {ctx->qry.path, NULL};
+	struct walk_tree_context walk_tree_ctx;
 
 	if (!rev)
 		rev = ctx->qry.head;
@@ -214,15 +219,15 @@ void cgit_print_plain(struct cgit_context *ctx)
 	}
 	if (!paths[0]) {
 		paths[0] = "";
-		match_baselen = -1;
+		walk_tree_ctx.match_baselen = -1;
 		print_dir(commit->tree->object.sha1, "", 0, "");
-		match = 2;
+		walk_tree_ctx.match = 2;
 	}
 	else
-		match_baselen = basedir_len(paths[0]);
-	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
-	if (!match)
+		walk_tree_ctx.match_baselen = basedir_len(paths[0]);
+	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
+	if (!walk_tree_ctx.match)
 		html_status(404, "Not found", 0);
-	else if (match == 2)
+	else if (walk_tree_ctx.match == 2)
 		print_dir_tail();
 }
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 4/7] ui-tree.c: Pass current revision to print_object()

[cgit]

No longer access the global curr_rev variable in print_object().

This will make it easier to squash the curr_rev variable into a context
structure without having to pass the context to the print_object()
function.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-tree.c | 6 +++---
 1 file changed, 3 insertions(+), 3 deletions(-)

diff --git a/ui-tree.c b/ui-tree.c
index 7ddc09c..133101c 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -84,7 +84,7 @@ static void print_binary_buffer(char *buf, unsigned long size)
 	html("</table>\n");
 }
 
-static void print_object(const unsigned char *sha1, char *path, const char *basename)
+static void print_object(const unsigned char *sha1, char *path, const char *basename, const char *rev)
 {
 	enum object_type type;
 	char *buf;
@@ -106,7 +106,7 @@ 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,
-		        curr_rev, path);
+		        rev, path);
 	html(")\n");
 
 	if (ctx.cfg.max_blob_size && size / 1024 > ctx.cfg.max_blob_size) {
@@ -234,7 +234,7 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 			ls_head();
 			return READ_TREE_RECURSIVE;
 		} else {
-			print_object(sha1, buffer, pathname);
+			print_object(sha1, buffer, pathname, curr_rev);
 			return 0;
 		}
 	}
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[cgit]

This allows for removing the header variable in a following patch. We
can use the state variable to check whether the tail needs to be printed
instead.

Note that the state variable will be moved into a context structure
later.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-tree.c | 3 ++-
 1 file changed, 2 insertions(+), 1 deletion(-)

diff --git a/ui-tree.c b/ui-tree.c
index 133101c..3887ecd 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -14,6 +14,7 @@
 char *curr_rev;
 char *match_path;
 int header = 0;
+static int state;
 
 static void print_text_buffer(const char *name, char *buf, unsigned long size)
 {
@@ -220,7 +221,6 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 		     const char *pathname, unsigned mode, int stage,
 		     void *cbdata)
 {
-	static int state;
 	static char buffer[PATH_MAX];
 
 	if (state == 0) {
@@ -274,6 +274,7 @@ void cgit_print_tree(const char *rev, char *path)
 	}
 
 	match_path = path;
+	state = 0;
 	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
 	ls_tail();
 }
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 6/7] ui-tree.c: Drop the header variable

[cgit]

Instead, use the value of the state variable to determine whether the
footer needs to be drawn.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-tree.c | 8 ++------
 1 file changed, 2 insertions(+), 6 deletions(-)

diff --git a/ui-tree.c b/ui-tree.c
index 3887ecd..744e039 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -13,7 +13,6 @@
 
 char *curr_rev;
 char *match_path;
-int header = 0;
 static int state;
 
 static void print_text_buffer(const char *name, char *buf, unsigned long size)
@@ -189,15 +188,11 @@ static void ls_head()
 	html("<th class='right'>Size</th>");
 	html("<th/>");
 	html("</tr>\n");
-	header = 1;
 }
 
 static void ls_tail()
 {
-	if (!header)
-		return;
 	html("</table>\n");
-	header = 0;
 }
 
 static void ls_tree(const unsigned char *sha1, char *path)
@@ -276,5 +271,6 @@ void cgit_print_tree(const char *rev, char *path)
 	match_path = path;
 	state = 0;
 	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
-	ls_tail();
+	if (state == 1)
+		ls_tail();
 }
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 7/7] ui-tree.c: Use a context structure in walk_tree()

[cgit]

Use the context pointer to pass context information instead of misusing
global variables, as we already did in "ui-blob.c" and in "ui-plain.c".

In addition to the fixes to walk_tree(), pass the same structure to
ls_tree() and ls_item() which is read_tree_recursive()-based as well.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-tree.c | 51 +++++++++++++++++++++++++++++----------------------
 1 file changed, 29 insertions(+), 22 deletions(-)

diff --git a/ui-tree.c b/ui-tree.c
index 744e039..bd95c3c 100644
--- a/ui-tree.c
+++ b/ui-tree.c
@@ -11,9 +11,11 @@
 #include "html.h"
 #include "ui-shared.h"
 
-char *curr_rev;
-char *match_path;
-static int state;
+struct walk_tree_context {
+	char *curr_rev;
+	char *match_path;
+	int state;
+};
 
 static void print_text_buffer(const char *name, char *buf, unsigned long size)
 {
@@ -126,6 +128,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
 		   const char *pathname, unsigned int mode, int stage,
 		   void *cbdata)
 {
+	struct walk_tree_context *walk_tree_ctx = cbdata;
 	char *name;
 	char *fullpath;
 	char *class;
@@ -153,7 +156,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
 		cgit_submodule_link("ls-mod", fullpath, sha1_to_hex(sha1));
 	} else if (S_ISDIR(mode)) {
 		cgit_tree_link(name, NULL, "ls-dir", ctx.qry.head,
-			       curr_rev, fullpath);
+			       walk_tree_ctx->curr_rev, fullpath);
 	} else {
 		class = strrchr(name, '.');
 		if (class != NULL) {
@@ -161,19 +164,20 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
 		} else
 			class = "ls-blob";
 		cgit_tree_link(name, NULL, class, ctx.qry.head,
-			       curr_rev, fullpath);
+			       walk_tree_ctx->curr_rev, fullpath);
 	}
 	htmlf("</td><td class='ls-size'>%li</td>", size);
 
 	html("<td>");
-	cgit_log_link("log", NULL, "button", ctx.qry.head, curr_rev,
-		      fullpath, 0, NULL, NULL, ctx.qry.showmsg);
+	cgit_log_link("log", NULL, "button", ctx.qry.head,
+		      walk_tree_ctx->curr_rev, fullpath, 0, NULL, NULL,
+		      ctx.qry.showmsg);
 	if (ctx.repo->max_stats)
 		cgit_stats_link("stats", NULL, "button", ctx.qry.head,
 				fullpath);
 	if (!S_ISGITLINK(mode))
-		cgit_plain_link("plain", NULL, "button", ctx.qry.head, curr_rev,
-				fullpath);
+		cgit_plain_link("plain", NULL, "button", ctx.qry.head,
+				walk_tree_ctx->curr_rev, fullpath);
 	html("</td></tr>\n");
 	free(name);
 	return 0;
@@ -195,7 +199,7 @@ static void ls_tail()
 	html("</table>\n");
 }
 
-static void ls_tree(const unsigned char *sha1, char *path)
+static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
 {
 	struct tree *tree;
 
@@ -207,7 +211,7 @@ static void ls_tree(const unsigned char *sha1, char *path)
 	}
 
 	ls_head();
-	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, NULL);
+	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, walk_tree_ctx);
 	ls_tail();
 }
 
@@ -216,24 +220,25 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 		     const char *pathname, unsigned mode, int stage,
 		     void *cbdata)
 {
+	struct walk_tree_context *walk_tree_ctx = cbdata;
 	static char buffer[PATH_MAX];
 
-	if (state == 0) {
+	if (walk_tree_ctx->state == 0) {
 		memcpy(buffer, base, baselen);
 		strcpy(buffer + baselen, pathname);
-		if (strcmp(match_path, buffer))
+		if (strcmp(walk_tree_ctx->match_path, buffer))
 			return READ_TREE_RECURSIVE;
 
 		if (S_ISDIR(mode)) {
-			state = 1;
+			walk_tree_ctx->state = 1;
 			ls_head();
 			return READ_TREE_RECURSIVE;
 		} else {
-			print_object(sha1, buffer, pathname, curr_rev);
+			print_object(sha1, buffer, pathname, walk_tree_ctx->curr_rev);
 			return 0;
 		}
 	}
-	ls_item(sha1, base, baselen, pathname, mode, stage, NULL);
+	ls_item(sha1, base, baselen, pathname, mode, stage, walk_tree_ctx);
 	return 0;
 }
 
@@ -248,11 +253,15 @@ void cgit_print_tree(const char *rev, char *path)
 	unsigned char sha1[20];
 	struct commit *commit;
 	const char *paths[] = {path, NULL};
+	struct walk_tree_context walk_tree_ctx = {
+		.match_path = path,
+		.state = 0
+	};
 
 	if (!rev)
 		rev = ctx.qry.head;
 
-	curr_rev = xstrdup(rev);
+	walk_tree_ctx.curr_rev = xstrdup(rev);
 	if (get_sha1(rev, sha1)) {
 		cgit_print_error(fmt("Invalid revision name: %s", rev));
 		return;
@@ -264,13 +273,11 @@ void cgit_print_tree(const char *rev, char *path)
 	}
 
 	if (path == NULL) {
-		ls_tree(commit->tree->object.sha1, NULL);
+		ls_tree(commit->tree->object.sha1, NULL, &walk_tree_ctx);
 		return;
 	}
 
-	match_path = path;
-	state = 0;
-	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
-	if (state == 1)
+	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
+	if (walk_tree_ctx.state == 1)
 		ls_tail();
 }
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[mailings]

On 03/03/13 18:06, Lukas Fleischer wrote:
> Do not misuse global variables to save the context. Instead, use the
> context pointer which was designed to share information between a
> read_tree_fn and the caller.
>
> This also prevents from potential misuse of the global pointers
> match_path and matched_sha1 after the referenced values have been
> overwritten on the stack.
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>   ui-blob.c | 42 ++++++++++++++++++++++++++----------------
>   1 file changed, 26 insertions(+), 16 deletions(-)
>
> diff --git a/ui-blob.c b/ui-blob.c
> index 3d07ce5..4bcbc82 100644
> --- a/ui-blob.c
> +++ b/ui-blob.c
> @@ -11,17 +11,22 @@
>   #include "html.h"
>   #include "ui-shared.h"
>
> -static char *match_path;
> -static unsigned char *matched_sha1;
> -static int found_path;
> +struct walk_tree_context {
> +	char *match_path;
> +	unsigned char *matched_sha1;
> +	int found_path;
> +};
>
>   static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> -	const char *pathname, unsigned mode, int stage, void *cbdata) {
> -	if(strncmp(base, match_path, baselen)
> -		|| strcmp(match_path + baselen, pathname))
> +	const char *pathname, unsigned mode, int stage, void *cbdata)
> +{
> +	struct walk_tree_context *walk_tree_ctx = cbdata;
> +
> +	if(strncmp(base, walk_tree_ctx->match_path, baselen)
> +		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
>   		return READ_TREE_RECURSIVE;
> -	memmove(matched_sha1, sha1, 20);
> -	found_path = 1;
> +	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
> +	walk_tree_ctx->found_path = 1;
>   	return 0;
>   }
>
> @@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
>   	unsigned long size;
>   	struct commit *commit;
>   	const char *paths[] = {path, NULL};
> +	struct walk_tree_context walk_tree_ctx = {
> +		.match_path = path,
> +		.matched_sha1 = sha1,
> +		.found_path = 0
> +	};
> +
>   	if (get_sha1(head, sha1))
>   		return -1;
>   	type = sha1_object_info(sha1, &size);
>   	if(type == OBJ_COMMIT && path) {
>   		commit = lookup_commit_reference(sha1);
> -		match_path = path;
> -		matched_sha1 = sha1;
> -		found_path = 0;
> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> -		if (!found_path)
> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> +		if (!walk_tree_ctx.found_path)
>   			return -1;
>   		type = sha1_object_info(sha1, &size);
>   	}
> @@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>   	unsigned long size;
>   	struct commit *commit;
>   	const char *paths[] = {path, NULL};
> +	struct walk_tree_context walk_tree_ctx = {
> +		.match_path = path,
> +		.matched_sha1 = sha1,

forgot to initialise found_path


> +	};
>
>   	if (hex) {
>   		if (get_sha1_hex(hex, sha1)){
> @@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>
>   	if((!hex) && type == OBJ_COMMIT && path) {
>   		commit = lookup_commit_reference(sha1);
> -		match_path = path;
> -		matched_sha1 = sha1;
> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
>   		type = sha1_object_info(sha1,&size);
>   	}
>
>

-- 
Ferry Huberts

[PATCH 3/7] ui-plain.c: Use a context structure in walk_tree()

[mailings]

On 03/03/13 18:06, Lukas Fleischer wrote:
> Do not misuse global variables to save the context. Instead, use the
> context pointer which was designed to share information between a
> read_tree_fn and the caller.
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>   ui-plain.c | 31 ++++++++++++++++++-------------
>   1 file changed, 18 insertions(+), 13 deletions(-)
>
> diff --git a/ui-plain.c b/ui-plain.c
> index 684d5ea..e5addb9 100644
> --- a/ui-plain.c
> +++ b/ui-plain.c
> @@ -11,8 +11,10 @@
>   #include "html.h"
>   #include "ui-shared.h"
>
> -int match_baselen;
> -int match;
> +struct walk_tree_context {
> +	int match_baselen;
> +	int match;
> +};
>
>   static char *get_mimetype_from_file(const char *filename, const char *ext)
>   {
> @@ -166,18 +168,20 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
>   		     const char *pathname, unsigned mode, int stage,
>   		     void *cbdata)
>   {
> -	if (baselen == match_baselen) {
> +	struct walk_tree_context *walk_tree_ctx = cbdata;
> +
> +	if (baselen == walk_tree_ctx->match_baselen) {
>   		if (S_ISREG(mode)) {
>   			if (print_object(sha1, pathname))
> -				match = 1;
> +				walk_tree_ctx->match = 1;
>   		} else if (S_ISDIR(mode)) {
>   			print_dir(sha1, base, baselen, pathname);
> -			match = 2;
> +			walk_tree_ctx->match = 2;
>   			return READ_TREE_RECURSIVE;
>   		}
> -	} else if (baselen > match_baselen) {
> +	} else if (baselen > walk_tree_ctx->match_baselen) {
>   		print_dir_entry(sha1, base, baselen, pathname, mode);
> -		match = 2;
> +		walk_tree_ctx->match = 2;
>   	} else if (S_ISDIR(mode)) {
>   		return READ_TREE_RECURSIVE;
>   	}
> @@ -199,6 +203,7 @@ void cgit_print_plain(struct cgit_context *ctx)
>   	unsigned char sha1[20];
>   	struct commit *commit;
>   	const char *paths[] = {ctx->qry.path, NULL};
> +	struct walk_tree_context walk_tree_ctx;

I'd prefer that you initialise the variable here

>
>   	if (!rev)
>   		rev = ctx->qry.head;
> @@ -214,15 +219,15 @@ void cgit_print_plain(struct cgit_context *ctx)
>   	}
>   	if (!paths[0]) {
>   		paths[0] = "";
> -		match_baselen = -1;
> +		walk_tree_ctx.match_baselen = -1;
>   		print_dir(commit->tree->object.sha1, "", 0, "");
> -		match = 2;
> +		walk_tree_ctx.match = 2;
>   	}
>   	else
> -		match_baselen = basedir_len(paths[0]);
> -	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> -	if (!match)
> +		walk_tree_ctx.match_baselen = basedir_len(paths[0]);
> +	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> +	if (!walk_tree_ctx.match)
>   		html_status(404, "Not found", 0);
> -	else if (match == 2)
> +	else if (walk_tree_ctx.match == 2)
>   		print_dir_tail();
>   }
>

-- 
Ferry Huberts

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[mailings]

On 03/03/13 18:06, Lukas Fleischer wrote:
> This allows for removing the header variable in a following patch. We
> can use the state variable to check whether the tail needs to be printed
> instead.
>
> Note that the state variable will be moved into a context structure
> later.
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>   ui-tree.c | 3 ++-
>   1 file changed, 2 insertions(+), 1 deletion(-)
>
> diff --git a/ui-tree.c b/ui-tree.c
> index 133101c..3887ecd 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -14,6 +14,7 @@
>   char *curr_rev;
>   char *match_path;
>   int header = 0;
> +static int state;
>

please inititialise


>   static void print_text_buffer(const char *name, char *buf, unsigned long size)
>   {
> @@ -220,7 +221,6 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
>   		     const char *pathname, unsigned mode, int stage,
>   		     void *cbdata)
>   {
> -	static int state;
>   	static char buffer[PATH_MAX];
>
>   	if (state == 0) {
> @@ -274,6 +274,7 @@ void cgit_print_tree(const char *rev, char *path)
>   	}
>
>   	match_path = path;
> +	state = 0;

why? where does this come from?

>   	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
>   	ls_tail();
>   }
>

-- 
Ferry Huberts

[PATCH 7/7] ui-tree.c: Use a context structure in walk_tree()

[mailings]

On 03/03/13 18:06, Lukas Fleischer wrote:
> Use the context pointer to pass context information instead of misusing
> global variables, as we already did in "ui-blob.c" and in "ui-plain.c".
>
> In addition to the fixes to walk_tree(), pass the same structure to
> ls_tree() and ls_item() which is read_tree_recursive()-based as well.
>
> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> ---
>   ui-tree.c | 51 +++++++++++++++++++++++++++++----------------------
>   1 file changed, 29 insertions(+), 22 deletions(-)
>
> diff --git a/ui-tree.c b/ui-tree.c
> index 744e039..bd95c3c 100644
> --- a/ui-tree.c
> +++ b/ui-tree.c
> @@ -11,9 +11,11 @@
>   #include "html.h"
>   #include "ui-shared.h"
>
> -char *curr_rev;
> -char *match_path;
> -static int state;
> +struct walk_tree_context {
> +	char *curr_rev;
> +	char *match_path;
> +	int state;
> +};
>
>   static void print_text_buffer(const char *name, char *buf, unsigned long size)
>   {
> @@ -126,6 +128,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
>   		   const char *pathname, unsigned int mode, int stage,
>   		   void *cbdata)
>   {
> +	struct walk_tree_context *walk_tree_ctx = cbdata;
>   	char *name;
>   	char *fullpath;
>   	char *class;
> @@ -153,7 +156,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
>   		cgit_submodule_link("ls-mod", fullpath, sha1_to_hex(sha1));
>   	} else if (S_ISDIR(mode)) {
>   		cgit_tree_link(name, NULL, "ls-dir", ctx.qry.head,
> -			       curr_rev, fullpath);
> +			       walk_tree_ctx->curr_rev, fullpath);
>   	} else {
>   		class = strrchr(name, '.');
>   		if (class != NULL) {
> @@ -161,19 +164,20 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
>   		} else
>   			class = "ls-blob";
>   		cgit_tree_link(name, NULL, class, ctx.qry.head,
> -			       curr_rev, fullpath);
> +			       walk_tree_ctx->curr_rev, fullpath);
>   	}
>   	htmlf("</td><td class='ls-size'>%li</td>", size);
>
>   	html("<td>");
> -	cgit_log_link("log", NULL, "button", ctx.qry.head, curr_rev,
> -		      fullpath, 0, NULL, NULL, ctx.qry.showmsg);
> +	cgit_log_link("log", NULL, "button", ctx.qry.head,
> +		      walk_tree_ctx->curr_rev, fullpath, 0, NULL, NULL,
> +		      ctx.qry.showmsg);
>   	if (ctx.repo->max_stats)
>   		cgit_stats_link("stats", NULL, "button", ctx.qry.head,
>   				fullpath);
>   	if (!S_ISGITLINK(mode))
> -		cgit_plain_link("plain", NULL, "button", ctx.qry.head, curr_rev,
> -				fullpath);
> +		cgit_plain_link("plain", NULL, "button", ctx.qry.head,
> +				walk_tree_ctx->curr_rev, fullpath);
>   	html("</td></tr>\n");
>   	free(name);
>   	return 0;
> @@ -195,7 +199,7 @@ static void ls_tail()
>   	html("</table>\n");
>   }
>
> -static void ls_tree(const unsigned char *sha1, char *path)
> +static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
>   {
>   	struct tree *tree;
>
> @@ -207,7 +211,7 @@ static void ls_tree(const unsigned char *sha1, char *path)
>   	}
>
>   	ls_head();
> -	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, NULL);
> +	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, walk_tree_ctx);
>   	ls_tail();
>   }
>
> @@ -216,24 +220,25 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
>   		     const char *pathname, unsigned mode, int stage,
>   		     void *cbdata)
>   {
> +	struct walk_tree_context *walk_tree_ctx = cbdata;
>   	static char buffer[PATH_MAX];
>
> -	if (state == 0) {
> +	if (walk_tree_ctx->state == 0) {
>   		memcpy(buffer, base, baselen);
>   		strcpy(buffer + baselen, pathname);
> -		if (strcmp(match_path, buffer))
> +		if (strcmp(walk_tree_ctx->match_path, buffer))
>   			return READ_TREE_RECURSIVE;
>
>   		if (S_ISDIR(mode)) {
> -			state = 1;
> +			walk_tree_ctx->state = 1;
>   			ls_head();
>   			return READ_TREE_RECURSIVE;
>   		} else {
> -			print_object(sha1, buffer, pathname, curr_rev);
> +			print_object(sha1, buffer, pathname, walk_tree_ctx->curr_rev);
>   			return 0;
>   		}
>   	}
> -	ls_item(sha1, base, baselen, pathname, mode, stage, NULL);
> +	ls_item(sha1, base, baselen, pathname, mode, stage, walk_tree_ctx);
>   	return 0;
>   }
>
> @@ -248,11 +253,15 @@ void cgit_print_tree(const char *rev, char *path)
>   	unsigned char sha1[20];
>   	struct commit *commit;
>   	const char *paths[] = {path, NULL};
> +	struct walk_tree_context walk_tree_ctx = {
> +		.match_path = path,
> +		.state = 0

forgot to initialise curr_rev

> +	};
>
>   	if (!rev)
>   		rev = ctx.qry.head;
>
> -	curr_rev = xstrdup(rev);
> +	walk_tree_ctx.curr_rev = xstrdup(rev);
>   	if (get_sha1(rev, sha1)) {
>   		cgit_print_error(fmt("Invalid revision name: %s", rev));
>   		return;
> @@ -264,13 +273,11 @@ void cgit_print_tree(const char *rev, char *path)
>   	}
>
>   	if (path == NULL) {
> -		ls_tree(commit->tree->object.sha1, NULL);
> +		ls_tree(commit->tree->object.sha1, NULL, &walk_tree_ctx);
>   		return;
>   	}
>
> -	match_path = path;
> -	state = 0;
> -	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> -	if (state == 1)
> +	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> +	if (walk_tree_ctx.state == 1)
>   		ls_tail();
>   }
>

-- 
Ferry Huberts

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[john]

On Sun, Mar 03, 2013 at 08:56:18PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 18:06, Lukas Fleischer wrote:
> > Do not misuse global variables to save the context. Instead, use the
> > context pointer which was designed to share information between a
> > read_tree_fn and the caller.
> >
> > This also prevents from potential misuse of the global pointers
> > match_path and matched_sha1 after the referenced values have been
> > overwritten on the stack.
> >
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> >   ui-blob.c | 42 ++++++++++++++++++++++++++----------------
> >   1 file changed, 26 insertions(+), 16 deletions(-)
> >
> > diff --git a/ui-blob.c b/ui-blob.c
> > index 3d07ce5..4bcbc82 100644
> > --- a/ui-blob.c
> > +++ b/ui-blob.c
> > @@ -11,17 +11,22 @@
> >   #include "html.h"
> >   #include "ui-shared.h"
> >
> > -static char *match_path;
> > -static unsigned char *matched_sha1;
> > -static int found_path;
> > +struct walk_tree_context {
> > +	char *match_path;
> > +	unsigned char *matched_sha1;
> > +	int found_path;
> > +};
> >
> >   static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> > -	const char *pathname, unsigned mode, int stage, void *cbdata) {
> > -	if(strncmp(base, match_path, baselen)
> > -		|| strcmp(match_path + baselen, pathname))
> > +	const char *pathname, unsigned mode, int stage, void *cbdata)
> > +{
> > +	struct walk_tree_context *walk_tree_ctx = cbdata;
> > +
> > +	if(strncmp(base, walk_tree_ctx->match_path, baselen)
> > +		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
> >   		return READ_TREE_RECURSIVE;
> > -	memmove(matched_sha1, sha1, 20);
> > -	found_path = 1;
> > +	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
> > +	walk_tree_ctx->found_path = 1;
> >   	return 0;
> >   }
> >
> > @@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
> >   	unsigned long size;
> >   	struct commit *commit;
> >   	const char *paths[] = {path, NULL};
> > +	struct walk_tree_context walk_tree_ctx = {
> > +		.match_path = path,
> > +		.matched_sha1 = sha1,
> > +		.found_path = 0
> > +	};
> > +
> >   	if (get_sha1(head, sha1))
> >   		return -1;
> >   	type = sha1_object_info(sha1, &size);
> >   	if(type == OBJ_COMMIT && path) {
> >   		commit = lookup_commit_reference(sha1);
> > -		match_path = path;
> > -		matched_sha1 = sha1;
> > -		found_path = 0;
> > -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> > -		if (!found_path)
> > +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> > +		if (!walk_tree_ctx.found_path)
> >   			return -1;
> >   		type = sha1_object_info(sha1, &size);
> >   	}
> > @@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >   	unsigned long size;
> >   	struct commit *commit;
> >   	const char *paths[] = {path, NULL};
> > +	struct walk_tree_context walk_tree_ctx = {
> > +		.match_path = path,
> > +		.matched_sha1 = sha1,
> 
> forgot to initialise found_path

Unnecessary - C99 6.7.8:

    If there are fewer initializers in a brace-enclosed list than there
    are elements or members of an aggregate, or fewer characters in a
    string literal used to initialize an array of known size than there
    are elements in the array, the remainder of the aggregate shall be
    initialized implicitly the same as objects that have static storage
    duration.

> 
> > +	};
> >
> >   	if (hex) {
> >   		if (get_sha1_hex(hex, sha1)){
> > @@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >
> >   	if((!hex) && type == OBJ_COMMIT && path) {
> >   		commit = lookup_commit_reference(sha1);
> > -		match_path = path;
> > -		matched_sha1 = sha1;
> > -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> > +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >   		type = sha1_object_info(sha1,&size);
> >   	}
> >
> >
> 
> -- 
> Ferry Huberts

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[john]

On Sun, Mar 03, 2013 at 09:08:04PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 18:06, Lukas Fleischer wrote:
> > This allows for removing the header variable in a following patch. We
> > can use the state variable to check whether the tail needs to be printed
> > instead.
> >
> > Note that the state variable will be moved into a context structure
> > later.
> >
> > Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> > ---
> >   ui-tree.c | 3 ++-
> >   1 file changed, 2 insertions(+), 1 deletion(-)
> >
> > diff --git a/ui-tree.c b/ui-tree.c
> > index 133101c..3887ecd 100644
> > --- a/ui-tree.c
> > +++ b/ui-tree.c
> > @@ -14,6 +14,7 @@
> >   char *curr_rev;
> >   char *match_path;
> >   int header = 0;
> > +static int state;
> >
> 
> please inititialise

Why?  It has static storage duration.

> >   static void print_text_buffer(const char *name, char *buf, unsigned long size)
> >   {
> > @@ -220,7 +221,6 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >   		     const char *pathname, unsigned mode, int stage,
> >   		     void *cbdata)
> >   {
> > -	static int state;
> >   	static char buffer[PATH_MAX];
> >
> >   	if (state == 0) {
> > @@ -274,6 +274,7 @@ void cgit_print_tree(const char *rev, char *path)
> >   	}
> >
> >   	match_path = path;
> > +	state = 0;
> 
> why? where does this come from?
> 
> >   	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >   	ls_tail();
> >   }
> >
> 
> -- 
> Ferry Huberts

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[mailings]

On 03/03/13 21:25, John Keeping wrote:
> On Sun, Mar 03, 2013 at 08:56:18PM +0100, Ferry Huberts wrote:
>>
>>
>> On 03/03/13 18:06, Lukas Fleischer wrote:
>>> Do not misuse global variables to save the context. Instead, use the
>>> context pointer which was designed to share information between a
>>> read_tree_fn and the caller.
>>>
>>> This also prevents from potential misuse of the global pointers
>>> match_path and matched_sha1 after the referenced values have been
>>> overwritten on the stack.
>>>
>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>> ---
>>>    ui-blob.c | 42 ++++++++++++++++++++++++++----------------
>>>    1 file changed, 26 insertions(+), 16 deletions(-)
>>>
>>> diff --git a/ui-blob.c b/ui-blob.c
>>> index 3d07ce5..4bcbc82 100644
>>> --- a/ui-blob.c
>>> +++ b/ui-blob.c
>>> @@ -11,17 +11,22 @@
>>>    #include "html.h"
>>>    #include "ui-shared.h"
>>>
>>> -static char *match_path;
>>> -static unsigned char *matched_sha1;
>>> -static int found_path;
>>> +struct walk_tree_context {
>>> +	char *match_path;
>>> +	unsigned char *matched_sha1;
>>> +	int found_path;
>>> +};
>>>
>>>    static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
>>> -	const char *pathname, unsigned mode, int stage, void *cbdata) {
>>> -	if(strncmp(base, match_path, baselen)
>>> -		|| strcmp(match_path + baselen, pathname))
>>> +	const char *pathname, unsigned mode, int stage, void *cbdata)
>>> +{
>>> +	struct walk_tree_context *walk_tree_ctx = cbdata;
>>> +
>>> +	if(strncmp(base, walk_tree_ctx->match_path, baselen)
>>> +		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
>>>    		return READ_TREE_RECURSIVE;
>>> -	memmove(matched_sha1, sha1, 20);
>>> -	found_path = 1;
>>> +	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
>>> +	walk_tree_ctx->found_path = 1;
>>>    	return 0;
>>>    }
>>>
>>> @@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
>>>    	unsigned long size;
>>>    	struct commit *commit;
>>>    	const char *paths[] = {path, NULL};
>>> +	struct walk_tree_context walk_tree_ctx = {
>>> +		.match_path = path,
>>> +		.matched_sha1 = sha1,
>>> +		.found_path = 0
>>> +	};
>>> +
>>>    	if (get_sha1(head, sha1))
>>>    		return -1;
>>>    	type = sha1_object_info(sha1, &size);
>>>    	if(type == OBJ_COMMIT && path) {
>>>    		commit = lookup_commit_reference(sha1);
>>> -		match_path = path;
>>> -		matched_sha1 = sha1;
>>> -		found_path = 0;
>>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
>>> -		if (!found_path)
>>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
>>> +		if (!walk_tree_ctx.found_path)
>>>    			return -1;
>>>    		type = sha1_object_info(sha1, &size);
>>>    	}
>>> @@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>>>    	unsigned long size;
>>>    	struct commit *commit;
>>>    	const char *paths[] = {path, NULL};
>>> +	struct walk_tree_context walk_tree_ctx = {
>>> +		.match_path = path,
>>> +		.matched_sha1 = sha1,
>>
>> forgot to initialise found_path
>
> Unnecessary - C99 6.7.8:
>
>      If there are fewer initializers in a brace-enclosed list than there
>      are elements or members of an aggregate, or fewer characters in a
>      string literal used to initialize an array of known size than there
>      are elements in the array, the remainder of the aggregate shall be
>      initialized implicitly the same as objects that have static storage
>      duration.

which is what? zero?

>
>>
>>> +	};
>>>
>>>    	if (hex) {
>>>    		if (get_sha1_hex(hex, sha1)){
>>> @@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>>>
>>>    	if((!hex) && type == OBJ_COMMIT && path) {
>>>    		commit = lookup_commit_reference(sha1);
>>> -		match_path = path;
>>> -		matched_sha1 = sha1;
>>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
>>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
>>>    		type = sha1_object_info(sha1,&size);
>>>    	}
>>>
>>>
>>
>> --
>> Ferry Huberts

-- 
Ferry Huberts

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[cgit]

On Sun, Mar 03, 2013 at 09:33:36PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 21:25, John Keeping wrote:
> >On Sun, Mar 03, 2013 at 08:56:18PM +0100, Ferry Huberts wrote:
> >>
> >>
> >>On 03/03/13 18:06, Lukas Fleischer wrote:
> >>>Do not misuse global variables to save the context. Instead, use the
> >>>context pointer which was designed to share information between a
> >>>read_tree_fn and the caller.
> >>>
> >>>This also prevents from potential misuse of the global pointers
> >>>match_path and matched_sha1 after the referenced values have been
> >>>overwritten on the stack.
> >>>
> >>>Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >>>---
> >>>   ui-blob.c | 42 ++++++++++++++++++++++++++----------------
> >>>   1 file changed, 26 insertions(+), 16 deletions(-)
> >>>
> >>>diff --git a/ui-blob.c b/ui-blob.c
> >>>index 3d07ce5..4bcbc82 100644
> >>>--- a/ui-blob.c
> >>>+++ b/ui-blob.c
> >>>@@ -11,17 +11,22 @@
> >>>   #include "html.h"
> >>>   #include "ui-shared.h"
> >>>
> >>>-static char *match_path;
> >>>-static unsigned char *matched_sha1;
> >>>-static int found_path;
> >>>+struct walk_tree_context {
> >>>+	char *match_path;
> >>>+	unsigned char *matched_sha1;
> >>>+	int found_path;
> >>>+};
> >>>
> >>>   static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >>>-	const char *pathname, unsigned mode, int stage, void *cbdata) {
> >>>-	if(strncmp(base, match_path, baselen)
> >>>-		|| strcmp(match_path + baselen, pathname))
> >>>+	const char *pathname, unsigned mode, int stage, void *cbdata)
> >>>+{
> >>>+	struct walk_tree_context *walk_tree_ctx = cbdata;
> >>>+
> >>>+	if(strncmp(base, walk_tree_ctx->match_path, baselen)
> >>>+		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
> >>>   		return READ_TREE_RECURSIVE;
> >>>-	memmove(matched_sha1, sha1, 20);
> >>>-	found_path = 1;
> >>>+	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
> >>>+	walk_tree_ctx->found_path = 1;
> >>>   	return 0;
> >>>   }
> >>>
> >>>@@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
> >>>   	unsigned long size;
> >>>   	struct commit *commit;
> >>>   	const char *paths[] = {path, NULL};
> >>>+	struct walk_tree_context walk_tree_ctx = {
> >>>+		.match_path = path,
> >>>+		.matched_sha1 = sha1,
> >>>+		.found_path = 0
> >>>+	};
> >>>+
> >>>   	if (get_sha1(head, sha1))
> >>>   		return -1;
> >>>   	type = sha1_object_info(sha1, &size);
> >>>   	if(type == OBJ_COMMIT && path) {
> >>>   		commit = lookup_commit_reference(sha1);
> >>>-		match_path = path;
> >>>-		matched_sha1 = sha1;
> >>>-		found_path = 0;
> >>>-		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >>>-		if (!found_path)
> >>>+		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >>>+		if (!walk_tree_ctx.found_path)
> >>>   			return -1;
> >>>   		type = sha1_object_info(sha1, &size);
> >>>   	}
> >>>@@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >>>   	unsigned long size;
> >>>   	struct commit *commit;
> >>>   	const char *paths[] = {path, NULL};
> >>>+	struct walk_tree_context walk_tree_ctx = {
> >>>+		.match_path = path,
> >>>+		.matched_sha1 = sha1,
> >>
> >>forgot to initialise found_path
> >
> >Unnecessary - C99 6.7.8:
> >
> >     If there are fewer initializers in a brace-enclosed list than there
> >     are elements or members of an aggregate, or fewer characters in a
> >     string literal used to initialize an array of known size than there
> >     are elements in the array, the remainder of the aggregate shall be
> >     initialized implicitly the same as objects that have static storage
> >     duration.
> 
> which is what? zero?

We don't care, since "found_path" is not used (read) in this code path.
It should be fine to leave it uninitialized.

> 
> >
> >>
> >>>+	};
> >>>
> >>>   	if (hex) {
> >>>   		if (get_sha1_hex(hex, sha1)){
> >>>@@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >>>
> >>>   	if((!hex) && type == OBJ_COMMIT && path) {
> >>>   		commit = lookup_commit_reference(sha1);
> >>>-		match_path = path;
> >>>-		matched_sha1 = sha1;
> >>>-		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >>>+		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >>>   		type = sha1_object_info(sha1,&size);
> >>>   	}
> >>>
> >>>
> >>
> >>--
> >>Ferry Huberts
> 
> -- 
> Ferry Huberts

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[john]

On Sun, Mar 03, 2013 at 09:33:36PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 21:25, John Keeping wrote:
> > On Sun, Mar 03, 2013 at 08:56:18PM +0100, Ferry Huberts wrote:
> >>
> >>
> >> On 03/03/13 18:06, Lukas Fleischer wrote:
> >>> Do not misuse global variables to save the context. Instead, use the
> >>> context pointer which was designed to share information between a
> >>> read_tree_fn and the caller.
> >>>
> >>> This also prevents from potential misuse of the global pointers
> >>> match_path and matched_sha1 after the referenced values have been
> >>> overwritten on the stack.
> >>>
> >>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >>> ---
> >>>    ui-blob.c | 42 ++++++++++++++++++++++++++----------------
> >>>    1 file changed, 26 insertions(+), 16 deletions(-)
> >>>
> >>> diff --git a/ui-blob.c b/ui-blob.c
> >>> index 3d07ce5..4bcbc82 100644
> >>> --- a/ui-blob.c
> >>> +++ b/ui-blob.c
> >>> @@ -11,17 +11,22 @@
> >>>    #include "html.h"
> >>>    #include "ui-shared.h"
> >>>
> >>> -static char *match_path;
> >>> -static unsigned char *matched_sha1;
> >>> -static int found_path;
> >>> +struct walk_tree_context {
> >>> +	char *match_path;
> >>> +	unsigned char *matched_sha1;
> >>> +	int found_path;
> >>> +};
> >>>
> >>>    static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >>> -	const char *pathname, unsigned mode, int stage, void *cbdata) {
> >>> -	if(strncmp(base, match_path, baselen)
> >>> -		|| strcmp(match_path + baselen, pathname))
> >>> +	const char *pathname, unsigned mode, int stage, void *cbdata)
> >>> +{
> >>> +	struct walk_tree_context *walk_tree_ctx = cbdata;
> >>> +
> >>> +	if(strncmp(base, walk_tree_ctx->match_path, baselen)
> >>> +		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
> >>>    		return READ_TREE_RECURSIVE;
> >>> -	memmove(matched_sha1, sha1, 20);
> >>> -	found_path = 1;
> >>> +	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
> >>> +	walk_tree_ctx->found_path = 1;
> >>>    	return 0;
> >>>    }
> >>>
> >>> @@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
> >>>    	unsigned long size;
> >>>    	struct commit *commit;
> >>>    	const char *paths[] = {path, NULL};
> >>> +	struct walk_tree_context walk_tree_ctx = {
> >>> +		.match_path = path,
> >>> +		.matched_sha1 = sha1,
> >>> +		.found_path = 0
> >>> +	};
> >>> +
> >>>    	if (get_sha1(head, sha1))
> >>>    		return -1;
> >>>    	type = sha1_object_info(sha1, &size);
> >>>    	if(type == OBJ_COMMIT && path) {
> >>>    		commit = lookup_commit_reference(sha1);
> >>> -		match_path = path;
> >>> -		matched_sha1 = sha1;
> >>> -		found_path = 0;
> >>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >>> -		if (!found_path)
> >>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >>> +		if (!walk_tree_ctx.found_path)
> >>>    			return -1;
> >>>    		type = sha1_object_info(sha1, &size);
> >>>    	}
> >>> @@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >>>    	unsigned long size;
> >>>    	struct commit *commit;
> >>>    	const char *paths[] = {path, NULL};
> >>> +	struct walk_tree_context walk_tree_ctx = {
> >>> +		.match_path = path,
> >>> +		.matched_sha1 = sha1,
> >>
> >> forgot to initialise found_path
> >
> > Unnecessary - C99 6.7.8:
> >
> >      If there are fewer initializers in a brace-enclosed list than there
> >      are elements or members of an aggregate, or fewer characters in a
> >      string literal used to initialize an array of known size than there
> >      are elements in the array, the remainder of the aggregate shall be
> >      initialized implicitly the same as objects that have static storage
> >      duration.
> 
> which is what? zero?

Yes:

    If an object that has static storage duration is not initialized
    explicitly, then:
    ? if it has pointer type, it is initialized to a null pointer;
    ? if it has arithmetic type, it is initialized to (positive or
      unsigned) zero;
    ? if it is an aggregate, every member is initialized (recursively)
      according to these rules;
    ? if it is a union, the first named member is initialized
      (recursively) according to these rules

> >
> >>
> >>> +	};
> >>>
> >>>    	if (hex) {
> >>>    		if (get_sha1_hex(hex, sha1)){
> >>> @@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
> >>>
> >>>    	if((!hex) && type == OBJ_COMMIT && path) {
> >>>    		commit = lookup_commit_reference(sha1);
> >>> -		match_path = path;
> >>> -		matched_sha1 = sha1;
> >>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >>>    		type = sha1_object_info(sha1,&size);
> >>>    	}
> >>>
> >>>
> >>
> >> --
> >> Ferry Huberts
> 
> -- 
> Ferry Huberts
> 
> _______________________________________________
> cgit mailing list
> cgit at hjemli.net
> http://hjemli.net/mailman/listinfo/cgit

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[cgit]

On Sun, Mar 03, 2013 at 09:08:04PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 18:06, Lukas Fleischer wrote:
> >This allows for removing the header variable in a following patch. We
> >can use the state variable to check whether the tail needs to be printed
> >instead.
> >
> >Note that the state variable will be moved into a context structure
> >later.
> >
> >Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >---
> >  ui-tree.c | 3 ++-
> >  1 file changed, 2 insertions(+), 1 deletion(-)
> >
> >diff --git a/ui-tree.c b/ui-tree.c
> >index 133101c..3887ecd 100644
> >--- a/ui-tree.c
> >+++ b/ui-tree.c
> >@@ -14,6 +14,7 @@
> >  char *curr_rev;
> >  char *match_path;
> >  int header = 0;
> >+static int state;
> >
> 
> please inititialise

It is initialized further below.

> 
> 
> >  static void print_text_buffer(const char *name, char *buf, unsigned long size)
> >  {
> >@@ -220,7 +221,6 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >  		     const char *pathname, unsigned mode, int stage,
> >  		     void *cbdata)
> >  {
> >-	static int state;
> >  	static char buffer[PATH_MAX];
> >
> >  	if (state == 0) {
> >@@ -274,6 +274,7 @@ void cgit_print_tree(const char *rev, char *path)
> >  	}
> >
> >  	match_path = path;
> >+	state = 0;
> 
> why? where does this come from?

This is the initialization you requested above. It makes much more sense
to initialize here since we actually want this to be reinitialized if
cgit_print_tree() is called more than once (even if there probably isn't
any code path that triggers it more than once yet).

> 
> >  	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >  	ls_tail();
> >  }
> >
> 
> -- 
> Ferry Huberts

[PATCH 7/7] ui-tree.c: Use a context structure in walk_tree()

[cgit]

On Sun, Mar 03, 2013 at 09:13:59PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 18:06, Lukas Fleischer wrote:
> >Use the context pointer to pass context information instead of misusing
> >global variables, as we already did in "ui-blob.c" and in "ui-plain.c".
> >
> >In addition to the fixes to walk_tree(), pass the same structure to
> >ls_tree() and ls_item() which is read_tree_recursive()-based as well.
> >
> >Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >---
> >  ui-tree.c | 51 +++++++++++++++++++++++++++++----------------------
> >  1 file changed, 29 insertions(+), 22 deletions(-)
> >
> >diff --git a/ui-tree.c b/ui-tree.c
> >index 744e039..bd95c3c 100644
> >--- a/ui-tree.c
> >+++ b/ui-tree.c
> >@@ -11,9 +11,11 @@
> >  #include "html.h"
> >  #include "ui-shared.h"
> >
> >-char *curr_rev;
> >-char *match_path;
> >-static int state;
> >+struct walk_tree_context {
> >+	char *curr_rev;
> >+	char *match_path;
> >+	int state;
> >+};
> >
> >  static void print_text_buffer(const char *name, char *buf, unsigned long size)
> >  {
> >@@ -126,6 +128,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
> >  		   const char *pathname, unsigned int mode, int stage,
> >  		   void *cbdata)
> >  {
> >+	struct walk_tree_context *walk_tree_ctx = cbdata;
> >  	char *name;
> >  	char *fullpath;
> >  	char *class;
> >@@ -153,7 +156,7 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
> >  		cgit_submodule_link("ls-mod", fullpath, sha1_to_hex(sha1));
> >  	} else if (S_ISDIR(mode)) {
> >  		cgit_tree_link(name, NULL, "ls-dir", ctx.qry.head,
> >-			       curr_rev, fullpath);
> >+			       walk_tree_ctx->curr_rev, fullpath);
> >  	} else {
> >  		class = strrchr(name, '.');
> >  		if (class != NULL) {
> >@@ -161,19 +164,20 @@ static int ls_item(const unsigned char *sha1, const char *base, int baselen,
> >  		} else
> >  			class = "ls-blob";
> >  		cgit_tree_link(name, NULL, class, ctx.qry.head,
> >-			       curr_rev, fullpath);
> >+			       walk_tree_ctx->curr_rev, fullpath);
> >  	}
> >  	htmlf("</td><td class='ls-size'>%li</td>", size);
> >
> >  	html("<td>");
> >-	cgit_log_link("log", NULL, "button", ctx.qry.head, curr_rev,
> >-		      fullpath, 0, NULL, NULL, ctx.qry.showmsg);
> >+	cgit_log_link("log", NULL, "button", ctx.qry.head,
> >+		      walk_tree_ctx->curr_rev, fullpath, 0, NULL, NULL,
> >+		      ctx.qry.showmsg);
> >  	if (ctx.repo->max_stats)
> >  		cgit_stats_link("stats", NULL, "button", ctx.qry.head,
> >  				fullpath);
> >  	if (!S_ISGITLINK(mode))
> >-		cgit_plain_link("plain", NULL, "button", ctx.qry.head, curr_rev,
> >-				fullpath);
> >+		cgit_plain_link("plain", NULL, "button", ctx.qry.head,
> >+				walk_tree_ctx->curr_rev, fullpath);
> >  	html("</td></tr>\n");
> >  	free(name);
> >  	return 0;
> >@@ -195,7 +199,7 @@ static void ls_tail()
> >  	html("</table>\n");
> >  }
> >
> >-static void ls_tree(const unsigned char *sha1, char *path)
> >+static void ls_tree(const unsigned char *sha1, char *path, struct walk_tree_context *walk_tree_ctx)
> >  {
> >  	struct tree *tree;
> >
> >@@ -207,7 +211,7 @@ static void ls_tree(const unsigned char *sha1, char *path)
> >  	}
> >
> >  	ls_head();
> >-	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, NULL);
> >+	read_tree_recursive(tree, "", 0, 1, NULL, ls_item, walk_tree_ctx);
> >  	ls_tail();
> >  }
> >
> >@@ -216,24 +220,25 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >  		     const char *pathname, unsigned mode, int stage,
> >  		     void *cbdata)
> >  {
> >+	struct walk_tree_context *walk_tree_ctx = cbdata;
> >  	static char buffer[PATH_MAX];
> >
> >-	if (state == 0) {
> >+	if (walk_tree_ctx->state == 0) {
> >  		memcpy(buffer, base, baselen);
> >  		strcpy(buffer + baselen, pathname);
> >-		if (strcmp(match_path, buffer))
> >+		if (strcmp(walk_tree_ctx->match_path, buffer))
> >  			return READ_TREE_RECURSIVE;
> >
> >  		if (S_ISDIR(mode)) {
> >-			state = 1;
> >+			walk_tree_ctx->state = 1;
> >  			ls_head();
> >  			return READ_TREE_RECURSIVE;
> >  		} else {
> >-			print_object(sha1, buffer, pathname, curr_rev);
> >+			print_object(sha1, buffer, pathname, walk_tree_ctx->curr_rev);
> >  			return 0;
> >  		}
> >  	}
> >-	ls_item(sha1, base, baselen, pathname, mode, stage, NULL);
> >+	ls_item(sha1, base, baselen, pathname, mode, stage, walk_tree_ctx);
> >  	return 0;
> >  }
> >
> >@@ -248,11 +253,15 @@ void cgit_print_tree(const char *rev, char *path)
> >  	unsigned char sha1[20];
> >  	struct commit *commit;
> >  	const char *paths[] = {path, NULL};
> >+	struct walk_tree_context walk_tree_ctx = {
> >+		.match_path = path,
> >+		.state = 0
> 
> forgot to initialise curr_rev

It is initialized below. We cannot initialize here since we need to
xstrdup() the revision first.

> 
> >+	};
> >
> >  	if (!rev)
> >  		rev = ctx.qry.head;
> >
> >-	curr_rev = xstrdup(rev);
> >+	walk_tree_ctx.curr_rev = xstrdup(rev);
> >  	if (get_sha1(rev, sha1)) {
> >  		cgit_print_error(fmt("Invalid revision name: %s", rev));
> >  		return;
> >@@ -264,13 +273,11 @@ void cgit_print_tree(const char *rev, char *path)
> >  	}
> >
> >  	if (path == NULL) {
> >-		ls_tree(commit->tree->object.sha1, NULL);
> >+		ls_tree(commit->tree->object.sha1, NULL, &walk_tree_ctx);
> >  		return;
> >  	}
> >
> >-	match_path = path;
> >-	state = 0;
> >-	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >-	if (state == 1)
> >+	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >+	if (walk_tree_ctx.state == 1)
> >  		ls_tail();
> >  }
> >
> 
> -- 
> Ferry Huberts

[PATCH 3/7] ui-plain.c: Use a context structure in walk_tree()

[cgit]

On Sun, Mar 03, 2013 at 09:04:28PM +0100, Ferry Huberts wrote:
> 
> 
> On 03/03/13 18:06, Lukas Fleischer wrote:
> >Do not misuse global variables to save the context. Instead, use the
> >context pointer which was designed to share information between a
> >read_tree_fn and the caller.
> >
> >Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
> >---
> >  ui-plain.c | 31 ++++++++++++++++++-------------
> >  1 file changed, 18 insertions(+), 13 deletions(-)
> >
> >diff --git a/ui-plain.c b/ui-plain.c
> >index 684d5ea..e5addb9 100644
> >--- a/ui-plain.c
> >+++ b/ui-plain.c
> >@@ -11,8 +11,10 @@
> >  #include "html.h"
> >  #include "ui-shared.h"
> >
> >-int match_baselen;
> >-int match;
> >+struct walk_tree_context {
> >+	int match_baselen;
> >+	int match;
> >+};
> >
> >  static char *get_mimetype_from_file(const char *filename, const char *ext)
> >  {
> >@@ -166,18 +168,20 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
> >  		     const char *pathname, unsigned mode, int stage,
> >  		     void *cbdata)
> >  {
> >-	if (baselen == match_baselen) {
> >+	struct walk_tree_context *walk_tree_ctx = cbdata;
> >+
> >+	if (baselen == walk_tree_ctx->match_baselen) {
> >  		if (S_ISREG(mode)) {
> >  			if (print_object(sha1, pathname))
> >-				match = 1;
> >+				walk_tree_ctx->match = 1;
> >  		} else if (S_ISDIR(mode)) {
> >  			print_dir(sha1, base, baselen, pathname);
> >-			match = 2;
> >+			walk_tree_ctx->match = 2;
> >  			return READ_TREE_RECURSIVE;
> >  		}
> >-	} else if (baselen > match_baselen) {
> >+	} else if (baselen > walk_tree_ctx->match_baselen) {
> >  		print_dir_entry(sha1, base, baselen, pathname, mode);
> >-		match = 2;
> >+		walk_tree_ctx->match = 2;
> >  	} else if (S_ISDIR(mode)) {
> >  		return READ_TREE_RECURSIVE;
> >  	}
> >@@ -199,6 +203,7 @@ void cgit_print_plain(struct cgit_context *ctx)
> >  	unsigned char sha1[20];
> >  	struct commit *commit;
> >  	const char *paths[] = {ctx->qry.path, NULL};
> >+	struct walk_tree_context walk_tree_ctx;
> 
> I'd prefer that you initialise the variable here

walk_tree_ctx.match_baselen cannot be initialized here, and setting a
default value doesn't really make sense if it is overwritten in each
code path later.

Setting walk_tree_ctx.match to 0 might make sense, though. I will add
that and resubmit. Thanks!

> 
> >
> >  	if (!rev)
> >  		rev = ctx->qry.head;
> >@@ -214,15 +219,15 @@ void cgit_print_plain(struct cgit_context *ctx)
> >  	}
> >  	if (!paths[0]) {
> >  		paths[0] = "";
> >-		match_baselen = -1;
> >+		walk_tree_ctx.match_baselen = -1;
> >  		print_dir(commit->tree->object.sha1, "", 0, "");
> >-		match = 2;
> >+		walk_tree_ctx.match = 2;
> >  	}
> >  	else
> >-		match_baselen = basedir_len(paths[0]);
> >-	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
> >-	if (!match)
> >+		walk_tree_ctx.match_baselen = basedir_len(paths[0]);
> >+	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
> >+	if (!walk_tree_ctx.match)
> >  		html_status(404, "Not found", 0);
> >-	else if (match == 2)
> >+	else if (walk_tree_ctx.match == 2)
> >  		print_dir_tail();
> >  }
> >
> 
> -- 
> Ferry Huberts

[PATCH v2] ui-plain.c: Use a context structure in walk_tree()

[cgit]

Do not misuse global variables to save the context. Instead, use the
context pointer which was designed to share information between a
read_tree_fn and the caller.

Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
---
 ui-plain.c | 33 ++++++++++++++++++++-------------
 1 file changed, 20 insertions(+), 13 deletions(-)

diff --git a/ui-plain.c b/ui-plain.c
index 684d5ea..dfa77fa 100644
--- a/ui-plain.c
+++ b/ui-plain.c
@@ -11,8 +11,10 @@
 #include "html.h"
 #include "ui-shared.h"
 
-int match_baselen;
-int match;
+struct walk_tree_context {
+	int match_baselen;
+	int match;
+};
 
 static char *get_mimetype_from_file(const char *filename, const char *ext)
 {
@@ -166,18 +168,20 @@ static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
 		     const char *pathname, unsigned mode, int stage,
 		     void *cbdata)
 {
-	if (baselen == match_baselen) {
+	struct walk_tree_context *walk_tree_ctx = cbdata;
+
+	if (baselen == walk_tree_ctx->match_baselen) {
 		if (S_ISREG(mode)) {
 			if (print_object(sha1, pathname))
-				match = 1;
+				walk_tree_ctx->match = 1;
 		} else if (S_ISDIR(mode)) {
 			print_dir(sha1, base, baselen, pathname);
-			match = 2;
+			walk_tree_ctx->match = 2;
 			return READ_TREE_RECURSIVE;
 		}
-	} else if (baselen > match_baselen) {
+	} else if (baselen > walk_tree_ctx->match_baselen) {
 		print_dir_entry(sha1, base, baselen, pathname, mode);
-		match = 2;
+		walk_tree_ctx->match = 2;
 	} else if (S_ISDIR(mode)) {
 		return READ_TREE_RECURSIVE;
 	}
@@ -199,6 +203,9 @@ void cgit_print_plain(struct cgit_context *ctx)
 	unsigned char sha1[20];
 	struct commit *commit;
 	const char *paths[] = {ctx->qry.path, NULL};
+	struct walk_tree_context walk_tree_ctx = {
+		.match = 0
+	};
 
 	if (!rev)
 		rev = ctx->qry.head;
@@ -214,15 +221,15 @@ void cgit_print_plain(struct cgit_context *ctx)
 	}
 	if (!paths[0]) {
 		paths[0] = "";
-		match_baselen = -1;
+		walk_tree_ctx.match_baselen = -1;
 		print_dir(commit->tree->object.sha1, "", 0, "");
-		match = 2;
+		walk_tree_ctx.match = 2;
 	}
 	else
-		match_baselen = basedir_len(paths[0]);
-	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
-	if (!match)
+		walk_tree_ctx.match_baselen = basedir_len(paths[0]);
+	read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
+	if (!walk_tree_ctx.match)
 		html_status(404, "Not found", 0);
-	else if (match == 2)
+	else if (walk_tree_ctx.match == 2)
 		print_dir_tail();
 }
-- 
1.8.2.rc0.247.g811e0c0

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[mailings]

On 03/03/13 21:41, John Keeping wrote:
> On Sun, Mar 03, 2013 at 09:33:36PM +0100, Ferry Huberts wrote:
>>
>>
>> On 03/03/13 21:25, John Keeping wrote:
>>> On Sun, Mar 03, 2013 at 08:56:18PM +0100, Ferry Huberts wrote:
>>>>
>>>>
>>>> On 03/03/13 18:06, Lukas Fleischer wrote:
>>>>> Do not misuse global variables to save the context. Instead, use the
>>>>> context pointer which was designed to share information between a
>>>>> read_tree_fn and the caller.
>>>>>
>>>>> This also prevents from potential misuse of the global pointers
>>>>> match_path and matched_sha1 after the referenced values have been
>>>>> overwritten on the stack.
>>>>>
>>>>> Signed-off-by: Lukas Fleischer <cgit at cryptocrack.de>
>>>>> ---
>>>>>     ui-blob.c | 42 ++++++++++++++++++++++++++----------------
>>>>>     1 file changed, 26 insertions(+), 16 deletions(-)
>>>>>
>>>>> diff --git a/ui-blob.c b/ui-blob.c
>>>>> index 3d07ce5..4bcbc82 100644
>>>>> --- a/ui-blob.c
>>>>> +++ b/ui-blob.c
>>>>> @@ -11,17 +11,22 @@
>>>>>     #include "html.h"
>>>>>     #include "ui-shared.h"
>>>>>
>>>>> -static char *match_path;
>>>>> -static unsigned char *matched_sha1;
>>>>> -static int found_path;
>>>>> +struct walk_tree_context {
>>>>> +	char *match_path;
>>>>> +	unsigned char *matched_sha1;
>>>>> +	int found_path;
>>>>> +};
>>>>>
>>>>>     static int walk_tree(const unsigned char *sha1, const char *base, int baselen,
>>>>> -	const char *pathname, unsigned mode, int stage, void *cbdata) {
>>>>> -	if(strncmp(base, match_path, baselen)
>>>>> -		|| strcmp(match_path + baselen, pathname))
>>>>> +	const char *pathname, unsigned mode, int stage, void *cbdata)
>>>>> +{
>>>>> +	struct walk_tree_context *walk_tree_ctx = cbdata;
>>>>> +
>>>>> +	if(strncmp(base, walk_tree_ctx->match_path, baselen)
>>>>> +		|| strcmp(walk_tree_ctx->match_path + baselen, pathname))
>>>>>     		return READ_TREE_RECURSIVE;
>>>>> -	memmove(matched_sha1, sha1, 20);
>>>>> -	found_path = 1;
>>>>> +	memmove(walk_tree_ctx->matched_sha1, sha1, 20);
>>>>> +	walk_tree_ctx->found_path = 1;
>>>>>     	return 0;
>>>>>     }
>>>>>
>>>>> @@ -33,16 +38,19 @@ int cgit_print_file(char *path, const char *head)
>>>>>     	unsigned long size;
>>>>>     	struct commit *commit;
>>>>>     	const char *paths[] = {path, NULL};
>>>>> +	struct walk_tree_context walk_tree_ctx = {
>>>>> +		.match_path = path,
>>>>> +		.matched_sha1 = sha1,
>>>>> +		.found_path = 0
>>>>> +	};
>>>>> +
>>>>>     	if (get_sha1(head, sha1))
>>>>>     		return -1;
>>>>>     	type = sha1_object_info(sha1, &size);
>>>>>     	if(type == OBJ_COMMIT && path) {
>>>>>     		commit = lookup_commit_reference(sha1);
>>>>> -		match_path = path;
>>>>> -		matched_sha1 = sha1;
>>>>> -		found_path = 0;
>>>>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
>>>>> -		if (!found_path)
>>>>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
>>>>> +		if (!walk_tree_ctx.found_path)
>>>>>     			return -1;
>>>>>     		type = sha1_object_info(sha1, &size);
>>>>>     	}
>>>>> @@ -64,6 +72,10 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>>>>>     	unsigned long size;
>>>>>     	struct commit *commit;
>>>>>     	const char *paths[] = {path, NULL};
>>>>> +	struct walk_tree_context walk_tree_ctx = {
>>>>> +		.match_path = path,
>>>>> +		.matched_sha1 = sha1,
>>>>
>>>> forgot to initialise found_path
>>>
>>> Unnecessary - C99 6.7.8:
>>>
>>>       If there are fewer initializers in a brace-enclosed list than there
>>>       are elements or members of an aggregate, or fewer characters in a
>>>       string literal used to initialize an array of known size than there
>>>       are elements in the array, the remainder of the aggregate shall be
>>>       initialized implicitly the same as objects that have static storage
>>>       duration.
>>
>> which is what? zero?
>
> Yes:
>
>      If an object that has static storage duration is not initialized
>      explicitly, then:
>      ? if it has pointer type, it is initialized to a null pointer;
>      ? if it has arithmetic type, it is initialized to (positive or
>        unsigned) zero;
>      ? if it is an aggregate, every member is initialized (recursively)
>        according to these rules;
>      ? if it is a union, the first named member is initialized
>        (recursively) according to these rules


ok. didn't know that.
maybe I'm too used to old compilers??? :-)

>
>>>
>>>>
>>>>> +	};
>>>>>
>>>>>     	if (hex) {
>>>>>     		if (get_sha1_hex(hex, sha1)){
>>>>> @@ -81,9 +93,7 @@ void cgit_print_blob(const char *hex, char *path, const char *head)
>>>>>
>>>>>     	if((!hex) && type == OBJ_COMMIT && path) {
>>>>>     		commit = lookup_commit_reference(sha1);
>>>>> -		match_path = path;
>>>>> -		matched_sha1 = sha1;
>>>>> -		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, NULL);
>>>>> +		read_tree_recursive(commit->tree, "", 0, 0, paths, walk_tree, &walk_tree_ctx);
>>>>>     		type = sha1_object_info(sha1,&size);
>>>>>     	}
>>>>>
>>>>>
>>>>
>>>> --
>>>> Ferry Huberts
>>
>> --
>> Ferry Huberts
>>
>> _______________________________________________
>> cgit mailing list
>> cgit at hjemli.net
>> http://hjemli.net/mailman/listinfo/cgit

-- 
Ferry Huberts

[PATCH 0/7] Use context structures in walk_tree() functions

[mailings]

ok then :-)
with the clarifications you provided

Reviewed-by: Ferry Huberts <ferry.huberts at pelagic.nl>


On 03/03/13 18:06, Lukas Fleischer wrote:
> This turned out to be a bit harder than I expected. I split my initial
> patch (plus additions) into several chunks that are hopefully a bit
> easier to understand/follow.
>
> All patches are based on the "Fix several whitespace errors" I submitted
> earlier.
>
> Lukas Fleischer (7):
>    ui-blob.c: Use a context structure in walk_tree()
>    ui-plain.c: Do not access match variable in print_*()
>    ui-plain.c: Use a context structure in walk_tree()
>    ui-tree.c: Pass current revision to print_object()
>    ui-tree.c: Declare the state variable globally
>    ui-tree.c: Drop the header variable
>    ui-tree.c: Use a context structure in walk_tree()
>
>   ui-blob.c  | 42 ++++++++++++++++++++++++++----------------
>   ui-plain.c | 47 +++++++++++++++++++++++++++--------------------
>   ui-tree.c  | 60 ++++++++++++++++++++++++++++++++----------------------------
>   3 files changed, 85 insertions(+), 64 deletions(-)
>

-- 
Ferry Huberts

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[Jason]

On Sun, Mar 3, 2013 at 12:06 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> Note that the state variable will be moved into a context structure
> later.

The sooner the better. It's nice to be getting rid of the globals
altogether indeed. Should make the eventual concurrency / scgi
additions a bit easier down the road.

[PATCH 1/7] ui-blob.c: Use a context structure in walk_tree()

[Jason]

For a good example of this, check out the various Linux function
pointer tables. Implementers define the functions they'd like, and the
rest get zeroed out. Quite elegant indeed.

[PATCH 5/7] ui-tree.c: Declare the state variable globally

[cgit]

On Sun, Mar 03, 2013 at 06:47:48PM -0500, Jason A. Donenfeld wrote:
> On Sun, Mar 3, 2013 at 12:06 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> > Note that the state variable will be moved into a context structure
> > later.
> 
> The sooner the better. It's nice to be getting rid of the globals
> altogether indeed. Should make the eventual concurrency / scgi
> additions a bit easier down the road.

Just to clarify: This is already done in patch 7/7. I only moved it into
a global variable here to make the following patch sets a bit easier to
read.

[PATCH 0/7] Use context structures in walk_tree() functions

[Jason]

On Sun, Mar 3, 2013 at 12:06 PM, Lukas Fleischer <cgit at cryptocrack.de> wrote:
> This turned out to be a bit harder than I expected. I split my initial
> patch (plus additions) into several chunks that are hopefully a bit
> easier to understand/follow.

Merged! Made a few fix-up commits, but for the most part, I left
things in tact. This is a quality series -- thanks a lot.