[PATCH 1/5] Add streq() to compat-util

[PATCH 1/5] Add streq() to compat-util

[Christian Thaeter]

streq() compares strings for equality, it differs from strcmp() in that
it can handle NULL pointers and that it short-circruits if a==b.

returns 1 if the strings are equal (or both are NULL) and 0 when not.

Signed-off-by: Christian Thaeter <ct@pipapo.org>
---
Here comes a series of patches to reinit libgit with subsequent
setup_git_directory() calls. So far, this only suffices for simple
tasks like scanning a list of repositories (buiding the start page
of a repository browser). There are still a lot of things not
fixed yet (grep for '\tstatic', object & graft cache etc.).
Nevertheless, it works for me so far.

 git-compat-util.h |    9 +++++++++
 1 files changed, 9 insertions(+), 0 deletions(-)

diff --git a/git-compat-util.h b/git-compat-util.h
index b6ef544..759e94c 100644
--- a/git-compat-util.h
+++ b/git-compat-util.h
@@ -424,4 +424,13 @@ static inline int strtol_i(char const *s, int base, int *result)
 	return 0;
 }
 
+static inline int streq(const char *a, const char* b)
+{
+	if (a == b)
+		return 1;
+	if (a && b)
+		return !strcmp(a, b);
+	return 0;
+}
+
 #endif
-- 
1.5.3.7

[PATCH 2/5] First step, making setup (somewhat) reentrant

[Christian Thaeter]

setup_git_env() just initialized some static data and then let it
leak. For reentrant operation, as in using a git process to access
serveral repositories in order, this data has to be freed and properly
reinitialized.

get_git_work_tree() used a once only initialization flag, we now provide
a reset_git_work_tree() to free allocated resources and let it initialize
itself on the next call.

create_default_files() in init-db needed to be fixed to compare strings
properly.

Signed-off-by: Christian Thaeter <ct@pipapo.org>
---
 builtin-init-db.c |    4 ++--
 environment.c     |   43 ++++++++++++++++++++++++++++++++++---------
 2 files changed, 36 insertions(+), 11 deletions(-)

diff --git a/builtin-init-db.c b/builtin-init-db.c
index e1393b8..d5a5dd9 100644
--- a/builtin-init-db.c
+++ b/builtin-init-db.c
@@ -270,8 +270,8 @@ static int create_default_files(const char *git_dir, const char *template_path)
 		git_config_set("core.bare", "false");
 		/* allow template config file to override the default */
 		if (log_all_ref_updates == -1)
-		    git_config_set("core.logallrefupdates", "true");
-		if (work_tree != git_work_tree_cfg)
+			git_config_set("core.logallrefupdates", "true");
+		if (!streq(work_tree, git_work_tree_cfg))
 			git_config_set("core.worktree", work_tree);
 	}
 
diff --git a/environment.c b/environment.c
index 18a1c4e..492d87c 100644
--- a/environment.c
+++ b/environment.c
@@ -38,31 +38,48 @@ int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
 unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
 
 /* This is set by setup_git_dir_gently() and/or git_default_config() */
-char *git_work_tree_cfg;
-static const char *work_tree;
+char *git_work_tree_cfg = NULL;
+static char *work_tree = NULL;
+static int work_tree_initialized = 0;
 
 static const char *git_dir;
-static char *git_object_dir, *git_index_file, *git_refs_dir, *git_graft_file;
+static char *git_object_dir = NULL, *git_index_file = NULL, *git_refs_dir = NULL, *git_graft_file = NULL;
 
 static void setup_git_env(void)
 {
 	git_dir = getenv(GIT_DIR_ENVIRONMENT);
 	if (!git_dir)
 		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
+	if (git_object_dir)
+		free(git_object_dir);
 	git_object_dir = getenv(DB_ENVIRONMENT);
-	if (!git_object_dir) {
+	if (git_object_dir) {
+		git_object_dir = xstrdup(git_object_dir);
+	}
+	else {
 		git_object_dir = xmalloc(strlen(git_dir) + 9);
 		sprintf(git_object_dir, "%s/objects", git_dir);
 	}
+	if (git_refs_dir)
+		free(git_refs_dir);
 	git_refs_dir = xmalloc(strlen(git_dir) + 6);
 	sprintf(git_refs_dir, "%s/refs", git_dir);
+	if (git_index_file)
+		free(git_index_file);
 	git_index_file = getenv(INDEX_ENVIRONMENT);
-	if (!git_index_file) {
+	if (git_index_file) {
+		git_index_file = xstrdup(git_index_file);
+	}
+	else {
 		git_index_file = xmalloc(strlen(git_dir) + 7);
 		sprintf(git_index_file, "%s/index", git_dir);
 	}
+	if (git_graft_file)
+		free (git_graft_file);
 	git_graft_file = getenv(GRAFT_ENVIRONMENT);
-	if (!git_graft_file)
+	if (git_graft_file)
+		git_graft_file = xstrdup(git_graft_file);
+	else
 		git_graft_file = xstrdup(git_path("info/grafts"));
 }
 
@@ -79,10 +96,16 @@ const char *get_git_dir(void)
 	return git_dir;
 }
 
+void reset_git_work_tree(void)
+{
+	work_tree_initialized = 0;
+	if (work_tree)
+		free (work_tree);
+}
+
 const char *get_git_work_tree(void)
 {
-	static int initialized = 0;
-	if (!initialized) {
+	if (!work_tree_initialized) {
 		work_tree = getenv(GIT_WORK_TREE_ENVIRONMENT);
 		/* core.bare = true overrides implicit and config work tree */
 		if (!work_tree && is_bare_repository_cfg < 1) {
@@ -90,11 +113,13 @@ const char *get_git_work_tree(void)
 			/* make_absolute_path also normalizes the path */
 			if (work_tree && !is_absolute_path(work_tree))
 				work_tree = xstrdup(make_absolute_path(git_path(work_tree)));
+			else if (work_tree)
+				work_tree = xstrdup(work_tree);
 		} else if (work_tree)
 			work_tree = xstrdup(make_absolute_path(work_tree));
-		initialized = 1;
 		if (work_tree)
 			is_bare_repository_cfg = 0;
+		work_tree_initialized = 1;
 	}
 	return work_tree;
 }
-- 
1.5.3.7

[PATCH 3/5] provide a reset_packed_git() function

[Christian Thaeter]

this frees packs and pack-windows allocated by former operations and
reset some variables to let prepare_packed_git() reinit the packs on the
next call.

find_pack_entry() uses a static pointer to cache the last_found pack
which will be reseted too.

Signed-off-by: Christian Thaeter <ct@pipapo.org>
---
 sha1_file.c |   26 +++++++++++++++++++++-----
 1 files changed, 21 insertions(+), 5 deletions(-)

diff --git a/sha1_file.c b/sha1_file.c
index 6583797..139d5a2 100644
--- a/sha1_file.c
+++ b/sha1_file.c
@@ -418,6 +418,7 @@ static unsigned int pack_open_windows;
 static size_t peak_pack_mapped;
 static size_t pack_mapped;
 struct packed_git *packed_git;
+static struct packed_git *last_found_packed_git = (void *)1;
 
 void pack_report(void)
 {
@@ -969,6 +970,22 @@ void reprepare_packed_git(void)
 	prepare_packed_git();
 }
 
+void reset_packed_git(void)
+{
+	/* certainly not the best way to release all windows, but should work for now */
+	while (unuse_one_window(NULL, -1))
+		; /* nothing */
+
+	while (packed_git) {
+		struct packed_git* p = packed_git;
+		packed_git = packed_git->next;
+		free(p);
+	}
+
+	last_found_packed_git = (void *)1;
+	prepare_packed_git_run_once = 0;
+}
+
 int check_sha1_signature(const unsigned char *sha1, void *map, unsigned long size, const char *type)
 {
 	unsigned char real_sha1[20];
@@ -1705,14 +1722,13 @@ int matches_pack_name(struct packed_git *p, const char *name)
 
 static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, const char **ignore_packed)
 {
-	static struct packed_git *last_found = (void *)1;
 	struct packed_git *p;
 	off_t offset;
 
 	prepare_packed_git();
 	if (!packed_git)
 		return 0;
-	p = (last_found == (void *)1) ? packed_git : last_found;
+	p = (last_found_packed_git == (void *)1) ? packed_git : last_found_packed_git;
 
 	do {
 		if (ignore_packed) {
@@ -1741,16 +1757,16 @@ static int find_pack_entry(const unsigned char *sha1, struct pack_entry *e, cons
 			e->offset = offset;
 			e->p = p;
 			hashcpy(e->sha1, sha1);
-			last_found = p;
+			last_found_packed_git = p;
 			return 1;
 		}
 
 		next:
-		if (p == last_found)
+		if (p == last_found_packed_git)
 			p = packed_git;
 		else
 			p = p->next;
-		if (p == last_found)
+		if (p == last_found_packed_git)
 			p = p->next;
 	} while (p);
 	return 0;
-- 
1.5.3.7

[PATCH 4/5] Export some more functions to enable resetting the git state

[Christian Thaeter]

This declares the functions added by the former 2 commits as 'extern'.

Further invalidate_cached_refs() got renamed to
git_invalidate_cached_refs() and is exported too.

Signed-off-by: Christian Thaeter <ct@pipapo.org>
---
 cache.h |    3 +++
 refs.c  |    6 +++---
 2 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/cache.h b/cache.h
index 39331c2..4352d08 100644
--- a/cache.h
+++ b/cache.h
@@ -224,7 +224,9 @@ extern char *get_refs_directory(void);
 extern char *get_index_file(void);
 extern char *get_graft_file(void);
 extern int set_git_dir(const char *path);
+extern void reset_git_work_tree(void);
 extern const char *get_git_work_tree(void);
+extern void git_invalidate_cached_refs(void);
 
 #define ALTERNATE_DB_ENVIRONMENT "GIT_ALTERNATE_OBJECT_DIRECTORIES"
 
@@ -553,6 +555,7 @@ extern struct packed_git *parse_pack_index_file(const unsigned char *sha1,
 
 extern void prepare_packed_git(void);
 extern void reprepare_packed_git(void);
+extern void reset_packed_git(void);
 extern void install_packed_git(struct packed_git *pack);
 
 extern struct packed_git *find_sha1_pack(const unsigned char *sha1,
diff --git a/refs.c b/refs.c
index 58f6d17..900cc5d 100644
--- a/refs.c
+++ b/refs.c
@@ -167,7 +167,7 @@ static void free_ref_list(struct ref_list *list)
 	}
 }
 
-static void invalidate_cached_refs(void)
+void git_invalidate_cached_refs(void)
 {
 	struct cached_refs *ca = &cached_refs;
 
@@ -898,7 +898,7 @@ int delete_ref(const char *refname, const unsigned char *sha1)
 	if (err && errno != ENOENT)
 		fprintf(stderr, "warning: unlink(%s) failed: %s",
 			git_path("logs/%s", lock->ref_name), strerror(errno));
-	invalidate_cached_refs();
+	git_invalidate_cached_refs();
 	unlock_ref(lock);
 	return ret;
 }
@@ -1137,7 +1137,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_cached_refs();
+	git_invalidate_cached_refs();
 	if (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||
 	    (strcmp(lock->ref_name, lock->orig_ref_name) &&
 	     log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {
-- 
1.5.3.7

[PATCH 5/5] Make setup_git_directory reentrant

[Christian Thaeter]

Reentrant means here one can call it serveral times for different repos.

The functionality introduced here (based on earlier commits) is sufficent
to call setup_git_directory() for inspecting the refs like generating a
repository list for a browser. For full reset some more works needs
to be done later.

Signed-off-by: Christian Thaeter <ct@pipapo.org>
---
 setup.c |   13 ++++++++++++-
 1 files changed, 12 insertions(+), 1 deletions(-)

diff --git a/setup.c b/setup.c
index adede16..770dc41 100644
--- a/setup.c
+++ b/setup.c
@@ -200,6 +200,8 @@ static const char *set_work_tree(const char *dir)
 
 	if (!getcwd(buffer, sizeof(buffer)))
 		die ("Could not get the current working directory");
+	if (git_work_tree_cfg)
+		free (git_work_tree_cfg);
 	git_work_tree_cfg = xstrdup(buffer);
 	inside_work_tree = 1;
 
@@ -329,6 +331,8 @@ const char *setup_git_directory_gently(int *nongit_ok)
 	inside_git_dir = 0;
 	if (!work_tree_env)
 		inside_work_tree = 1;
+	if (git_work_tree_cfg)
+		free (git_work_tree_cfg);
 	git_work_tree_cfg = xstrndup(cwd, offset);
 	if (check_repository_format_gently(nongit_ok))
 		return NULL;
@@ -387,7 +391,14 @@ int check_repository_format(void)
 
 const char *setup_git_directory(void)
 {
-	const char *retval = setup_git_directory_gently(NULL);
+	const char *retval;
+
+	inside_git_dir = -1;
+	inside_work_tree = -1;
+	git_invalidate_cached_refs();
+	reset_packed_git();
+	reset_git_work_tree();
+	retval = setup_git_directory_gently(NULL);
 
 	/* If the work tree is not the default one, recompute prefix */
 	if (inside_work_tree < 0) {
-- 
1.5.3.7

Re: [PATCH 2/5] First step, making setup (somewhat) reentrant

[Johannes Schindelin]

Hi,

[I assume that you mean this series post-1.5.4]

On Mon, 7 Jan 2008, Christian Thaeter wrote:

> diff --git a/environment.c b/environment.c
> index 18a1c4e..492d87c 100644
> --- a/environment.c
> +++ b/environment.c
> @@ -38,31 +38,48 @@ int auto_crlf = 0;	/* 1: both ways, -1: only when adding git objects */
>  unsigned whitespace_rule_cfg = WS_DEFAULT_RULE;
>  
>  /* This is set by setup_git_dir_gently() and/or git_default_config() */
> -char *git_work_tree_cfg;
> -static const char *work_tree;
> +char *git_work_tree_cfg = NULL;
> +static char *work_tree = NULL;
> +static int work_tree_initialized = 0;

Global variables do not need initialisation, if all what you do is set 
them to NULL.  Therefore, most of this hunk is not necessary, and only 
distracts from what is really relevant, "work_tree_initialized = 0".

>  static void setup_git_env(void)
>  {
>  	git_dir = getenv(GIT_DIR_ENVIRONMENT);
>  	if (!git_dir)
>  		git_dir = DEFAULT_GIT_DIR_ENVIRONMENT;
> +	if (git_object_dir)
> +		free(git_object_dir);

Logically, this belongs into a "cleanup_git_env()" function, no?

>  	git_object_dir = getenv(DB_ENVIRONMENT);
> -	if (!git_object_dir) {
> +	if (git_object_dir) {
> +		git_object_dir = xstrdup(git_object_dir);
> +	}

Are you sure that you want to keep the object directory, even if you want 
to initialise to a new repository?

Which brings me to a more fundamental question: what do you need reentrant 
setup_directory() for?  If it is just to allow multiple calls to that 
function for the _same_ repository, I say clean up your code.

Ciao,
Dscho

Re: [PATCH 2/5] First step, making setup (somewhat) reentrant

[Junio C Hamano]

Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:

> Which brings me to a more fundamental question: what do you need reentrant 
> setup_directory() for?  If it is just to allow multiple calls to that 
> function for the _same_ repository, I say clean up your code.

IIRC, he is writing a set-of-repositories browser.

But I think the right approach of cleaning up is to design a
layer of containers like we did for "the_index" libification
[*1*].  We would probably want:

 * a container that abstracts accesses to a single repository
   that includes:

    - set of refs;

    - object database and object hash (because different
      repositories can have different notion of who is parent of
      whom);

 * a container that abstracts accesses to a state of the index
   (we already have this -- the_index).

 * a container that abstracts a manipulation state in a single
   repository, that points at the first one but somehow knows
   what in-core objects are rewritten during its operation
   (e.g. history simplification rewrites parents list in in-core
   commit objects).

and a relatively low impact approach to transition to such a
scheme would be to use "the_index" libification as the model.
I.e. make the real API to take an explicit container as one of
their parameter, but define the default container to work on for
each level, and express the existing API in terms of the updated
API that works on the default container.


[Reference]

  228e94f93570b580da388069900c56b813c91953 (Move index-related
  variables into a structure.)

  4aab5b46f44a7ba860e07a52be506b7b57b2bd5f (Make read-cache.c
  "the_index" free.)

Re: [PATCH 2/5] First step, making setup (somewhat) reentrant

[Johannes Schindelin]

Hi,

On Mon, 7 Jan 2008, Junio C Hamano wrote:

> Johannes Schindelin <Johannes.Schindelin@gmx.de> writes:
> 
> > Which brings me to a more fundamental question: what do you need 
> > reentrant setup_directory() for?  If it is just to allow multiple 
> > calls to that function for the _same_ repository, I say clean up your 
> > code.
> 
> IIRC, he is writing a set-of-repositories browser.
> 
> But I think the right approach of cleaning up is to design a
> layer of containers like we did for "the_index" libification

Yes, I concur. That was a nice abstraction without impacting existing code 
too much.

Ciao,
Dscho