{"thread":{"id":"11503","subject":"[PATCH 2/5] First step, making setup (somewhat) reentrant","startedAt":"2008-01-07T05:47:31Z","lastAt":"2008-01-07T11:28:44Z","messageCount":8,"participants":["Christian Thaeter","Johannes Schindelin","Junio C Hamano"],"isPatch":true,"patchVersion":1,"patchTotal":5},"messages":[{"id":"64623","messageId":"1199684855-14246-1-git-send-email-ct@pipapo.org","threadId":"11503","inReplyTo":null,"subject":"[PATCH 1/5] Add streq() to compat-util","fromName":"Christian Thaeter","fromEmail":"ct@pipapo.org","sentAt":"2008-01-07T05:47:31Z","receivedAt":"2008-01-07T05:47:31Z","isPatch":true,"sender":{"key":"ct@pipapo.org","avatar":null},"body":"streq() compares strings for equality, it differs from strcmp() in that\nit can handle NULL pointers and that it short-circruits if a==b.\n\nreturns 1 if the strings are equal (or both are NULL) and 0 when not.\n\nSigned-off-by: Christian Thaeter \n---\nHere comes a series of patches to reinit libgit with subsequent\nsetup_git_directory() calls. So far, this only suffices for simple\ntasks like scanning a list of repositories (buiding the start page\nof a repository browser). There are still a lot of things not\nfixed yet (grep for '\\tstatic', object & graft cache etc.).\nNevertheless, it works for me so far.\n\n git-compat-util.h | 9 +++++++++\n 1 files changed, 9 insertions(+), 0 deletions(-)\n\ndiff --git a/git-compat-util.h b/git-compat-util.h\nindex b6ef544..759e94c 100644\n--- a/git-compat-util.h\n+++ b/git-compat-util.h\n@@ -424,4 +424,13 @@ static inline int strtol_i(char const *s, int base, int *result)\n \treturn 0;\n }\n \n+static inline int streq(const char *a, const char* b)\n+{\n+\tif (a == b)\n+\t\treturn 1;\n+\tif (a && b)\n+\t\treturn !strcmp(a, b);\n+\treturn 0;\n+}\n+\n #endif\n-- \n1.5.3.7\n"},{"id":"64622","messageId":"1199684855-14246-2-git-send-email-ct@pipapo.org","threadId":"11503","inReplyTo":"1199684855-14246-1-git-send-email-ct@pipapo.org","subject":"[PATCH 2/5] First step, making setup (somewhat) reentrant","fromName":"Christian Thaeter","fromEmail":"ct@pipapo.org","sentAt":"2008-01-07T05:47:32Z","receivedAt":"2008-01-07T05:47:32Z","isPatch":true,"sender":{"key":"ct@pipapo.org","avatar":null},"body":"setup_git_env() just initialized some static data and then let it\nleak. For reentrant operation, as in using a git process to access\nserveral repositories in order, this data has to be freed and properly\nreinitialized.\n\nget_git_work_tree() used a once only initialization flag, we now provide\na reset_git_work_tree() to free allocated resources and let it initialize\nitself on the next call.\n\ncreate_default_files() in init-db needed to be fixed to compare strings\nproperly.\n\nSigned-off-by: Christian Thaeter \n---\n builtin-init-db.c | 4 ++--\n environment.c | 43 ++++++++++++++++++++++++++++++++++---------\n 2 files changed, 36 insertions(+), 11 deletions(-)\n\ndiff --git a/builtin-init-db.c b/builtin-init-db.c\nindex e1393b8..d5a5dd9 100644\n--- a/builtin-init-db.c\n+++ b/builtin-init-db.c\n@@ -270,8 +270,8 @@ static int create_default_files(const char *git_dir, const char *template_path)\n \t\tgit_config_set(\"core.bare\", \"false\");\n \t\t/* allow template config file to override the default */\n \t\tif (log_all_ref_updates == -1)\n-\t\t git_config_set(\"core.logallrefupdates\", \"true\");\n-\t\tif (work_tree != git_work_tree_cfg)\n+\t\t\tgit_config_set(\"core.logallrefupdates\", \"true\");\n+\t\tif (!streq(work_tree, git_work_tree_cfg))\n \t\t\tgit_config_set(\"core.worktree\", work_tree);\n \t}\n \ndiff --git a/environment.c b/environment.c\nindex 18a1c4e..492d87c 100644\n--- a/environment.c\n+++ b/environment.c\n@@ -38,31 +38,48 @@ int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n \n /* This is set by setup_git_dir_gently() and/or git_default_config() */\n-char *git_work_tree_cfg;\n-static const char *work_tree;\n+char *git_work_tree_cfg = NULL;\n+static char *work_tree = NULL;\n+static int work_tree_initialized = 0;\n \n static const char *git_dir;\n-static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;\n+static char *git_object_dir = NULL, *git_index_file = NULL, *git_refs_dir = NULL, *git_graft_file = NULL;\n \n static void setup_git_env(void)\n {\n \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n \tif (!git_dir)\n \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n+\tif (git_object_dir)\n+\t\tfree(git_object_dir);\n \tgit_object_dir = getenv(DB_ENVIRONMENT);\n-\tif (!git_object_dir) {\n+\tif (git_object_dir) {\n+\t\tgit_object_dir = xstrdup(git_object_dir);\n+\t}\n+\telse {\n \t\tgit_object_dir = xmalloc(strlen(git_dir) + 9);\n \t\tsprintf(git_object_dir, \"%s/objects\", git_dir);\n \t}\n+\tif (git_refs_dir)\n+\t\tfree(git_refs_dir);\n \tgit_refs_dir = xmalloc(strlen(git_dir) + 6);\n \tsprintf(git_refs_dir, \"%s/refs\", git_dir);\n+\tif (git_index_file)\n+\t\tfree(git_index_file);\n \tgit_index_file = getenv(INDEX_ENVIRONMENT);\n-\tif (!git_index_file) {\n+\tif (git_index_file) {\n+\t\tgit_index_file = xstrdup(git_index_file);\n+\t}\n+\telse {\n \t\tgit_index_file = xmalloc(strlen(git_dir) + 7);\n \t\tsprintf(git_index_file, \"%s/index\", git_dir);\n \t}\n+\tif (git_graft_file)\n+\t\tfree (git_graft_file);\n \tgit_graft_file = getenv(GRAFT_ENVIRONMENT);\n-\tif (!git_graft_file)\n+\tif (git_graft_file)\n+\t\tgit_graft_file = xstrdup(git_graft_file);\n+\telse\n \t\tgit_graft_file = xstrdup(git_path(\"info/grafts\"));\n }\n \n@@ -79,10 +96,16 @@ const char *get_git_dir(void)\n \treturn git_dir;\n }\n \n+void reset_git_work_tree(void)\n+{\n+\twork_tree_initialized = 0;\n+\tif (work_tree)\n+\t\tfree (work_tree);\n+}\n+\n const char *get_git_work_tree(void)\n {\n-\tstatic int initialized = 0;\n-\tif (!initialized) {\n+\tif (!work_tree_initialized) {\n \t\twork_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);\n \t\t/* core.bare = true overrides implicit and config work tree */\n \t\tif (!work_tree && is_bare_repository_cfg < 1) {\n@@ -90,11 +113,13 @@ const char *get_git_work_tree(void)\n \t\t\t/* make_absolute_path also normalizes the path */\n \t\t\tif (work_tree && !is_absolute_path(work_tree))\n \t\t\t\twork_tree = xstrdup(make_absolute_path(git_path(work_tree)));\n+\t\t\telse if (work_tree)\n+\t\t\t\twork_tree = xstrdup(work_tree);\n \t\t} else if (work_tree)\n \t\t\twork_tree = xstrdup(make_absolute_path(work_tree));\n-\t\tinitialized = 1;\n \t\tif (work_tree)\n \t\t\tis_bare_repository_cfg = 0;\n+\t\twork_tree_initialized = 1;\n \t}\n \treturn work_tree;\n }\n-- \n1.5.3.7\n"},{"id":"64625","messageId":"1199684855-14246-3-git-send-email-ct@pipapo.org","threadId":"11503","inReplyTo":"1199684855-14246-2-git-send-email-ct@pipapo.org","subject":"[PATCH 3/5] provide a reset_packed_git() function","fromName":"Christian Thaeter","fromEmail":"ct@pipapo.org","sentAt":"2008-01-07T05:47:33Z","receivedAt":"2008-01-07T05:47:33Z","isPatch":true,"sender":{"key":"ct@pipapo.org","avatar":null},"body":"this frees packs and pack-windows allocated by former operations and\nreset some variables to let prepare_packed_git() reinit the packs on the\nnext call.\n\nfind_pack_entry() uses a static pointer to cache the last_found pack\nwhich will be reseted too.\n\nSigned-off-by: Christian Thaeter \n---\n sha1_file.c | 26 +++++++++++++++++++++-----\n 1 files changed, 21 insertions(+), 5 deletions(-)\n\ndiff --git a/sha1_file.c b/sha1_file.c\nindex 6583797..139d5a2 100644\n--- a/sha1_file.c\n+++ b/sha1_file.c\n@@ -418,6 +418,7 @@ static unsigned int pack_open_windows;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n+static struct packed_git *last_found_packed_git = (void *)1;\n \n void pack_report(void)\n {\n@@ -969,6 +970,22 @@ void reprepare_packed_git(void)\n \tprepare_packed_git();\n }\n \n+void reset_packed_git(void)\n+{\n+\t/* certainly not the best way to release all windows, but should work for now */\n+\twhile (unuse_one_window(NULL, -1))\n+\t\t; /* nothing */\n+\n+\twhile (packed_git) {\n+\t\tstruct packed_git* p = packed_git;\n+\t\tpacked_git = packed_git->next;\n+\t\tfree(p);\n+\t}\n+\n+\tlast_found_packed_git = (void *)1;\n+\tprepare_packed_git_run_once = 0;\n+}\n+\n int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)\n {\n \tunsigned char real_sha1[20];\n@@ -1705,14 +1722,13 @@ int matches_pack_name(struct packed_git *p, const char *name)\n \n static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, const char **ignore_packed)\n {\n-\tstatic struct packed_git *last_found = (void *)1;\n \tstruct packed_git *p;\n \toff_t offset;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n-\tp = (last_found == (void *)1) ? packed_git : last_found;\n+\tp = (last_found_packed_git == (void *)1) ? packed_git : last_found_packed_git;\n \n \tdo {\n \t\tif (ignore_packed) {\n@@ -1741,16 +1757,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons\n \t\t\te->offset = offset;\n \t\t\te->p = p;\n \t\t\thashcpy(e->sha1, sha1);\n-\t\t\tlast_found = p;\n+\t\t\tlast_found_packed_git = p;\n \t\t\treturn 1;\n \t\t}\n \n \t\tnext:\n-\t\tif (p == last_found)\n+\t\tif (p == last_found_packed_git)\n \t\t\tp = packed_git;\n \t\telse\n \t\t\tp = p->next;\n-\t\tif (p == last_found)\n+\t\tif (p == last_found_packed_git)\n \t\t\tp = p->next;\n \t} while (p);\n \treturn 0;\n-- \n1.5.3.7\n"},{"id":"64624","messageId":"1199684855-14246-4-git-send-email-ct@pipapo.org","threadId":"11503","inReplyTo":"1199684855-14246-3-git-send-email-ct@pipapo.org","subject":"[PATCH 4/5] Export some more functions to enable resetting the git state","fromName":"Christian Thaeter","fromEmail":"ct@pipapo.org","sentAt":"2008-01-07T05:47:34Z","receivedAt":"2008-01-07T05:47:34Z","isPatch":true,"sender":{"key":"ct@pipapo.org","avatar":null},"body":"This declares the functions added by the former 2 commits as 'extern'.\n\nFurther invalidate_cached_refs() got renamed to\ngit_invalidate_cached_refs() and is exported too.\n\nSigned-off-by: Christian Thaeter \n---\n cache.h | 3 +++\n refs.c | 6 +++---\n 2 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/cache.h b/cache.h\nindex 39331c2..4352d08 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -224,7 +224,9 @@ extern char *get_refs_directory(void);\n extern char *get_index_file(void);\n extern char *get_graft_file(void);\n extern int set_git_dir(const char *path);\n+extern void reset_git_work_tree(void);\n extern const char *get_git_work_tree(void);\n+extern void git_invalidate_cached_refs(void);\n \n #define ALTERNATE_DB_ENVIRONMENT \"GIT_ALTERNATE_OBJECT_DIRECTORIES\"\n \n@@ -553,6 +555,7 @@ extern struct packed_git *parse_pack_index_file(const unsigned char *sha1,\n \n extern void prepare_packed_git(void);\n extern void reprepare_packed_git(void);\n+extern void reset_packed_git(void);\n extern void install_packed_git(struct packed_git *pack);\n \n extern struct packed_git *find_sha1_pack(const unsigned char *sha1,\ndiff --git a/refs.c b/refs.c\nindex 58f6d17..900cc5d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -167,7 +167,7 @@ static void free_ref_list(struct ref_list *list)\n \t}\n }\n \n-static void invalidate_cached_refs(void)\n+void git_invalidate_cached_refs(void)\n {\n \tstruct cached_refs *ca = &cached_refs;\n \n@@ -898,7 +898,7 @@ int delete_ref(const char *refname, const unsigned char *sha1)\n \tif (err && errno != ENOENT)\n \t\tfprintf(stderr, \"warning: unlink(%s) failed: %s\",\n \t\t\tgit_path(\"logs/%s\", lock->ref_name), strerror(errno));\n-\tinvalidate_cached_refs();\n+\tgit_invalidate_cached_refs();\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1137,7 +1137,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_cached_refs();\n+\tgit_invalidate_cached_refs();\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.5.3.7\n"},{"id":"64626","messageId":"1199684855-14246-5-git-send-email-ct@pipapo.org","threadId":"11503","inReplyTo":"1199684855-14246-4-git-send-email-ct@pipapo.org","subject":"[PATCH 5/5] Make setup_git_directory reentrant","fromName":"Christian Thaeter","fromEmail":"ct@pipapo.org","sentAt":"2008-01-07T05:47:35Z","receivedAt":"2008-01-07T05:47:35Z","isPatch":true,"sender":{"key":"ct@pipapo.org","avatar":null},"body":"Reentrant means here one can call it serveral times for different repos.\n\nThe functionality introduced here (based on earlier commits) is sufficent\nto call setup_git_directory() for inspecting the refs like generating a\nrepository list for a browser. For full reset some more works needs\nto be done later.\n\nSigned-off-by: Christian Thaeter \n---\n setup.c | 13 ++++++++++++-\n 1 files changed, 12 insertions(+), 1 deletions(-)\n\ndiff --git a/setup.c b/setup.c\nindex adede16..770dc41 100644\n--- a/setup.c\n+++ b/setup.c\n@@ -200,6 +200,8 @@ static const char *set_work_tree(const char *dir)\n \n \tif (!getcwd(buffer, sizeof(buffer)))\n \t\tdie (\"Could not get the current working directory\");\n+\tif (git_work_tree_cfg)\n+\t\tfree (git_work_tree_cfg);\n \tgit_work_tree_cfg = xstrdup(buffer);\n \tinside_work_tree = 1;\n \n@@ -329,6 +331,8 @@ const char *setup_git_directory_gently(int *nongit_ok)\n \tinside_git_dir = 0;\n \tif (!work_tree_env)\n \t\tinside_work_tree = 1;\n+\tif (git_work_tree_cfg)\n+\t\tfree (git_work_tree_cfg);\n \tgit_work_tree_cfg = xstrndup(cwd, offset);\n \tif (check_repository_format_gently(nongit_ok))\n \t\treturn NULL;\n@@ -387,7 +391,14 @@ int check_repository_format(void)\n \n const char *setup_git_directory(void)\n {\n-\tconst char *retval = setup_git_directory_gently(NULL);\n+\tconst char *retval;\n+\n+\tinside_git_dir = -1;\n+\tinside_work_tree = -1;\n+\tgit_invalidate_cached_refs();\n+\treset_packed_git();\n+\treset_git_work_tree();\n+\tretval = setup_git_directory_gently(NULL);\n \n \t/* If the work tree is not the default one, recompute prefix */\n \tif (inside_work_tree < 0) {\n-- \n1.5.3.7\n"},{"id":"64631","messageId":"alpine.LSU.1.00.0801070845270.10101@racer.site","threadId":"11503","inReplyTo":"1199684855-14246-2-git-send-email-ct@pipapo.org","subject":"Re: [PATCH 2/5] First step, making setup (somewhat) reentrant","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-07T08:50:40Z","receivedAt":"2008-01-07T08:50:40Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\n[I assume that you mean this series post-1.5.4]\n\nOn Mon, 7 Jan 2008, Christian Thaeter wrote:\n\n> diff --git a/environment.c b/environment.c\n> index 18a1c4e..492d87c 100644\n> --- a/environment.c\n> +++ b/environment.c\n> @@ -38,31 +38,48 @@ int auto_crlf = 0;\t/* 1: both ways, -1: only when adding git objects */\n> unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;\n> \n> /* This is set by setup_git_dir_gently() and/or git_default_config() */\n> -char *git_work_tree_cfg;\n> -static const char *work_tree;\n> +char *git_work_tree_cfg = NULL;\n> +static char *work_tree = NULL;\n> +static int work_tree_initialized = 0;\n\nGlobal variables do not need initialisation, if all what you do is set \nthem to NULL. Therefore, most of this hunk is not necessary, and only \ndistracts from what is really relevant, \"work_tree_initialized = 0\".\n\n> static void setup_git_env(void)\n> {\n> \tgit_dir = getenv(GIT_DIR_ENVIRONMENT);\n> \tif (!git_dir)\n> \t\tgit_dir = DEFAULT_GIT_DIR_ENVIRONMENT;\n> +\tif (git_object_dir)\n> +\t\tfree(git_object_dir);\n\nLogically, this belongs into a \"cleanup_git_env()\" function, no?\n\n> \tgit_object_dir = getenv(DB_ENVIRONMENT);\n> -\tif (!git_object_dir) {\n> +\tif (git_object_dir) {\n> +\t\tgit_object_dir = xstrdup(git_object_dir);\n> +\t}\n\nAre you sure that you want to keep the object directory, even if you want \nto initialise to a new repository?\n\nWhich brings me to a more fundamental question: what do you need reentrant \nsetup_directory() for? If it is just to allow multiple calls to that \nfunction for the _same_ repository, I say clean up your code.\n\nCiao,\nDscho\n"},{"id":"64635","messageId":"7v4pdq0z30.fsf@gitster.siamese.dyndns.org","threadId":"11503","inReplyTo":"alpine.LSU.1.00.0801070845270.10101@racer.site","subject":"Re: [PATCH 2/5] First step, making setup (somewhat) reentrant","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2008-01-07T09:22:43Z","receivedAt":"2008-01-07T09:22:43Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Johannes Schindelin writes:\n\n> Which brings me to a more fundamental question: what do you need reentrant \n> setup_directory() for? If it is just to allow multiple calls to that \n> function for the _same_ repository, I say clean up your code.\n\nIIRC, he is writing a set-of-repositories browser.\n\nBut I think the right approach of cleaning up is to design a\nlayer of containers like we did for \"the_index\" libification\n[*1*]. We would probably want:\n\n * a container that abstracts accesses to a single repository\n that includes:\n\n - set of refs;\n\n - object database and object hash (because different\n repositories can have different notion of who is parent of\n whom);\n\n * a container that abstracts accesses to a state of the index\n (we already have this -- the_index).\n\n * a container that abstracts a manipulation state in a single\n repository, that points at the first one but somehow knows\n what in-core objects are rewritten during its operation\n (e.g. history simplification rewrites parents list in in-core\n commit objects).\n\nand a relatively low impact approach to transition to such a\nscheme would be to use \"the_index\" libification as the model.\nI.e. make the real API to take an explicit container as one of\ntheir parameter, but define the default container to work on for\neach level, and express the existing API in terms of the updated\nAPI that works on the default container.\n\n\n[Reference]\n\n 228e94f93570b580da388069900c56b813c91953 (Move index-related\n variables into a structure.)\n\n 4aab5b46f44a7ba860e07a52be506b7b57b2bd5f (Make read-cache.c\n \"the_index\" free.)\n"},{"id":"64643","messageId":"Pine.LNX.4.64.0801071227110.14355@wbgn129.biozentrum.uni-wuerzburg.de","threadId":"11503","inReplyTo":"7v4pdq0z30.fsf@gitster.siamese.dyndns.org","subject":"Re: [PATCH 2/5] First step, making setup (somewhat) reentrant","fromName":"Johannes Schindelin","fromEmail":"johannes.schindelin@gmx.de","sentAt":"2008-01-07T11:28:44Z","receivedAt":"2008-01-07T11:28:44Z","isPatch":true,"sender":{"key":"johannes.schindelin@gmx.de","avatar":"https://avatars.githubusercontent.com/u/127790?v=4"},"body":"Hi,\n\nOn Mon, 7 Jan 2008, Junio C Hamano wrote:\n\n> Johannes Schindelin writes:\n> \n> > Which brings me to a more fundamental question: what do you need \n> > reentrant setup_directory() for? If it is just to allow multiple \n> > calls to that function for the _same_ repository, I say clean up your \n> > code.\n> \n> IIRC, he is writing a set-of-repositories browser.\n> \n> But I think the right approach of cleaning up is to design a\n> layer of containers like we did for \"the_index\" libification\n\nYes, I concur. That was a nice abstraction without impacting existing code \ntoo much.\n\nCiao,\nDscho\n"}]}