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

[Ferry Huberts]

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 <c...@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@hjemli.net
http://hjemli.net/mailman/listinfo/cgit

--
Ferry Huberts

_______________________________________________
cgit mailing list
cgit@hjemli.net
http://hjemli.net/mailman/listinfo/cgit