{"thread":{"id":"46783","subject":"[OUTREACHY] pack: make packed_git_mru global a value instead of a pointer","startedAt":"2017-09-18T17:22:05Z","lastAt":"2017-09-19T05:20:24Z","messageCount":3,"participants":["phionah bugosi","Jonathan Nieder","Jeff King"],"isPatch":false,"patchVersion":null,"patchTotal":null},"messages":[{"id":"328310","messageId":"1505755303-5583-1-git-send-email-bugosip@gmail.com","threadId":"46783","inReplyTo":null,"subject":"[OUTREACHY] pack: make packed_git_mru global a value instead of a pointer","fromName":"phionah bugosi","fromEmail":"bugosip@gmail.com","sentAt":"2017-09-18T17:21:43Z","receivedAt":"2017-09-18T17:22:05Z","isPatch":false,"sender":{"key":"bugosip@gmail.com","avatar":null},"body":"Just to reecho a previous change requested before in one of the mail threads, we currently have\ntwo global variables declared:\n\nstruct mru packed_git_mru_storage;\nstruct mru *packed_git_mru = &packed_git_mru_storage;\n\nWe normally use pointers in C to point or refer to the same location or space in memory from multiple places. That\nmeans in situations where we will have the pointer be assigned to in many places in our code. But in this case, we do not\nhave any other logic refering or assigning to the pointer packed_git_mru. In simple words we actually dont \nneed a pointer in this case.\n\nTherefore this patch makes packed_git_mru global a value instead of a pointer and\nmakes use of list.h\n\n\nSigned-off-by: phionah bugosi <bugosip@gmail.com>\n---\n builtin/pack-objects.c |  5 +++--\n cache.h                |  7 -------\n list.h                 |  6 ++++++\n packfile.c             | 12 ++++++------\n 4 files changed, 15 insertions(+), 15 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex a57b4f0..189123f 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -1,5 +1,6 @@\n #include \"builtin.h\"\n #include \"cache.h\"\n+#include \"list.h\"\n #include \"config.h\"\n #include \"attr.h\"\n #include \"object.h\"\n@@ -1012,7 +1013,7 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\treturn want;\n \t}\n \n-\tfor (entry = packed_git_mru->head; entry; entry = entry->next) {\n+\tfor (entry = packed_git_mru.head; entry; entry = entry->next) {\n \t\tstruct packed_git *p = entry->item;\n \t\toff_t offset;\n \n@@ -1030,7 +1031,7 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t}\n \t\t\twant = want_found_object(exclude, p);\n \t\t\tif (!exclude && want > 0)\n-\t\t\t\tmru_mark(packed_git_mru, entry);\n+\t\t\t\tmru_mark(&packed_git_mru, entry);\n \t\t\tif (want != -1)\n \t\t\t\treturn want;\n \t\t}\ndiff --git a/cache.h b/cache.h\nindex a916bc7..c8d7086 100644\n--- a/cache.h\n+++ b/cache.h\n@@ -1585,13 +1585,6 @@ extern struct packed_git {\n \tchar pack_name[FLEX_ARRAY]; /* more */\n } *packed_git;\n \n-/*\n- * A most-recently-used ordered version of the packed_git list, which can\n- * be iterated instead of packed_git (and marked via mru_mark).\n- */\n-struct mru;\n-extern struct mru *packed_git_mru;\n-\n struct pack_entry {\n \toff_t offset;\n \tunsigned char sha1[20];\ndiff --git a/list.h b/list.h\nindex a226a87..525703b 100644\n--- a/list.h\n+++ b/list.h\n@@ -26,6 +26,12 @@\n #define LIST_H\t1\n \n /*\n+ * A most-recently-used ordered version of the packed_git list, which can\n+ * be iterated instead of packed_git (and marked via mru_mark).\n+ */\n+extern struct mru packed_git_mru\n+\n+/*\n  * The definitions of this file are adopted from those which can be\n  * found in the Linux kernel headers to enable people familiar with the\n  * latter find their way in these sources as well.\ndiff --git a/packfile.c b/packfile.c\nindex f86fa05..61a61aa 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -1,4 +1,5 @@\n #include \"cache.h\"\n+#include \"list.h\"\n #include \"mru.h\"\n #include \"pack.h\"\n #include \"dir.h\"\n@@ -41,8 +42,7 @@ static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n \n-static struct mru packed_git_mru_storage;\n-struct mru *packed_git_mru = &packed_git_mru_storage;\n+struct mru packed_git_mru;\n \n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n@@ -861,9 +861,9 @@ static void prepare_packed_git_mru(void)\n {\n \tstruct packed_git *p;\n \n-\tmru_clear(packed_git_mru);\n+\tmru_clear(&packed_git_mru);\n \tfor (p = packed_git; p; p = p->next)\n-\t\tmru_append(packed_git_mru, p);\n+\t\tmru_append(&packed_git_mru, p);\n }\n \n static int prepare_packed_git_run_once = 0;\n@@ -1832,9 +1832,9 @@ int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tfor (p = packed_git_mru->head; p; p = p->next) {\n+\tfor (p = packed_git_mru.head; p; p = p->next) {\n \t\tif (fill_pack_entry(sha1, e, p->item)) {\n-\t\t\tmru_mark(packed_git_mru, p);\n+\t\t\tmru_mark(&packed_git_mru, p);\n \t\t\treturn 1;\n \t\t}\n \t}\n-- \n2.7.4\n\n"},{"id":"328322","messageId":"20170918231724.GY27425@aiede.mtv.corp.google.com","threadId":"46783","inReplyTo":"1505755303-5583-1-git-send-email-bugosip@gmail.com","subject":"Re: [OUTREACHY] pack: make packed_git_mru global a value instead of a pointer","fromName":"Jonathan Nieder","fromEmail":"jrnieder@gmail.com","sentAt":"2017-09-18T23:17:24Z","receivedAt":"2017-09-18T23:17:32Z","isPatch":false,"sender":{"key":"jrnieder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/281595?v=4"},"body":"Hi,\n\nphionah bugosi wrote:\n\n> Just to reecho a previous change requested before in one of the mail\n> threads, we currently have two global variables declared:\n>\n> struct mru packed_git_mru_storage;\n> struct mru *packed_git_mru = &packed_git_mru_storage;\n>\n> We normally use pointers in C to point or refer to the same location\n> or space in memory from multiple places. That means in situations\n> where we will have the pointer be assigned to in many places in our\n> code. But in this case, we do not have any other logic refering or\n> assigning to the pointer packed_git_mru. In simple words we actually\n> dont need a pointer in this case.\n>\n> Therefore this patch makes packed_git_mru global a value instead of\n> a pointer and makes use of list.h\n>\n> Signed-off-by: phionah bugosi <bugosip@gmail.com>\n> ---\n>  builtin/pack-objects.c |  5 +++--\n>  cache.h                |  7 -------\n>  list.h                 |  6 ++++++\n>  packfile.c             | 12 ++++++------\n>  4 files changed, 15 insertions(+), 15 deletions(-)\n\n*puzzled* This appears to already be in \"pu\", with a different author.\nDid you independently make the same change?  Or are you asking for a\nprogress report on that change, and just phrasing it in a confusing\nway?\n\nConfused,\nJonathan\n"},{"id":"328345","messageId":"20170919052017.mcspwuse3vlobpxf@sigill.intra.peff.net","threadId":"46783","inReplyTo":"20170918231724.GY27425@aiede.mtv.corp.google.com","subject":"Re: [OUTREACHY] pack: make packed_git_mru global a value instead of a pointer","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-19T05:20:17Z","receivedAt":"2017-09-19T05:20:24Z","isPatch":false,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Sep 18, 2017 at 04:17:24PM -0700, Jonathan Nieder wrote:\n\n> phionah bugosi wrote:\n> \n> > Just to reecho a previous change requested before in one of the mail\n> > threads, we currently have two global variables declared:\n> >\n> > struct mru packed_git_mru_storage;\n> > struct mru *packed_git_mru = &packed_git_mru_storage;\n> >\n> > We normally use pointers in C to point or refer to the same location\n> > or space in memory from multiple places. That means in situations\n> > where we will have the pointer be assigned to in many places in our\n> > code. But in this case, we do not have any other logic refering or\n> > assigning to the pointer packed_git_mru. In simple words we actually\n> > dont need a pointer in this case.\n> >\n> > Therefore this patch makes packed_git_mru global a value instead of\n> > a pointer and makes use of list.h\n> >\n> > Signed-off-by: phionah bugosi <bugosip@gmail.com>\n> > ---\n> >  builtin/pack-objects.c |  5 +++--\n> >  cache.h                |  7 -------\n> >  list.h                 |  6 ++++++\n> >  packfile.c             | 12 ++++++------\n> >  4 files changed, 15 insertions(+), 15 deletions(-)\n> \n> *puzzled* This appears to already be in \"pu\", with a different author.\n> Did you independently make the same change?  Or are you asking for a\n> progress report on that change, and just phrasing it in a confusing\n> way?\n\nI pointed Phionah at your #leftoverbits comment in:\n\n  https://public-inbox.org/git/20170912172839.GB144745@aiede.mtv.corp.google.com/\n\nabout moving packed_git_mru to list.h. But I'm afraid I wasn't very\nclear in giving further guidance.\n\nThe goal is to build on _top_ of the patch in that message, and convert\nthe doubly-linked list implementation used in \"struct mru\" to use the\nshared code in list.h. That should mostly involve touching mru.c and\nmru.h, though I think we'd need to tweak the name of the \"next\" pointer\nduring the traversal, too.\n\n-Peff\n"}]}