{"thread":{"id":"28085","subject":"[PATCH 1/6] Extract a function clear_cached_refs()","startedAt":"2011-08-12T22:36:23Z","lastAt":"2011-11-03T18:57:01Z","messageCount":54,"participants":["Michael Haggerty","Heiko Voigt","Junio C Hamano","Julian Phillips"],"isPatch":true,"patchVersion":1,"patchTotal":6},"messages":[{"id":"173431","messageId":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":null,"subject":"[PATCH 0/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:23Z","receivedAt":"2011-08-12T22:36:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"...and work towards storing refs hierarchically.\n\nCurrently, the refs for submodules are put into a cache when accessed\nbut the cache is never reused. Whenever the refs for a submodule are\naccessed, the cache is cleared and refilled, even if the submodule\ncache already contains data for that submodule. Essentially, the\nsubmodule cache only controls the lifetime of the data structures.\nThe main module is currently stored in a separate cache that is reused\nproperly.\n\nThis patch series institutes proper caching of submodule refs:\nmaintain a linked list of caches for each submodule, and add a new\nentry whenever the refs for a new submodule are accessed. Also store\nthe cache for the main project in the same linked list for uniformity.\n\nThis change accomplishes two things:\n\n* Proper caching of submodule refs. I'm not sure whether this is a\n significant win by itself; it depends on the usage patterns and I'm\n not too familiar with how submodules are used. But it seems pretty\n clear that this is an improvement on the old kludge.\n\n* It is a first step towards storing refs hierarchically *within*\n modules. My plan is to build out \"struct cached_refs\" into a\n hierarchical data structure mimicking the reference namespace\n hierarchy, with one cached_ref instance for each \"directory\" of\n refs. Then (the real goal) change the code to only populate the\n parts of the cache hierarchy that are actually accessed.\n\nI believe that this change is useful by itself, self-contained, and\nready to be committed. I plan to build the hierarchical-refs changes\non top of it. But if it is preferred, I can submit this series plus\nthe hierarchical-refs patches as a single patch series (once the\nlatter is done, which will still take some time).\n\nThis patch series applies on top of \"next\" rather than \"master\"\nbecause it would otherwise conflict with the changes made by\njs/ref-namespaces.\n\nI am on vacation and don't know when I will have internet access, so\nplease don't be offended if I don't respond quickly to feedback.\n\nMichael Haggerty (6):\n Extract a function clear_cached_refs()\n Access reference caches only through new function get_cached_refs().\n Change the signature of read_packed_refs()\n Allocate cached_refs objects dynamically\n Store the submodule name in struct cached_refs.\n Retain caches of submodule refs\n\n refs.c | 106 ++++++++++++++++++++++++++++++++++++++++++++--------------------\n 1 files changed, 73 insertions(+), 33 deletions(-)\n\n-- \n1.7.6.8.gd2879\n"},{"id":"173430","messageId":"1313188589-2330-2-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/6] Extract a function clear_cached_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:24Z","receivedAt":"2011-08-12T22:36:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 9 ++++++---\n 1 files changed, 6 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6f313a9..b0c8308 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -171,10 +171,8 @@ static void free_ref_list(struct ref_list *list)\n \t}\n }\n \n-static void invalidate_cached_refs(void)\n+static void clear_cached_refs(struct cached_refs *ca)\n {\n-\tstruct cached_refs *ca = &cached_refs;\n-\n \tif (ca->did_loose && ca->loose)\n \t\tfree_ref_list(ca->loose);\n \tif (ca->did_packed && ca->packed)\n@@ -183,6 +181,11 @@ static void invalidate_cached_refs(void)\n \tca->did_loose = ca->did_packed = 0;\n }\n \n+static void invalidate_cached_refs(void)\n+{\n+\tclear_cached_refs(&cached_refs);\n+}\n+\n static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)\n {\n \tstruct ref_list *list = NULL;\n-- \n1.7.6.8.gd2879\n"},{"id":"173432","messageId":"1313188589-2330-3-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/6] Access reference caches only through new function get_cached_refs().","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:25Z","receivedAt":"2011-08-12T22:36:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 54 +++++++++++++++++++++++++++++++-----------------------\n 1 files changed, 31 insertions(+), 23 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b0c8308..89840d7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -181,9 +181,26 @@ static void clear_cached_refs(struct cached_refs *ca)\n \tca->did_loose = ca->did_packed = 0;\n }\n \n+/*\n+ * Return a pointer to a cached_refs for the specified submodule. For\n+ * the main repository, use submodule==NULL. The returned structure\n+ * will be allocated and initialized but not necessarily populated; it\n+ * should not be freed.\n+ */\n+static struct cached_refs *get_cached_refs(const char *submodule)\n+{\n+\tif (! submodule)\n+\t\treturn &cached_refs;\n+\telse {\n+\t\t/* For now, don't reuse the refs cache for submodules. */\n+\t\tclear_cached_refs(&submodule_refs);\n+\t\treturn &submodule_refs;\n+\t}\n+}\n+\n static void invalidate_cached_refs(void)\n {\n-\tclear_cached_refs(&cached_refs);\n+\tclear_cached_refs(get_cached_refs(NULL));\n }\n \n static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)\n@@ -234,19 +251,14 @@ void clear_extra_refs(void)\n \n static struct ref_list *get_packed_refs(const char *submodule)\n {\n-\tconst char *packed_refs_file;\n-\tstruct cached_refs *refs;\n+\tstruct cached_refs *refs = get_cached_refs(submodule);\n \n-\tif (submodule) {\n-\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n-\t\trefs = &submodule_refs;\n-\t\tfree_ref_list(refs->packed);\n-\t} else {\n-\t\tpacked_refs_file = git_path(\"packed-refs\");\n-\t\trefs = &cached_refs;\n-\t}\n-\n-\tif (!refs->did_packed || submodule) {\n+\tif (!refs->did_packed) {\n+\t\tconst char *packed_refs_file;\n+\t\tif (submodule)\n+\t\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n+\t\telse\n+\t\t\tpacked_refs_file = git_path(\"packed-refs\");\n \t\tFILE *f = fopen(packed_refs_file, \"r\");\n \t\trefs->packed = NULL;\n \t\tif (f) {\n@@ -361,17 +373,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n \n static struct ref_list *get_loose_refs(const char *submodule)\n {\n-\tif (submodule) {\n-\t\tfree_ref_list(submodule_refs.loose);\n-\t\tsubmodule_refs.loose = get_ref_dir(submodule, \"refs\", NULL);\n-\t\treturn submodule_refs.loose;\n-\t}\n+\tstruct cached_refs *refs = get_cached_refs(submodule);\n \n-\tif (!cached_refs.did_loose) {\n-\t\tcached_refs.loose = get_ref_dir(NULL, \"refs\", NULL);\n-\t\tcached_refs.did_loose = 1;\n+\tif (!refs->did_loose) {\n+\t\trefs->loose = get_ref_dir(submodule, \"refs\", NULL);\n+\t\trefs->did_loose = 1;\n \t}\n-\treturn cached_refs.loose;\n+\treturn refs->loose;\n }\n \n /* We allow \"recursive\" symbolic refs. Only within reason, though */\n-- \n1.7.6.8.gd2879\n"},{"id":"173433","messageId":"1313188589-2330-4-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 3/6] Change the signature of read_packed_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:26Z","receivedAt":"2011-08-12T22:36:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Change it to return a (struct ref_list *) instead of writing into\na cached_refs structure. (This removes the need to create a\ncached_refs structure in resolve_gitlink_packed_ref(), where it\nis otherwise unneeded.)\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 89840d7..e043555 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -203,7 +203,7 @@ static void invalidate_cached_refs(void)\n \tclear_cached_refs(get_cached_refs(NULL));\n }\n \n-static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)\n+static struct ref_list *read_packed_refs(FILE *f)\n {\n \tstruct ref_list *list = NULL;\n \tstruct ref_list *last = NULL;\n@@ -235,7 +235,7 @@ static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)\n \t\t !get_sha1_hex(refline + 1, sha1))\n \t\t\thashcpy(last->peeled, sha1);\n \t}\n-\tcached_refs->packed = sort_ref_list(list);\n+\treturn sort_ref_list(list);\n }\n \n void add_extra_ref(const char *name, const unsigned char *sha1, int flag)\n@@ -262,7 +262,7 @@ static struct ref_list *get_packed_refs(const char *submodule)\n \t\tFILE *f = fopen(packed_refs_file, \"r\");\n \t\trefs->packed = NULL;\n \t\tif (f) {\n-\t\t\tread_packed_refs(f, refs);\n+\t\t\trefs->packed = read_packed_refs(f);\n \t\t\tfclose(f);\n \t\t}\n \t\trefs->did_packed = 1;\n@@ -389,7 +389,7 @@ static struct ref_list *get_loose_refs(const char *submodule)\n static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refname, unsigned char *result)\n {\n \tFILE *f;\n-\tstruct cached_refs refs;\n+\tstruct ref_list *packed_refs;\n \tstruct ref_list *ref;\n \tint retval;\n \n@@ -397,9 +397,9 @@ static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refna\n \tf = fopen(name, \"r\");\n \tif (!f)\n \t\treturn -1;\n-\tread_packed_refs(f, &refs);\n+\tpacked_refs = read_packed_refs(f);\n \tfclose(f);\n-\tref = refs.packed;\n+\tref = packed_refs;\n \tretval = -1;\n \twhile (ref) {\n \t\tif (!strcmp(ref->name, refname)) {\n@@ -409,7 +409,7 @@ static int resolve_gitlink_packed_ref(char *name, int pathlen, const char *refna\n \t\t}\n \t\tref = ref->next;\n \t}\n-\tfree_ref_list(refs.packed);\n+\tfree_ref_list(packed_refs);\n \treturn retval;\n }\n \n-- \n1.7.6.8.gd2879\n"},{"id":"173434","messageId":"1313188589-2330-5-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 4/6] Allocate cached_refs objects dynamically","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:27Z","receivedAt":"2011-08-12T22:36:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 28 +++++++++++++++++++++-------\n 1 files changed, 21 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex e043555..102ed03 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -157,7 +157,7 @@ static struct cached_refs {\n \tchar did_packed;\n \tstruct ref_list *loose;\n \tstruct ref_list *packed;\n-} cached_refs, submodule_refs;\n+} *cached_refs, *submodule_refs;\n static struct ref_list *current_ref;\n \n static struct ref_list *extra_refs;\n@@ -181,6 +181,15 @@ static void clear_cached_refs(struct cached_refs *ca)\n \tca->did_loose = ca->did_packed = 0;\n }\n \n+struct cached_refs *create_cached_refs()\n+{\n+\tstruct cached_refs *refs;\n+\trefs = xmalloc(sizeof(struct cached_refs));\n+\trefs->did_loose = refs->did_packed = 0;\n+\trefs->loose = refs->packed = NULL;\n+\treturn refs;\n+}\n+\n /*\n * Return a pointer to a cached_refs for the specified submodule. For\n * the main repository, use submodule==NULL. The returned structure\n@@ -189,12 +198,17 @@ static void clear_cached_refs(struct cached_refs *ca)\n */\n static struct cached_refs *get_cached_refs(const char *submodule)\n {\n-\tif (! submodule)\n-\t\treturn &cached_refs;\n-\telse {\n-\t\t/* For now, don't reuse the refs cache for submodules. */\n-\t\tclear_cached_refs(&submodule_refs);\n-\t\treturn &submodule_refs;\n+\tif (! submodule) {\n+\t\tif (!cached_refs)\n+\t\t\tcached_refs = create_cached_refs();\n+\t\treturn cached_refs;\n+\t} else {\n+\t\tif (!submodule_refs)\n+\t\t\tsubmodule_refs = create_cached_refs();\n+\t\telse\n+\t\t\t/* For now, don't reuse the refs cache for submodules. */\n+\t\t\tclear_cached_refs(submodule_refs);\n+\t\treturn submodule_refs;\n \t}\n }\n \n-- \n1.7.6.8.gd2879\n"},{"id":"173435","messageId":"1313188589-2330-6-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 5/6] Store the submodule name in struct cached_refs.","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:28Z","receivedAt":"2011-08-12T22:36:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"\nSigned-off-by: Michael Haggerty \n---\n refs.c | 15 +++++++++++----\n 1 files changed, 11 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 102ed03..8d1055d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -157,6 +157,8 @@ static struct cached_refs {\n \tchar did_packed;\n \tstruct ref_list *loose;\n \tstruct ref_list *packed;\n+\t/* The submodule name, or \"\" for the main repo. */\n+\tchar name[FLEX_ARRAY];\n } *cached_refs, *submodule_refs;\n static struct ref_list *current_ref;\n \n@@ -181,12 +183,17 @@ static void clear_cached_refs(struct cached_refs *ca)\n \tca->did_loose = ca->did_packed = 0;\n }\n \n-struct cached_refs *create_cached_refs()\n+struct cached_refs *create_cached_refs(const char *submodule)\n {\n+\tint len;\n \tstruct cached_refs *refs;\n-\trefs = xmalloc(sizeof(struct cached_refs));\n+\tif (!submodule)\n+\t\tsubmodule = \"\";\n+\tlen = strlen(submodule) + 1;\n+\trefs = xmalloc(sizeof(struct cached_refs) + len);\n \trefs->did_loose = refs->did_packed = 0;\n \trefs->loose = refs->packed = NULL;\n+\tmemcpy(refs->name, submodule, len);\n \treturn refs;\n }\n \n@@ -200,11 +207,11 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n {\n \tif (! submodule) {\n \t\tif (!cached_refs)\n-\t\t\tcached_refs = create_cached_refs();\n+\t\t\tcached_refs = create_cached_refs(submodule);\n \t\treturn cached_refs;\n \t} else {\n \t\tif (!submodule_refs)\n-\t\t\tsubmodule_refs = create_cached_refs();\n+\t\t\tsubmodule_refs = create_cached_refs(submodule);\n \t\telse\n \t\t\t/* For now, don't reuse the refs cache for submodules. */\n \t\t\tclear_cached_refs(submodule_refs);\n-- \n1.7.6.8.gd2879\n"},{"id":"173436","messageId":"1313188589-2330-7-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 6/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-12T22:36:29Z","receivedAt":"2011-08-12T22:36:29Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of keeping track of one cache for refs in the main repo and\nanother single cache shared among submodules, keep a linked list of\ncached_refs objects, one for each module/submodule. Change\ninvalidate_cached_refs() to invalidate all caches. (Previously, it\nonly invalidated the cache of the main repo because the submodule\ncaches were not reused anyway.)\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 34 +++++++++++++++++++++-------------\n 1 files changed, 21 insertions(+), 13 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 8d1055d..f02cf94 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -153,13 +153,15 @@ static struct ref_list *sort_ref_list(struct ref_list *list)\n * when doing a full libification.\n */\n static struct cached_refs {\n+\tstruct cached_refs *next;\n \tchar did_loose;\n \tchar did_packed;\n \tstruct ref_list *loose;\n \tstruct ref_list *packed;\n \t/* The submodule name, or \"\" for the main repo. */\n \tchar name[FLEX_ARRAY];\n-} *cached_refs, *submodule_refs;\n+} *cached_refs;\n+\n static struct ref_list *current_ref;\n \n static struct ref_list *extra_refs;\n@@ -191,6 +193,7 @@ struct cached_refs *create_cached_refs(const char *submodule)\n \t\tsubmodule = \"\";\n \tlen = strlen(submodule) + 1;\n \trefs = xmalloc(sizeof(struct cached_refs) + len);\n+\trefs->next = NULL;\n \trefs->did_loose = refs->did_packed = 0;\n \trefs->loose = refs->packed = NULL;\n \tmemcpy(refs->name, submodule, len);\n@@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)\n */\n static struct cached_refs *get_cached_refs(const char *submodule)\n {\n-\tif (! submodule) {\n-\t\tif (!cached_refs)\n-\t\t\tcached_refs = create_cached_refs(submodule);\n-\t\treturn cached_refs;\n-\t} else {\n-\t\tif (!submodule_refs)\n-\t\t\tsubmodule_refs = create_cached_refs(submodule);\n-\t\telse\n-\t\t\t/* For now, don't reuse the refs cache for submodules. */\n-\t\t\tclear_cached_refs(submodule_refs);\n-\t\treturn submodule_refs;\n+\tstruct cached_refs *refs = cached_refs;\n+\tif (! submodule)\n+\t\tsubmodule = \"\";\n+\twhile (refs) {\n+\t\tif (!strcmp(submodule, refs->name))\n+\t\t\treturn refs;\n+\t\trefs = refs->next;\n \t}\n+\n+\trefs = create_cached_refs(submodule);\n+\trefs->next = cached_refs;\n+\tcached_refs = refs;\n+\treturn refs;\n }\n \n static void invalidate_cached_refs(void)\n {\n-\tclear_cached_refs(get_cached_refs(NULL));\n+\tstruct cached_refs *refs = cached_refs;\n+\twhile (refs) {\n+\t\tclear_cached_refs(refs);\n+\t\trefs = refs->next;\n+\t}\n }\n \n static struct ref_list *read_packed_refs(FILE *f)\n-- \n1.7.6.8.gd2879\n"},{"id":"173445","messageId":"20110813123409.GB4783@book.hvoigt.net","threadId":"28085","inReplyTo":"1313188589-2330-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 0/6] Retain caches of submodule refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-13T12:34:10Z","receivedAt":"2011-08-13T12:34:10Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Sat, Aug 13, 2011 at 12:36:23AM +0200, Michael Haggerty wrote:\n> ...and work towards storing refs hierarchically.\n\nNice stuff!\n\nCheers Heiko\n"},{"id":"173447","messageId":"20110813125438.GC4783@book.hvoigt.net","threadId":"28085","inReplyTo":"1313188589-2330-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-13T12:54:38Z","receivedAt":"2011-08-13T12:54:38Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:\n> diff --git a/refs.c b/refs.c\n> index 8d1055d..f02cf94 100644\n> --- a/refs.c\n> +++ b/refs.c\n\n...\n\n> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)\n> */\n> static struct cached_refs *get_cached_refs(const char *submodule)\n> {\n> -\tif (! submodule) {\n> -\t\tif (!cached_refs)\n> -\t\t\tcached_refs = create_cached_refs(submodule);\n> -\t\treturn cached_refs;\n> -\t} else {\n> -\t\tif (!submodule_refs)\n> -\t\t\tsubmodule_refs = create_cached_refs(submodule);\n> -\t\telse\n> -\t\t\t/* For now, don't reuse the refs cache for submodules. */\n> -\t\t\tclear_cached_refs(submodule_refs);\n> -\t\treturn submodule_refs;\n> +\tstruct cached_refs *refs = cached_refs;\n> +\tif (! submodule)\n> +\t\tsubmodule = \"\";\n\nMaybe instead of searching for the main refs store a pointer to them\nlocally so you can immediately return here. That will keep the\nperformance when requesting the main refs the same.\n\nIf I see it correctly you are always prepending to the linked list and\nin case many submodules get cached this could slow down the iteration\nover the refs of the main repository.\n\n> +\twhile (refs) {\n> +\t\tif (!strcmp(submodule, refs->name))\n> +\t\t\treturn refs;\n> +\t\trefs = refs->next;\n> \t}\n> +\n> +\trefs = create_cached_refs(submodule);\n> +\trefs->next = cached_refs;\n> +\tcached_refs = refs;\n> +\treturn refs;\n> }\n\n...\n\nCheers Heiko\n"},{"id":"173519","messageId":"7vzkjblk22.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1313188589-2330-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 2/6] Access reference caches only through new function get_cached_refs().","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-14T22:12:05Z","receivedAt":"2011-08-14T22:12:05Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 54 +++++++++++++++++++++++++++++++-----------------------\n> 1 files changed, 31 insertions(+), 23 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index b0c8308..89840d7 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -181,9 +181,26 @@ static void clear_cached_refs(struct cached_refs *ca)\n> \tca->did_loose = ca->did_packed = 0;\n> }\n> \n> +/*\n> + * Return a pointer to a cached_refs for the specified submodule. For\n> + * the main repository, use submodule==NULL. The returned structure\n> + * will be allocated and initialized but not necessarily populated; it\n> + * should not be freed.\n> + */\n> +static struct cached_refs *get_cached_refs(const char *submodule)\n> +{\n> +\tif (! submodule)\n\n(style) lose the SP before \"submodule\".\n\n> +\t\treturn &cached_refs;\n> +\telse {\n> +\t\t/* For now, don't reuse the refs cache for submodules. */\n> +\t\tclear_cached_refs(&submodule_refs);\n> +\t\treturn &submodule_refs;\n> +\t}\n> +}\n> +\n> static void invalidate_cached_refs(void)\n> {\n> -\tclear_cached_refs(&cached_refs);\n> +\tclear_cached_refs(get_cached_refs(NULL));\n> }\n> \n> static void read_packed_refs(FILE *f, struct cached_refs *cached_refs)\n> @@ -234,19 +251,14 @@ void clear_extra_refs(void)\n> \n> static struct ref_list *get_packed_refs(const char *submodule)\n> {\n> -\tconst char *packed_refs_file;\n> -\tstruct cached_refs *refs;\n> +\tstruct cached_refs *refs = get_cached_refs(submodule);\n> \n> -\tif (submodule) {\n> -\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n> -\t\trefs = &submodule_refs;\n> -\t\tfree_ref_list(refs->packed);\n> -\t} else {\n> -\t\tpacked_refs_file = git_path(\"packed-refs\");\n> -\t\trefs = &cached_refs;\n> -\t}\n> -\n> -\tif (!refs->did_packed || submodule) {\n> +\tif (!refs->did_packed) {\n> +\t\tconst char *packed_refs_file;\n> +\t\tif (submodule)\n> +\t\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n> +\t\telse\n> +\t\t\tpacked_refs_file = git_path(\"packed-refs\");\n> \t\tFILE *f = fopen(packed_refs_file, \"r\");\n\ndecl-after-statement.\n\n> \t\trefs->packed = NULL;\n> \t\tif (f) {\n> @@ -361,17 +373,13 @@ void warn_dangling_symref(FILE *fp, const char *msg_fmt, const char *refname)\n> \n> static struct ref_list *get_loose_refs(const char *submodule)\n> {\n> -\tif (submodule) {\n> -\t\tfree_ref_list(submodule_refs.loose);\n> -\t\tsubmodule_refs.loose = get_ref_dir(submodule, \"refs\", NULL);\n> -\t\treturn submodule_refs.loose;\n> -\t}\n> +\tstruct cached_refs *refs = get_cached_refs(submodule);\n> \n> -\tif (!cached_refs.did_loose) {\n> -\t\tcached_refs.loose = get_ref_dir(NULL, \"refs\", NULL);\n> -\t\tcached_refs.did_loose = 1;\n> +\tif (!refs->did_loose) {\n> +\t\trefs->loose = get_ref_dir(submodule, \"refs\", NULL);\n> +\t\trefs->did_loose = 1;\n> \t}\n> -\treturn cached_refs.loose;\n> +\treturn refs->loose;\n> }\n> \n> /* We allow \"recursive\" symbolic refs. Only within reason, though */\n"},{"id":"173520","messageId":"7vmxfbljly.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1313188589-2330-5-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 4/6] Allocate cached_refs objects dynamically","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-14T22:21:45Z","receivedAt":"2011-08-14T22:21:45Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 28 +++++++++++++++++++++-------\n> 1 files changed, 21 insertions(+), 7 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index e043555..102ed03 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -157,7 +157,7 @@ static struct cached_refs {\n> \tchar did_packed;\n> \tstruct ref_list *loose;\n> \tstruct ref_list *packed;\n> -} cached_refs, submodule_refs;\n> +} *cached_refs, *submodule_refs;\n> static struct ref_list *current_ref;\n> \n> static struct ref_list *extra_refs;\n> @@ -181,6 +181,15 @@ static void clear_cached_refs(struct cached_refs *ca)\n> \tca->did_loose = ca->did_packed = 0;\n> }\n> \n> +struct cached_refs *create_cached_refs()\n\nstruct cached_refs *create_cached_refs(void)\n"},{"id":"173642","messageId":"7v4o1hgemp.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1313188589-2330-7-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-08-16T22:45:02Z","receivedAt":"2011-08-16T22:45:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"All the changes except for this one made sense to me, but I am not sure\nabout this one. How often do we look into different submodule refs in the\nsame process over and over again?\n"},{"id":"174074","messageId":"4E532AE3.300@alum.mit.edu","threadId":"28085","inReplyTo":"7vzkjblk22.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 2/6] Access reference caches only through new function get_cached_refs().","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-23T04:21:55Z","receivedAt":"2011-08-23T04:21:55Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/15/2011 12:12 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> +\tif (! submodule)\n> \n> (style) lose the SP before \"submodule\".\n\nWill be fixed in re-roll.\n\n>> -\tif (!refs->did_packed || submodule) {\n>> +\tif (!refs->did_packed) {\n>> +\t\tconst char *packed_refs_file;\n>> +\t\tif (submodule)\n>> +\t\t\tpacked_refs_file = git_path_submodule(submodule, \"packed-refs\");\n>> +\t\telse\n>> +\t\t\tpacked_refs_file = git_path(\"packed-refs\");\n>> \t\tFILE *f = fopen(packed_refs_file, \"r\");\n> \n> decl-after-statement.\n\nWill be fixed.\n\nOn 08/15/2011 12:21 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> +struct cached_refs *create_cached_refs()\n>\n> struct cached_refs *create_cached_refs(void)\n\nWill be fixed.\n\nThanks for your feedback.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"174141","messageId":"4E54B394.2070006@alum.mit.edu","threadId":"28085","inReplyTo":"20110813125438.GC4783@book.hvoigt.net","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-24T08:17:24Z","receivedAt":"2011-08-24T08:17:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/13/2011 02:54 PM, Heiko Voigt wrote:\n> On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:\n>> diff --git a/refs.c b/refs.c\n>> index 8d1055d..f02cf94 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n> \n> ...\n> \n>> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)\n>> */\n>> static struct cached_refs *get_cached_refs(const char *submodule)\n>> {\n>> -\tif (! submodule) {\n>> -\t\tif (!cached_refs)\n>> -\t\t\tcached_refs = create_cached_refs(submodule);\n>> -\t\treturn cached_refs;\n>> -\t} else {\n>> -\t\tif (!submodule_refs)\n>> -\t\t\tsubmodule_refs = create_cached_refs(submodule);\n>> -\t\telse\n>> -\t\t\t/* For now, don't reuse the refs cache for submodules. */\n>> -\t\t\tclear_cached_refs(submodule_refs);\n>> -\t\treturn submodule_refs;\n>> +\tstruct cached_refs *refs = cached_refs;\n>> +\tif (! submodule)\n>> +\t\tsubmodule = \"\";\n> \n> Maybe instead of searching for the main refs store a pointer to them\n> locally so you can immediately return here. That will keep the\n> performance when requesting the main refs the same.\n\nI am assuming that the number of submodules will be small compared to\nthe number of refs in a typical submodule. Therefore, given that the\nrefs are stored in a big linked list and that the only thing that can\nrealistically be done with the list is to iterate through it, the cost\nof finding the reference cache for a specific (sub-)module should be\nnegligible compared to the cost of the iteration over refs in the cache.\n Treating the main module like the other submodules makes the code\nsimpler, so I would prefer to leave it this way.\n\nIf iteration over refs ever becomes a bottleneck, then optimization of\nthe storage of refs within a (sub-)module would be a bigger win than\nspecial-casing the main module. And that is what I would like to work\ntowards.\n\n> If I see it correctly you are always prepending to the linked list \n\nThis is true.\n\n> and\n> in case many submodules get cached this could slow down the iteration\n> over the refs of the main repository.\n\nIs this a realistic concern? Remember that the extra search over\nsubmodules only occurs once for each scan through the list of references.\n\nI wrote an additional patch that moves the least-recently accessed\nmodule to the front of the list. But I doubt that the savings justify\nthe 10-odd extra lines of code, so I kept it to myself. Please note\nthat this approach gives pessimal performance (2x slower) if the\nsubmodules are iterated over repeatedly. If you would like me to add\nthis patch to the patch series, please let me know.\n\nLong-term, it would be better to implement a \"struct submodule\" and use\nit to hold submodule-specific data like the ref cache. Then users could\nhold on to the \"struct submodule *\" and pass it, rather than the\nsubmodule name, to the functions that need it. But this goes beyond the\nscope of what I want to change now, especially since I have no\nexperience even working with submodules.\n\nSummary: I hope I have convinced you that the extra overhead is\nnegligible and does not justify additional code. But if you insist on\nmore emphasis on performance (or obviously if you have numbers to back\nup your concerns) then I would be willing to put more work into it.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"174147","messageId":"4E54E188.10802@alum.mit.edu","threadId":"28085","inReplyTo":"7v4o1hgemp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-08-24T11:33:28Z","receivedAt":"2011-08-24T11:33:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/17/2011 12:45 AM, Junio C Hamano wrote:\n> All the changes except for this one made sense to me, but I am not sure\n> about this one. How often do we look into different submodule refs in the\n> same process over and over again?\n\nAs I've mentioned, I am not very familiar with submodules and I don't\nknow what actions in submodules can be triggered from the top-level\nproject. Here is the only code path that I can find in the current git\ncode that causes submodule refs to be read at all:\n\nsetup_revisions() with opt->submodule is set,\n which can only happen when called via merge_submodule(),\n which is called from merge_file() in merge-recursive.c,\n which is called by process_renames() and merge_content(),\n which are both ultimately called from merge_trees().\n\nI don't know how often this can happen during the lifetime of one process.\n\nThe cost of retaining the submodule ref caches is roughly 100 bytes per\nreference of memory. Another side effect is that the submodule caches\nnever have to be cleaned up; this saves a list walk and one free() per\ncached reference if the refs for more than one submodule are accessed.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"174182","messageId":"20110824200520.GE45292@book.hvoigt.net","threadId":"28085","inReplyTo":"4E54B394.2070006@alum.mit.edu","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-08-24T20:05:20Z","receivedAt":"2011-08-24T20:05:20Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"On Wed, Aug 24, 2011 at 10:17:24AM +0200, Michael Haggerty wrote:\n> On 08/13/2011 02:54 PM, Heiko Voigt wrote:\n> > On Sat, Aug 13, 2011 at 12:36:29AM +0200, Michael Haggerty wrote:\n> >> diff --git a/refs.c b/refs.c\n> >> index 8d1055d..f02cf94 100644\n> >> --- a/refs.c\n> >> +++ b/refs.c\n> > \n> > ...\n> > \n> >> @@ -205,23 +208,28 @@ struct cached_refs *create_cached_refs(const char *submodule)\n> >> */\n> >> static struct cached_refs *get_cached_refs(const char *submodule)\n> >> {\n> >> -\tif (! submodule) {\n> >> -\t\tif (!cached_refs)\n> >> -\t\t\tcached_refs = create_cached_refs(submodule);\n> >> -\t\treturn cached_refs;\n> >> -\t} else {\n> >> -\t\tif (!submodule_refs)\n> >> -\t\t\tsubmodule_refs = create_cached_refs(submodule);\n> >> -\t\telse\n> >> -\t\t\t/* For now, don't reuse the refs cache for submodules. */\n> >> -\t\t\tclear_cached_refs(submodule_refs);\n> >> -\t\treturn submodule_refs;\n> >> +\tstruct cached_refs *refs = cached_refs;\n> >> +\tif (! submodule)\n> >> +\t\tsubmodule = \"\";\n> > \n> > Maybe instead of searching for the main refs store a pointer to them\n> > locally so you can immediately return here. That will keep the\n> > performance when requesting the main refs the same.\n> \n> I am assuming that the number of submodules will be small compared to\n> the number of refs in a typical submodule. Therefore, given that the\n> refs are stored in a big linked list and that the only thing that can\n> realistically be done with the list is to iterate through it, the cost\n> of finding the reference cache for a specific (sub-)module should be\n> negligible compared to the cost of the iteration over refs in the cache.\n> Treating the main module like the other submodules makes the code\n> simpler, so I would prefer to leave it this way.\n\nI just thought I mention it since it is a change and you are already\nspecial casing the main module with this if(!submodule) but you are\nprobably right and in most cases (many refs and few submodules) this\nchange is negligible.\n\n> If iteration over refs ever becomes a bottleneck, then optimization of\n> the storage of refs within a (sub-)module would be a bigger win than\n> special-casing the main module. And that is what I would like to work\n> towards.\n> \n> > If I see it correctly you are always prepending to the linked list \n> \n> This is true.\n> \n> > and\n> > in case many submodules get cached this could slow down the iteration\n> > over the refs of the main repository.\n> \n> Is this a realistic concern? Remember that the extra search over\n> submodules only occurs once for each scan through the list of references.\n> \n> I wrote an additional patch that moves the least-recently accessed\n> module to the front of the list. But I doubt that the savings justify\n> the 10-odd extra lines of code, so I kept it to myself. Please note\n> that this approach gives pessimal performance (2x slower) if the\n> submodules are iterated over repeatedly. If you would like me to add\n> this patch to the patch series, please let me know.\n\nNo I don't think this is necessary.\n\n> Long-term, it would be better to implement a \"struct submodule\" and use\n> it to hold submodule-specific data like the ref cache. Then users could\n> hold on to the \"struct submodule *\" and pass it, rather than the\n> submodule name, to the functions that need it. But this goes beyond the\n> scope of what I want to change now, especially since I have no\n> experience even working with submodules.\n> \n> Summary: I hope I have convinced you that the extra overhead is\n> negligible and does not justify additional code. But if you insist on\n> more emphasis on performance (or obviously if you have numbers to back\n> up your concerns) then I would be willing to put more work into it.\n\nYes I am also fine with the current state. I do not have a strong\nopinion about this. This already adds enough improvements of the\nsubmodule ref caching which looks a lot nicer than before.\n\nCheers Heiko\n"},{"id":"177270","messageId":"4E918194.5060102@alum.mit.edu","threadId":"28085","inReplyTo":"7v4o1hgemp.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-09T11:12:20Z","receivedAt":"2011-10-09T11:12:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 08/17/2011 12:45 AM, Junio C Hamano wrote:\n> All the changes except for this one made sense to me, but I am not sure\n> about this one. How often do we look into different submodule refs in the\n> same process over and over again?\n\nI am having pangs of uncertainty about this patch.\n\nPrevious to this patch, the submodule reference cache was only used for\nthe duration of one call to do_for_each_ref(). (It was not *discarded*\nuntil later, but the old cache was never reused.) Therefore, the\nsubmodule reference cache was implicitly invalidated between successive\nuses.\n\nAfter this change, submodule ref caches are invalidated whenever\ninvalidate_cached_refs() is called. But this function is static, and it\nis only called when main-module refs are changed.\n\nAFAIK there is no way within refs.c to add, modify, or delete a\nsubmodule reference. But if other code modifies submodule references\ndirectly, then the submodule ref cache in refs.c would become stale.\nMoreover, there is currently no API for invalidating the cache.\n\nSo I think I need help from a submodule guru (Heiko?) who can tell me\nwhat is done with submodule references and whether they might be\nmodified while a git process is executing in the main module. If so,\nthen either this patch has to be withdrawn, or more work has to be put\nin to make such code invalidate the submodule reference cache.\n\nSorry for the oversight, but I forgot that not all code necessarily uses\nthe refs.c API when dealing with references (a regrettable situation, BTW).\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177283","messageId":"7v39f2rko5.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"4E918194.5060102@alum.mit.edu","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-09T20:10:02Z","receivedAt":"2011-10-09T20:10:02Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> So I think I need help from a submodule guru (Heiko?) who can tell me\n> what is done with submodule references and whether they might be\n> modified while a git process is executing in the main module. If so,\n> then either this patch has to be withdrawn, or more work has to be put\n> in to make such code invalidate the submodule reference cache.\n>\n> Sorry for the oversight, but I forgot that not all code necessarily uses\n> the refs.c API when dealing with references (a regrettable situation, BTW).\n\nIn the longer term, I would agree with you that we very much prefer all\nthe ref accesses to go through the refs API, and I also foresee that the\nsubmodule support would need to become more aware of the status of refs in\nchecked out submodules. For example, a recursive \"submodule update\" may\nwant to inspect the refs in a submodule directory, compare them with the\ncommit bound in the superproject tree for the submodule path, and decide\nto spawn a \"fetch && checkout $branch\" in there. The same process then may\nwant to run \"status\" at the superproject level to show the result, which\nin turn would inspect the relationship between the commit bound in the\nsuperproject tree and the commit at the HEAD of the submodule directory.\n\nSo let's not rip out the commit but instead give submodule machinery an\nexplicit way to say \"We do not know the current status of the refs in this\nsubmodule anymore\".\n\nThanks.\n"},{"id":"177296","messageId":"1318225574-18785-1-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"7v39f2rko5.fsf@alter.siamese.dyndns.org","subject":"[PATCH 0/2] Provide API to invalidate refs cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T05:46:12Z","receivedAt":"2011-10-10T05:46:12Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"I am happy to provide the API; see the following patches.\n\nBut I won't have time to figure out who, outside of refs.c, has to\n*call* invalidate_cached_refs(). The candidates that I know off the\ntop of my head are git-clone, git-submodule, and git-pack-refs. It\nwould be great if experts in those areas would insert calls to\ninvalidate_cached_refs() where needed.\n\nEven better would be if the meddlesome code were changed to use the\nrefs API. I'd be happy to help expanding the refs API if needed to\naccommodate your needs.\n\nMichael Haggerty (2):\n invalidate_cached_refs(): take the submodule as parameter\n invalidate_cached_refs(): expose this function in refs API\n\n refs.c | 12 ++++--------\n refs.h | 8 ++++++++\n 2 files changed, 12 insertions(+), 8 deletions(-)\n\n-- \n1.7.7.rc2\n"},{"id":"177297","messageId":"1318225574-18785-2-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318225574-18785-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 1/2] invalidate_cached_refs(): take the submodule as parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T05:46:13Z","receivedAt":"2011-10-10T05:46:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of invalidating the refs cache on an all-or-nothing basis,\nallow the cache for individual submodules to be invalidated.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 ++++--------\n 1 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2cb93e2..49b73c4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -223,13 +223,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_cached_refs(void)\n+static void invalidate_cached_refs(const char *submodule)\n {\n-\tstruct cached_refs *refs = cached_refs;\n-\twhile (refs) {\n-\t\tclear_cached_refs(refs);\n-\t\trefs = refs->next;\n-\t}\n+\tclear_cached_refs(get_cached_refs(submodule));\n }\n \n static struct ref_list *read_packed_refs(FILE *f)\n@@ -1212,7 +1208,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(refname);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_cached_refs();\n+\tinvalidate_cached_refs(NULL);\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1511,7 +1507,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_cached_refs();\n+\tinvalidate_cached_refs(NULL);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177298","messageId":"1318225574-18785-3-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318225574-18785-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH 2/2] invalidate_cached_refs(): expose this function in refs API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T05:46:14Z","receivedAt":"2011-10-10T05:46:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make invalidate_cached_refs() an official part of the refs API. It is\ncurrently a fact of life that code outside of refs.c mucks about with\nreferences. This change gives such code a way of informing the refs\nmodule that it should no longer trust its cache.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n refs.h | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 49b73c4..0483ecc 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_cached_refs(const char *submodule)\n+void invalidate_cached_refs(const char *submodule)\n {\n \tclear_cached_refs(get_cached_refs(submodule));\n }\ndiff --git a/refs.h b/refs.h\nindex 5de06e5..63dc68c 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -77,6 +77,14 @@ extern void unlock_ref(struct ref_lock *lock);\n /** Writes sha1 into the ref specified by the lock. **/\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n+/*\n+ * Invalidate the reference cache for the specified submodule. Use\n+ * submodule=NULL to invalidate the cache for the main module. This\n+ * function must be called if references are changed via a mechanism\n+ * other than the refs API.\n+ */\n+extern void invalidate_cached_refs(const char *submodule);\n+\n /** Setup reflog before using. **/\n int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n \n-- \n1.7.7.rc2\n"},{"id":"177305","messageId":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318225574-18785-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 0/7] Provide API to invalidate refs cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:17Z","receivedAt":"2011-10-10T08:24:17Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Sorry for superseding my own patch series, but I prefer this patch\nseries to the one that I submitted earlier today:\n\n1. An API function deserves a more carefully-selected name:\n invalidate_ref_cache().\n\n2. It gives me a chance to submit the first \"bite\" of my scalable-refs\n changes.\n\nThese patches apply on top of mh/iterate-refs, which is in next but\nnot in master.\n\nThis patch series provides an API for external code to invalidate the\nref cache that is used internally to refs.c. It also allows code\n*within* refs.c to invalidate only the packed or only the loose refs\nfor a module/submodule.\n\nIMPORTANT:\n\nI won't myself have time to figure out who, outside of refs.c, has to\n*call* invalidate_ref_cache(). The candidates that I know off the top\nof my head are git-clone, git-submodule, and git-pack-refs. It would\nbe great if experts in those areas would insert calls to\ninvalidate_ref_cache() where needed.\n\nEven better would be if the meddlesome code were changed to use the\nrefs API. I'd be happy to help expanding the refs API if needed to\naccommodate your needs.\n\nThis is why the API for invalidating only packed or loose refs is\nprivate. After code outside refs.c is changed to use the refs API, it\nwill get the optimal behavior for free (and at that time\ninvalidate_ref_cache() can be removed again).\n\nMichael Haggerty (7):\n invalidate_ref_cache(): rename function from invalidate_cached_refs()\n invalidate_ref_cache(): take the submodule as parameter\n invalidate_ref_cache(): expose this function in refs API\n clear_cached_refs(): rename parameter\n clear_cached_refs(): extract two new functions\n write_ref_sha1(): only invalidate the loose ref cache\n clear_cached_refs(): inline function\n\n refs.c | 34 +++++++++++++++++++---------------\n refs.h | 8 ++++++++\n 2 files changed, 27 insertions(+), 15 deletions(-)\n\n-- \n1.7.7.rc2\n"},{"id":"177306","messageId":"1318235064-25915-2-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:18Z","receivedAt":"2011-10-10T08:24:18Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is the cache that is being invalidated, not the references.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 2cb93e2..56e4254 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_cached_refs(void)\n+static void invalidate_ref_cache(void)\n {\n \tstruct cached_refs *refs = cached_refs;\n \twhile (refs) {\n@@ -1212,7 +1212,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(refname);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_cached_refs();\n+\tinvalidate_ref_cache();\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1511,7 +1511,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_cached_refs();\n+\tinvalidate_ref_cache();\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177307","messageId":"1318235064-25915-3-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:19Z","receivedAt":"2011-10-10T08:24:19Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of invalidating the ref cache on an all-or-nothing basis,\nallow the cache for individual submodules to be invalidated.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 ++++--------\n 1 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 56e4254..89b2a0e 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -223,13 +223,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_ref_cache(void)\n+static void invalidate_ref_cache(const char *submodule)\n {\n-\tstruct cached_refs *refs = cached_refs;\n-\twhile (refs) {\n-\t\tclear_cached_refs(refs);\n-\t\trefs = refs->next;\n-\t}\n+\tclear_cached_refs(get_cached_refs(submodule));\n }\n \n static struct ref_list *read_packed_refs(FILE *f)\n@@ -1212,7 +1208,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(refname);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_ref_cache();\n+\tinvalidate_ref_cache(NULL);\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1511,7 +1507,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_ref_cache();\n+\tinvalidate_ref_cache(NULL);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177311","messageId":"1318235064-25915-4-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 3/7] invalidate_ref_cache(): expose this function in refs API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:20Z","receivedAt":"2011-10-10T08:24:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make invalidate_ref_cache() an official part of the refs API. It is\ncurrently a fact of life that code outside of refs.c mucks about with\nreferences. This change gives such code a way of informing the refs\nmodule that it should no longer trust its cache.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n refs.h | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 89b2a0e..fb46cf5 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_ref_cache(const char *submodule)\n+void invalidate_ref_cache(const char *submodule)\n {\n \tclear_cached_refs(get_cached_refs(submodule));\n }\ndiff --git a/refs.h b/refs.h\nindex 5de06e5..3ddc4e4 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -77,6 +77,14 @@ extern void unlock_ref(struct ref_lock *lock);\n /** Writes sha1 into the ref specified by the lock. **/\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n+/*\n+ * Invalidate the reference cache for the specified submodule. Use\n+ * submodule=NULL to invalidate the cache for the main module. This\n+ * function must be called if references are changed via a mechanism\n+ * other than the refs API.\n+ */\n+extern void invalidate_ref_cache(const char *submodule);\n+\n /** Setup reflog before using. **/\n int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n \n-- \n1.7.7.rc2\n"},{"id":"177304","messageId":"1318235064-25915-5-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 4/7] clear_cached_refs(): rename parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:21Z","receivedAt":"2011-10-10T08:24:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"...for consistency with the rest of this module.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 14 +++++++-------\n 1 files changed, 7 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex fb46cf5..9f004ce 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -175,14 +175,14 @@ static void free_ref_list(struct ref_list *list)\n \t}\n }\n \n-static void clear_cached_refs(struct cached_refs *ca)\n+static void clear_cached_refs(struct cached_refs *refs)\n {\n-\tif (ca->did_loose && ca->loose)\n-\t\tfree_ref_list(ca->loose);\n-\tif (ca->did_packed && ca->packed)\n-\t\tfree_ref_list(ca->packed);\n-\tca->loose = ca->packed = NULL;\n-\tca->did_loose = ca->did_packed = 0;\n+\tif (refs->did_loose && refs->loose)\n+\t\tfree_ref_list(refs->loose);\n+\tif (refs->did_packed && refs->packed)\n+\t\tfree_ref_list(refs->packed);\n+\trefs->loose = refs->packed = NULL;\n+\trefs->did_loose = refs->did_packed = 0;\n }\n \n static struct cached_refs *create_cached_refs(const char *submodule)\n-- \n1.7.7.rc2\n"},{"id":"177308","messageId":"1318235064-25915-6-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 5/7] clear_cached_refs(): extract two new functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:22Z","receivedAt":"2011-10-10T08:24:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract two new functions from clear_cached_refs():\nclear_loose_ref_cache() and clear_packed_ref_cache().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 22 +++++++++++++++++-----\n 1 files changed, 17 insertions(+), 5 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9f004ce..6e480ad 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -175,14 +175,26 @@ static void free_ref_list(struct ref_list *list)\n \t}\n }\n \n-static void clear_cached_refs(struct cached_refs *refs)\n+static void clear_cached_packed_refs(struct cached_refs *refs)\n {\n-\tif (refs->did_loose && refs->loose)\n-\t\tfree_ref_list(refs->loose);\n \tif (refs->did_packed && refs->packed)\n \t\tfree_ref_list(refs->packed);\n-\trefs->loose = refs->packed = NULL;\n-\trefs->did_loose = refs->did_packed = 0;\n+\trefs->packed = NULL;\n+\trefs->did_packed = 0;\n+}\n+\n+static void clear_cached_loose_refs(struct cached_refs *refs)\n+{\n+\tif (refs->did_loose && refs->loose)\n+\t\tfree_ref_list(refs->loose);\n+\trefs->loose = NULL;\n+\trefs->did_loose = 0;\n+}\n+\n+static void clear_cached_refs(struct cached_refs *refs)\n+{\n+\tclear_cached_packed_refs(refs);\n+\tclear_cached_loose_refs(refs);\n }\n \n static struct cached_refs *create_cached_refs(const char *submodule)\n-- \n1.7.7.rc2\n"},{"id":"177309","messageId":"1318235064-25915-7-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 6/7] write_ref_sha1(): only invalidate the loose ref cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:23Z","receivedAt":"2011-10-10T08:24:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since write_ref_sha1() can only write loose refs and cannot write\nsymbolic refs, there is no need for it to invalidate the packed ref\ncache.\n\nSuggested by: Martin Fick \n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 6e480ad..01a5958 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1519,7 +1519,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_ref_cache(NULL);\n+\tclear_cached_loose_refs(get_cached_refs(NULL));\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177310","messageId":"1318235064-25915-8-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v2 7/7] clear_cached_refs(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-10T08:24:24Z","receivedAt":"2011-10-10T08:24:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"clear_cached_refs() was only called from one place, so inline it\nthere.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 +++-------\n 1 files changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 01a5958..bf53189 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -191,12 +191,6 @@ static void clear_cached_loose_refs(struct cached_refs *refs)\n \trefs->did_loose = 0;\n }\n \n-static void clear_cached_refs(struct cached_refs *refs)\n-{\n-\tclear_cached_packed_refs(refs);\n-\tclear_cached_loose_refs(refs);\n-}\n-\n static struct cached_refs *create_cached_refs(const char *submodule)\n {\n \tint len;\n@@ -237,7 +231,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \n void invalidate_ref_cache(const char *submodule)\n {\n-\tclear_cached_refs(get_cached_refs(submodule));\n+\tstruct cached_refs *refs = get_cached_refs(submodule);\n+\tclear_cached_packed_refs(refs);\n+\tclear_cached_loose_refs(refs);\n }\n \n static struct ref_list *read_packed_refs(FILE *f)\n-- \n1.7.7.rc2\n"},{"id":"177325","messageId":"20111010195325.GA5981@sandbox-rc","threadId":"28085","inReplyTo":"4E918194.5060102@alum.mit.edu","subject":"Re: Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-10-10T19:53:26Z","receivedAt":"2011-10-10T19:53:26Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi Michael,\n\nOn Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:\n> On 08/17/2011 12:45 AM, Junio C Hamano wrote:\n> > All the changes except for this one made sense to me, but I am not sure\n> > about this one. How often do we look into different submodule refs in the\n> > same process over and over again?\n> \n> I am having pangs of uncertainty about this patch.\n\nCurrently when doing a 3 way merge of submodule hashes and we find a\nconflict. We then use this api to look into the submodule to search for\ncommit suggestions which contain both sides.\n\nIf there are multiple submodules that conflict in this way we look into\nthose as well.\n\nSince the setup_revision() api can currently not be used to safely\niterate twice over the same submodule my patch\n\n\tallow multiple calls to submodule merge search for the same path\n\nrewrites the search into using a child process. AFAIK the submodule ref\niteration api would then even be unused.\n\n> Previous to this patch, the submodule reference cache was only used for\n> the duration of one call to do_for_each_ref(). (It was not *discarded*\n> until later, but the old cache was never reused.) Therefore, the\n> submodule reference cache was implicitly invalidated between successive\n> uses.\n\nThe implicit discarding was just done because it was the quickest way to\nget a handle on submodule refs from the main process. There was no need\nthat they get reloaded every time.\n\n> After this change, submodule ref caches are invalidated whenever\n> invalidate_cached_refs() is called. But this function is static, and it\n> is only called when main-module refs are changed.\n> \n> AFAIK there is no way within refs.c to add, modify, or delete a\n> submodule reference. But if other code modifies submodule references\n> directly, then the submodule ref cache in refs.c would become stale.\n> Moreover, there is currently no API for invalidating the cache.\n> \n> So I think I need help from a submodule guru (Heiko?) who can tell me\n> what is done with submodule references and whether they might be\n> modified while a git process is executing in the main module. If so,\n> then either this patch has to be withdrawn, or more work has to be put\n> in to make such code invalidate the submodule reference cache.\n\nAt least in my code there is no place where a submodule ref is changed.\nI only used it for merging submodule which only modifies the main\nmodule. So I would say its currently safe to assume that submodule refs\ndo not get modified. If we do need that later on we can still add\ninvalidation for submodule refs.\n\nCheers Heiko\n"},{"id":"177346","messageId":"7vfwj0iehl.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1318235064-25915-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T00:00:38Z","receivedAt":"2011-10-11T00:00:38Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It is the cache that is being invalidated, not the references.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\nAlthough I think one can say \"ref cache is the container for cached refs\"\nand invalidating the \"ref cache\" as the container and invalidating the\n\"cached refs\" as a collection mean essentially the same thing, probably\nthe new name makes more sense.\n\n> refs.c | 6 +++---\n> 1 files changed, 3 insertions(+), 3 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 2cb93e2..56e4254 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -223,7 +223,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n> \treturn refs;\n> }\n> \n> -static void invalidate_cached_refs(void)\n> +static void invalidate_ref_cache(void)\n> {\n> \tstruct cached_refs *refs = cached_refs;\n> \twhile (refs) {\n> @@ -1212,7 +1212,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n> \tret |= repack_without_ref(refname);\n> \n> \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n> -\tinvalidate_cached_refs();\n> +\tinvalidate_ref_cache();\n> \tunlock_ref(lock);\n> \treturn ret;\n> }\n> @@ -1511,7 +1511,7 @@ int write_ref_sha1(struct ref_lock *lock,\n> \t\tunlock_ref(lock);\n> \t\treturn -1;\n> \t}\n> -\tinvalidate_cached_refs();\n> +\tinvalidate_ref_cache();\n> \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n> \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n> \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n"},{"id":"177349","messageId":"7vty7ggzum.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1318235064-25915-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v2 0/7] Provide API to invalidate refs cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T00:02:09Z","receivedAt":"2011-10-11T00:02:09Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> These patches apply on top of mh/iterate-refs, which is in next but\n> not in master.\n\nBuilding your series on mh/iterate-refs would unfortunately make the\nconflict resolution worse. It would have been better if this were based on\na merge between mh/iterate-refs and jp/get-ref-dir-unsorted (which already\nhas happened on 'master' as of fifteen minutes ago).\n\nI could rebase your series, but it always is more error prone to have\nsomebody who is not the original author rebase a series than the original\nauthor build for the intended base tree from the beginning.\n\nThanks.\n"},{"id":"177354","messageId":"4E93C232.9090400@alum.mit.edu","threadId":"28085","inReplyTo":"20111010195325.GA5981@sandbox-rc","subject":"Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-11T04:12:34Z","receivedAt":"2011-10-11T04:12:34Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/10/2011 09:53 PM, Heiko Voigt wrote:\n> On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:\n> Since the setup_revision() api can currently not be used to safely\n> iterate twice over the same submodule my patch\n> \n> \tallow multiple calls to submodule merge search for the same path\n> \n> rewrites the search into using a child process. AFAIK the submodule ref\n> iteration api would then even be unused.\n\nIf your patch is accepted, then we should check whether anything should\nbe ripped out.\n\n> At least in my code there is no place where a submodule ref is changed.\n> I only used it for merging submodule which only modifies the main\n> module. So I would say its currently safe to assume that submodule refs\n> do not get modified. If we do need that later on we can still add\n> invalidation for submodule refs.\n\nOK, thanks!\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177355","messageId":"4E93D932.6020001@alum.mit.edu","threadId":"28085","inReplyTo":"7vty7ggzum.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 0/7] Provide API to invalidate refs cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-11T05:50:42Z","receivedAt":"2011-10-11T05:50:42Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/11/2011 02:02 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> These patches apply on top of mh/iterate-refs, which is in next but\n>> not in master.\n> \n> Building your series on mh/iterate-refs would unfortunately make the\n> conflict resolution worse. It would have been better if this were based on\n> a merge between mh/iterate-refs and jp/get-ref-dir-unsorted (which already\n> has happened on 'master' as of fifteen minutes ago).\n\nAAAAaaaarrrgghh did that really have to happen?!?\n\n> I could rebase your series, but it always is more error prone to have\n> somebody who is not the original author rebase a series than the original\n> author build for the intended base tree from the beginning.\n\nI don't mind rebasing this little series on jp/get-ref-dir-unsorted.\nBut it's going to be an utter nightmare (as in, \"can I even muster the\nenergy to do so much pointless work\") to rebase my much bigger\nhierarchical-refs series [1] onto jp/get-ref-dir-unsorted. The latter\nmakes changes all over refs.c and changes several things at once\n(separate ref_entry out of ref_list, change current_ref to a ref_entry*,\nrename ref_list to ref_array, change data structure to array plus\nrewrite all loops, change to binary search). And\njp/get-ref-dir-unsorted includes a change that was inspired by my patch\nseries [2], so it is not like jp/get-ref-dir-unsorted was developed in\ncomplete isolation from hierarchical-refs.\n\nAnd this rebase will be work with no benefit, because my series includes\nall of the improvements of jp/get-ref-dir-unsorted plus much more. But\nmy change to the data structure is implemented in a different order and\nfollowing other improvements. For example, I add a lot of comments,\nchange a lot of code to use the cached_refs data structure more\nconsistently, and accommodate partly-sorted lists by the time my patch\nseries includes everything that is in jp/get-ref-dir-unsorted.\n\nRebasing 78 patches is going to be a morass of clerical work. Is there\nany alternative?\n\nMichael\n\nPS: I see that some confusion might have been caused by one of my emails\n[3], where I mistakenly approved of the merge of jp/get-ref-dir-unsorted\n(meaning the \"Don't sort ref_list too early\" part) just before asking\nthat jp/get-ref-dir-unsorted not be merged (meaning the rest). So maybe\nI brought this whole mess down on my own head :-(\n\n[1] branch hierarchical-refs on git://github.com/mhagger/git.git\n[2] http://marc.info/?l=git&m=131740585620461&w=2\n[3] http://marc.info/?l=git&m=131753257824405&w=2\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177356","messageId":"4E93D9DF.2080004@alum.mit.edu","threadId":"28085","inReplyTo":"7vfwj0iehl.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v2 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-11T05:53:35Z","receivedAt":"2011-10-11T05:53:35Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/11/2011 02:00 AM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> It is the cache that is being invalidated, not the references.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n> Although I think one can say \"ref cache is the container for cached refs\"\n> and invalidating the \"ref cache\" as the container and invalidating the\n> \"cached refs\" as a collection mean essentially the same thing, probably\n> the new name makes more sense.\n\nI certainly didn't mean to imply that the old name was incorrect. I\njust think that the new name removes a tiny bit of ambiguity.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177359","messageId":"42eba56f4da8f642e806a667bfa22884@quantumfyre.co.uk","threadId":"28085","inReplyTo":"4E93D932.6020001@alum.mit.edu","subject":"Re: [PATCH v2 0/7] Provide API to invalidate refs cache","fromName":"Julian Phillips","fromEmail":"julian@quantumfyre.co.uk","sentAt":"2011-10-11T08:09:58Z","receivedAt":"2011-10-11T08:09:58Z","isPatch":true,"sender":{"key":"julian@quantumfyre.co.uk","avatar":"https://avatars.githubusercontent.com/u/948888?v=4"},"body":"On Tue, 11 Oct 2011 07:50:42 +0200, Michael Haggerty wrote:\n> And this rebase will be work with no benefit, because my series \n> includes\n> all of the improvements of jp/get-ref-dir-unsorted plus much more. \n> But\n> my change to the data structure is implemented in a different order \n> and\n> following other improvements. For example, I add a lot of comments,\n> change a lot of code to use the cached_refs data structure more\n> consistently, and accommodate partly-sorted lists by the time my \n> patch\n> series includes everything that is in jp/get-ref-dir-unsorted.\n>\n> Rebasing 78 patches is going to be a morass of clerical work. Is \n> there\n> any alternative?\n\nIf you create a new commit that reverts the problematic changes to \nrefs.c with some suitable message about doing the same thing more \npiecemeal as the first change of a new series, and rebase your changes \non top, you should be able to create a 79 patch series with little work. \nDunno if that would acceptable for merging?\n\n-- \nJulian\n"},{"id":"177380","messageId":"7v4nzfh21o.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"4E93D932.6020001@alum.mit.edu","subject":"Re: [PATCH v2 0/7] Provide API to invalidate refs cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-11T17:26:59Z","receivedAt":"2011-10-11T17:26:59Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 10/11/2011 02:02 AM, Junio C Hamano wrote:\n> ...\n>> I could rebase your series, but it always is more error prone to have\n>> somebody who is not the original author rebase a series than the original\n>> author build for the intended base tree from the beginning.\n>\n> I don't mind rebasing this little series on jp/get-ref-dir-unsorted.\n> ...\n> Rebasing 78 patches is going to be a morass of clerical work.\n\nI do not think it is \"clerical\" in the first place.\n\nRealistically, I expect that a 50+ patch series that touch fairly an\nimportant part of the system to take 2 cycles and a half before it hits a\nreleased version, judging from our recent experience with the recursive\nmerge fix-up series.\n\nWhen we already have a patch that has been discussed well enough on the\nlist to fix somebody's real world problem, can we afford to block it and\ngive an exclusive write lock to part of the codebase for 2 cycles to your\nseries, or anybody's for that matter?\n\n> Is there any alternative?\n\nI think an alternative is not to hold on to a series before it gets so\nlarge to make you feel adjusting to the needs to other changes in the\ncodebase is \"clerical\". Commit often and early while developing the\ninitial pass, re-read often and throughout the whole process looking for\nthings you regret you would have done in early in the series that you\ndidn't (aka \"oops, here is a fixup for the thinko in the early patch in\nthe series), and clean-up early while preparing to publish. Reorder the\nparts that you are more confident that they do not need to change to come\nearly in the series, and unleash these early parts when you reach certain\nconfidence level.\n\nI think your iterate-refs series was an example of good execution. It made\nthe codeflow a lot clearer by reducing the special casing of the submodule\nparameter. In your grand scheme of things (e.g. read only parts of the ref\nnamespace as needed) you might consider it a mere side effect, but the\nseries by itself was a good thing to have.\n\nSometimes you may feel that a part of your series when taken out of\ncontext would not justify itself like the iterate-refs series did, until\nlater parts of the series start taking advantage of the change. But that\nis what commit log messages are for: e.g. \"this change to encapsulate\nthese global variables into a single structure does not make a difference\nin the current codebase, but in a later patch this and that callers will\nneed additional pieces of information passed aruond in the callchain, and\nwill add new members to the structure\".\n\n> ... So maybe\n> I brought this whole mess down on my own head :-(\n\nNo, it is not anybody's fault in particular. That's life and open source.\n"},{"id":"177383","messageId":"20111011174101.GC2647@sandbox-rc","threadId":"28085","inReplyTo":"4E93C232.9090400@alum.mit.edu","subject":"Re: Re: [PATCH 6/6] Retain caches of submodule refs","fromName":"Heiko Voigt","fromEmail":"hvoigt@hvoigt.net","sentAt":"2011-10-11T17:41:01Z","receivedAt":"2011-10-11T17:41:01Z","isPatch":true,"sender":{"key":"hvoigt@hvoigt.net","avatar":"https://avatars.githubusercontent.com/u/184958?v=4"},"body":"Hi,\n\nOn Tue, Oct 11, 2011 at 06:12:34AM +0200, Michael Haggerty wrote:\n> On 10/10/2011 09:53 PM, Heiko Voigt wrote:\n> > On Sun, Oct 09, 2011 at 01:12:20PM +0200, Michael Haggerty wrote:\n> > Since the setup_revision() api can currently not be used to safely\n> > iterate twice over the same submodule my patch\n> > \n> > \tallow multiple calls to submodule merge search for the same path\n> > \n> > rewrites the search into using a child process. AFAIK the submodule ref\n> > iteration api would then even be unused.\n> \n> If your patch is accepted, then we should check whether anything should\n> be ripped out.\n\nI would rather like to extend the setup_revision() api so that we can\niterate multiple times. Additionally I have a feeling that this API\nmight be useful for further extensions of the recursive-push support of\nsubmodules which is currently under development.\n\nSo in summary: I would like to wait with ripping anything out until we\nget to a final state with submodule support.\n\nCheers Heiko\n"},{"id":"177465","messageId":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"7vty7ggzum.fsf@alter.siamese.dyndns.org","subject":"[PATCH v3 0/7] Provide API to invalidate refs cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:20Z","receivedAt":"2011-10-12T18:44:20Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"These patches are re-rolled onto master.\n\nThis patch series provides an API for external code to invalidate the\nref cache that is used internally to refs.c. It also allows code\n*within* refs.c to invalidate only the packed or only the loose refs\nfor a module/submodule.\n\nIMPORTANT:\n\nI won't myself have time to figure out who, outside of refs.c, has to\n*call* invalidate_ref_cache(). The candidates that I know off the top\nof my head are git-clone, git-submodule [1], and git-pack-refs. It\nwould be great if experts in those areas would insert calls to\ninvalidate_ref_cache() where needed.\n\nEven better would be if the meddlesome code were changed to use the\nrefs API. I'd be happy to help expanding the refs API if needed to\naccommodate your needs.\n\nThis is why the API for invalidating only packed or loose refs is\nprivate. After code outside refs.c is changed to use the refs API, it\nwill get the optimal behavior for free (and at that time\ninvalidate_ref_cache() can be removed again).\n\n[1] http://marc.info/?l=git&m=131827641227965&w=2\n In this mailing list thread, Heiko Voigt stated that git-submodule\n does not modify any references, so it should not have to use the\n API.\n\nMichael Haggerty (7):\n invalidate_ref_cache(): rename function from invalidate_cached_refs()\n invalidate_ref_cache(): take the submodule as parameter\n invalidate_ref_cache(): expose this function in refs API\n clear_cached_refs(): rename parameter\n clear_cached_refs(): extract two new functions\n write_ref_sha1(): only invalidate the loose ref cache\n clear_cached_refs(): inline function\n\n refs.c | 31 +++++++++++++++++--------------\n refs.h | 8 ++++++++\n 2 files changed, 25 insertions(+), 14 deletions(-)\n\n-- \n1.7.7.rc2\n"},{"id":"177461","messageId":"1318445067-19279-2-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:21Z","receivedAt":"2011-10-12T18:44:21Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"It is the cache that is being invalidated, not the references.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 6 +++---\n 1 files changed, 3 insertions(+), 3 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9911c97..120b8e4 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_cached_refs(void)\n+static void invalidate_ref_cache(void)\n {\n \tstruct cached_refs *refs = cached_refs;\n \twhile (refs) {\n@@ -1228,7 +1228,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(refname);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_cached_refs();\n+\tinvalidate_ref_cache();\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1527,7 +1527,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_cached_refs();\n+\tinvalidate_ref_cache();\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177464","messageId":"1318445067-19279-3-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:22Z","receivedAt":"2011-10-12T18:44:22Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Instead of invalidating the ref cache on an all-or-nothing basis,\nallow the cache for individual submodules to be invalidated.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 ++++--------\n 1 files changed, 4 insertions(+), 8 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 120b8e4..cc72609 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_ref_cache(void)\n+static void invalidate_ref_cache(const char *submodule)\n {\n-\tstruct cached_refs *refs = cached_refs;\n-\twhile (refs) {\n-\t\tclear_cached_refs(refs);\n-\t\trefs = refs->next;\n-\t}\n+\tclear_cached_refs(get_cached_refs(submodule));\n }\n \n static void read_packed_refs(FILE *f, struct ref_array *array)\n@@ -1228,7 +1224,7 @@ int delete_ref(const char *refname, const unsigned char *sha1, int delopt)\n \tret |= repack_without_ref(refname);\n \n \tunlink_or_warn(git_path(\"logs/%s\", lock->ref_name));\n-\tinvalidate_ref_cache();\n+\tinvalidate_ref_cache(NULL);\n \tunlock_ref(lock);\n \treturn ret;\n }\n@@ -1527,7 +1523,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_ref_cache();\n+\tinvalidate_ref_cache(NULL);\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177462","messageId":"1318445067-19279-4-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 3/7] invalidate_ref_cache(): expose this function in refs API","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:23Z","receivedAt":"2011-10-12T18:44:23Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Make invalidate_ref_cache() an official part of the refs API. It is\ncurrently a fact of life that code outside of refs.c mucks about with\nreferences. This change gives such code a way of informing the refs\nmodule that it should no longer trust its cache.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n refs.h | 8 ++++++++\n 2 files changed, 9 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex cc72609..b08d476 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \treturn refs;\n }\n \n-static void invalidate_ref_cache(const char *submodule)\n+void invalidate_ref_cache(const char *submodule)\n {\n \tclear_cached_refs(get_cached_refs(submodule));\n }\ndiff --git a/refs.h b/refs.h\nindex 0229c57..f439c54 100644\n--- a/refs.h\n+++ b/refs.h\n@@ -80,6 +80,14 @@ extern void unlock_ref(struct ref_lock *lock);\n /** Writes sha1 into the ref specified by the lock. **/\n extern int write_ref_sha1(struct ref_lock *lock, const unsigned char *sha1, const char *msg);\n \n+/*\n+ * Invalidate the reference cache for the specified submodule. Use\n+ * submodule=NULL to invalidate the cache for the main module. This\n+ * function must be called if references are changed via a mechanism\n+ * other than the refs API.\n+ */\n+extern void invalidate_ref_cache(const char *submodule);\n+\n /** Setup reflog before using. **/\n int log_ref_setup(const char *ref_name, char *logfile, int bufsize);\n \n-- \n1.7.7.rc2\n"},{"id":"177463","messageId":"1318445067-19279-5-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 4/7] clear_cached_refs(): rename parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:24Z","receivedAt":"2011-10-12T18:44:24Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"...for consistency with the rest of this module.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 12 ++++++------\n 1 files changed, 6 insertions(+), 6 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex b08d476..79e3576 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -158,13 +158,13 @@ static void free_ref_array(struct ref_array *array)\n \tarray->refs = NULL;\n }\n \n-static void clear_cached_refs(struct cached_refs *ca)\n+static void clear_cached_refs(struct cached_refs *refs)\n {\n-\tif (ca->did_loose)\n-\t\tfree_ref_array(&ca->loose);\n-\tif (ca->did_packed)\n-\t\tfree_ref_array(&ca->packed);\n-\tca->did_loose = ca->did_packed = 0;\n+\tif (refs->did_loose)\n+\t\tfree_ref_array(&refs->loose);\n+\tif (refs->did_packed)\n+\t\tfree_ref_array(&refs->packed);\n+\trefs->did_loose = refs->did_packed = 0;\n }\n \n static struct cached_refs *create_cached_refs(const char *submodule)\n-- \n1.7.7.rc2\n"},{"id":"177466","messageId":"1318445067-19279-6-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 5/7] clear_cached_refs(): extract two new functions","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:25Z","receivedAt":"2011-10-12T18:44:25Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Extract two new functions from clear_cached_refs():\nclear_loose_ref_cache() and clear_packed_ref_cache().\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 19 +++++++++++++++----\n 1 files changed, 15 insertions(+), 4 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 79e3576..9d962bd 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -158,13 +158,24 @@ static void free_ref_array(struct ref_array *array)\n \tarray->refs = NULL;\n }\n \n-static void clear_cached_refs(struct cached_refs *refs)\n+static void clear_cached_packed_refs(struct cached_refs *refs)\n {\n-\tif (refs->did_loose)\n-\t\tfree_ref_array(&refs->loose);\n \tif (refs->did_packed)\n \t\tfree_ref_array(&refs->packed);\n-\trefs->did_loose = refs->did_packed = 0;\n+\trefs->did_packed = 0;\n+}\n+\n+static void clear_cached_loose_refs(struct cached_refs *refs)\n+{\n+\tif (refs->did_loose)\n+\t\tfree_ref_array(&refs->loose);\n+\trefs->did_loose = 0;\n+}\n+\n+static void clear_cached_refs(struct cached_refs *refs)\n+{\n+\tclear_cached_packed_refs(refs);\n+\tclear_cached_loose_refs(refs);\n }\n \n static struct cached_refs *create_cached_refs(const char *submodule)\n-- \n1.7.7.rc2\n"},{"id":"177468","messageId":"1318445067-19279-7-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 6/7] write_ref_sha1(): only invalidate the loose ref cache","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:26Z","receivedAt":"2011-10-12T18:44:26Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"Since write_ref_sha1() can only write loose refs and cannot write\nsymbolic refs, there is no need for it to invalidate the packed ref\ncache.\n\nSuggested by: Martin Fick \n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 2 +-\n 1 files changed, 1 insertions(+), 1 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9d962bd..9e9edf7 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -1534,7 +1534,7 @@ int write_ref_sha1(struct ref_lock *lock,\n \t\tunlock_ref(lock);\n \t\treturn -1;\n \t}\n-\tinvalidate_ref_cache(NULL);\n+\tclear_cached_loose_refs(get_cached_refs(NULL));\n \tif (log_ref_write(lock->ref_name, lock->old_sha1, sha1, logmsg) < 0 ||\n \t (strcmp(lock->ref_name, lock->orig_ref_name) &&\n \t log_ref_write(lock->orig_ref_name, lock->old_sha1, sha1, logmsg) < 0)) {\n-- \n1.7.7.rc2\n"},{"id":"177467","messageId":"1318445067-19279-8-git-send-email-mhagger@alum.mit.edu","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"[PATCH v3 7/7] clear_cached_refs(): inline function","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T18:44:27Z","receivedAt":"2011-10-12T18:44:27Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"clear_cached_refs() was only called from one place, so inline it\nthere.\n\nSigned-off-by: Michael Haggerty \n---\n refs.c | 10 +++-------\n 1 files changed, 3 insertions(+), 7 deletions(-)\n\ndiff --git a/refs.c b/refs.c\nindex 9e9edf7..409314d 100644\n--- a/refs.c\n+++ b/refs.c\n@@ -172,12 +172,6 @@ static void clear_cached_loose_refs(struct cached_refs *refs)\n \trefs->did_loose = 0;\n }\n \n-static void clear_cached_refs(struct cached_refs *refs)\n-{\n-\tclear_cached_packed_refs(refs);\n-\tclear_cached_loose_refs(refs);\n-}\n-\n static struct cached_refs *create_cached_refs(const char *submodule)\n {\n \tint len;\n@@ -215,7 +209,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n \n void invalidate_ref_cache(const char *submodule)\n {\n-\tclear_cached_refs(get_cached_refs(submodule));\n+\tstruct cached_refs *refs = get_cached_refs(submodule);\n+\tclear_cached_packed_refs(refs);\n+\tclear_cached_loose_refs(refs);\n }\n \n static void read_packed_refs(FILE *f, struct ref_array *array)\n-- \n1.7.7.rc2\n"},{"id":"177472","messageId":"7v1uui9g56.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1318445067-19279-2-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T19:14:13Z","receivedAt":"2011-10-12T19:14:13Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> It is the cache that is being invalidated, not the references.\n>\n> Signed-off-by: Michael Haggerty \n> ---\n\n> diff --git a/refs.c b/refs.c\n> index 9911c97..120b8e4 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n> \treturn refs;\n> }\n> \n> -static void invalidate_cached_refs(void)\n> +static void invalidate_ref_cache(void)\n> {\n> \tstruct cached_refs *refs = cached_refs;\n> \twhile (refs) {\n\nIf you call the operation \"invalidate ref_cache\", shouldn't the data\nstructure that holds that cache also be renamed to \"struct ref_cache\" from\n\"struct \"cached_refs\" at the same time?\n"},{"id":"177473","messageId":"7vwrca81c7.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1318445067-19279-3-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T19:19:20Z","receivedAt":"2011-10-12T19:19:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Instead of invalidating the ref cache on an all-or-nothing basis,\n> allow the cache for individual submodules to be invalidated.\n\nThat \"allow\" does not seem to describe what this patch does. It disallows\nthe wholesale invalidation and forces the caller to invalidate ref cache\nindividually.\n\nProbably that is what all the existing callers want, but I would have\nexpected that an existing feature would be kept, perhaps like this\ninstead:\n\n\tif (!submodule) {\n\t\tstruct ref_cache *c;\n for (c = ref_cache; c; c = c->next)\n \tclear_ref_cache(c);\n\t} else {\n\t\tclear_ref_cache(get_ref_cache(submodule);\n\t}\n\nNot a major \"vetoing\" objection, just a comment.\n\n> Signed-off-by: Michael Haggerty \n> ---\n> refs.c | 12 ++++--------\n> 1 files changed, 4 insertions(+), 8 deletions(-)\n>\n> diff --git a/refs.c b/refs.c\n> index 120b8e4..cc72609 100644\n> --- a/refs.c\n> +++ b/refs.c\n> @@ -202,13 +202,9 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n> \treturn refs;\n> }\n> \n> -static void invalidate_ref_cache(void)\n> +static void invalidate_ref_cache(const char *submodule)\n> {\n> -\tstruct cached_refs *refs = cached_refs;\n> -\twhile (refs) {\n> -\t\tclear_cached_refs(refs);\n> -\t\trefs = refs->next;\n> -\t}\n> +\tclear_cached_refs(get_cached_refs(submodule));\n> }\n"},{"id":"177477","messageId":"7vehyi80xo.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"1318445067-19279-1-git-send-email-mhagger@alum.mit.edu","subject":"Re: [PATCH v3 0/7] Provide API to invalidate refs cache","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-12T19:28:03Z","receivedAt":"2011-10-12T19:28:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> These patches are re-rolled onto master.\n\nThanks.\n"},{"id":"177493","messageId":"4E960F91.5020103@alum.mit.edu","threadId":"28085","inReplyTo":"7vwrca81c7.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T22:07:13Z","receivedAt":"2011-10-12T22:07:13Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/12/2011 09:19 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> Instead of invalidating the ref cache on an all-or-nothing basis,\n>> allow the cache for individual submodules to be invalidated.\n> \n> That \"allow\" does not seem to describe what this patch does. It disallows\n> the wholesale invalidation and forces the caller to invalidate ref cache\n> individually.\n> \n> Probably that is what all the existing callers want, but I would have\n> expected that an existing feature would be kept, perhaps like this\n> instead:\n> \n> \tif (!submodule) {\n> \t\tstruct ref_cache *c;\n> for (c = ref_cache; c; c = c->next)\n> \tclear_ref_cache(c);\n> \t} else {\n> \t\tclear_ref_cache(get_ref_cache(submodule);\n> \t}\n> \n> Not a major \"vetoing\" objection, just a comment.\n\nIndeed, it is currently not possible for code outside of refs.c to\nimplement \"forget everything\" using the \"forget one\" function (because\nthere is no API for getting the list of caches that are currently in\nmemory).\n\nA \"forget everything\" function might be useful for code that delegates\nto a subprocess, if it does not know what submodules the subprocess has\ntinkered with. Heiko, does that apply to the future submodule code?\n\nYour specific suggestion would not work because currently\nsubmodule==NULL signifies the main module. However, it would be easy to\nadd the few-line function when/if it is needed.\n\n\nI guess the bigger issue for me is whether the whole submodule cache\nthing is going to continue to be needed. I really am too ignorant of\nhow submodules work to be able to judge. From Heiko's recent email it\nsounds like things might be moving in the direction of \"top-level git\ndoesn't need to know much about submodules because it delegates to\nsubprocesses\". He also said that submodule references are not modified\nby the top-level git process, meaning that it might be sensible for the\nsubmodule reference cache to be less capable than the main module\nreference cache.\n\nBut if things move in the other direction (submodules handled by the\ntop-level git process), let alone if git is libified, then it seems\ninevitable that there will someday be a \"submodule\" object that keeps\ntrack of its own ref cache, with the submodule objects rather than the\nsubmodule reference caches looked up by submodule name.\n\nGiven that I'm still very new to the codebase, I'm mostly making\n\"peephole changes\" and so I'm happy to get your feedback about how this\nfits into the grand scheme of things.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177494","messageId":"4E9610BE.7000801@alum.mit.edu","threadId":"28085","inReplyTo":"7v1uui9g56.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 1/7] invalidate_ref_cache(): rename function from invalidate_cached_refs()","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-10-12T22:12:14Z","receivedAt":"2011-10-12T22:12:14Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/12/2011 09:14 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n> \n>> It is the cache that is being invalidated, not the references.\n>>\n>> Signed-off-by: Michael Haggerty \n>> ---\n> \n>> diff --git a/refs.c b/refs.c\n>> index 9911c97..120b8e4 100644\n>> --- a/refs.c\n>> +++ b/refs.c\n>> @@ -202,7 +202,7 @@ static struct cached_refs *get_cached_refs(const char *submodule)\n>> \treturn refs;\n>> }\n>> \n>> -static void invalidate_cached_refs(void)\n>> +static void invalidate_ref_cache(void)\n>> {\n>> \tstruct cached_refs *refs = cached_refs;\n>> \twhile (refs) {\n> \n> If you call the operation \"invalidate ref_cache\", shouldn't the data\n> structure that holds that cache also be renamed to \"struct ref_cache\" from\n> \"struct \"cached_refs\" at the same time?\n\nI don't think it is a logical necessity but I agree that it would be\nmore consistent. I'll make the change in the next round.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"177868","messageId":"7vmxczmrb0.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"4E960F91.5020103@alum.mit.edu","subject":"Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-10-17T18:00:35Z","receivedAt":"2011-10-17T18:00:35Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> On 10/12/2011 09:19 PM, Junio C Hamano wrote:\n>> Michael Haggerty writes:\n>> ...\n>> Probably that is what all the existing callers want, but I would have\n>> expected that an existing feature would be kept, perhaps like this\n>> instead:\n>> \n>> \tif (!submodule) {\n>> \t\tstruct ref_cache *c;\n>> for (c = ref_cache; c; c = c->next)\n>> \tclear_ref_cache(c);\n>> \t} else {\n>> \t\tclear_ref_cache(get_ref_cache(submodule);\n>> \t}\n> ...\n> Your specific suggestion would not work because currently\n> submodule==NULL signifies the main module. However, it would be easy to\n> add the few-line function when/if it is needed.\n\nI think \"submodule==NULL\" is probably a mistake; \"\" would make more sense\ngiven that you are storing the string in name[FLEX_ARRAY] field.\n"},{"id":"178764","messageId":"4EB26BA0.9030609@alum.mit.edu","threadId":"28085","inReplyTo":"7vmxczmrb0.fsf@alter.siamese.dyndns.org","subject":"Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Michael Haggerty","fromEmail":"mhagger@alum.mit.edu","sentAt":"2011-11-03T10:23:28Z","receivedAt":"2011-11-03T10:23:28Z","isPatch":true,"sender":{"key":"mhagger@alum.mit.edu","avatar":"https://avatars.githubusercontent.com/u/119718?v=4"},"body":"On 10/17/2011 08:00 PM, Junio C Hamano wrote:\n> Michael Haggerty writes:\n>> On 10/12/2011 09:19 PM, Junio C Hamano wrote:\n>>> Michael Haggerty writes:\n>>> ...\n>>> Probably that is what all the existing callers want, but I would have\n>>> expected that an existing feature would be kept, perhaps like this\n>>> instead:\n>>>\n>>> \tif (!submodule) {\n>>> \t\tstruct ref_cache *c;\n>>> for (c = ref_cache; c; c = c->next)\n>>> \tclear_ref_cache(c);\n>>> \t} else {\n>>> \t\tclear_ref_cache(get_ref_cache(submodule);\n>>> \t}\n>> ...\n>> Your specific suggestion would not work because currently\n>> submodule==NULL signifies the main module. However, it would be easy to\n>> add the few-line function when/if it is needed.\n> \n> I think \"submodule==NULL\" is probably a mistake; \"\" would make more sense\n> given that you are storing the string in name[FLEX_ARRAY] field.\n\nSorry I didn't respond to this earlier.\n\nThe public API convention (which predates my changes) is that \"char\n*submodule\" arguments either point at the relative path to the submodule\nor are NULL to denote the main module. But since these are stored\ninternally in a name[FLEX_ARRAY] field, I have been using \"\" internally\nto denote the main module. I believe that everything is done correctly,\nbut I admit that the use of different conventions internally and\nexternally is a potential source of programming errors.\n\nIf this is viewed as something that needs changing, the easiest thing\nwould probably be to add a \"const char *submodule\" in the ref_cache data\nstructure that either contains NULL or points at the name field, and to\nconsistently use the convention that the main module must always be\ndenoted by NULL. The space overhead would be negligible because the\nnumber of ref_cache objects is limited to the number of submodules plus 1.\n\nMichael\n\n-- \nMichael Haggerty\nmhagger@alum.mit.edu\nhttp://softwareswirl.blogspot.com/\n"},{"id":"178788","messageId":"7vr51pf30i.fsf@alter.siamese.dyndns.org","threadId":"28085","inReplyTo":"4EB26BA0.9030609@alum.mit.edu","subject":"Re: [PATCH v3 2/7] invalidate_ref_cache(): take the submodule as parameter","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2011-11-03T18:57:01Z","receivedAt":"2011-11-03T18:57:01Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Michael Haggerty writes:\n\n> Sorry I didn't respond to this earlier.\n>\n> The public API convention (which predates my changes) is that \"char\n> *submodule\" arguments either point at the relative path to the submodule\n> or are NULL to denote the main module. But since these are stored\n> internally in a name[FLEX_ARRAY] field, I have been using \"\" internally\n> to denote the main module. I believe that everything is done correctly,\n> but I admit that the use of different conventions internally and\n> externally is a potential source of programming errors.\n\nYes, it would have been better if the original also used \"\". After all,\nthat would make it more consistent---\"sub/\" means the repository goverend\nby \"sub/.git\", and \"\" would mean the repository governed by \".git\".\n\nIs it hard to change to do so now, given that we won't be rushing this for\nthe upcoming release and we have plenty of time?\n"}]}