{"thread":{"id":"46843","subject":"[PATCH] [Outreachy] cleanup: use list.h in mru.h and mru.c","startedAt":"2017-09-27T10:18:54Z","lastAt":"2017-11-10T11:51:56Z","messageCount":26,"participants":["Оля Тележная","Christian Couder","Olga Telezhnaya","Junio C Hamano","Jeff King"],"isPatch":true,"patchVersion":1,"patchTotal":null},"messages":[{"id":"329032","messageId":"CAL21BmnvJSaN+Tnw7Hdc5P5biAnM5dfWR7gX5FrAG1r_D8th=A@mail.gmail.com","threadId":"46843","inReplyTo":null,"subject":"[PATCH] [Outreachy] cleanup: use list.h in mru.h and mru.c","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-27T10:18:48Z","receivedAt":"2017-09-27T10:18:54Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Remove implementation of double-linked list in mru.c and mru.h and use\nimplementation from list.h.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder , Jeff King\n\n---\n builtin/pack-objects.c | 5 +++--\n mru.c | 51 +++++++++++++++-----------------------------------\n mru.h | 31 +++++++++++++-----------------\n packfile.c | 6 ++++--\n 4 files changed, 35 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f721137ea..fb4c9be89 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n struct packed_git **found_pack,\n off_t *found_offset)\n {\n- struct mru_entry *entry;\n int want;\n+ struct list_head *pos;\n\n if (!exclude && local && has_loose_object_nonlocal(sha1))\n return 0;\n@@ -1012,7 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n return want;\n }\n\n- for (entry = packed_git_mru.head; entry; entry = entry->next) {\n+ list_for_each(pos, &packed_git_mru.list) {\n+ struct mru *entry = list_entry(pos, struct mru, list);\n struct packed_git *p = entry->item;\n off_t offset;\n\ndiff --git a/mru.c b/mru.c\nindex 9dedae028..8b6ba3d9b 100644\n--- a/mru.c\n+++ b/mru.c\n@@ -1,50 +1,29 @@\n #include \"cache.h\"\n #include \"mru.h\"\n\n-void mru_append(struct mru *mru, void *item)\n+void mru_append(struct mru *head, void *item)\n {\n- struct mru_entry *cur = xmalloc(sizeof(*cur));\n+ struct mru *cur = xmalloc(sizeof(*cur));\n cur->item = item;\n- cur->prev = mru->tail;\n- cur->next = NULL;\n-\n- if (mru->tail)\n- mru->tail->next = cur;\n- else\n- mru->head = cur;\n- mru->tail = cur;\n+ list_add_tail(&cur->list, &head->list);\n }\n\n-void mru_mark(struct mru *mru, struct mru_entry *entry)\n+void mru_mark(struct mru *head, struct mru *entry)\n {\n- /* If we're already at the front of the list, nothing to do */\n- if (mru->head == entry)\n- return;\n-\n- /* Otherwise, remove us from our current slot... */\n- if (entry->prev)\n- entry->prev->next = entry->next;\n- if (entry->next)\n- entry->next->prev = entry->prev;\n- else\n- mru->tail = entry->prev;\n-\n- /* And insert us at the beginning. */\n- entry->prev = NULL;\n- entry->next = mru->head;\n- if (mru->head)\n- mru->head->prev = entry;\n- mru->head = entry;\n+ /* To mark means to put at the front of the list. */\n+ list_del(&entry->list);\n+ list_add(&entry->list, &head->list);\n }\n\n-void mru_clear(struct mru *mru)\n+void mru_clear(struct mru *head)\n {\n- struct mru_entry *p = mru->head;\n-\n- while (p) {\n- struct mru_entry *to_free = p;\n- p = p->next;\n+ struct list_head *p1;\n+ struct list_head *p2;\n+ struct mru *to_free;\n+\n+ list_for_each_safe(p1, p2, &head->list) {\n+ to_free = list_entry(p1, struct mru, list);\n free(to_free);\n }\n- mru->head = mru->tail = NULL;\n+ INIT_LIST_HEAD(&head->list);\n }\ndiff --git a/mru.h b/mru.h\nindex 42e4aeaa1..36a332af0 100644\n--- a/mru.h\n+++ b/mru.h\n@@ -1,6 +1,8 @@\n #ifndef MRU_H\n #define MRU_H\n\n+#include \"list.h\"\n+\n /**\n * A simple most-recently-used cache, backed by a doubly-linked list.\n *\n@@ -8,18 +10,15 @@\n *\n * // Create a list. Zero-initialization is required.\n * static struct mru cache;\n- * mru_append(&cache, item);\n- * ...\n+ * INIT_LIST_HEAD(&cache.list);\n *\n- * // Iterate in MRU order.\n- * struct mru_entry *p;\n- * for (p = cache.head; p; p = p->next) {\n- * if (matches(p->item))\n- * break;\n- * }\n+ * // Add new item to the end of the list.\n+ * void *item;\n+ * ...\n+ * mru_append(&cache, item);\n *\n * // Mark an item as used, moving it to the front of the list.\n- * mru_mark(&cache, p);\n+ * mru_mark(&cache, item);\n *\n * // Reset the list to empty, cleaning up all resources.\n * mru_clear(&cache);\n@@ -29,17 +28,13 @@\n * you will begin traversing the whole list again.\n */\n\n-struct mru_entry {\n- void *item;\n- struct mru_entry *prev, *next;\n-};\n-\n struct mru {\n- struct mru_entry *head, *tail;\n+ struct list_head list;\n+ void *item;\n };\n\n-void mru_append(struct mru *mru, void *item);\n-void mru_mark(struct mru *mru, struct mru_entry *entry);\n-void mru_clear(struct mru *mru);\n+void mru_append(struct mru *head, void *item);\n+void mru_mark(struct mru *head, struct mru *entry);\n+void mru_clear(struct mru *head);\n\n #endif /* MRU_H */\ndiff --git a/packfile.c b/packfile.c\nindex f69a5c8d6..ae3b0b2e9 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -876,6 +876,7 @@ void prepare_packed_git(void)\n for (alt = alt_odb_list; alt; alt = alt->next)\n prepare_packed_git_one(alt->path, 0);\n rearrange_packed_git();\n+ INIT_LIST_HEAD(&packed_git_mru.list);\n prepare_packed_git_mru();\n prepare_packed_git_run_once = 1;\n }\n@@ -1824,13 +1825,14 @@ static int fill_pack_entry(const unsigned char *sha1,\n */\n int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n- struct mru_entry *p;\n+ struct list_head *pos;\n\n prepare_packed_git();\n if (!packed_git)\n return 0;\n\n- for (p = packed_git_mru.head; p; p = p->next) {\n+ list_for_each(pos, &packed_git_mru.list) {\n+ struct mru *p = list_entry(pos, struct mru, list);\n if (fill_pack_entry(sha1, e, p->item)) {\n mru_mark(&packed_git_mru, p);\n return 1;\n-- \n2.14.1.727.g9ddaf86b0\n"},{"id":"329034","messageId":"CAP8UFD24A8rxMfLMFmSStWnBMMeB58SqUdNoo3niQuc7LqRMMg@mail.gmail.com","threadId":"46843","inReplyTo":"CAL21BmnvJSaN+Tnw7Hdc5P5biAnM5dfWR7gX5FrAG1r_D8th=A@mail.gmail.com","subject":"Re: [PATCH] [Outreachy] cleanup: use list.h in mru.h and mru.c","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-09-27T11:30:00Z","receivedAt":"2017-09-27T11:30:06Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"About the title, we don't usually start with \"fix:\" or \"cleanup:\" or\n\"feature:\". We usually start with the (main) area of the code that is\nchanged.\n\nSo maybe something like \"mru: use double-linked list implementation\nfrom list.h\" would be better.\n\nOn Wed, Sep 27, 2017 at 12:18 PM, Оля Тележная wrote:\n> Remove implementation of double-linked list in mru.c and mru.h and use\n> implementation from list.h.\n\nIt is important in the commit message to get a good idea of the reason\nwhy the patch is a good thing.\nSo for example \"Simplify mru.c, mru.h and related code by reusing the\ndouble-linked list implementation from list.h instead of a custom\none.\" could be a bit better.\n\n> Signed-off-by: Olga Telezhnaia \n> Mentored-by: Christian Couder , Jeff King\n> \n\nI think it's better if Peff is on another \"Mentored-by: ...\" line.\n\n> ---\n> builtin/pack-objects.c | 5 +++--\n> mru.c | 51 +++++++++++++++-----------------------------------\n> mru.h | 31 +++++++++++++-----------------\n> packfile.c | 6 ++++--\n> 4 files changed, 35 insertions(+), 58 deletions(-)\n\nHere we can see that saying that we simplify things is probably true\nas the patch deletes more lines than it adds.\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index f721137ea..fb4c9be89 100644\n> --- a/builtin/pack-objects.c\n> +++ b/builtin/pack-objects.c\n> @@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n> struct packed_git **found_pack,\n> off_t *found_offset)\n> {\n> - struct mru_entry *entry;\n> int want;\n> + struct list_head *pos;\n\nIt looks like there are indentation problems in the patch.\n\nDid you try to send it to you and apply what you received?\n\nWhen I try to apply it to master I get:\n\n> git am ~/Downloads/original_msg.txt\nApplying: cleanup: use list.h in mru.h and mru.c\n.git/rebase-apply/patch:18: indent with spaces.\n struct list_head *pos;\n.git/rebase-apply/patch:152: indent with spaces.\n void *item;\nerror: patch failed: builtin/pack-objects.c:995\nerror: builtin/pack-objects.c: patch does not apply\nerror: patch failed: mru.c:1\nerror: mru.c: patch does not apply\nerror: patch failed: mru.h:8\nerror: mru.h: patch does not apply\nerror: patch failed: packfile.c:876\nerror: packfile.c: patch does not apply\nPatch failed at 0001 cleanup: use list.h in mru.h and mru.c\nThe copy of the patch that failed is found in: .git/rebase-apply/patch\nWhen you have resolved this problem, run \"git am --continue\".\nIf you prefer to skip this patch, run \"git am --skip\" instead.\nTo restore the original branch and stop patching, run \"git am --abort\".\n\nI am stopping here as it's quite difficult to read patches that have\nindentation problems.\n"},{"id":"329101","messageId":"0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com","threadId":"46843","inReplyTo":"CAP8UFD24A8rxMfLMFmSStWnBMMeB58SqUdNoo3niQuc7LqRMMg@mail.gmail.com","subject":"[PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-28T08:38:39Z","receivedAt":"2017-09-28T08:39:00Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Simplify mru.c, mru.h and related code by reusing the double-linked list implementation from list.h instead of a custom one.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/pack-objects.c | 5 +++--\n mru.c | 51 +++++++++++++++-----------------------------------\n mru.h | 31 +++++++++++++-----------------\n packfile.c | 6 ++++--\n 4 files changed, 35 insertions(+), 58 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f721137eaf881..ba812349e0aab 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t struct packed_git **found_pack,\n \t\t\t off_t *found_offset)\n {\n-\tstruct mru_entry *entry;\n \tint want;\n+\tstruct list_head *pos;\n \n \tif (!exclude && local && has_loose_object_nonlocal(sha1))\n \t\treturn 0;\n@@ -1012,7 +1012,8 @@ 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+\tlist_for_each(pos, &packed_git_mru.list) {\n+\t\tstruct mru *entry = list_entry(pos, struct mru, list);\n \t\tstruct packed_git *p = entry->item;\n \t\toff_t offset;\n \ndiff --git a/mru.c b/mru.c\nindex 9dedae0287ed2..8b6ba3d9b7fad 100644\n--- a/mru.c\n+++ b/mru.c\n@@ -1,50 +1,29 @@\n #include \"cache.h\"\n #include \"mru.h\"\n \n-void mru_append(struct mru *mru, void *item)\n+void mru_append(struct mru *head, void *item)\n {\n-\tstruct mru_entry *cur = xmalloc(sizeof(*cur));\n+\tstruct mru *cur = xmalloc(sizeof(*cur));\n \tcur->item = item;\n-\tcur->prev = mru->tail;\n-\tcur->next = NULL;\n-\n-\tif (mru->tail)\n-\t\tmru->tail->next = cur;\n-\telse\n-\t\tmru->head = cur;\n-\tmru->tail = cur;\n+\tlist_add_tail(&cur->list, &head->list);\n }\n \n-void mru_mark(struct mru *mru, struct mru_entry *entry)\n+void mru_mark(struct mru *head, struct mru *entry)\n {\n-\t/* If we're already at the front of the list, nothing to do */\n-\tif (mru->head == entry)\n-\t\treturn;\n-\n-\t/* Otherwise, remove us from our current slot... */\n-\tif (entry->prev)\n-\t\tentry->prev->next = entry->next;\n-\tif (entry->next)\n-\t\tentry->next->prev = entry->prev;\n-\telse\n-\t\tmru->tail = entry->prev;\n-\n-\t/* And insert us at the beginning. */\n-\tentry->prev = NULL;\n-\tentry->next = mru->head;\n-\tif (mru->head)\n-\t\tmru->head->prev = entry;\n-\tmru->head = entry;\n+\t/* To mark means to put at the front of the list. */\n+\tlist_del(&entry->list);\n+\tlist_add(&entry->list, &head->list);\n }\n \n-void mru_clear(struct mru *mru)\n+void mru_clear(struct mru *head)\n {\n-\tstruct mru_entry *p = mru->head;\n-\n-\twhile (p) {\n-\t\tstruct mru_entry *to_free = p;\n-\t\tp = p->next;\n+\tstruct list_head *p1;\n+\tstruct list_head *p2;\n+\tstruct mru *to_free;\n+\t\n+\tlist_for_each_safe(p1, p2, &head->list) {\n+\t\tto_free = list_entry(p1, struct mru, list);\n \t\tfree(to_free);\n \t}\n-\tmru->head = mru->tail = NULL;\n+\tINIT_LIST_HEAD(&head->list);\n }\ndiff --git a/mru.h b/mru.h\nindex 42e4aeaa1098a..36a332af0bf88 100644\n--- a/mru.h\n+++ b/mru.h\n@@ -1,6 +1,8 @@\n #ifndef MRU_H\n #define MRU_H\n \n+#include \"list.h\"\n+\n /**\n * A simple most-recently-used cache, backed by a doubly-linked list.\n *\n@@ -8,18 +10,15 @@\n *\n * // Create a list. Zero-initialization is required.\n * static struct mru cache;\n- * mru_append(&cache, item);\n- * ...\n+ * INIT_LIST_HEAD(&cache.list);\n *\n- * // Iterate in MRU order.\n- * struct mru_entry *p;\n- * for (p = cache.head; p; p = p->next) {\n- *\tif (matches(p->item))\n- *\t\tbreak;\n- * }\n+ * // Add new item to the end of the list.\n+ * void *item;\n+ * ...\n+ * mru_append(&cache, item);\n *\n * // Mark an item as used, moving it to the front of the list.\n- * mru_mark(&cache, p);\n+ * mru_mark(&cache, item);\n *\n * // Reset the list to empty, cleaning up all resources.\n * mru_clear(&cache);\n@@ -29,17 +28,13 @@\n * you will begin traversing the whole list again.\n */\n \n-struct mru_entry {\n-\tvoid *item;\n-\tstruct mru_entry *prev, *next;\n-};\n-\n struct mru {\n-\tstruct mru_entry *head, *tail;\n+\tstruct list_head list;\n+ void *item;\n };\n \n-void mru_append(struct mru *mru, void *item);\n-void mru_mark(struct mru *mru, struct mru_entry *entry);\n-void mru_clear(struct mru *mru);\n+void mru_append(struct mru *head, void *item);\n+void mru_mark(struct mru *head, struct mru *entry);\n+void mru_clear(struct mru *head);\n \n #endif /* MRU_H */\ndiff --git a/packfile.c b/packfile.c\nindex f69a5c8d607af..ae3b0b2e9c09a 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -876,6 +876,7 @@ void prepare_packed_git(void)\n \tfor (alt = alt_odb_list; alt; alt = alt->next)\n \t\tprepare_packed_git_one(alt->path, 0);\n \trearrange_packed_git();\n+\tINIT_LIST_HEAD(&packed_git_mru.list);\n \tprepare_packed_git_mru();\n \tprepare_packed_git_run_once = 1;\n }\n@@ -1824,13 +1825,14 @@ static int fill_pack_entry(const unsigned char *sha1,\n */\n int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstruct mru_entry *p;\n+\tstruct list_head *pos;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tfor (p = packed_git_mru.head; p; p = p->next) {\n+\tlist_for_each(pos, &packed_git_mru.list) {\n+\t\tstruct mru *p = list_entry(pos, struct mru, list);\n \t\tif (fill_pack_entry(sha1, e, p->item)) {\n \t\t\tmru_mark(&packed_git_mru, p);\n \t\t\treturn 1;\n\n--\nhttps://github.com/git/git/pull/409\n"},{"id":"329103","messageId":"xmqq8tgz13x7.fsf@gitster.mtv.corp.google.com","threadId":"46843","inReplyTo":"0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T11:03:00Z","receivedAt":"2017-09-28T11:03:20Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Olga Telezhnaya writes:\n\n> Simplify mru.c, mru.h and related code by reusing the double-linked list implementation from list.h instead of a custom one.\n\nAn overlong line (I can locally wrap it, so the patch does not have\nto be re-sent only to fix this alone).\n\n> Signed-off-by: Olga Telezhnaia \n\nThanks for making your \"From:\" name match the \"Signed-off-by:\" name;\nanglicising like you did is probably more friendly to the readers\nthan writing both in Cyrillic, which is another valid way to make\nthem match.\n\n> Mentored-by: Christian Couder \n> Mentored by: Jeff King \n> ---\n> builtin/pack-objects.c | 5 +++--\n> mru.c | 51 +++++++++++++++-----------------------------------\n> mru.h | 31 +++++++++++++-----------------\n> packfile.c | 6 ++++--\n> 4 files changed, 35 insertions(+), 58 deletions(-)\n\nAs Christian mentioned earlier, nice line reduction ;-)\n\n> diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\n> index f721137eaf881..ba812349e0aab 100644\n> --- a/builtin/pack-objects.c\n%> +++ b/builtin/pack-objects.c\n> @@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n> \t\t\t struct packed_git **found_pack,\n> \t\t\t off_t *found_offset)\n> {\n> -\tstruct mru_entry *entry;\n> \tint want;\n> +\tstruct list_head *pos;\n> \n> \tif (!exclude && local && has_loose_object_nonlocal(sha1))\n> \t\treturn 0;\n> @@ -1012,7 +1012,8 @@ 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> +\tlist_for_each(pos, &packed_git_mru.list) {\n> +\t\tstruct mru *entry = list_entry(pos, struct mru, list);\n> \t\tstruct packed_git *p = entry->item;\n> \t\toff_t offset;\n\nI was a bit surprised to see a change outside mru.[ch] like this\none. The reason why I was surprised was because I expected mru.[ch]\nwould offer its own API that encapsulates enumeration like this one\nand this patch would just be reimplementing that API using the list\nAPI, instead of rewriting the users of mru API to directly access\nthe list API.\n\nAlas, there is no such mru API that lets a mru user to iterate over\nelements, so the original of the above code were using mru's\nimplementation detail directly.\n\nWe probably want to invent mru_for_each() that hides the fact that\nmru is implemented in terms of list_head from the users of mru API\nand use it here.\n\n> diff --git a/mru.c b/mru.c\n> index 9dedae0287ed2..8b6ba3d9b7fad 100644\n> --- a/mru.c\n> +++ b/mru.c\n> @@ -1,50 +1,29 @@\n> #include \"cache.h\"\n> #include \"mru.h\"\n> \n> -void mru_append(struct mru *mru, void *item)\n> +void mru_append(struct mru *head, void *item)\n> {\n> -\tstruct mru_entry *cur = xmalloc(sizeof(*cur));\n> +\tstruct mru *cur = xmalloc(sizeof(*cur));\n> \tcur->item = item;\n> -\tcur->prev = mru->tail;\n> -\tcur->next = NULL;\n> -\n> -\tif (mru->tail)\n> -\t\tmru->tail->next = cur;\n> -\telse\n> -\t\tmru->head = cur;\n> -\tmru->tail = cur;\n> +\tlist_add_tail(&cur->list, &head->list);\n> }\n> \n> -void mru_mark(struct mru *mru, struct mru_entry *entry)\n> +void mru_mark(struct mru *head, struct mru *entry)\n> {\n> -\t/* If we're already at the front of the list, nothing to do */\n> -\tif (mru->head == entry)\n> -\t\treturn;\n> -\n> -\t/* Otherwise, remove us from our current slot... */\n> -\tif (entry->prev)\n> -\t\tentry->prev->next = entry->next;\n> -\tif (entry->next)\n> -\t\tentry->next->prev = entry->prev;\n> -\telse\n> -\t\tmru->tail = entry->prev;\n> -\n> -\t/* And insert us at the beginning. */\n> -\tentry->prev = NULL;\n> -\tentry->next = mru->head;\n> -\tif (mru->head)\n> -\t\tmru->head->prev = entry;\n> -\tmru->head = entry;\n> +\t/* To mark means to put at the front of the list. */\n> +\tlist_del(&entry->list);\n> +\tlist_add(&entry->list, &head->list);\n> }\n> \n> -void mru_clear(struct mru *mru)\n> +void mru_clear(struct mru *head)\n> {\n> -\tstruct mru_entry *p = mru->head;\n> -\n> -\twhile (p) {\n> -\t\tstruct mru_entry *to_free = p;\n> -\t\tp = p->next;\n> +\tstruct list_head *p1;\n> +\tstruct list_head *p2;\n> +\tstruct mru *to_free;\n> +\t\n> +\tlist_for_each_safe(p1, p2, &head->list) {\n> +\t\tto_free = list_entry(p1, struct mru, list);\n> \t\tfree(to_free);\n> \t}\n> -\tmru->head = mru->tail = NULL;\n> +\tINIT_LIST_HEAD(&head->list);\n> }\n> diff --git a/mru.h b/mru.h\n> index 42e4aeaa1098a..36a332af0bf88 100644\n> --- a/mru.h\n> +++ b/mru.h\n> @@ -1,6 +1,8 @@\n> #ifndef MRU_H\n> #define MRU_H\n> \n> +#include \"list.h\"\n> +\n> /**\n> * A simple most-recently-used cache, backed by a doubly-linked list.\n> *\n> @@ -8,18 +10,15 @@\n> *\n> * // Create a list. Zero-initialization is required.\n> * static struct mru cache;\n> - * mru_append(&cache, item);\n> - * ...\n> + * INIT_LIST_HEAD(&cache.list);\n\n\"Zero-initialization is required.\" is no longer true, it seems, and\nthe comment above needs to be updated, right?\n\nMore importantly, this leaks to the user of the API the fact that\nmru is internally implemented in terms of the list API, which is\nnot necessary (when we want to update the implementation later, we'd\nneed to update all the users again). Perhaps you'd want\n\n\tINIT_MRU(cache);\n\nwhich is #define'd in this file, perhaps like so:\n\n\t#define INIT_MRU(mru)\tINIT_LIST_HEAD(&((mru).list))\n\n\n\n> - * // Iterate in MRU order.\n> - * struct mru_entry *p;\n> - * for (p = cache.head; p; p = p->next) {\n> - *\tif (matches(p->item))\n> - *\t\tbreak;\n> - * }\n\nAh, here is a good piece that illustrates what I meant earlier.\nThis could have been something like:\n\n\t// Iterate in MRU order.\n\tstruct mru *item;\n\tmru_for_each(item, &cache) {\n\t\tif (matches(item))\n\t\t\tbreak;\n\t}\n\nand then the user would not have to know mru is implemented in terms\nof the list API.\n\n> + * // Add new item to the end of the list.\n> + * void *item;\n> + * ...\n> + * mru_append(&cache, item);\n> *\n> * // Mark an item as used, moving it to the front of the list.\n> - * mru_mark(&cache, p);\n> + * mru_mark(&cache, item);\n> *\n> * // Reset the list to empty, cleaning up all resources.\n> * mru_clear(&cache);\n> @@ -29,17 +28,13 @@\n> * you will begin traversing the whole list again.\n> */\n> \n> -struct mru_entry {\n> -\tvoid *item;\n> -\tstruct mru_entry *prev, *next;\n> -};\n> -\n> struct mru {\n> -\tstruct mru_entry *head, *tail;\n> +\tstruct list_head list;\n> + void *item;\n> };\n> \n> -void mru_append(struct mru *mru, void *item);\n> -void mru_mark(struct mru *mru, struct mru_entry *entry);\n> -void mru_clear(struct mru *mru);\n> +void mru_append(struct mru *head, void *item);\n> +void mru_mark(struct mru *head, struct mru *entry);\n> +void mru_clear(struct mru *head);\n> \n> #endif /* MRU_H */\n> diff --git a/packfile.c b/packfile.c\n> index f69a5c8d607af..ae3b0b2e9c09a 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -876,6 +876,7 @@ void prepare_packed_git(void)\n> \tfor (alt = alt_odb_list; alt; alt = alt->next)\n> \t\tprepare_packed_git_one(alt->path, 0);\n> \trearrange_packed_git();\n> +\tINIT_LIST_HEAD(&packed_git_mru.list);\n> \tprepare_packed_git_mru();\n> \tprepare_packed_git_run_once = 1;\n> }\n> @@ -1824,13 +1825,14 @@ static int fill_pack_entry(const unsigned char *sha1,\n> */\n> int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n> {\n> -\tstruct mru_entry *p;\n> +\tstruct list_head *pos;\n> \n> \tprepare_packed_git();\n> \tif (!packed_git)\n> \t\treturn 0;\n> \n> -\tfor (p = packed_git_mru.head; p; p = p->next) {\n> +\tlist_for_each(pos, &packed_git_mru.list) {\n> +\t\tstruct mru *p = list_entry(pos, struct mru, list);\n> \t\tif (fill_pack_entry(sha1, e, p->item)) {\n> \t\t\tmru_mark(&packed_git_mru, p);\n> \t\t\treturn 1;\n\nThe same comment as the first hunk applies here, too.\n\nThanks.\n"},{"id":"329119","messageId":"20170928204705.7ixxspiflmhsdh7d@sigill.intra.peff.net","threadId":"46843","inReplyTo":"xmqq8tgz13x7.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T20:47:05Z","receivedAt":"2017-09-28T20:47:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 28, 2017 at 08:03:00PM +0900, Junio C Hamano wrote:\n\n> > -\tfor (entry = packed_git_mru.head; entry; entry = entry->next) {\n> > +\tlist_for_each(pos, &packed_git_mru.list) {\n> > +\t\tstruct mru *entry = list_entry(pos, struct mru, list);\n> > \t\tstruct packed_git *p = entry->item;\n> > \t\toff_t offset;\n> \n> I was a bit surprised to see a change outside mru.[ch] like this\n> one. The reason why I was surprised was because I expected mru.[ch]\n> would offer its own API that encapsulates enumeration like this one\n> and this patch would just be reimplementing that API using the list\n> API, instead of rewriting the users of mru API to directly access\n> the list API.\n> \n> Alas, there is no such mru API that lets a mru user to iterate over\n> elements, so the original of the above code were using mru's\n> implementation detail directly.\n> \n> We probably want to invent mru_for_each() that hides the fact that\n> mru is implemented in terms of list_head from the users of mru API\n> and use it here.\n\nI agree that the caller would be a little shorter with an mru-specific\niterator (e.g., we could probably do the list_entry() part\nautomatically).\n\nBut I also think this patch may be a stepping stone to dropping \"struct\nmru\" entirely, and just pushing a \"struct list_head mru\" into the\npacked_git object itself (or of course any object you like). At which\npoint we'd just directly use the list iterators anyway.\n\n(One could argue that if that's our end goal, we could go straight\nthere. But I think this middle state has value, because the individual\nsteps are easier to verify).\n\n> > @@ -8,18 +10,15 @@\n> > *\n> > * // Create a list. Zero-initialization is required.\n> > * static struct mru cache;\n> > - * mru_append(&cache, item);\n> > - * ...\n> > + * INIT_LIST_HEAD(&cache.list);\n> \n> \"Zero-initialization is required.\" is no longer true, it seems, and\n> the comment above needs to be updated, right?\n> \n> More importantly, this leaks to the user of the API the fact that\n> mru is internally implemented in terms of the list API, which is\n> not necessary (when we want to update the implementation later, we'd\n> need to update all the users again). Perhaps you'd want\n> \n> \tINIT_MRU(cache);\n> \n> which is #define'd in this file, perhaps like so:\n> \n> \t#define INIT_MRU(mru)\tINIT_LIST_HEAD(&((mru).list))\n\nI'd make the same claims here as above (both that I agree your proposed\ninterface looks nicer, but also that I think we eventually do want to\nexpose that this is tightly coupled with list.h).\n\n-Peff\n"},{"id":"329121","messageId":"20170928210439.msmzxlih4ykrsmee@sigill.intra.peff.net","threadId":"46843","inReplyTo":"0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T21:04:39Z","receivedAt":"2017-09-28T21:04:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 28, 2017 at 08:38:39AM +0000, Olga Telezhnaya wrote:\n\n> Simplify mru.c, mru.h and related code by reusing the double-linked\n> list implementation from list.h instead of a custom one.\n\nThe commit message is a good reason to talk about why we want to do\nthis. In this case, the answer may be fairly obvious. But I sometimes\nfind that things that are obvious to me as the patch author are not\nquite as obvious to people reading it later (either reviewing, or six\nmonths from now when they are hunting the cause of a bug).\n\n> -void mru_mark(struct mru *mru, struct mru_entry *entry)\n> +void mru_mark(struct mru *head, struct mru *entry)\n> {\n> -\t/* If we're already at the front of the list, nothing to do */\n> -\tif (mru->head == entry)\n> -\t\treturn;\n> -\n> -\t/* Otherwise, remove us from our current slot... */\n> -\tif (entry->prev)\n> -\t\tentry->prev->next = entry->next;\n> -\tif (entry->next)\n> -\t\tentry->next->prev = entry->prev;\n> -\telse\n> -\t\tmru->tail = entry->prev;\n> -\n> -\t/* And insert us at the beginning. */\n> -\tentry->prev = NULL;\n> -\tentry->next = mru->head;\n> -\tif (mru->head)\n> -\t\tmru->head->prev = entry;\n> -\tmru->head = entry;\n> +\t/* To mark means to put at the front of the list. */\n> +\tlist_del(&entry->list);\n> +\tlist_add(&entry->list, &head->list);\n> }\n\nNice, this hunk is very satisfying. :)\n\n> -void mru_clear(struct mru *mru)\n> +void mru_clear(struct mru *head)\n> {\n> -\tstruct mru_entry *p = mru->head;\n> -\n> -\twhile (p) {\n> -\t\tstruct mru_entry *to_free = p;\n> -\t\tp = p->next;\n> +\tstruct list_head *p1;\n> +\tstruct list_head *p2;\n> +\tstruct mru *to_free;\n> +\t\n> +\tlist_for_each_safe(p1, p2, &head->list) {\n> +\t\tto_free = list_entry(p1, struct mru, list);\n> \t\tfree(to_free);\n> \t}\n> -\tmru->head = mru->tail = NULL;\n> +\tINIT_LIST_HEAD(&head->list);\n\nTwo minor style comments here:\n\n - Perhaps \"tmp\" is a better name than \"p2\" for the second argument of\n a list_for_each_safe, as it makes it less likely to confuse p1 and\n p2 (though admittedly the whole function is short enough that it\n probably doesn't matter much either way).\n\n - It's a good practice to declare variables in the smallest scope\n possible. So I think the declaration of to_free could go inside the\n loop.\n\n You could actually get rid of it entirely with:\n\n free(list_entry(p1, struct mru, list));\n\n but I certainly don't mind using a variable for better readability.\n\n> @@ -29,17 +28,13 @@\n> * you will begin traversing the whole list again.\n> */\n> \n> -struct mru_entry {\n> -\tvoid *item;\n> -\tstruct mru_entry *prev, *next;\n> -};\n> -\n> struct mru {\n> -\tstruct mru_entry *head, *tail;\n> +\tstruct list_head list;\n> + void *item;\n> };\n\nThe decision to get rid of the \"mru versus mru_entry\" distinction\nsurprised me a little. In the original, a \"struct mru\" represented the\nwhole list. In the list.h implementation, a \"struct list_head\" serves\nthat purpose, as a sentinel value. But that sentinel doesn't need to\nhave an \"item\", right? I.e., we could have:\n\n struct mru {\n struct list_head head;\n };\n\n struct mru_entry {\n void *item;\n\t struct list_head list;\n };\n\nAs I said in my response to Junio (and as we discussed a little\noff-list), I think we can eventually move to having no structs at all\n(just list_heads embedded inside the existing packfile objects). At\nwhich point the user of the API would just declare:\n\n LIST_HEAD(packed_git_mru);\n\nthemselves. So I'm actually fine with this direction if we're using it\nas the \"middle step\" that I mentioned there.\n\n> struct mru {\n> -\tstruct mru_entry *head, *tail;\n> +\tstruct list_head list;\n> + void *item;\n> };\n\nThe funny indentation in this diff shows that \"void *item\" is indented\nwith spaces, not a tab.\n\n> [...]\n\nI pointed out a few minor bits, but overall this is looking very strong.\nGreat work!\n\n-Peff\n"},{"id":"329125","messageId":"xmqq4lrm1o8j.fsf@gitster.mtv.corp.google.com","threadId":"46843","inReplyTo":"20170928204705.7ixxspiflmhsdh7d@sigill.intra.peff.net","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-28T21:56:28Z","receivedAt":"2017-09-28T21:56:34Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> But I also think this patch may be a stepping stone to dropping \"struct\n> mru\" entirely, and just pushing a \"struct list_head mru\" into the\n> packed_git object itself (or of course any object you like). At which\n> point we'd just directly use the list iterators anyway.\n\nThe endgame state implied by your statement would mean that we won't\nhave mru_mark() and instead have these open-coded in terms of the\ntwo calls into the list API. There only are two callers of it in\nthe current system, so it is not the end of the world, so I guess I\ncan agree that this is a good preparation step toward the longer\nterm goal, which says \"mru API is over-engineered and what it offers\nover the plain vanilla list API is almost never used except for a\nfew callsite; let's remove it and use the bare list API instead\".\n\n> I'd make the same claims here as above (both that I agree your proposed\n> interface looks nicer, but also that I think we eventually do want to\n> expose that this is tightly coupled with list.h).\n\nI agree. I just do not yet know if I fully embrace the idea that we\njust should use bare list API, getting rid of the mru API.\n\n"},{"id":"329129","messageId":"20170928221939.zxssv7jlzes56tq7@sigill.intra.peff.net","threadId":"46843","inReplyTo":"xmqq4lrm1o8j.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T22:19:39Z","receivedAt":"2017-09-28T22:19:46Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 29, 2017 at 06:56:28AM +0900, Junio C Hamano wrote:\n\n> Jeff King writes:\n> \n> > But I also think this patch may be a stepping stone to dropping \"struct\n> > mru\" entirely, and just pushing a \"struct list_head mru\" into the\n> > packed_git object itself (or of course any object you like). At which\n> > point we'd just directly use the list iterators anyway.\n> \n> The endgame state implied by your statement would mean that we won't\n> have mru_mark() and instead have these open-coded in terms of the\n> two calls into the list API. There only are two callers of it in\n> the current system, so it is not the end of the world, so I guess I\n> can agree that this is a good preparation step toward the longer\n> term goal, which says \"mru API is over-engineered and what it offers\n> over the plain vanilla list API is almost never used except for a\n> few callsite; let's remove it and use the bare list API instead\".\n\nThanks, that last sentence is a good summary of my thinking (I think\nwhat I find most silly about it is that it allocates a whole extra\nstruct per item, which is where most of the complication comes from).\n\nI had envisioned leaving mru_mark() as a wrapper for \"move to the front\"\nthat could operate on any list. But seeing how Olga's patch takes it\ndown to two trivial lines, I'd also be fine with an endgame that just\neliminates it.\n\n> > I'd make the same claims here as above (both that I agree your proposed\n> > interface looks nicer, but also that I think we eventually do want to\n> > expose that this is tightly coupled with list.h).\n> \n> I agree. I just do not yet know if I fully embrace the idea that we\n> just should use bare list API, getting rid of the mru API.\n\nFair enough.\n\n-Peff\n"},{"id":"329133","messageId":"20170928224244.pi34zwifnornssqk@sigill.intra.peff.net","threadId":"46843","inReplyTo":"0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-28T22:42:44Z","receivedAt":"2017-09-28T22:42:50Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Thu, Sep 28, 2017 at 08:38:39AM +0000, Olga Telezhnaya wrote:\n\n> diff --git a/packfile.c b/packfile.c\n> index f69a5c8d607af..ae3b0b2e9c09a 100644\n> --- a/packfile.c\n> +++ b/packfile.c\n> @@ -876,6 +876,7 @@ void prepare_packed_git(void)\n> \tfor (alt = alt_odb_list; alt; alt = alt->next)\n> \t\tprepare_packed_git_one(alt->path, 0);\n> \trearrange_packed_git();\n> +\tINIT_LIST_HEAD(&packed_git_mru.list);\n> \tprepare_packed_git_mru();\n> \tprepare_packed_git_run_once = 1;\n> }\n\nI was thinking on this hunk a bit more, and I think it's not quite\nright.\n\nThe prepare_packed_git_mru() function will clear the mru list and then\nre-add each item from the packed_git list. But by calling\nINIT_LIST_HEAD() here, we're effectively clearing the packed_git_mru\nlist, and we end up leaking whatever was on the list before.\n\nSo for the first call to prepare_packed_git, we really need this\nINIT_LIST_HEAD() call. But for subsequent calls (which come from\nreprepare_packed_git()), we must not call it.\n\nThere are a few ways to work around it that I can think of:\n\n 1. Check whether packed_git_mru.list.head is NULL, and only initialize\n in that case.\n\n 2. Use a static initializer for packed_git_mru.list, so that we don't\n have do the first-time initializing here.\n\n 3. Teach reprepare_packed_git() to do the mru_clear() call, so that we\n know the list is empty when we get here.\n\nOne final and more invasive option is to stop regenerating the\npacked_git_mru list from scratch during each prepare_packed_git(). I did\nit that way so that we start with the same order that\nrearrange_packed_git() will give us, but I'm not sure how much value\nthat has in practice (it probably had a lot more when we didn't have the\nmru, and the time-sorted pack order helped find recent objects more\nquickly).\n\nThe alternative would be to just teach install_packed_git() to add each\nnewly-added pack to the mru list, and then never clear the list (and we\nwouldn't need an mru_clear() at all, then).\n\n-Peff\n"},{"id":"329161","messageId":"CAP8UFD13obkLWyuCGUpFxryr8DWfQ8W4JNn04ajO50PvF0SnXQ@mail.gmail.com","threadId":"46843","inReplyTo":"20170928224244.pi34zwifnornssqk@sigill.intra.peff.net","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-09-29T07:18:11Z","receivedAt":"2017-09-29T07:18:17Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Sep 29, 2017 at 12:42 AM, Jeff King wrote:\n> On Thu, Sep 28, 2017 at 08:38:39AM +0000, Olga Telezhnaya wrote:\n>\n>> diff --git a/packfile.c b/packfile.c\n>> index f69a5c8d607af..ae3b0b2e9c09a 100644\n>> --- a/packfile.c\n>> +++ b/packfile.c\n>> @@ -876,6 +876,7 @@ void prepare_packed_git(void)\n>> for (alt = alt_odb_list; alt; alt = alt->next)\n>> prepare_packed_git_one(alt->path, 0);\n>> rearrange_packed_git();\n>> + INIT_LIST_HEAD(&packed_git_mru.list);\n>> prepare_packed_git_mru();\n>> prepare_packed_git_run_once = 1;\n>> }\n>\n> I was thinking on this hunk a bit more, and I think it's not quite\n> right.\n>\n> The prepare_packed_git_mru() function will clear the mru list and then\n> re-add each item from the packed_git list. But by calling\n> INIT_LIST_HEAD() here, we're effectively clearing the packed_git_mru\n> list, and we end up leaking whatever was on the list before.\n\nThe current code is:\n\nstatic int prepare_packed_git_run_once = 0;\nvoid prepare_packed_git(void)\n{\n struct alternate_object_database *alt;\n\n if (prepare_packed_git_run_once)\n return;\n prepare_packed_git_one(get_object_directory(), 1);\n prepare_alt_odb();\n for (alt = alt_odb_list; alt; alt = alt->next)\n prepare_packed_git_one(alt->path, 0);\n rearrange_packed_git();\n prepare_packed_git_mru();\n prepare_packed_git_run_once = 1;\n}\n\nAs we use the \"prepare_packed_git_run_once\" static, this function will\nonly be called only once when packed_git_mru has not yet been\ninitialized, so there will be no leak.\n"},{"id":"329162","messageId":"20170929072354.fw4eclt56dmfj4a5@sigill.intra.peff.net","threadId":"46843","inReplyTo":"CAP8UFD13obkLWyuCGUpFxryr8DWfQ8W4JNn04ajO50PvF0SnXQ@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-29T07:23:55Z","receivedAt":"2017-09-29T07:24:02Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 29, 2017 at 09:18:11AM +0200, Christian Couder wrote:\n\n> On Fri, Sep 29, 2017 at 12:42 AM, Jeff King wrote:\n> > On Thu, Sep 28, 2017 at 08:38:39AM +0000, Olga Telezhnaya wrote:\n> >\n> >> diff --git a/packfile.c b/packfile.c\n> >> index f69a5c8d607af..ae3b0b2e9c09a 100644\n> >> --- a/packfile.c\n> >> +++ b/packfile.c\n> >> @@ -876,6 +876,7 @@ void prepare_packed_git(void)\n> >> for (alt = alt_odb_list; alt; alt = alt->next)\n> >> prepare_packed_git_one(alt->path, 0);\n> >> rearrange_packed_git();\n> >> + INIT_LIST_HEAD(&packed_git_mru.list);\n> >> prepare_packed_git_mru();\n> >> prepare_packed_git_run_once = 1;\n> >> }\n> >\n> > I was thinking on this hunk a bit more, and I think it's not quite\n> > right.\n> >\n> > The prepare_packed_git_mru() function will clear the mru list and then\n> > re-add each item from the packed_git list. But by calling\n> > INIT_LIST_HEAD() here, we're effectively clearing the packed_git_mru\n> > list, and we end up leaking whatever was on the list before.\n> \n> The current code is:\n> \n> static int prepare_packed_git_run_once = 0;\n> void prepare_packed_git(void)\n> {\n> struct alternate_object_database *alt;\n> \n> if (prepare_packed_git_run_once)\n> return;\n> prepare_packed_git_one(get_object_directory(), 1);\n> prepare_alt_odb();\n> for (alt = alt_odb_list; alt; alt = alt->next)\n> prepare_packed_git_one(alt->path, 0);\n> rearrange_packed_git();\n> prepare_packed_git_mru();\n> prepare_packed_git_run_once = 1;\n> }\n> \n> As we use the \"prepare_packed_git_run_once\" static, this function will\n> only be called only once when packed_git_mru has not yet been\n> initialized, so there will be no leak.\n\nCheck reprepare_packed_git(). It unsets the run_once flag, and then\ncalls prepare_packed_git() again.\n\n-Peff\n"},{"id":"329171","messageId":"CAP8UFD1-9dYSX-VKZSPN9Ei75V8mGC-wusieL45ArxxJ08tO9Q@mail.gmail.com","threadId":"46843","inReplyTo":"20170929072354.fw4eclt56dmfj4a5@sigill.intra.peff.net","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Christian Couder","fromEmail":"christian.couder@gmail.com","sentAt":"2017-09-29T11:50:38Z","receivedAt":"2017-09-29T11:52:43Z","isPatch":true,"sender":{"key":"christian.couder@gmail.com","avatar":"https://avatars.githubusercontent.com/u/208954?v=4"},"body":"On Fri, Sep 29, 2017 at 9:23 AM, Jeff King wrote:\n> On Fri, Sep 29, 2017 at 09:18:11AM +0200, Christian Couder wrote:\n>\n>> As we use the \"prepare_packed_git_run_once\" static, this function will\n>> only be called only once when packed_git_mru has not yet been\n>> initialized, so there will be no leak.\n>\n> Check reprepare_packed_git(). It unsets the run_once flag, and then\n> calls prepare_packed_git() again.\n\nAh ok.\n"},{"id":"329180","messageId":"CAL21BmkcVSEhEK+tAE-RNVabb0pnokYwbagueUrp9giZ3zqT8A@mail.gmail.com","threadId":"46843","inReplyTo":"CAP8UFD1-9dYSX-VKZSPN9Ei75V8mGC-wusieL45ArxxJ08tO9Q@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-29T16:08:28Z","receivedAt":"2017-09-29T16:08:34Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Hi everyone,\nMany thanks to all of you, I am interested in every opinion. Sorry\nthat I wasn't in the discussion, unfortunately I got sick, that's why\nI skipped all the process.\nI want to reply to the main moments and also ask some questions.\n\n>> Simplify mru.c, mru.h and related code by reusing the double-linked list implementation from list.h instead of a custom one.\n> An overlong line (I can locally wrap it, so the patch does not have\n> to be re-sent only to fix this alone).\nI've read only about 50 characters max in commit head (and\nhighlighting repeats it), but there's nothing about max length of line\nin commit message. Sorry, next time I will make it shorter.\n\nAbout many different opinions how to improve the code: I agree with\nthe idea that my commit is a middle step to get rid of MRU at all. If\nwe really need to add initializer/mru_for_each/smth_else - it's\nabsolutely not a problem, but as it was said, not sure that we need\nit.\nIt really looks that using list implementation from list.h directly\nwon't be worse.\n\n> I had envisioned leaving mru_mark() as a wrapper for \"move to the front\"\n> that could operate on any list. But seeing how Olga's patch takes it\n> down to two trivial lines, I'd also be fine with an endgame that just\n> eliminates it.\nLet's add needed function to list.h directly? I also wanted to add\nlist_for_each_entry function to list.h as it's in Linux kernel.\nhttps://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each-entry.html\nIt will simplify the code even more, guess that not only in MRU\nrelated code. Maybe we need to do that in separate patch.\n\nAbout minor issues ( \"tmp\" vs \"p2\", variable scope, space indentation)\n- fully agree, I will fix it.\n\nSo finally I think that I need to fix that minor issues and that's\nall. I have plans to rewrite (with --amend) my current commit (I think\nso because I will add no new features, so it's better to have single\ncommit for all changes).\nAs I understand, Submitgit will send an update in a new thread. And I\nneed to say there [PATCH v2].\nPlease correct me if I am wrong in any of the moments mentioned earlier.\n\nBy the way, other contributors write smth like \"[PATCH v6 0/3]\". What\ndoes mean \"0/3\"? It's about editing separate commits in a single\npatch, am I right?\n\nThank you one more time!\nOlga\n"},{"id":"329214","messageId":"CAL21Bmma8gOYx9u4kxRaHJKcF3YsfrQP9=wdAiQX14f9uSPRAQ@mail.gmail.com","threadId":"46843","inReplyTo":"CAL21BmkcVSEhEK+tAE-RNVabb0pnokYwbagueUrp9giZ3zqT8A@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-29T20:38:27Z","receivedAt":"2017-09-29T20:38:36Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"> About minor issues ( \"tmp\" vs \"p2\", variable scope, space indentation)\n> - fully agree, I will fix it.\n> ...\n> So finally I think that I need to fix that minor issues and that's\n> all.\n\nI forgot about leak. I also need to add checking in mru_clear. That's\nnot beautiful solution but it works reliably.\nQuestion remains the same - do you see something more to fix in this patch?\n\nThank you!\nOlga\n"},{"id":"329227","messageId":"20170929233723.c7ixg5fb3flbgaom@sigill.intra.peff.net","threadId":"46843","inReplyTo":"CAL21BmkcVSEhEK+tAE-RNVabb0pnokYwbagueUrp9giZ3zqT8A@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-29T23:37:23Z","receivedAt":"2017-09-29T23:37:31Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 29, 2017 at 07:08:28PM +0300, Оля Тележная wrote:\n\n> Many thanks to all of you, I am interested in every opinion. Sorry\n> that I wasn't in the discussion, unfortunately I got sick, that's why\n> I skipped all the process.\n\nNo problem. It's often reasonable to let review comments come in and\nthink about them as a whole before responding or re-posting anyway.\n\n> > An overlong line (I can locally wrap it, so the patch does not have\n> > to be re-sent only to fix this alone).\n> I've read only about 50 characters max in commit head (and\n> highlighting repeats it), but there's nothing about max length of line\n> in commit message. Sorry, next time I will make it shorter.\n\nUsually we shoot for making things look good in an 80-column terminal,\nincluding both code and commit messages. For commit message bodies, we\ntend to make them a little shorter, since \"git log\" will indent them. 72\ncharacters is reasonable there. And we tend to make subject lines a\nlittle shorter than that, even since they often appear with \"Subject:\"\nand \"[PATCH]\" prefixed. I usually go for about 60 characters, but will\ngo over if it helps make the subject more clear.\n\n> > I had envisioned leaving mru_mark() as a wrapper for \"move to the front\"\n> > that could operate on any list. But seeing how Olga's patch takes it\n> > down to two trivial lines, I'd also be fine with an endgame that just\n> > eliminates it.\n> Let's add needed function to list.h directly?\n\nYes, I think we could just call this \"list_move_to_front()\" or\nsomething. The fact that it's operating on a list called\n\"packed_git_mru\" is probably sufficient to make it clear that the\npurpose is managing recentness.\n\n> I also wanted to add\n> list_for_each_entry function to list.h as it's in Linux kernel.\n> https://www.kernel.org/doc/htmldocs/kernel-api/API-list-for-each-entry.html\n> It will simplify the code even more, guess that not only in MRU\n> related code. Maybe we need to do that in separate patch.\n\nIt would be nice to have list_for_each_entry(), but unfortunately it's\nnot portable. It relies on having a typeof operator, and we build on\nplatforms that lack it. It was omitted in 94e99012fc (http-walker:\nreduce O(n) ops with doubly-linked list, 2016-07-11) for that reason.\n\n> About minor issues ( \"tmp\" vs \"p2\", variable scope, space indentation)\n> - fully agree, I will fix it.\n\nThanks.\n\n> So finally I think that I need to fix that minor issues and that's\n> all. I have plans to rewrite (with --amend) my current commit (I think\n> so because I will add no new features, so it's better to have single\n> commit for all changes).\n> As I understand, Submitgit will send an update in a new thread. And I\n> need to say there [PATCH v2].\n> Please correct me if I am wrong in any of the moments mentioned earlier.\n\nCorrect. Until a patch is merged to Junio's \"next\" branch (at which\npoint it is set in stone), we generally prefer to rewrite it with\n\"--amend\" (or git-rebase) to fix anything that comes up during the\nreview.\n\n> By the way, other contributors write smth like \"[PATCH v6 0/3]\". What\n> does mean \"0/3\"? It's about editing separate commits in a single\n> patch, am I right?\n\nRight, it means multiple commits in a logical series that are meant to\nbe applied together. Your patch is small enough that it makes as a\nsingle patch. If we wanted to do the second step of dropping mru.[ch]\nentirely now, then you'd probably have at least a 2-patch series.\n\n(I'm OK with not doing that second step for now, though if we are not\ngoing to polish up the mru_for_each() interface, it may make sense to\nmake the final step sooner rather than later).\n\n-Peff\n"},{"id":"329228","messageId":"20170929234002.3fzaksoarz75p7e2@sigill.intra.peff.net","threadId":"46843","inReplyTo":"CAL21Bmma8gOYx9u4kxRaHJKcF3YsfrQP9=wdAiQX14f9uSPRAQ@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-09-29T23:40:03Z","receivedAt":"2017-09-29T23:40:11Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Fri, Sep 29, 2017 at 11:38:27PM +0300, Оля Тележная wrote:\n\n> > About minor issues ( \"tmp\" vs \"p2\", variable scope, space indentation)\n> > - fully agree, I will fix it.\n> > ...\n> > So finally I think that I need to fix that minor issues and that's\n> > all.\n> \n> I forgot about leak. I also need to add checking in mru_clear. That's\n> not beautiful solution but it works reliably.\n\nI'm not sure what you mean about checking in mru_clear(). It may make\nsense to just send your v2 patch and I can see there what you do.\n\n> Question remains the same - do you see something more to fix in this patch?\n\nI think that's it. Think about writing a bit in the commit message about\nthe \"why\" of this. Like I said earlier, the rationale for this commit is\nperhaps obvious, but it never hurts to be explicit (and it may be good\npractice, since part of the point of this task is getting you familiar\nwith the patch submission process).\n\n-Peff\n"},{"id":"329229","messageId":"xmqq7ewhvyjq.fsf@gitster.mtv.corp.google.com","threadId":"46843","inReplyTo":"20170929233723.c7ixg5fb3flbgaom@sigill.intra.peff.net","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-09-30T00:07:53Z","receivedAt":"2017-09-30T00:08:03Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> Yes, I think we could just call this \"list_move_to_front()\" or\n> something. The fact that it's operating on a list called\n> \"packed_git_mru\" is probably sufficient to make it clear that the\n> purpose is managing recentness.\n\nI earlier said I wasn't sure, but I fully agree with your envisioned\nendgame state, my understanding of which is that we will not have an\nAPI that is only to be used to manage a MRU list (hence there will\nbe no mru.[ch] in that future), as there is very small MRU-specific\noperation to be offered anyway. Namely, mru_mark() that is to be\nused to \"mark the fact that the item was used---do whatever necessary\nto maintain the MRU list\".\n\nInstead, everybody understands how the list API works, and the fact\nthat one instance of list_head is called MRU is sufficient to see\nthat use of \"move this to the front of the list\" function means we\nare marking the item as most-recently-used.\n\nThanks.\n"},{"id":"329249","messageId":"0102015ed3e9b1a8-74821a55-aa9a-4e5a-b267-c3d2462e3eed-000000@eu-west-1.amazonses.com","threadId":"46843","inReplyTo":"0102015ec7a3424b-529be659-bdb6-42c4-a48f-db264f33d53a-000000@eu-west-1.amazonses.com","subject":"[PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Olga Telezhnaya","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-30T17:51:01Z","receivedAt":"2017-09-30T17:51:08Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"Simplify mru.[ch] and related code by reusing the double-linked list\nimplementation from list.h instead of a custom one.\nThis commit is an intermediate step. Our final goal is to get rid of\nmru.[ch] at all and inline all logic.\n\nSigned-off-by: Olga Telezhnaia \nMentored-by: Christian Couder \nMentored by: Jeff King \n---\n builtin/pack-objects.c | 5 +++--\n mru.c | 49 +++++++++++++------------------------------------\n mru.h | 31 +++++++++++++------------------\n packfile.c | 7 ++++---\n 4 files changed, 33 insertions(+), 59 deletions(-)\n\ndiff --git a/builtin/pack-objects.c b/builtin/pack-objects.c\nindex f721137eaf881..ba812349e0aab 100644\n--- a/builtin/pack-objects.c\n+++ b/builtin/pack-objects.c\n@@ -995,8 +995,8 @@ static int want_object_in_pack(const unsigned char *sha1,\n \t\t\t struct packed_git **found_pack,\n \t\t\t off_t *found_offset)\n {\n-\tstruct mru_entry *entry;\n \tint want;\n+\tstruct list_head *pos;\n \n \tif (!exclude && local && has_loose_object_nonlocal(sha1))\n \t\treturn 0;\n@@ -1012,7 +1012,8 @@ 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+\tlist_for_each(pos, &packed_git_mru.list) {\n+\t\tstruct mru *entry = list_entry(pos, struct mru, list);\n \t\tstruct packed_git *p = entry->item;\n \t\toff_t offset;\n \ndiff --git a/mru.c b/mru.c\nindex 9dedae0287ed2..8f3f34c5ba91d 100644\n--- a/mru.c\n+++ b/mru.c\n@@ -1,50 +1,27 @@\n #include \"cache.h\"\n #include \"mru.h\"\n \n-void mru_append(struct mru *mru, void *item)\n+void mru_append(struct mru *head, void *item)\n {\n-\tstruct mru_entry *cur = xmalloc(sizeof(*cur));\n+\tstruct mru *cur = xmalloc(sizeof(*cur));\n \tcur->item = item;\n-\tcur->prev = mru->tail;\n-\tcur->next = NULL;\n-\n-\tif (mru->tail)\n-\t\tmru->tail->next = cur;\n-\telse\n-\t\tmru->head = cur;\n-\tmru->tail = cur;\n+\tlist_add_tail(&cur->list, &head->list);\n }\n \n-void mru_mark(struct mru *mru, struct mru_entry *entry)\n+void mru_mark(struct mru *head, struct mru *entry)\n {\n-\t/* If we're already at the front of the list, nothing to do */\n-\tif (mru->head == entry)\n-\t\treturn;\n-\n-\t/* Otherwise, remove us from our current slot... */\n-\tif (entry->prev)\n-\t\tentry->prev->next = entry->next;\n-\tif (entry->next)\n-\t\tentry->next->prev = entry->prev;\n-\telse\n-\t\tmru->tail = entry->prev;\n-\n-\t/* And insert us at the beginning. */\n-\tentry->prev = NULL;\n-\tentry->next = mru->head;\n-\tif (mru->head)\n-\t\tmru->head->prev = entry;\n-\tmru->head = entry;\n+\t/* To mark means to put at the front of the list. */\n+\tlist_del(&entry->list);\n+\tlist_add(&entry->list, &head->list);\n }\n \n-void mru_clear(struct mru *mru)\n+void mru_clear(struct mru *head)\n {\n-\tstruct mru_entry *p = mru->head;\n+\tstruct list_head *pos;\n+\tstruct list_head *tmp;\n \n-\twhile (p) {\n-\t\tstruct mru_entry *to_free = p;\n-\t\tp = p->next;\n-\t\tfree(to_free);\n+\tlist_for_each_safe(pos, tmp, &head->list) {\n+\t\tfree(list_entry(pos, struct mru, list));\n \t}\n-\tmru->head = mru->tail = NULL;\n+\tINIT_LIST_HEAD(&head->list);\n }\ndiff --git a/mru.h b/mru.h\nindex 42e4aeaa1098a..80a589eb4c0eb 100644\n--- a/mru.h\n+++ b/mru.h\n@@ -1,6 +1,8 @@\n #ifndef MRU_H\n #define MRU_H\n \n+#include \"list.h\"\n+\n /**\n * A simple most-recently-used cache, backed by a doubly-linked list.\n *\n@@ -8,18 +10,15 @@\n *\n * // Create a list. Zero-initialization is required.\n * static struct mru cache;\n- * mru_append(&cache, item);\n- * ...\n+ * INIT_LIST_HEAD(&cache.list);\n *\n- * // Iterate in MRU order.\n- * struct mru_entry *p;\n- * for (p = cache.head; p; p = p->next) {\n- *\tif (matches(p->item))\n- *\t\tbreak;\n- * }\n+ * // Add new item to the end of the list.\n+ * void *item;\n+ * ...\n+ * mru_append(&cache, item);\n *\n * // Mark an item as used, moving it to the front of the list.\n- * mru_mark(&cache, p);\n+ * mru_mark(&cache, item);\n *\n * // Reset the list to empty, cleaning up all resources.\n * mru_clear(&cache);\n@@ -29,17 +28,13 @@\n * you will begin traversing the whole list again.\n */\n \n-struct mru_entry {\n-\tvoid *item;\n-\tstruct mru_entry *prev, *next;\n-};\n-\n struct mru {\n-\tstruct mru_entry *head, *tail;\n+\tstruct list_head list;\n+\tvoid *item;\n };\n \n-void mru_append(struct mru *mru, void *item);\n-void mru_mark(struct mru *mru, struct mru_entry *entry);\n-void mru_clear(struct mru *mru);\n+void mru_append(struct mru *head, void *item);\n+void mru_mark(struct mru *head, struct mru *entry);\n+void mru_clear(struct mru *head);\n \n #endif /* MRU_H */\ndiff --git a/packfile.c b/packfile.c\nindex f69a5c8d607af..502d915991bee 100644\n--- a/packfile.c\n+++ b/packfile.c\n@@ -40,7 +40,7 @@ static unsigned int pack_max_fds;\n static size_t peak_pack_mapped;\n static size_t pack_mapped;\n struct packed_git *packed_git;\n-struct mru packed_git_mru;\n+struct mru packed_git_mru = {{&packed_git_mru.list, &packed_git_mru.list}};\n \n #define SZ_FMT PRIuMAX\n static inline uintmax_t sz_fmt(size_t s) { return s; }\n@@ -1824,13 +1824,14 @@ static int fill_pack_entry(const unsigned char *sha1,\n */\n int find_pack_entry(const unsigned char *sha1, struct pack_entry *e)\n {\n-\tstruct mru_entry *p;\n+\tstruct list_head *pos;\n \n \tprepare_packed_git();\n \tif (!packed_git)\n \t\treturn 0;\n \n-\tfor (p = packed_git_mru.head; p; p = p->next) {\n+\tlist_for_each(pos, &packed_git_mru.list) {\n+\t\tstruct mru *p = list_entry(pos, struct mru, list);\n \t\tif (fill_pack_entry(sha1, e, p->item)) {\n \t\t\tmru_mark(&packed_git_mru, p);\n \t\t\treturn 1;\n\n--\nhttps://github.com/git/git/pull/409\n"},{"id":"329250","messageId":"CAL21Bmk2oN61CxJA0eju=FtAh2Ei9dLqRMPZKonCND4sC504Sg@mail.gmail.com","threadId":"46843","inReplyTo":"20170929234002.3fzaksoarz75p7e2@sigill.intra.peff.net","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-09-30T18:09:11Z","receivedAt":"2017-09-30T18:09:20Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"I added \"v2\" after \"PATCH\", but it does not appeared. Actually it was\nwritten automatically and it was \"PATCH Outreachy v2\". I rearranged it\nin the middle of the phrase.\n\n>> I forgot about leak. I also need to add checking in mru_clear. That's\n>> not beautiful solution but it works reliably.\n>\n> I'm not sure what you mean about checking in mru_clear(). It may make\n> sense to just send your v2 patch and I can see there what you do.\n\nI realized that I said something strange before. I solved the problem\nwith leak by deleting INIT in prepare_packed_git and adding init as\nyou suggested here:\nhttps://github.com/telezhnaya/git/commit/7f8995835949f83e54efdfd88445feb6d54cb3e9#commitcomment-24576103\n\nBy the way, I asked to edit FAQ for Linux kernel newbies about linked\nlist that confused me a week ago, and that funny picture was deleted.\nhttps://kernelnewbies.org/FAQ/LinkedLists\nMaybe it will help to someone else :)\n"},{"id":"329421","messageId":"20171002082020.c7ravpwgz45osrmz@sigill.intra.peff.net","threadId":"46843","inReplyTo":"0102015ed3e9b1a8-74821a55-aa9a-4e5a-b267-c3d2462e3eed-000000@eu-west-1.amazonses.com","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T08:20:20Z","receivedAt":"2017-10-02T08:20:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 30, 2017 at 05:51:01PM +0000, Olga Telezhnaya wrote:\n\n> Simplify mru.[ch] and related code by reusing the double-linked list\n> implementation from list.h instead of a custom one.\n> This commit is an intermediate step. Our final goal is to get rid of\n> mru.[ch] at all and inline all logic.\n\nThanks, this version looks correct to me.\n\nI do think there are a few ugly bits in the result (like that\ninitializer for packed_git_mru :) ), so I'd prefer not to merge this\ndown until we do that final step.\n\nSo the big question is: who wants to do it?\n\nI think you've done a good job here, and this would count for your\nOutreachy application's contribution. But if you'd like to do that next\nstep, you are welcome to.\n\nWe could also consider it a #leftoverbits that perhaps some other\nOutreachy candidate would pick up[1].\n\nIn the meantime, Junio, I think we'd want to queue this with the intent\nto graduate it to \"pu\" or possibly \"next\", but not \"master\". Then if\nsomebody (Olga or another applicant) produces the endgame patch, we can\nqueue it on top and move it further. And if nobody does, I can pick it\nafter the application period is over.\n\n-Peff\n\n[1] For those who find this mail through the archive, there's more\n discussion in this thread:\n\n https://public-inbox.org/git/CAL21BmnvJSaN+Tnw7Hdc5P5biAnM5dfWR7gX5FrAG1r_D8th=A@mail.gmail.com/\n"},{"id":"329422","messageId":"20171002082213.bpnhql4rfl523p3t@sigill.intra.peff.net","threadId":"46843","inReplyTo":"CAL21Bmk2oN61CxJA0eju=FtAh2Ei9dLqRMPZKonCND4sC504Sg@mail.gmail.com","subject":"Re: [PATCH Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-02T08:22:13Z","receivedAt":"2017-10-02T08:22:20Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Sat, Sep 30, 2017 at 09:09:11PM +0300, Оля Тележная wrote:\n\n> I added \"v2\" after \"PATCH\", but it does not appeared. Actually it was\n> written automatically and it was \"PATCH Outreachy v2\". I rearranged it\n> in the middle of the phrase.\n\nThat looks fine.\n\n> > I'm not sure what you mean about checking in mru_clear(). It may make\n> > sense to just send your v2 patch and I can see there what you do.\n> \n> I realized that I said something strange before. I solved the problem\n> with leak by deleting INIT in prepare_packed_git and adding init as\n> you suggested here:\n> https://github.com/telezhnaya/git/commit/7f8995835949f83e54efdfd88445feb6d54cb3e9#commitcomment-24576103\n\nOK, that makes sense.\n\n> By the way, I asked to edit FAQ for Linux kernel newbies about linked\n> list that confused me a week ago, and that funny picture was deleted.\n> https://kernelnewbies.org/FAQ/LinkedLists\n> Maybe it will help to someone else :)\n\nThank you. :)\n\nRemember (and this applies to other Outreachy folks, too) to submit your\napplication in the Outreachy system:\n\n https://outreachy.gnome.org\n\n-Peff\n"},{"id":"329425","messageId":"CAL21Bmn+-2PwkXtK19fvVw-kEx_nftAx9QwvR=h-B-O-ofTqQQ@mail.gmail.com","threadId":"46843","inReplyTo":"20171002082020.c7ravpwgz45osrmz@sigill.intra.peff.net","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-10-02T09:37:53Z","receivedAt":"2017-10-02T09:38:00Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":">> Simplify mru.[ch] and related code by reusing the double-linked list\n>> implementation from list.h instead of a custom one.\n>> This commit is an intermediate step. Our final goal is to get rid of\n>> mru.[ch] at all and inline all logic.\n>\n> Thanks, this version looks correct to me.\n\nGreat! What is better - to complete my application now (and say only\nabout this patch) or to complete it in last days (and say about\neverything that I've done. Maybe there would be something new).\n\n> I do think there are a few ugly bits in the result (like that\n> initializer for packed_git_mru :) ), so I'd prefer not to merge this\n> down until we do that final step.\n>\n> So the big question is: who wants to do it?\n>\n> I think you've done a good job here, and this would count for your\n> Outreachy application's contribution. But if you'd like to do that next\n> step, you are welcome to.\n\nI was thinking about starting my small research about main task of the\ninternship. I could postpone it and end with this task, there's no\nproblem for me.\nOr, if someone from the newbies wants to have a small simple task -\nit's a great opportunity for him/her. By the way, I am ready to help\nif he/she will have questions or difficulties.\nSo please make a decision on your own, I will be happy in any scenario.\n"},{"id":"329546","messageId":"20171003101029.i6frkaipi5dq5bvr@sigill.intra.peff.net","threadId":"46843","inReplyTo":"CAL21Bmn+-2PwkXtK19fvVw-kEx_nftAx9QwvR=h-B-O-ofTqQQ@mail.gmail.com","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-10-03T10:10:29Z","receivedAt":"2017-10-03T10:10:36Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Mon, Oct 02, 2017 at 12:37:53PM +0300, Оля Тележная wrote:\n\n> >> Simplify mru.[ch] and related code by reusing the double-linked list\n> >> implementation from list.h instead of a custom one.\n> >> This commit is an intermediate step. Our final goal is to get rid of\n> >> mru.[ch] at all and inline all logic.\n> >\n> > Thanks, this version looks correct to me.\n> \n> Great! What is better - to complete my application now (and say only\n> about this patch) or to complete it in last days (and say about\n> everything that I've done. Maybe there would be something new).\n\nI think it's good to get it started now. You can always make changes to\nit later. And in particular, you should fill out the eligibility section\ns we can make sure that part is good before going further.\n\n> > I think you've done a good job here, and this would count for your\n> > Outreachy application's contribution. But if you'd like to do that next\n> > step, you are welcome to.\n> \n> I was thinking about starting my small research about main task of the\n> internship. I could postpone it and end with this task, there's no\n> problem for me.\n\nI do think you should spend some time thinking about and researching the\nmain task. I'd hope to see in the applications that the candidate has\na sense of the problem space and a realistic plan for approaching the\nwork.\n\n> Or, if someone from the newbies wants to have a small simple task -\n> it's a great opportunity for him/her. By the way, I am ready to help\n> if he/she will have questions or difficulties.\n\nWonderful. :) I do hope to get Outreachy interns involve in normal list\nactivities like discussion and review, not just producing their own\npatches.\n\n> So please make a decision on your own, I will be happy in any scenario.\n\nWhy don't we give it a week or so to see if anybody else picks it up.\n\n-Peff\n"},{"id":"332057","messageId":"xmqqshdpmtm9.fsf@gitster.mtv.corp.google.com","threadId":"46843","inReplyTo":"20171002082020.c7ravpwgz45osrmz@sigill.intra.peff.net","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Junio C Hamano","fromEmail":"gitster@pobox.com","sentAt":"2017-11-08T01:44:46Z","receivedAt":"2017-11-08T01:44:53Z","isPatch":true,"sender":{"key":"gitster@pobox.com","avatar":"https://avatars.githubusercontent.com/u/54884?v=4"},"body":"Jeff King writes:\n\n> I do think there are a few ugly bits in the result (like that\n> initializer for packed_git_mru :) ), so I'd prefer not to merge this\n> down until we do that final step.\n>\n> So the big question is: who wants to do it?\n>\n> I think you've done a good job here, and this would count for your\n> Outreachy application's contribution. But if you'd like to do that next\n> step, you are welcome to.\n>\n> We could also consider it a #leftoverbits that perhaps some other\n> Outreachy candidate would pick up[1].\n>\n> In the meantime, Junio, I think we'd want to queue this with the intent\n> to graduate it to \"pu\" or possibly \"next\", but not \"master\". Then if\n> somebody (Olga or another applicant) produces the endgame patch, we can\n> queue it on top and move it further. And if nobody does, I can pick it\n> after the application period is over.\n\nSo... do we still want to keep this in 'next', or does somebody\nwants to do honors? \n\nI'd really prefer *not* to see you or me ending up finishing it up,\nbut at the same time, I really hate seeing a halfway or 3/4 done\ntopic hanging around in 'pu'.\n\n> [1] For those who find this mail through the archive, there's more\n> discussion in this thread:\n>\n> https://public-inbox.org/git/CAL21BmnvJSaN+Tnw7Hdc5P5biAnM5dfWR7gX5FrAG1r_D8th=A@mail.gmail.com/\n"},{"id":"332064","messageId":"20171108042219.xfx7tevgkwzrqdtd@sigill.intra.peff.net","threadId":"46843","inReplyTo":"xmqqshdpmtm9.fsf@gitster.mtv.corp.google.com","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Jeff King","fromEmail":"peff@peff.net","sentAt":"2017-11-08T04:22:20Z","receivedAt":"2017-11-08T04:22:26Z","isPatch":true,"sender":{"key":"peff@peff.net","avatar":"https://avatars.githubusercontent.com/u/45925?v=4"},"body":"On Wed, Nov 08, 2017 at 10:44:46AM +0900, Junio C Hamano wrote:\n\n> > We could also consider it a #leftoverbits that perhaps some other\n> > Outreachy candidate would pick up[1].\n> >\n> > In the meantime, Junio, I think we'd want to queue this with the intent\n> > to graduate it to \"pu\" or possibly \"next\", but not \"master\". Then if\n> > somebody (Olga or another applicant) produces the endgame patch, we can\n> > queue it on top and move it further. And if nobody does, I can pick it\n> > after the application period is over.\n> \n> So... do we still want to keep this in 'next', or does somebody\n> wants to do honors? \n> \n> I'd really prefer *not* to see you or me ending up finishing it up,\n> but at the same time, I really hate seeing a halfway or 3/4 done\n> topic hanging around in 'pu'.\n\nWe hung back on it to leave it as low-hanging fruit for other Outreachy\napplicants. Perhaps Olga would like to pick it up now that the\napplication period is over.\n\nAlternatively, it might be a nice task for the Bloomberg hackathon this\nweekend. I'll add it to the list of topics there, too.\n\n-Peff\n"},{"id":"332169","messageId":"CAL21Bm=+p0bTc7R5mHbrSevkGc6vw4FT=U-ghCZCNB7OoU7Kdw@mail.gmail.com","threadId":"46843","inReplyTo":"20171108042219.xfx7tevgkwzrqdtd@sigill.intra.peff.net","subject":"Re: [PATCH v2 Outreachy] mru: use double-linked list from list.h","fromName":"Оля Тележная","fromEmail":"olyatelezhnaya@gmail.com","sentAt":"2017-11-10T11:51:48Z","receivedAt":"2017-11-10T11:51:56Z","isPatch":true,"sender":{"key":"olyatelezhnaya@gmail.com","avatar":"https://avatars.githubusercontent.com/u/11246099?v=4"},"body":"> We hung back on it to leave it as low-hanging fruit for other Outreachy\n> applicants. Perhaps Olga would like to pick it up now that the\n> application period is over.\n\nIt's absolutely not a problem for me, I can do that as one more\nwarm-up exercise in the beginning of the internship.\n\nThanks!\n"}]}