[PATCH 0/6] Retain caches of submodule refs

[PATCH 0/6] Retain caches of submodule refs

[Michael Haggerty]

...and work towards storing refs hierarchically.

Currently, the refs for submodules are put into a cache when accessed
but the cache is never reused.  Whenever the refs for a submodule are
accessed, the cache is cleared and refilled, even if the submodule
cache already contains data for that submodule.  Essentially, the
submodule cache only controls the lifetime of the data structures.
The main module is currently stored in a separate cache that is reused
properly.

This patch series institutes proper caching of submodule refs:
maintain a linked list of caches for each submodule, and add a new
entry whenever the refs for a new submodule are accessed.  Also store
the cache for the main project in the same linked list for uniformity.

This change accomplishes two things:

* Proper caching of submodule refs.  I'm not sure whether this is a
  significant win by itself; it depends on the usage patterns and I'm
  not too familiar with how submodules are used.  But it seems pretty
  clear that this is an improvement on the old kludge.

* It is a first step towards storing refs hierarchically *within*
  modules.  My plan is to build out "struct cached_refs" into a
  hierarchical data structure mimicking the reference namespace
  hierarchy, with one cached_ref instance for each "directory" of
  refs.  Then (the real goal) change the code to only populate the
  parts of the cache hierarchy that are actually accessed.

I believe that this change is useful by itself, self-contained, and
ready to be committed.  I plan to build the hierarchical-refs changes
on top of it.  But if it is preferred, I can submit this series plus
the hierarchical-refs patches as a single patch series (once the
latter is done, which will still take some time).

This patch series applies on top of "next" rather than "master"
because it would otherwise conflict with the changes made by
js/ref-namespaces.

I am on vacation and don't know when I will have internet access, so
please don't be offended if I don't respond quickly to feedback.

Michael Haggerty (6):
  Extract a function clear_cached_refs()
  Access reference caches only through new function get_cached_refs().
  Change the signature of read_packed_refs()
  Allocate cached_refs objects dynamically
  Store the submodule name in struct cached_refs.
  Retain caches of submodule refs

 refs.c |  106 ++++++++++++++++++++++++++++++++++++++++++++--------------------
 1 files changed, 73 insertions(+), 33 deletions(-)

-- 
1.7.6.8.gd2879

[PATCH 1/6] Extract a function clear_cached_refs()

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    9 ++++++---
 1 files changed, 6 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 6f313a9..b0c8308 100644
--- a/refs.c
+++ b/refs.c
@@ -171,10 +171,8 @@ static void free_ref_list(struct ref_list *list)
 	}
 }
 
-static void invalidate_cached_refs(void)
+static void clear_cached_refs(struct cached_refs *ca)
 {
-	struct cached_refs *ca = &cached_refs;
-
 	if (ca->did_loose && ca->loose)
 		free_ref_list(ca->loose);
 	if (ca->did_packed && ca->packed)
@@ -183,6 +181,11 @@ static void invalidate_cached_refs(void)
 	ca->did_loose = ca->did_packed = 0;
 }
 
+static void invalidate_cached_refs(void)
+{
+	clear_cached_refs(&cached_refs);
+}
+
 static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
 {
 	struct ref_list *list = NULL;
-- 
1.7.6.8.gd2879

[PATCH 2/6] Access reference caches only through new function get_cached_refs().

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   54 +++++++++++++++++++++++++++++++-----------------------
 1 files changed, 31 insertions(+), 23 deletions(-)

diff --git a/refs.c b/refs.c
index b0c8308..89840d7 100644
--- a/refs.c
+++ b/refs.c
@@ -181,9 +181,26 @@ static void clear_cached_refs(struct cached_refs *ca)
 	ca->did_loose = ca->did_packed = 0;
 }
 
+/*
+ * Return a pointer to a cached_refs for the specified submodule. For
+ * the main repository, use submodule==NULL. The returned structure
+ * will be allocated and initialized but not necessarily populated; it
+ * should not be freed.
+ */
+static struct cached_refs *get_cached_refs(const char *submodule)
+{
+	if (! submodule)
+		return &cached_refs;
+	else {
+		/* For now, don't reuse the refs cache for submodules. */
+		clear_cached_refs(&submodule_refs);
+		return &submodule_refs;
+	}
+}
+
 static void invalidate_cached_refs(void)
 {
-	clear_cached_refs(&cached_refs);
+	clear_cached_refs(get_cached_refs(NULL));
 }
 
 static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
@@ -234,19 +251,14 @@ void clear_extra_refs(void)
 
 static struct ref_list *get_packed_refs(const char *submodule)
 {
-	const char *packed_refs_file;
-	struct cached_refs *refs;
+	struct cached_refs *refs = get_cached_refs(submodule);
 
-	if (submodule) {
-		packed_refs_file = git_path_submodule(submodule, "packed-refs");
-		refs = &submodule_refs;
-		free_ref_list(refs->packed);
-	} else {
-		packed_refs_file = git_path("packed-refs");
-		refs = &cached_refs;
-	}
-
-	if (!refs->did_packed || submodule) {
+	if (!refs->did_packed) {
+		const char *packed_refs_file;
+		if (submodule)
+			packed_refs_file = git_path_submodule(submodule, "packed-refs");
+		else
+			packed_refs_file = git_path("packed-refs");
 		FILE *f = fopen(packed_refs_file, "r");
 		refs->packed = NULL;
 		if (f) {
@@ -361,17 +373,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
 
 static struct ref_list *get_loose_refs(const char *submodule)
 {
-	if (submodule) {
-		free_ref_list(submodule_refs.loose);
-		submodule_refs.loose = get_ref_dir(submodule, "refs", NULL);
-		return submodule_refs.loose;
-	}
+	struct cached_refs *refs = get_cached_refs(submodule);
 
-	if (!cached_refs.did_loose) {
-		cached_refs.loose = get_ref_dir(NULL, "refs", NULL);
-		cached_refs.did_loose = 1;
+	if (!refs->did_loose) {
+		refs->loose = get_ref_dir(submodule, "refs", NULL);
+		refs->did_loose = 1;
 	}
-	return cached_refs.loose;
+	return refs->loose;
 }
 
 /* We allow "recursive" symbolic refs. Only within reason, though */
-- 
1.7.6.8.gd2879

Re: [PATCH 2/6] Access reference caches only through new function get_cached_refs().

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c |   54 +++++++++++++++++++++++++++++++-----------------------
>  1 files changed, 31 insertions(+), 23 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index b0c8308..89840d7 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -181,9 +181,26 @@ static void clear_cached_refs(struct cached_refs *ca)
>  	ca->did_loose = ca->did_packed = 0;
>  }
>  
> +/*
> + * Return a pointer to a cached_refs for the specified submodule. For
> + * the main repository, use submodule==NULL. The returned structure
> + * will be allocated and initialized but not necessarily populated; it
> + * should not be freed.
> + */
> +static struct cached_refs *get_cached_refs(const char *submodule)
> +{
> +	if (! submodule)

(style) lose the SP before "submodule".

> +		return &cached_refs;
> +	else {
> +		/* For now, don't reuse the refs cache for submodules. */
> +		clear_cached_refs(&submodule_refs);
> +		return &submodule_refs;
> +	}
> +}
> +
>  static void invalidate_cached_refs(void)
>  {
> -	clear_cached_refs(&cached_refs);
> +	clear_cached_refs(get_cached_refs(NULL));
>  }
>  
>  static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
> @@ -234,19 +251,14 @@ void clear_extra_refs(void)
>  
>  static struct ref_list *get_packed_refs(const char *submodule)
>  {
> -	const char *packed_refs_file;
> -	struct cached_refs *refs;
> +	struct cached_refs *refs = get_cached_refs(submodule);
>  
> -	if (submodule) {
> -		packed_refs_file = git_path_submodule(submodule, "packed-refs");
> -		refs = &submodule_refs;
> -		free_ref_list(refs->packed);
> -	} else {
> -		packed_refs_file = git_path("packed-refs");
> -		refs = &cached_refs;
> -	}
> -
> -	if (!refs->did_packed || submodule) {
> +	if (!refs->did_packed) {
> +		const char *packed_refs_file;
> +		if (submodule)
> +			packed_refs_file = git_path_submodule(submodule, "packed-refs");
> +		else
> +			packed_refs_file = git_path("packed-refs");
>  		FILE *f = fopen(packed_refs_file, "r");

decl-after-statement.

>  		refs->packed = NULL;
>  		if (f) {
> @@ -361,17 +373,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)
>  
>  static struct ref_list *get_loose_refs(const char *submodule)
>  {
> -	if (submodule) {
> -		free_ref_list(submodule_refs.loose);
> -		submodule_refs.loose = get_ref_dir(submodule, "refs", NULL);
> -		return submodule_refs.loose;
> -	}
> +	struct cached_refs *refs = get_cached_refs(submodule);
>  
> -	if (!cached_refs.did_loose) {
> -		cached_refs.loose = get_ref_dir(NULL, "refs", NULL);
> -		cached_refs.did_loose = 1;
> +	if (!refs->did_loose) {
> +		refs->loose = get_ref_dir(submodule, "refs", NULL);
> +		refs->did_loose = 1;
>  	}
> -	return cached_refs.loose;
> +	return refs->loose;
>  }
>  
>  /* We allow "recursive" symbolic refs. Only within reason, though */

Re: [PATCH 2/6] Access reference caches only through new function get_cached_refs().

[Michael Haggerty]

On 08/15/2011 12:12 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> +	if (! submodule)
> 
> (style) lose the SP before "submodule".

Will be fixed in re-roll.

>> -	if (!refs->did_packed || submodule) {
>> +	if (!refs->did_packed) {
>> +		const char *packed_refs_file;
>> +		if (submodule)
>> +			packed_refs_file = git_path_submodule(submodule, "packed-refs");
>> +		else
>> +			packed_refs_file = git_path("packed-refs");
>>  		FILE *f = fopen(packed_refs_file, "r");
> 
> decl-after-statement.

Will be fixed.

On 08/15/2011 12:21 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> +struct cached_refs *create_cached_refs()
>
> struct cached_refs *create_cached_refs(void)

Will be fixed.

Thanks for your feedback.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH 3/6] Change the signature of read_packed_refs()

[Michael Haggerty]

Change it to return a (struct ref_list *) instead of writing into
a cached_refs structure.  (This removes the need to create a
cached_refs structure in resolve_gitlink_packed_ref(), where it
is otherwise unneeded.)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 89840d7..e043555 100644
--- a/refs.c
+++ b/refs.c
@@ -203,7 +203,7 @@ static void invalidate_cached_refs(void)
 	clear_cached_refs(get_cached_refs(NULL));
 }
 
-static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
+static struct ref_list *read_packed_refs(FILE *f)
 {
 	struct ref_list *list = NULL;
 	struct ref_list *last = NULL;
@@ -235,7 +235,7 @@ static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)
 		    !get_sha1_hex(refline + 1, sha1))
 			hashcpy(last->peeled, sha1);
 	}
-	cached_refs->packed = sort_ref_list(list);
+	return sort_ref_list(list);
 }
 
 void add_extra_ref(const char *name, const unsigned char *sha1, int flag)
@@ -262,7 +262,7 @@ static struct ref_list *get_packed_refs(const char *submodule)
 		FILE *f = fopen(packed_refs_file, "r");
 		refs->packed = NULL;
 		if (f) {
-			read_packed_refs(f, refs);
+			refs->packed = read_packed_refs(f);
 			fclose(f);
 		}
 		refs->did_packed = 1;
@@ -389,7 +389,7 @@ static struct ref_list *get_loose_refs(const char *submodule)
 static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refname, unsigned char *result)
 {
 	FILE *f;
-	struct cached_refs refs;
+	struct ref_list *packed_refs;
 	struct ref_list *ref;
 	int retval;
 
@@ -397,9 +397,9 @@ static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refna
 	f = fopen(name, "r");
 	if (!f)
 		return -1;
-	read_packed_refs(f, &refs);
+	packed_refs = read_packed_refs(f);
 	fclose(f);
-	ref = refs.packed;
+	ref = packed_refs;
 	retval = -1;
 	while (ref) {
 		if (!strcmp(ref->name, refname)) {
@@ -409,7 +409,7 @@ static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refna
 		}
 		ref = ref->next;
 	}
-	free_ref_list(refs.packed);
+	free_ref_list(packed_refs);
 	return retval;
 }
 
-- 
1.7.6.8.gd2879

[PATCH 4/6] Allocate cached_refs objects dynamically

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   28 +++++++++++++++++++++-------
 1 files changed, 21 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index e043555..102ed03 100644
--- a/refs.c
+++ b/refs.c
@@ -157,7 +157,7 @@ static struct cached_refs {
 	char did_packed;
 	struct ref_list *loose;
 	struct ref_list *packed;
-} cached_refs, submodule_refs;
+} *cached_refs, *submodule_refs;
 static struct ref_list *current_ref;
 
 static struct ref_list *extra_refs;
@@ -181,6 +181,15 @@ static void clear_cached_refs(struct cached_refs *ca)
 	ca->did_loose = ca->did_packed = 0;
 }
 
+struct cached_refs *create_cached_refs()
+{
+	struct cached_refs *refs;
+	refs = xmalloc(sizeof(struct cached_refs));
+	refs->did_loose = refs->did_packed = 0;
+	refs->loose = refs->packed = NULL;
+	return refs;
+}
+
 /*
  * Return a pointer to a cached_refs for the specified submodule. For
  * the main repository, use submodule==NULL. The returned structure
@@ -189,12 +198,17 @@ static void clear_cached_refs(struct cached_refs *ca)
  */
 static struct cached_refs *get_cached_refs(const char *submodule)
 {
-	if (! submodule)
-		return &cached_refs;
-	else {
-		/* For now, don't reuse the refs cache for submodules. */
-		clear_cached_refs(&submodule_refs);
-		return &submodule_refs;
+	if (! submodule) {
+		if (!cached_refs)
+			cached_refs = create_cached_refs();
+		return cached_refs;
+	} else {
+		if (!submodule_refs)
+			submodule_refs = create_cached_refs();
+		else
+			/* For now, don't reuse the refs cache for submodules. */
+			clear_cached_refs(submodule_refs);
+		return submodule_refs;
 	}
 }
 
-- 
1.7.6.8.gd2879

Re: [PATCH 4/6] Allocate cached_refs objects dynamically

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c |   28 +++++++++++++++++++++-------
>  1 files changed, 21 insertions(+), 7 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index e043555..102ed03 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -157,7 +157,7 @@ static struct cached_refs {
>  	char did_packed;
>  	struct ref_list *loose;
>  	struct ref_list *packed;
> -} cached_refs, submodule_refs;
> +} *cached_refs, *submodule_refs;
>  static struct ref_list *current_ref;
>  
>  static struct ref_list *extra_refs;
> @@ -181,6 +181,15 @@ static void clear_cached_refs(struct cached_refs *ca)
>  	ca->did_loose = ca->did_packed = 0;
>  }
>  
> +struct cached_refs *create_cached_refs()

struct cached_refs *create_cached_refs(void)

[PATCH 5/6] Store the submodule name in struct cached_refs.

[Michael Haggerty]

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   15 +++++++++++----
 1 files changed, 11 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 102ed03..8d1055d 100644
--- a/refs.c
+++ b/refs.c
@@ -157,6 +157,8 @@ static struct cached_refs {
 	char did_packed;
 	struct ref_list *loose;
 	struct ref_list *packed;
+	/* The submodule name, or "" for the main repo. */
+	char name[FLEX_ARRAY];
 } *cached_refs, *submodule_refs;
 static struct ref_list *current_ref;
 
@@ -181,12 +183,17 @@ static void clear_cached_refs(struct cached_refs *ca)
 	ca->did_loose = ca->did_packed = 0;
 }
 
-struct cached_refs *create_cached_refs()
+struct cached_refs *create_cached_refs(const char *submodule)
 {
+	int len;
 	struct cached_refs *refs;
-	refs = xmalloc(sizeof(struct cached_refs));
+	if (!submodule)
+		submodule = "";
+	len = strlen(submodule) + 1;
+	refs = xmalloc(sizeof(struct cached_refs) + len);
 	refs->did_loose = refs->did_packed = 0;
 	refs->loose = refs->packed = NULL;
+	memcpy(refs->name, submodule, len);
 	return refs;
 }
 
@@ -200,11 +207,11 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 {
 	if (! submodule) {
 		if (!cached_refs)
-			cached_refs = create_cached_refs();
+			cached_refs = create_cached_refs(submodule);
 		return cached_refs;
 	} else {
 		if (!submodule_refs)
-			submodule_refs = create_cached_refs();
+			submodule_refs = create_cached_refs(submodule);
 		else
 			/* For now, don't reuse the refs cache for submodules. */
 			clear_cached_refs(submodule_refs);
-- 
1.7.6.8.gd2879

[PATCH 6/6] Retain caches of submodule refs

[Michael Haggerty]

Instead of keeping track of one cache for refs in the main repo and
another single cache shared among submodules, keep a linked list of
cached_refs objects, one for each module/submodule. Change
invalidate_cached_refs() to invalidate all caches. (Previously, it
only invalidated the cache of the main repo because the submodule
caches were not reused anyway.)

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   34 +++++++++++++++++++++-------------
 1 files changed, 21 insertions(+), 13 deletions(-)

diff --git a/refs.c b/refs.c
index 8d1055d..f02cf94 100644
--- a/refs.c
+++ b/refs.c
@@ -153,13 +153,15 @@ static struct ref_list *sort_ref_list(struct ref_list *list)
  * when doing a full libification.
  */
 static struct cached_refs {
+	struct cached_refs *next;
 	char did_loose;
 	char did_packed;
 	struct ref_list *loose;
 	struct ref_list *packed;
 	/* The submodule name, or "" for the main repo. */
 	char name[FLEX_ARRAY];
-} *cached_refs, *submodule_refs;
+} *cached_refs;
+
 static struct ref_list *current_ref;
 
 static struct ref_list *extra_refs;
@@ -191,6 +193,7 @@ struct cached_refs *create_cached_refs(const char *submodule)
 		submodule = "";
 	len = strlen(submodule) + 1;
 	refs = xmalloc(sizeof(struct cached_refs) + len);
+	refs->next = NULL;
 	refs->did_loose = refs->did_packed = 0;
 	refs->loose = refs->packed = NULL;
 	memcpy(refs->name, submodule, len);
@@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)
  */
 static struct cached_refs *get_cached_refs(const char *submodule)
 {
-	if (! submodule) {
-		if (!cached_refs)
-			cached_refs = create_cached_refs(submodule);
-		return cached_refs;
-	} else {
-		if (!submodule_refs)
-			submodule_refs = create_cached_refs(submodule);
-		else
-			/* For now, don't reuse the refs cache for submodules. */
-			clear_cached_refs(submodule_refs);
-		return submodule_refs;
+	struct cached_refs *refs = cached_refs;
+	if (! submodule)
+		submodule = "";
+	while (refs) {
+		if (!strcmp(submodule, refs->name))
+			return refs;
+		refs = refs->next;
 	}
+
+	refs = create_cached_refs(submodule);
+	refs->next = cached_refs;
+	cached_refs = refs;
+	return refs;
 }
 
 static void invalidate_cached_refs(void)
 {
-	clear_cached_refs(get_cached_refs(NULL));
+	struct cached_refs *refs = cached_refs;
+	while (refs) {
+		clear_cached_refs(refs);
+		refs = refs->next;
+	}
 }
 
 static struct ref_list *read_packed_refs(FILE *f)
-- 
1.7.6.8.gd2879

Re: [PATCH 6/6] Retain caches of submodule refs

[Heiko Voigt]

Hi,

On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:
> diff --git a/refs.c b/refs.c
> index 8d1055d..f02cf94 100644
> --- a/refs.c
> +++ b/refs.c

...

> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)
>   */
>  static struct cached_refs *get_cached_refs(const char *submodule)
>  {
> -	if (! submodule) {
> -		if (!cached_refs)
> -			cached_refs = create_cached_refs(submodule);
> -		return cached_refs;
> -	} else {
> -		if (!submodule_refs)
> -			submodule_refs = create_cached_refs(submodule);
> -		else
> -			/* For now, don't reuse the refs cache for submodules. */
> -			clear_cached_refs(submodule_refs);
> -		return submodule_refs;
> +	struct cached_refs *refs = cached_refs;
> +	if (! submodule)
> +		submodule = "";

Maybe instead of searching for the main refs store a pointer to them
locally so you can immediately return here. That will keep the
performance when requesting the main refs the same.

If I see it correctly you are always prepending to the linked list and
in case many submodules get cached this could slow down the iteration
over the refs of the main repository.

> +	while (refs) {
> +		if (!strcmp(submodule, refs->name))
> +			return refs;
> +		refs = refs->next;
>  	}
> +
> +	refs = create_cached_refs(submodule);
> +	refs->next = cached_refs;
> +	cached_refs = refs;
> +	return refs;
>  }

...

Cheers Heiko

Re: [PATCH 6/6] Retain caches of submodule refs

[Michael Haggerty]

On 08/13/2011 02:54 PM, Heiko Voigt wrote:
> On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:
>> diff --git a/refs.c b/refs.c
>> index 8d1055d..f02cf94 100644
>> --- a/refs.c
>> +++ b/refs.c
> 
> ...
> 
>> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)
>>   */
>>  static struct cached_refs *get_cached_refs(const char *submodule)
>>  {
>> -	if (! submodule) {
>> -		if (!cached_refs)
>> -			cached_refs = create_cached_refs(submodule);
>> -		return cached_refs;
>> -	} else {
>> -		if (!submodule_refs)
>> -			submodule_refs = create_cached_refs(submodule);
>> -		else
>> -			/* For now, don't reuse the refs cache for submodules. */
>> -			clear_cached_refs(submodule_refs);
>> -		return submodule_refs;
>> +	struct cached_refs *refs = cached_refs;
>> +	if (! submodule)
>> +		submodule = "";
> 
> Maybe instead of searching for the main refs store a pointer to them
> locally so you can immediately return here. That will keep the
> performance when requesting the main refs the same.

I am assuming that the number of submodules will be small compared to
the number of refs in a typical submodule.  Therefore, given that the
refs are stored in a big linked list and that the only thing that can
realistically be done with the list is to iterate through it, the cost
of finding the reference cache for a specific (sub-)module should be
negligible compared to the cost of the iteration over refs in the cache.
 Treating the main module like the other submodules makes the code
simpler, so I would prefer to leave it this way.

If iteration over refs ever becomes a bottleneck, then optimization of
the storage of refs within a (sub-)module would be a bigger win than
special-casing the main module.  And that is what I would like to work
towards.

> If I see it correctly you are always prepending to the linked list 

This is true.

>                                                                    and
> in case many submodules get cached this could slow down the iteration
> over the refs of the main repository.

Is this a realistic concern?  Remember that the extra search over
submodules only occurs once for each scan through the list of references.

I wrote an additional patch that moves the least-recently accessed
module to the front of the list.  But I doubt that the savings justify
the 10-odd extra lines of code, so I kept it to myself.  Please note
that this approach gives pessimal performance (2x slower) if the
submodules are iterated over repeatedly.  If you would like me to add
this patch to the patch series, please let me know.

Long-term, it would be better to implement a "struct submodule" and use
it to hold submodule-specific data like the ref cache.  Then users could
hold on to the "struct submodule *" and pass it, rather than the
submodule name, to the functions that need it.  But this goes beyond the
scope of what I want to change now, especially since I have no
experience even working with submodules.

Summary: I hope I have convinced you that the extra overhead is
negligible and does not justify additional code.  But if you insist on
more emphasis on performance (or obviously if you have numbers to back
up your concerns) then I would be willing to put more work into it.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 6/6] Retain caches of submodule refs

[Heiko Voigt]

On Wed, Aug 24, 2011 at 10:17:24AM +0200, Michael Haggerty wrote:
> On 08/13/2011 02:54 PM, Heiko Voigt wrote:
> > On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:
> >> diff --git a/refs.c b/refs.c
> >> index 8d1055d..f02cf94 100644
> >> --- a/refs.c
> >> +++ b/refs.c
> > 
> > ...
> > 
> >> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)
> >>   */
> >>  static struct cached_refs *get_cached_refs(const char *submodule)
> >>  {
> >> -	if (! submodule) {
> >> -		if (!cached_refs)
> >> -			cached_refs = create_cached_refs(submodule);
> >> -		return cached_refs;
> >> -	} else {
> >> -		if (!submodule_refs)
> >> -			submodule_refs = create_cached_refs(submodule);
> >> -		else
> >> -			/* For now, don't reuse the refs cache for submodules. */
> >> -			clear_cached_refs(submodule_refs);
> >> -		return submodule_refs;
> >> +	struct cached_refs *refs = cached_refs;
> >> +	if (! submodule)
> >> +		submodule = "";
> > 
> > Maybe instead of searching for the main refs store a pointer to them
> > locally so you can immediately return here. That will keep the
> > performance when requesting the main refs the same.
> 
> I am assuming that the number of submodules will be small compared to
> the number of refs in a typical submodule.  Therefore, given that the
> refs are stored in a big linked list and that the only thing that can
> realistically be done with the list is to iterate through it, the cost
> of finding the reference cache for a specific (sub-)module should be
> negligible compared to the cost of the iteration over refs in the cache.
>  Treating the main module like the other submodules makes the code
> simpler, so I would prefer to leave it this way.

I just thought I mention it since it is a change and you are already
special casing the main module with this if(!submodule) but you are
probably right and in most cases (many refs and few submodules) this
change is negligible.

> If iteration over refs ever becomes a bottleneck, then optimization of
> the storage of refs within a (sub-)module would be a bigger win than
> special-casing the main module.  And that is what I would like to work
> towards.
> 
> > If I see it correctly you are always prepending to the linked list 
> 
> This is true.
> 
> >                                                                    and
> > in case many submodules get cached this could slow down the iteration
> > over the refs of the main repository.
> 
> Is this a realistic concern?  Remember that the extra search over
> submodules only occurs once for each scan through the list of references.
> 
> I wrote an additional patch that moves the least-recently accessed
> module to the front of the list.  But I doubt that the savings justify
> the 10-odd extra lines of code, so I kept it to myself.  Please note
> that this approach gives pessimal performance (2x slower) if the
> submodules are iterated over repeatedly.  If you would like me to add
> this patch to the patch series, please let me know.

No I don't think this is necessary.

> Long-term, it would be better to implement a "struct submodule" and use
> it to hold submodule-specific data like the ref cache.  Then users could
> hold on to the "struct submodule *" and pass it, rather than the
> submodule name, to the functions that need it.  But this goes beyond the
> scope of what I want to change now, especially since I have no
> experience even working with submodules.
> 
> Summary: I hope I have convinced you that the extra overhead is
> negligible and does not justify additional code.  But if you insist on
> more emphasis on performance (or obviously if you have numbers to back
> up your concerns) then I would be willing to put more work into it.

Yes I am also fine with the current state. I do not have a strong
opinion about this. This already adds enough improvements of the
submodule ref caching which looks a lot nicer than before.

Cheers Heiko

Re: [PATCH 6/6] Retain caches of submodule refs

[Junio C Hamano]

All the changes except for this one made sense to me, but I am not sure
about this one. How often do we look into different submodule refs in the
same process over and over again?

Re: [PATCH 6/6] Retain caches of submodule refs

[Michael Haggerty]

On 08/17/2011 12:45 AM, Junio C Hamano wrote:
> All the changes except for this one made sense to me, but I am not sure
> about this one. How often do we look into different submodule refs in the
> same process over and over again?

As I've mentioned, I am not very familiar with submodules and I don't
know what actions in submodules can be triggered from the top-level
project.  Here is the only code path that I can find in the current git
code that causes submodule refs to be read at all:

setup_revisions() with opt->submodule is set,
    which can only happen when called via merge_submodule(),
    which is called from merge_file() in merge-recursive.c,
    which is called by process_renames() and merge_content(),
    which are both ultimately called from merge_trees().

I don't know how often this can happen during the lifetime of one process.

The cost of retaining the submodule ref caches is roughly 100 bytes per
reference of memory.  Another side effect is that the submodule caches
never have to be cleaned up; this saves a list walk and one free() per
cached reference if the refs for more than one submodule are accessed.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 6/6] Retain caches of submodule refs

[Michael Haggerty]

On 08/17/2011 12:45 AM, Junio C Hamano wrote:
> All the changes except for this one made sense to me, but I am not sure
> about this one. How often do we look into different submodule refs in the
> same process over and over again?

I am having pangs of uncertainty about this patch.

Previous to this patch, the submodule reference cache was only used for
the duration of one call to do_for_each_ref().  (It was not *discarded*
until later, but the old cache was never reused.)  Therefore, the
submodule reference cache was implicitly invalidated between successive
uses.

After this change, submodule ref caches are invalidated whenever
invalidate_cached_refs() is called.  But this function is static, and it
is only called when main-module refs are changed.

AFAIK there is no way within refs.c to add, modify, or delete a
submodule reference.  But if other code modifies submodule references
directly, then the submodule ref cache in refs.c would become stale.
Moreover, there is currently no API for invalidating the cache.

So I think I need help from a submodule guru (Heiko?) who can tell me
what is done with submodule references and whether they might be
modified while a git process is executing in the main module.  If so,
then either this patch has to be withdrawn, or more work has to be put
in to make such code invalidate the submodule reference cache.

Sorry for the oversight, but I forgot that not all code necessarily uses
the refs.c API when dealing with references (a regrettable situation, BTW).

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH 6/6] Retain caches of submodule refs

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> So I think I need help from a submodule guru (Heiko?) who can tell me
> what is done with submodule references and whether they might be
> modified while a git process is executing in the main module.  If so,
> then either this patch has to be withdrawn, or more work has to be put
> in to make such code invalidate the submodule reference cache.
>
> Sorry for the oversight, but I forgot that not all code necessarily uses
> the refs.c API when dealing with references (a regrettable situation, BTW).

In the longer term, I would agree with you that we very much prefer all
the ref accesses to go through the refs API, and I also foresee that the
submodule support would need to become more aware of the status of refs in
checked out submodules. For example, a recursive "submodule update" may
want to inspect the refs in a submodule directory, compare them with the
commit bound in the superproject tree for the submodule path, and decide
to spawn a "fetch && checkout $branch" in there. The same process then may
want to run "status" at the superproject level to show the result, which
in turn would inspect the relationship between the commit bound in the
superproject tree and the commit at the HEAD of the submodule directory.

So let's not rip out the commit but instead give submodule machinery an
explicit way to say "We do not know the current status of the refs in this
submodule anymore".

Thanks.

[PATCH 0/2] Provide API to invalidate refs cache

[Michael Haggerty]

I am happy to provide the API; see the following patches.

But I won't have time to figure out who, outside of refs.c, has to
*call* invalidate_cached_refs().  The candidates that I know off the
top of my head are git-clone, git-submodule, and git-pack-refs.  It
would be great if experts in those areas would insert calls to
invalidate_cached_refs() where needed.

Even better would be if the meddlesome code were changed to use the
refs API.  I'd be happy to help expanding the refs API if needed to
accommodate your needs.

Michael Haggerty (2):
  invalidate_cached_refs(): take the submodule as parameter
  invalidate_cached_refs(): expose this function in refs API

 refs.c |   12 ++++--------
 refs.h |    8 ++++++++
 2 files changed, 12 insertions(+), 8 deletions(-)

-- 
1.7.7.rc2

[PATCH 1/2] invalidate_cached_refs(): take the submodule as parameter

[Michael Haggerty]

Instead of invalidating the refs cache on an all-or-nothing basis,
allow the cache for individual submodules to be invalidated.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 2cb93e2..49b73c4 100644
--- a/refs.c
+++ b/refs.c
@@ -223,13 +223,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_cached_refs(void)
+static void invalidate_cached_refs(const char *submodule)
 {
-	struct cached_refs *refs = cached_refs;
-	while (refs) {
-		clear_cached_refs(refs);
-		refs = refs->next;
-	}
+	clear_cached_refs(get_cached_refs(submodule));
 }
 
 static struct ref_list *read_packed_refs(FILE *f)
@@ -1212,7 +1208,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(refname);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_cached_refs();
+	invalidate_cached_refs(NULL);
 	unlock_ref(lock);
 	return ret;
 }
@@ -1511,7 +1507,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_cached_refs();
+	invalidate_cached_refs(NULL);
 	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.7.7.rc2

[PATCH 2/2] invalidate_cached_refs(): expose this function in refs API

[Michael Haggerty]

Make invalidate_cached_refs() an official part of the refs API.  It is
currently a fact of life that code outside of refs.c mucks about with
references.  This change gives such code a way of informing the refs
module that it should no longer trust its cache.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    2 +-
 refs.h |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 49b73c4..0483ecc 100644
--- a/refs.c
+++ b/refs.c
@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_cached_refs(const char *submodule)
+void invalidate_cached_refs(const char *submodule)
 {
 	clear_cached_refs(get_cached_refs(submodule));
 }
diff --git a/refs.h b/refs.h
index 5de06e5..63dc68c 100644
--- a/refs.h
+++ b/refs.h
@@ -77,6 +77,14 @@ extern void unlock_ref(struct ref_lock *lock);
 /** Writes sha1 into the ref specified by the lock. **/
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
+/*
+ * Invalidate the reference cache for the specified submodule.  Use
+ * submodule=NULL to invalidate the cache for the main module.  This
+ * function must be called if references are changed via a mechanism
+ * other than the refs API.
+ */
+extern void invalidate_cached_refs(const char *submodule);
+
 /** Setup reflog before using. **/
 int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
 
-- 
1.7.7.rc2

[PATCH v2 0/7] Provide API to invalidate refs cache

[Michael Haggerty]

Sorry for superseding my own patch series, but I prefer this patch
series to the one that I submitted earlier today:

1. An API function deserves a more carefully-selected name:
   invalidate_ref_cache().

2. It gives me a chance to submit the first "bite" of my scalable-refs
   changes.

These patches apply on top of mh/iterate-refs, which is in next but
not in master.

This patch series provides an API for external code to invalidate the
ref cache that is used internally to refs.c.  It also allows code
*within* refs.c to invalidate only the packed or only the loose refs
for a module/submodule.

IMPORTANT:

I won't myself have time to figure out who, outside of refs.c, has to
*call* invalidate_ref_cache().  The candidates that I know off the top
of my head are git-clone, git-submodule, and git-pack-refs.  It would
be great if experts in those areas would insert calls to
invalidate_ref_cache() where needed.

Even better would be if the meddlesome code were changed to use the
refs API.  I'd be happy to help expanding the refs API if needed to
accommodate your needs.

This is why the API for invalidating only packed or loose refs is
private.  After code outside refs.c is changed to use the refs API, it
will get the optimal behavior for free (and at that time
invalidate_ref_cache() can be removed again).

Michael Haggerty (7):
  invalidate_ref_cache(): rename function from invalidate_cached_refs()
  invalidate_ref_cache(): take the submodule as parameter
  invalidate_ref_cache(): expose this function in refs API
  clear_cached_refs(): rename parameter
  clear_cached_refs(): extract two new functions
  write_ref_sha1(): only invalidate the loose ref cache
  clear_cached_refs(): inline function

 refs.c |   34 +++++++++++++++++++---------------
 refs.h |    8 ++++++++
 2 files changed, 27 insertions(+), 15 deletions(-)

-- 
1.7.7.rc2

[PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Michael Haggerty]

It is the cache that is being invalidated, not the references.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 2cb93e2..56e4254 100644
--- a/refs.c
+++ b/refs.c
@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_cached_refs(void)
+static void invalidate_ref_cache(void)
 {
 	struct cached_refs *refs = cached_refs;
 	while (refs) {
@@ -1212,7 +1212,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(refname);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_cached_refs();
+	invalidate_ref_cache();
 	unlock_ref(lock);
 	return ret;
 }
@@ -1511,7 +1511,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_cached_refs();
+	invalidate_ref_cache();
 	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.7.7.rc2

Re: [PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It is the cache that is being invalidated, not the references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

Although I think one can say "ref cache is the container for cached refs"
and invalidating the "ref cache" as the container and invalidating the
"cached refs" as a collection mean essentially the same thing, probably
the new name makes more sense.

>  refs.c |    6 +++---
>  1 files changed, 3 insertions(+), 3 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 2cb93e2..56e4254 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
>  	return refs;
>  }
>  
> -static void invalidate_cached_refs(void)
> +static void invalidate_ref_cache(void)
>  {
>  	struct cached_refs *refs = cached_refs;
>  	while (refs) {
> @@ -1212,7 +1212,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
>  	ret |= repack_without_ref(refname);
>  
>  	unlink_or_warn(git_path("logs/%s", lock->ref_name));
> -	invalidate_cached_refs();
> +	invalidate_ref_cache();
>  	unlock_ref(lock);
>  	return ret;
>  }
> @@ -1511,7 +1511,7 @@ int write_ref_sha1(struct ref_lock *lock,
>  		unlock_ref(lock);
>  		return -1;
>  	}
> -	invalidate_cached_refs();
> +	invalidate_ref_cache();
>  	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)) {

Re: [PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Michael Haggerty]

On 10/11/2011 02:00 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> It is the cache that is being invalidated, not the references.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
> Although I think one can say "ref cache is the container for cached refs"
> and invalidating the "ref cache" as the container and invalidating the
> "cached refs" as a collection mean essentially the same thing, probably
> the new name makes more sense.

I certainly didn't mean to imply that the old name was incorrect.  I
just think that the new name removes a tiny bit of ambiguity.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v2 2/7] invalidate_ref_cache(): take the submodule as parameter

[Michael Haggerty]

Instead of invalidating the ref cache on an all-or-nothing basis,
allow the cache for individual submodules to be invalidated.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 56e4254..89b2a0e 100644
--- a/refs.c
+++ b/refs.c
@@ -223,13 +223,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_ref_cache(void)
+static void invalidate_ref_cache(const char *submodule)
 {
-	struct cached_refs *refs = cached_refs;
-	while (refs) {
-		clear_cached_refs(refs);
-		refs = refs->next;
-	}
+	clear_cached_refs(get_cached_refs(submodule));
 }
 
 static struct ref_list *read_packed_refs(FILE *f)
@@ -1212,7 +1208,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(refname);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_ref_cache();
+	invalidate_ref_cache(NULL);
 	unlock_ref(lock);
 	return ret;
 }
@@ -1511,7 +1507,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_ref_cache();
+	invalidate_ref_cache(NULL);
 	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.7.7.rc2

[PATCH v2 3/7] invalidate_ref_cache(): expose this function in refs API

[Michael Haggerty]

Make invalidate_ref_cache() an official part of the refs API.  It is
currently a fact of life that code outside of refs.c mucks about with
references.  This change gives such code a way of informing the refs
module that it should no longer trust its cache.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    2 +-
 refs.h |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 89b2a0e..fb46cf5 100644
--- a/refs.c
+++ b/refs.c
@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_ref_cache(const char *submodule)
+void invalidate_ref_cache(const char *submodule)
 {
 	clear_cached_refs(get_cached_refs(submodule));
 }
diff --git a/refs.h b/refs.h
index 5de06e5..3ddc4e4 100644
--- a/refs.h
+++ b/refs.h
@@ -77,6 +77,14 @@ extern void unlock_ref(struct ref_lock *lock);
 /** Writes sha1 into the ref specified by the lock. **/
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
+/*
+ * Invalidate the reference cache for the specified submodule.  Use
+ * submodule=NULL to invalidate the cache for the main module.  This
+ * function must be called if references are changed via a mechanism
+ * other than the refs API.
+ */
+extern void invalidate_ref_cache(const char *submodule);
+
 /** Setup reflog before using. **/
 int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
 
-- 
1.7.7.rc2

[PATCH v2 4/7] clear_cached_refs(): rename parameter

[Michael Haggerty]

...for consistency with the rest of this module.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   14 +++++++-------
 1 files changed, 7 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index fb46cf5..9f004ce 100644
--- a/refs.c
+++ b/refs.c
@@ -175,14 +175,14 @@ static void free_ref_list(struct ref_list *list)
 	}
 }
 
-static void clear_cached_refs(struct cached_refs *ca)
+static void clear_cached_refs(struct cached_refs *refs)
 {
-	if (ca->did_loose && ca->loose)
-		free_ref_list(ca->loose);
-	if (ca->did_packed && ca->packed)
-		free_ref_list(ca->packed);
-	ca->loose = ca->packed = NULL;
-	ca->did_loose = ca->did_packed = 0;
+	if (refs->did_loose && refs->loose)
+		free_ref_list(refs->loose);
+	if (refs->did_packed && refs->packed)
+		free_ref_list(refs->packed);
+	refs->loose = refs->packed = NULL;
+	refs->did_loose = refs->did_packed = 0;
 }
 
 static struct cached_refs *create_cached_refs(const char *submodule)
-- 
1.7.7.rc2

[PATCH v2 5/7] clear_cached_refs(): extract two new functions

[Michael Haggerty]

Extract two new functions from clear_cached_refs():
clear_loose_ref_cache() and clear_packed_ref_cache().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   22 +++++++++++++++++-----
 1 files changed, 17 insertions(+), 5 deletions(-)

diff --git a/refs.c b/refs.c
index 9f004ce..6e480ad 100644
--- a/refs.c
+++ b/refs.c
@@ -175,14 +175,26 @@ static void free_ref_list(struct ref_list *list)
 	}
 }
 
-static void clear_cached_refs(struct cached_refs *refs)
+static void clear_cached_packed_refs(struct cached_refs *refs)
 {
-	if (refs->did_loose && refs->loose)
-		free_ref_list(refs->loose);
 	if (refs->did_packed && refs->packed)
 		free_ref_list(refs->packed);
-	refs->loose = refs->packed = NULL;
-	refs->did_loose = refs->did_packed = 0;
+	refs->packed = NULL;
+	refs->did_packed = 0;
+}
+
+static void clear_cached_loose_refs(struct cached_refs *refs)
+{
+	if (refs->did_loose && refs->loose)
+		free_ref_list(refs->loose);
+	refs->loose = NULL;
+	refs->did_loose = 0;
+}
+
+static void clear_cached_refs(struct cached_refs *refs)
+{
+	clear_cached_packed_refs(refs);
+	clear_cached_loose_refs(refs);
 }
 
 static struct cached_refs *create_cached_refs(const char *submodule)
-- 
1.7.7.rc2

[PATCH v2 6/7] write_ref_sha1(): only invalidate the loose ref cache

[Michael Haggerty]

Since write_ref_sha1() can only write loose refs and cannot write
symbolic refs, there is no need for it to invalidate the packed ref
cache.

Suggested by: Martin Fick <mfick@codeaurora.org>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 6e480ad..01a5958 100644
--- a/refs.c
+++ b/refs.c
@@ -1519,7 +1519,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_ref_cache(NULL);
+	clear_cached_loose_refs(get_cached_refs(NULL));
 	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.7.7.rc2

[PATCH v2 7/7] clear_cached_refs(): inline function

[Michael Haggerty]

clear_cached_refs() was only called from one place, so inline it
there.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 01a5958..bf53189 100644
--- a/refs.c
+++ b/refs.c
@@ -191,12 +191,6 @@ static void clear_cached_loose_refs(struct cached_refs *refs)
 	refs->did_loose = 0;
 }
 
-static void clear_cached_refs(struct cached_refs *refs)
-{
-	clear_cached_packed_refs(refs);
-	clear_cached_loose_refs(refs);
-}
-
 static struct cached_refs *create_cached_refs(const char *submodule)
 {
 	int len;
@@ -237,7 +231,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 
 void invalidate_ref_cache(const char *submodule)
 {
-	clear_cached_refs(get_cached_refs(submodule));
+	struct cached_refs *refs = get_cached_refs(submodule);
+	clear_cached_packed_refs(refs);
+	clear_cached_loose_refs(refs);
 }
 
 static struct ref_list *read_packed_refs(FILE *f)
-- 
1.7.7.rc2

Re: [PATCH v2 0/7] Provide API to invalidate refs cache

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> These patches apply on top of mh/iterate-refs, which is in next but
> not in master.

Building your series on mh/iterate-refs would unfortunately make the
conflict resolution worse. It would have been better if this were based on
a merge between mh/iterate-refs and jp/get-ref-dir-unsorted (which already
has happened on 'master' as of fifteen minutes ago).

I could rebase your series, but it always is more error prone to have
somebody who is not the original author rebase a series than the original
author build for the intended base tree from the beginning.

Thanks.

Re: [PATCH v2 0/7] Provide API to invalidate refs cache

[Michael Haggerty]

On 10/11/2011 02:02 AM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> These patches apply on top of mh/iterate-refs, which is in next but
>> not in master.
> 
> Building your series on mh/iterate-refs would unfortunately make the
> conflict resolution worse. It would have been better if this were based on
> a merge between mh/iterate-refs and jp/get-ref-dir-unsorted (which already
> has happened on 'master' as of fifteen minutes ago).

AAAAaaaarrrgghh did that really have to happen?!?

> I could rebase your series, but it always is more error prone to have
> somebody who is not the original author rebase a series than the original
> author build for the intended base tree from the beginning.

I don't mind rebasing this little series on jp/get-ref-dir-unsorted.
But it's going to be an utter nightmare (as in, "can I even muster the
energy to do so much pointless work") to rebase my much bigger
hierarchical-refs series [1] onto jp/get-ref-dir-unsorted.  The latter
makes changes all over refs.c and changes several things at once
(separate ref_entry out of ref_list, change current_ref to a ref_entry*,
rename ref_list to ref_array, change data structure to array plus
rewrite all loops, change to binary search).  And
jp/get-ref-dir-unsorted includes a change that was inspired by my patch
series [2], so it is not like jp/get-ref-dir-unsorted was developed in
complete isolation from hierarchical-refs.

And this rebase will be work with no benefit, because my series includes
all of the improvements of jp/get-ref-dir-unsorted plus much more.  But
my change to the data structure is implemented in a different order and
following other improvements.  For example, I add a lot of comments,
change a lot of code to use the cached_refs data structure more
consistently, and accommodate partly-sorted lists by the time my patch
series includes everything that is in jp/get-ref-dir-unsorted.

Rebasing 78 patches is going to be a morass of clerical work.  Is there
any alternative?

Michael

PS: I see that some confusion might have been caused by one of my emails
[3], where I mistakenly approved of the merge of jp/get-ref-dir-unsorted
(meaning the "Don't sort ref_list too early" part) just before asking
that jp/get-ref-dir-unsorted not be merged (meaning the rest).  So maybe
I brought this whole mess down on my own head :-(

[1] branch hierarchical-refs on git://github.com/mhagger/git.git
[2] http://marc.info/?l=git&m=131740585620461&w=2
[3] http://marc.info/?l=git&m=131753257824405&w=2

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v2 0/7] Provide API to invalidate refs cache

[Julian Phillips]

On Tue, 11 Oct 2011 07:50:42 +0200, Michael Haggerty wrote:
> And this rebase will be work with no benefit, because my series 
> includes
> all of the improvements of jp/get-ref-dir-unsorted plus much more.  
> But
> my change to the data structure is implemented in a different order 
> and
> following other improvements.  For example, I add a lot of comments,
> change a lot of code to use the cached_refs data structure more
> consistently, and accommodate partly-sorted lists by the time my 
> patch
> series includes everything that is in jp/get-ref-dir-unsorted.
>
> Rebasing 78 patches is going to be a morass of clerical work.  Is 
> there
> any alternative?

If you create a new commit that reverts the problematic changes to 
refs.c with some suitable message about doing the same thing more 
piecemeal as the first change of a new series, and rebase your changes 
on top, you should be able to create a 79 patch series with little work. 
Dunno if that would acceptable for merging?

-- 
Julian

Re: [PATCH v2 0/7] Provide API to invalidate refs cache

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 10/11/2011 02:02 AM, Junio C Hamano wrote:
> ...
>> I could rebase your series, but it always is more error prone to have
>> somebody who is not the original author rebase a series than the original
>> author build for the intended base tree from the beginning.
>
> I don't mind rebasing this little series on jp/get-ref-dir-unsorted.
> ...
> Rebasing 78 patches is going to be a morass of clerical work.

I do not think it is "clerical" in the first place.

Realistically, I expect that a 50+ patch series that touch fairly an
important part of the system to take 2 cycles and a half before it hits a
released version, judging from our recent experience with the recursive
merge fix-up series.

When we already have a patch that has been discussed well enough on the
list to fix somebody's real world problem, can we afford to block it and
give an exclusive write lock to part of the codebase for 2 cycles to your
series, or anybody's for that matter?

> Is there any alternative?

I think an alternative is not to hold on to a series before it gets so
large to make you feel adjusting to the needs to other changes in the
codebase is "clerical". Commit often and early while developing the
initial pass, re-read often and throughout the whole process looking for
things you regret you would have done in early in the series that you
didn't (aka "oops, here is a fixup for the thinko in the early patch in
the series), and clean-up early while preparing to publish. Reorder the
parts that you are more confident that they do not need to change to come
early in the series, and unleash these early parts when you reach certain
confidence level.

I think your iterate-refs series was an example of good execution. It made
the codeflow a lot clearer by reducing the special casing of the submodule
parameter. In your grand scheme of things (e.g. read only parts of the ref
namespace as needed) you might consider it a mere side effect, but the
series by itself was a good thing to have.

Sometimes you may feel that a part of your series when taken out of
context would not justify itself like the iterate-refs series did, until
later parts of the series start taking advantage of the change. But that
is what commit log messages are for: e.g. "this change to encapsulate
these global variables into a single structure does not make a difference
in the current codebase, but in a later patch this and that callers will
need additional pieces of information passed aruond in the callchain, and
will add new members to the structure".

> ...  So maybe
> I brought this whole mess down on my own head :-(

No, it is not anybody's fault in particular. That's life and open source.

[PATCH v3 0/7] Provide API to invalidate refs cache

[Michael Haggerty]

These patches are re-rolled onto master.

This patch series provides an API for external code to invalidate the
ref cache that is used internally to refs.c.  It also allows code
*within* refs.c to invalidate only the packed or only the loose refs
for a module/submodule.

IMPORTANT:

I won't myself have time to figure out who, outside of refs.c, has to
*call* invalidate_ref_cache().  The candidates that I know off the top
of my head are git-clone, git-submodule [1], and git-pack-refs.  It
would be great if experts in those areas would insert calls to
invalidate_ref_cache() where needed.

Even better would be if the meddlesome code were changed to use the
refs API.  I'd be happy to help expanding the refs API if needed to
accommodate your needs.

This is why the API for invalidating only packed or loose refs is
private.  After code outside refs.c is changed to use the refs API, it
will get the optimal behavior for free (and at that time
invalidate_ref_cache() can be removed again).

[1] http://marc.info/?l=git&m=131827641227965&w=2
    In this mailing list thread, Heiko Voigt stated that git-submodule
    does not modify any references, so it should not have to use the
    API.

Michael Haggerty (7):
  invalidate_ref_cache(): rename function from invalidate_cached_refs()
  invalidate_ref_cache(): take the submodule as parameter
  invalidate_ref_cache(): expose this function in refs API
  clear_cached_refs(): rename parameter
  clear_cached_refs(): extract two new functions
  write_ref_sha1(): only invalidate the loose ref cache
  clear_cached_refs(): inline function

 refs.c |   31 +++++++++++++++++--------------
 refs.h |    8 ++++++++
 2 files changed, 25 insertions(+), 14 deletions(-)

-- 
1.7.7.rc2

[PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Michael Haggerty]

It is the cache that is being invalidated, not the references.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    6 +++---
 1 files changed, 3 insertions(+), 3 deletions(-)

diff --git a/refs.c b/refs.c
index 9911c97..120b8e4 100644
--- a/refs.c
+++ b/refs.c
@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_cached_refs(void)
+static void invalidate_ref_cache(void)
 {
 	struct cached_refs *refs = cached_refs;
 	while (refs) {
@@ -1228,7 +1228,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(refname);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_cached_refs();
+	invalidate_ref_cache();
 	unlock_ref(lock);
 	return ret;
 }
@@ -1527,7 +1527,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_cached_refs();
+	invalidate_ref_cache();
 	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.7.7.rc2

Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> It is the cache that is being invalidated, not the references.
>
> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---

> diff --git a/refs.c b/refs.c
> index 9911c97..120b8e4 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
>  	return refs;
>  }
>  
> -static void invalidate_cached_refs(void)
> +static void invalidate_ref_cache(void)
>  {
>  	struct cached_refs *refs = cached_refs;
>  	while (refs) {

If you call the operation "invalidate ref_cache", shouldn't the data
structure that holds that cache also be renamed to "struct ref_cache" from
"struct "cached_refs" at the same time?

Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()

[Michael Haggerty]

On 10/12/2011 09:14 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> It is the cache that is being invalidated, not the references.
>>
>> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
>> ---
> 
>> diff --git a/refs.c b/refs.c
>> index 9911c97..120b8e4 100644
>> --- a/refs.c
>> +++ b/refs.c
>> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
>>  	return refs;
>>  }
>>  
>> -static void invalidate_cached_refs(void)
>> +static void invalidate_ref_cache(void)
>>  {
>>  	struct cached_refs *refs = cached_refs;
>>  	while (refs) {
> 
> If you call the operation "invalidate ref_cache", shouldn't the data
> structure that holds that cache also be renamed to "struct ref_cache" from
> "struct "cached_refs" at the same time?

I don't think it is a logical necessity but I agree that it would be
more consistent.  I'll make the change in the next round.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

[PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Michael Haggerty]

Instead of invalidating the ref cache on an all-or-nothing basis,
allow the cache for individual submodules to be invalidated.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   12 ++++--------
 1 files changed, 4 insertions(+), 8 deletions(-)

diff --git a/refs.c b/refs.c
index 120b8e4..cc72609 100644
--- a/refs.c
+++ b/refs.c
@@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_ref_cache(void)
+static void invalidate_ref_cache(const char *submodule)
 {
-	struct cached_refs *refs = cached_refs;
-	while (refs) {
-		clear_cached_refs(refs);
-		refs = refs->next;
-	}
+	clear_cached_refs(get_cached_refs(submodule));
 }
 
 static void read_packed_refs(FILE *f, struct ref_array *array)
@@ -1228,7 +1224,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)
 	ret |= repack_without_ref(refname);
 
 	unlink_or_warn(git_path("logs/%s", lock->ref_name));
-	invalidate_ref_cache();
+	invalidate_ref_cache(NULL);
 	unlock_ref(lock);
 	return ret;
 }
@@ -1527,7 +1523,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_ref_cache();
+	invalidate_ref_cache(NULL);
 	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.7.7.rc2

Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Instead of invalidating the ref cache on an all-or-nothing basis,
> allow the cache for individual submodules to be invalidated.

That "allow" does not seem to describe what this patch does. It disallows
the wholesale invalidation and forces the caller to invalidate ref cache
individually.

Probably that is what all the existing callers want, but I would have
expected that an existing feature would be kept, perhaps like this
instead:

	if (!submodule) {
		struct ref_cache *c;
                for (c = ref_cache; c; c = c->next)
                	clear_ref_cache(c);
	} else {
		clear_ref_cache(get_ref_cache(submodule);
	}

Not a major "vetoing" objection, just a comment.

> Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
> ---
>  refs.c |   12 ++++--------
>  1 files changed, 4 insertions(+), 8 deletions(-)
>
> diff --git a/refs.c b/refs.c
> index 120b8e4..cc72609 100644
> --- a/refs.c
> +++ b/refs.c
> @@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
>  	return refs;
>  }
>  
> -static void invalidate_ref_cache(void)
> +static void invalidate_ref_cache(const char *submodule)
>  {
> -	struct cached_refs *refs = cached_refs;
> -	while (refs) {
> -		clear_cached_refs(refs);
> -		refs = refs->next;
> -	}
> +	clear_cached_refs(get_cached_refs(submodule));
>  }

Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Michael Haggerty]

On 10/12/2011 09:19 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
> 
>> Instead of invalidating the ref cache on an all-or-nothing basis,
>> allow the cache for individual submodules to be invalidated.
> 
> That "allow" does not seem to describe what this patch does. It disallows
> the wholesale invalidation and forces the caller to invalidate ref cache
> individually.
> 
> Probably that is what all the existing callers want, but I would have
> expected that an existing feature would be kept, perhaps like this
> instead:
> 
> 	if (!submodule) {
> 		struct ref_cache *c;
>                 for (c = ref_cache; c; c = c->next)
>                 	clear_ref_cache(c);
> 	} else {
> 		clear_ref_cache(get_ref_cache(submodule);
> 	}
> 
> Not a major "vetoing" objection, just a comment.

Indeed, it is currently not possible for code outside of refs.c to
implement "forget everything" using the "forget one" function (because
there is no API for getting the list of caches that are currently in
memory).

A "forget everything" function might be useful for code that delegates
to a subprocess, if it does not know what submodules the subprocess has
tinkered with.  Heiko, does that apply to the future submodule code?

Your specific suggestion would not work because currently
submodule==NULL signifies the main module.  However, it would be easy to
add the few-line function when/if it is needed.


I guess the bigger issue for me is whether the whole submodule cache
thing is going to continue to be needed.  I really am too ignorant of
how submodules work to be able to judge.  From Heiko's recent email it
sounds like things might be moving in the direction of "top-level git
doesn't need to know much about submodules because it delegates to
subprocesses".  He also said that submodule references are not modified
by the top-level git process, meaning that it might be sensible for the
submodule reference cache to be less capable than the main module
reference cache.

But if things move in the other direction (submodules handled by the
top-level git process), let alone if git is libified, then it seems
inevitable that there will someday be a "submodule" object that keeps
track of its own ref cache, with the submodule objects rather than the
submodule reference caches looked up by submodule name.

Given that I'm still very new to the codebase, I'm mostly making
"peephole changes" and so I'm happy to get your feedback about how this
fits into the grand scheme of things.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> On 10/12/2011 09:19 PM, Junio C Hamano wrote:
>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>  ...
>> Probably that is what all the existing callers want, but I would have
>> expected that an existing feature would be kept, perhaps like this
>> instead:
>> 
>> 	if (!submodule) {
>> 		struct ref_cache *c;
>>                 for (c = ref_cache; c; c = c->next)
>>                 	clear_ref_cache(c);
>> 	} else {
>> 		clear_ref_cache(get_ref_cache(submodule);
>> 	}
> ...
> Your specific suggestion would not work because currently
> submodule==NULL signifies the main module.  However, it would be easy to
> add the few-line function when/if it is needed.

I think "submodule==NULL" is probably a mistake; "" would make more sense
given that you are storing the string in name[FLEX_ARRAY] field.

Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Michael Haggerty]

On 10/17/2011 08:00 PM, Junio C Hamano wrote:
> Michael Haggerty <mhagger@alum.mit.edu> writes:
>> On 10/12/2011 09:19 PM, Junio C Hamano wrote:
>>> Michael Haggerty <mhagger@alum.mit.edu> writes:
>>>  ...
>>> Probably that is what all the existing callers want, but I would have
>>> expected that an existing feature would be kept, perhaps like this
>>> instead:
>>>
>>> 	if (!submodule) {
>>> 		struct ref_cache *c;
>>>                 for (c = ref_cache; c; c = c->next)
>>>                 	clear_ref_cache(c);
>>> 	} else {
>>> 		clear_ref_cache(get_ref_cache(submodule);
>>> 	}
>> ...
>> Your specific suggestion would not work because currently
>> submodule==NULL signifies the main module.  However, it would be easy to
>> add the few-line function when/if it is needed.
> 
> I think "submodule==NULL" is probably a mistake; "" would make more sense
> given that you are storing the string in name[FLEX_ARRAY] field.

Sorry I didn't respond to this earlier.

The public API convention (which predates my changes) is that "char
*submodule" arguments either point at the relative path to the submodule
or are NULL to denote the main module.  But since these are stored
internally in a name[FLEX_ARRAY] field, I have been using "" internally
to denote the main module.  I believe that everything is done correctly,
but I admit that the use of different conventions internally and
externally is a potential source of programming errors.

If this is viewed as something that needs changing, the easiest thing
would probably be to add a "const char *submodule" in the ref_cache data
structure that either contains NULL or points at the name field, and to
consistently use the convention that the main module must always be
denoted by NULL.  The space overhead would be negligible because the
number of ref_cache objects is limited to the number of submodules plus 1.

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> Sorry I didn't respond to this earlier.
>
> The public API convention (which predates my changes) is that "char
> *submodule" arguments either point at the relative path to the submodule
> or are NULL to denote the main module.  But since these are stored
> internally in a name[FLEX_ARRAY] field, I have been using "" internally
> to denote the main module.  I believe that everything is done correctly,
> but I admit that the use of different conventions internally and
> externally is a potential source of programming errors.

Yes, it would have been better if the original also used "". After all,
that would make it more consistent---"sub/" means the repository goverend
by "sub/.git", and "" would mean the repository governed by ".git".

Is it hard to change to do so now, given that we won't be rushing this for
the upcoming release and we have plenty of time?

[PATCH v3 3/7] invalidate_ref_cache(): expose this function in refs API

[Michael Haggerty]

Make invalidate_ref_cache() an official part of the refs API.  It is
currently a fact of life that code outside of refs.c mucks about with
references.  This change gives such code a way of informing the refs
module that it should no longer trust its cache.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    2 +-
 refs.h |    8 ++++++++
 2 files changed, 9 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index cc72609..b08d476 100644
--- a/refs.c
+++ b/refs.c
@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 	return refs;
 }
 
-static void invalidate_ref_cache(const char *submodule)
+void invalidate_ref_cache(const char *submodule)
 {
 	clear_cached_refs(get_cached_refs(submodule));
 }
diff --git a/refs.h b/refs.h
index 0229c57..f439c54 100644
--- a/refs.h
+++ b/refs.h
@@ -80,6 +80,14 @@ extern void unlock_ref(struct ref_lock *lock);
 /** Writes sha1 into the ref specified by the lock. **/
 extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);
 
+/*
+ * Invalidate the reference cache for the specified submodule.  Use
+ * submodule=NULL to invalidate the cache for the main module.  This
+ * function must be called if references are changed via a mechanism
+ * other than the refs API.
+ */
+extern void invalidate_ref_cache(const char *submodule);
+
 /** Setup reflog before using. **/
 int log_ref_setup(const char *ref_name, char *logfile, int bufsize);
 
-- 
1.7.7.rc2

[PATCH v3 4/7] clear_cached_refs(): rename parameter

[Michael Haggerty]

...for consistency with the rest of this module.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   12 ++++++------
 1 files changed, 6 insertions(+), 6 deletions(-)

diff --git a/refs.c b/refs.c
index b08d476..79e3576 100644
--- a/refs.c
+++ b/refs.c
@@ -158,13 +158,13 @@ static void free_ref_array(struct ref_array *array)
 	array->refs = NULL;
 }
 
-static void clear_cached_refs(struct cached_refs *ca)
+static void clear_cached_refs(struct cached_refs *refs)
 {
-	if (ca->did_loose)
-		free_ref_array(&ca->loose);
-	if (ca->did_packed)
-		free_ref_array(&ca->packed);
-	ca->did_loose = ca->did_packed = 0;
+	if (refs->did_loose)
+		free_ref_array(&refs->loose);
+	if (refs->did_packed)
+		free_ref_array(&refs->packed);
+	refs->did_loose = refs->did_packed = 0;
 }
 
 static struct cached_refs *create_cached_refs(const char *submodule)
-- 
1.7.7.rc2

[PATCH v3 5/7] clear_cached_refs(): extract two new functions

[Michael Haggerty]

Extract two new functions from clear_cached_refs():
clear_loose_ref_cache() and clear_packed_ref_cache().

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   19 +++++++++++++++----
 1 files changed, 15 insertions(+), 4 deletions(-)

diff --git a/refs.c b/refs.c
index 79e3576..9d962bd 100644
--- a/refs.c
+++ b/refs.c
@@ -158,13 +158,24 @@ static void free_ref_array(struct ref_array *array)
 	array->refs = NULL;
 }
 
-static void clear_cached_refs(struct cached_refs *refs)
+static void clear_cached_packed_refs(struct cached_refs *refs)
 {
-	if (refs->did_loose)
-		free_ref_array(&refs->loose);
 	if (refs->did_packed)
 		free_ref_array(&refs->packed);
-	refs->did_loose = refs->did_packed = 0;
+	refs->did_packed = 0;
+}
+
+static void clear_cached_loose_refs(struct cached_refs *refs)
+{
+	if (refs->did_loose)
+		free_ref_array(&refs->loose);
+	refs->did_loose = 0;
+}
+
+static void clear_cached_refs(struct cached_refs *refs)
+{
+	clear_cached_packed_refs(refs);
+	clear_cached_loose_refs(refs);
 }
 
 static struct cached_refs *create_cached_refs(const char *submodule)
-- 
1.7.7.rc2

[PATCH v3 6/7] write_ref_sha1(): only invalidate the loose ref cache

[Michael Haggerty]

Since write_ref_sha1() can only write loose refs and cannot write
symbolic refs, there is no need for it to invalidate the packed ref
cache.

Suggested by: Martin Fick <mfick@codeaurora.org>

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |    2 +-
 1 files changed, 1 insertions(+), 1 deletions(-)

diff --git a/refs.c b/refs.c
index 9d962bd..9e9edf7 100644
--- a/refs.c
+++ b/refs.c
@@ -1534,7 +1534,7 @@ int write_ref_sha1(struct ref_lock *lock,
 		unlock_ref(lock);
 		return -1;
 	}
-	invalidate_ref_cache(NULL);
+	clear_cached_loose_refs(get_cached_refs(NULL));
 	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.7.7.rc2

[PATCH v3 7/7] clear_cached_refs(): inline function

[Michael Haggerty]

clear_cached_refs() was only called from one place, so inline it
there.

Signed-off-by: Michael Haggerty <mhagger@alum.mit.edu>
---
 refs.c |   10 +++-------
 1 files changed, 3 insertions(+), 7 deletions(-)

diff --git a/refs.c b/refs.c
index 9e9edf7..409314d 100644
--- a/refs.c
+++ b/refs.c
@@ -172,12 +172,6 @@ static void clear_cached_loose_refs(struct cached_refs *refs)
 	refs->did_loose = 0;
 }
 
-static void clear_cached_refs(struct cached_refs *refs)
-{
-	clear_cached_packed_refs(refs);
-	clear_cached_loose_refs(refs);
-}
-
 static struct cached_refs *create_cached_refs(const char *submodule)
 {
 	int len;
@@ -215,7 +209,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)
 
 void invalidate_ref_cache(const char *submodule)
 {
-	clear_cached_refs(get_cached_refs(submodule));
+	struct cached_refs *refs = get_cached_refs(submodule);
+	clear_cached_packed_refs(refs);
+	clear_cached_loose_refs(refs);
 }
 
 static void read_packed_refs(FILE *f, struct ref_array *array)
-- 
1.7.7.rc2

Re: [PATCH v3 0/7] Provide API to invalidate refs cache

[Junio C Hamano]

Michael Haggerty <mhagger@alum.mit.edu> writes:

> These patches are re-rolled onto master.

Thanks.

Re: Re: [PATCH 6/6] Retain caches of submodule refs

[Heiko Voigt]

Hi Michael,

On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:
> On 08/17/2011 12:45 AM, Junio C Hamano wrote:
> > All the changes except for this one made sense to me, but I am not sure
> > about this one. How often do we look into different submodule refs in the
> > same process over and over again?
> 
> I am having pangs of uncertainty about this patch.

Currently when doing a 3 way merge of submodule hashes and we find a
conflict. We then use this api to look into the submodule to search for
commit suggestions which contain both sides.

If there are multiple submodules that conflict in this way we look into
those as well.

Since the setup_revision() api can currently not be used to safely
iterate twice over the same submodule my patch

	allow multiple calls to submodule merge search for the same path

rewrites the search into using a child process. AFAIK the submodule ref
iteration api would then even be unused.

> Previous to this patch, the submodule reference cache was only used for
> the duration of one call to do_for_each_ref().  (It was not *discarded*
> until later, but the old cache was never reused.)  Therefore, the
> submodule reference cache was implicitly invalidated between successive
> uses.

The implicit discarding was just done because it was the quickest way to
get a handle on submodule refs from the main process. There was no need
that they get reloaded every time.

> After this change, submodule ref caches are invalidated whenever
> invalidate_cached_refs() is called.  But this function is static, and it
> is only called when main-module refs are changed.
> 
> AFAIK there is no way within refs.c to add, modify, or delete a
> submodule reference.  But if other code modifies submodule references
> directly, then the submodule ref cache in refs.c would become stale.
> Moreover, there is currently no API for invalidating the cache.
> 
> So I think I need help from a submodule guru (Heiko?) who can tell me
> what is done with submodule references and whether they might be
> modified while a git process is executing in the main module.  If so,
> then either this patch has to be withdrawn, or more work has to be put
> in to make such code invalidate the submodule reference cache.

At least in my code there is no place where a submodule ref is changed.
I only used it for merging submodule which only modifies the main
module. So I would say its currently safe to assume that submodule refs
do not get modified. If we do need that later on we can still add
invalidation for submodule refs.

Cheers Heiko

Re: [PATCH 6/6] Retain caches of submodule refs

[Michael Haggerty]

On 10/10/2011 09:53 PM, Heiko Voigt wrote:
> On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:
> Since the setup_revision() api can currently not be used to safely
> iterate twice over the same submodule my patch
> 
> 	allow multiple calls to submodule merge search for the same path
> 
> rewrites the search into using a child process. AFAIK the submodule ref
> iteration api would then even be unused.

If your patch is accepted, then we should check whether anything should
be ripped out.

> At least in my code there is no place where a submodule ref is changed.
> I only used it for merging submodule which only modifies the main
> module. So I would say its currently safe to assume that submodule refs
> do not get modified. If we do need that later on we can still add
> invalidation for submodule refs.

OK, thanks!

Michael

-- 
Michael Haggerty
mhagger@alum.mit.edu
http://softwareswirl.blogspot.com/

Re: Re: [PATCH 6/6] Retain caches of submodule refs

[Heiko Voigt]

Hi,

On Tue, Oct 11, 2011 at 06:12:34AM +0200, Michael Haggerty wrote:
> On 10/10/2011 09:53 PM, Heiko Voigt wrote:
> > On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:
> > Since the setup_revision() api can currently not be used to safely
> > iterate twice over the same submodule my patch
> > 
> > 	allow multiple calls to submodule merge search for the same path
> > 
> > rewrites the search into using a child process. AFAIK the submodule ref
> > iteration api would then even be unused.
> 
> If your patch is accepted, then we should check whether anything should
> be ripped out.

I would rather like to extend the setup_revision() api so that we can
iterate multiple times. Additionally I have a feeling that this API
might be useful for further extensions of the recursive-push support of
submodules which is currently under development.

So in summary: I would like to wait with ripping anything out until we
get to a final state with submodule support.

Cheers Heiko

Re: [PATCH 0/6] Retain caches of submodule refs

[Heiko Voigt]

Hi,

On Sat, Aug 13, 2011 at 12:36:23AM +0200, Michael Haggerty wrote:
> ...and work towards storing refs hierarchically.

Nice stuff!

Cheers Heiko