[PATCH v6] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is
the final step in removing the mru API completely and inlining the logic.
This patch leads to significant code reduction and the mru API hence, is
not a useful abstraction anymore.
Signed-off-by: Gargi Sharma <gs051...@gmail.com>
---
The commit has been built on top of ot/mru-on-list branch.
Changes in v6:
- Replace mru_clear with INIT_LIST_HEAD() to reset
the mru list.
Changes in v5:
- Remove list_move_to_front() as it is redundant.
Changes in v4:
- Fixing minor style issues.
Changes in v3:
- Make the commit message more descriptive.
- Remove braces after if statement.
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.
A future improvement could be removing/changing the
type of nect pointer or dropping it entirely. See discussion
here:
https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/
---
---
Makefile | 1 -
builtin/pack-objects.c | 9 -
cache.h| 8
mru.c | 27 ---
mru.h | 40
packfile.c | 17 +
sha1_file.c| 1 -
7 files changed, 17 insertions(+), 86 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..6a0b8e8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
if (p == *found_pack)
@@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ list_move(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..cc09e3b 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,9 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
struct pack_entry {
off_t offset;
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to put at the front of the list. */
- list_del(>list);
- list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
- struct list_head *pos;
- struct list_head *tmp;
-
- list_for_each_safe(pos, tmp, >list) {
- free(list_entry(pos, struct mru, list));
- }
- INIT_LIST_HEAD(>list);
-}
diff --git a/mru.h b/mru.h
deleted fil
[PATCH v5] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is
the final step in removing the mru API completely and inlining the logic.
This patch leads to significant code reduction and the mru API hence, is
not a useful abstraction anymore.
Signed-off-by: Gargi Sharma <gs051...@gmail.com>
---
The commit has been built on top of ot/mru-on-list branch.
Changes in v5:
- Remove list_move_to_front() as it is redundant.
Changes in v4:
- Fixing minor style issues.
Changes in v3:
- Make the commit message more descriptive.
- Remove braces after if statement.
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.
A future improvement could be removing/changing the
type of nect pointer or dropping it entirely. See discussion
here:
https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/
---
Makefile | 1 -
builtin/pack-objects.c | 9 -
cache.h| 8
mru.c | 27 ---
mru.h | 40
packfile.c | 15 +++
sha1_file.c| 1 -
7 files changed, 15 insertions(+), 86 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..6a0b8e8 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
if (p == *found_pack)
@@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ list_move(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..cc09e3b 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,9 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
struct pack_entry {
off_t offset;
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to put at the front of the list. */
- list_del(>list);
- list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
- struct list_head *pos;
- struct list_head *tmp;
-
- list_for_each_safe(pos, tmp, >list) {
- free(list_entry(pos, struct mru, list));
- }
- INIT_LIST_HEAD(>list);
-}
diff --git a/mru.h b/mru.h
deleted file mode 100644
index 80a589e..000
--- a/mru.h
+++ /dev/null
@@ -1,40 +0,0 @@
-#ifndef MRU_H
-#define MRU_H
-
-
Re: [PATCH v3] mru: Replace mru.[ch] with list.h implementation
On Sun, Jan 21, 2018 at 4:38 AM, René Scharfe <l@web.de> wrote:
>
> Am 20.01.2018 um 23:24 schrieb Gargi Sharma:
> > On Sat, Jan 20, 2018 at 1:02 AM, Eric Wong <e...@80x24.org> wrote:
> >> Gargi Sharma <gs051...@gmail.com> wrote:
> >>> --- a/list.h
> >>> +++ b/list.h
> >>> @@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem,
> >>> struct list_head *head)
> >>>list_add(elem, head);
> >>> }
> >>>
> >>> +/* Move to the front of the list. */
> >>> +static inline void list_move_to_front(struct list_head *elem, struct
> >>> list_head *head)
> >>> +{
> >>> + list_del(elem);
> >>> + list_add(elem, head);
> >>> +}
> >>> +
> >>
> >> Since we already have list_move and it does the same thing,
> >> I don't think we need a new function, here.
> >>
> >> Hackers coming from other projects (glibc/urcu/Linux kernel)
> >> might appreciate having fewer differences from what they're used
> >> to.
> >
> > I think the idea behind this function was that it is already being used in
> > two
> > places in the code and might be used in other places in the future. I agree
> > with your stance, but a list_move_to_front is pretty self explanatory and
> > not
> > too different, so it should be alright.
>
> Not sure I understand the point about the function being already used as
> an argument for adding it, but if there is already one which has the
> exact sane behavior (list_move() in this case) then that should be used
> instead of adding a duplicate.
Thanks for bringing this to my attention, René. I can use list_move()
to do the exact
same thing as list_move_to_front(). Will send another version.
Thanks,
Gargi
>
> René
>
[PATCH v4] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is
the final step in removing the mru API completely and inlining the logic.
This patch leads to significant code reduction and the mru API hence, is
not a useful abstraction anymore.
Signed-off-by: Gargi Sharma <gs051...@gmail.com>
---
Changes in v4:
- Fixing minor style issues.
Changes in v3:
- Make the commit message more descriptive.
- Remove braces after if statement.
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.
The commit has been built on top of ot/mru-on-list branch.
A future improvement could be removing/changing the
type of nect pointer or dropping it entirely. See discussion
here:
https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/
---
Makefile | 1 -
builtin/pack-objects.c | 9 -
cache.h| 8
list.h | 7 +++
mru.c | 27 ---
mru.h | 40
packfile.c | 15 +++
sha1_file.c| 1 -
8 files changed, 22 insertions(+), 86 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..188ba3e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
if (p == *found_pack)
@@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ list_move_to_front(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..cc09e3b 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,9 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
struct pack_entry {
off_t offset;
diff --git a/list.h b/list.h
index eb60119..5129b0a 100644
--- a/list.h
+++ b/list.h
@@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, struct
list_head *head)
list_add(elem, head);
}
+/* Move to the front of the list. */
+static inline void list_move_to_front(struct list_head *elem, struct list_head
*head)
+{
+ list_del(elem);
+ list_add(elem, head);
+}
+
/* Replace an old entry. */
static inline void list_replace(struct list_head *old, struct list_head *newp)
{
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to
Re: [PATCH v3] mru: Replace mru.[ch] with list.h implementation
On Sat, Jan 20, 2018 at 12:59 AM, Eric Sunshine <sunsh...@sunshineco.com> wrote:
> On Fri, Jan 19, 2018 at 6:36 PM, Gargi Sharma <gs051...@gmail.com> wrote:
>> Replace the custom calls to mru.[ch] with calls to list.h. This patch is
>> the final step in removing the mru API completely and inlining the logic.
>> This patch leads to significant code reduction and the mru API hence, is
>> not a useful abstraction anymore.
>
> A couple minor style nits below... (may not be worth a re-roll)
I can send a v4, it shouldn't be a problem. :)
>
>> Signed-off-by: Gargi Sharma <gs051...@gmail.com>
>> ---
>> diff --git a/cache.h b/cache.h
>> @@ -1587,10 +1588,10 @@ extern struct packed_git {
>> } *packed_git;
>>
>> /*
>> - * A most-recently-used ordered version of the packed_git list, which can
>> - * be iterated instead of packed_git (and marked via mru_mark).
>> + * A most-recently-used ordered version of the packed_git list.
>> */
>> -extern struct mru packed_git_mru;
>> +extern struct list_head packed_git_mru;
>> +
>
> Unnecessary extra blank line.
>
>> struct pack_entry {
>> off_t offset;
>> diff --git a/packfile.c b/packfile.c
>> @@ -859,9 +859,9 @@ static void prepare_packed_git_mru(void)
>> {
>> struct packed_git *p;
>>
>> - mru_clear(_git_mru);
>> - for (p = packed_git; p; p = p->next)
>> - mru_append(_git_mru, p);
>> + for (p = packed_git; p; p = p->next) {
>> + list_add_tail(>mru, _git_mru);
>> + }
>
> Unnecessary braces on for-loop.
>
>> }
Re: [PATCH v3] mru: Replace mru.[ch] with list.h implementation
On Sat, Jan 20, 2018 at 1:02 AM, Eric Wong <e...@80x24.org> wrote:
> Gargi Sharma <gs051...@gmail.com> wrote:
>> --- a/list.h
>> +++ b/list.h
>> @@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem,
>> struct list_head *head)
>> list_add(elem, head);
>> }
>>
>> +/* Move to the front of the list. */
>> +static inline void list_move_to_front(struct list_head *elem, struct
>> list_head *head)
>> +{
>> + list_del(elem);
>> + list_add(elem, head);
>> +}
>> +
>
> Since we already have list_move and it does the same thing,
> I don't think we need a new function, here.
>
> Hackers coming from other projects (glibc/urcu/Linux kernel)
> might appreciate having fewer differences from what they're used
> to.
I think the idea behind this function was that it is already being used in two
places in the code and might be used in other places in the future. I agree
with your stance, but a list_move_to_front is pretty self explanatory and not
too different, so it should be alright.
>
> Anyways thanks for working on this!
Re: [PATCH] mru: Replace mru.[ch] with list.h implementation
On Fri, Jan 19, 2018 at 9:46 PM, Jeff King <p...@peff.net> wrote: > > On Mon, Jan 15, 2018 at 08:46:25PM -0500, Gargi Sharma wrote: > > > Replace the custom calls to mru.[ch] with calls to list.h. This patch is the > > final step in removing the mru API completely and inlining the logic. > > > > Another discussion, here > > (https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/) > > was on what has to be done with the next pointer of packed git type > > inside the > > packed_git structure. It can be removed _given_ that no one needs to > > access the list in order and can be sent as another patch. > > Thanks for picking this up again. I agree that this is probably a good > stopping point for now, as I think just combining this with the 'next' > pointer may carry more side effects. Agreed, hence just thought that if the discussion is started again, we can point them to the email thread. > > Aside from the braces thing that Christian mentioned (and the missing > signoff), this all looks good to me. Thanks, made the changes and sent a v3, Best, Gargi > > -Peff
[PATCH v3] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is
the final step in removing the mru API completely and inlining the logic.
This patch leads to significant code reduction and the mru API hence, is
not a useful abstraction anymore.
Signed-off-by: Gargi Sharma <gs051...@gmail.com>
---
Changes in v3:
- Make the commit message more descriptive.
- Remove braces after if statement.
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.
The commit has been built on top of ot/mru-on-list branch.
A future improvement could be removing/changing the
type of nect pointer or dropping it entirely. See discussion
here:
https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/
---
Makefile | 1 -
builtin/pack-objects.c | 9 -
cache.h| 9 +
list.h | 7 +++
mru.c | 27 ---
mru.h | 40
packfile.c | 18 +-
sha1_file.c| 1 -
8 files changed, 25 insertions(+), 87 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..188ba3e 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
if (p == *found_pack)
@@ -1031,7 +1030,7 @@ static int want_object_in_pack(const unsigned char *sha1,
}
want = want_found_object(exclude, p);
if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ list_move_to_front(>mru, _git_mru);
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..1a275ae 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,10 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
+
struct pack_entry {
off_t offset;
diff --git a/list.h b/list.h
index eb60119..5129b0a 100644
--- a/list.h
+++ b/list.h
@@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, struct
list_head *head)
list_add(elem, head);
}
+/* Move to the front of the list. */
+static inline void list_move_to_front(struct list_head *elem, struct list_head
*head)
+{
+ list_del(elem);
+ list_add(elem, head);
+}
+
/* Replace an old entry. */
static inline void list_replace(struct list_head *old, struct list_head *newp)
{
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to put at the front of the list. */
- list_
[PATCH] mru: Replace mru.[ch] with list.h implementation
Replace the custom calls to mru.[ch] with calls to list.h. This patch is the
final step in removing the mru API completely and inlining the logic.
Another discussion, here
(https://public-inbox.org/git/caoci2dgyqr4jff5oby2buyhnjeaapqkf8tbojn2w0b18eo+...@mail.gmail.com/)
was on what has to be done with the next pointer of packed git type
inside the
packed_git structure. It can be removed _given_ that no one needs to
access the list in order and can be sent as another patch.
---
Changes in v2:
- Add a move to front function to the list API.
- Remove memory leak.
- Remove redundant remove operations on the list.
The commit has been built on top of ot/mru-on-list branch.
Makefile | 1 -
builtin/pack-objects.c | 12 ++--
cache.h| 9 +
list.h | 7 +++
mru.c | 27 ---
mru.h | 40
packfile.c | 18 +-
sha1_file.c| 1 -
8 files changed, 27 insertions(+), 88 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/Makefile b/Makefile
index ed4ca43..4a79ec5 100644
--- a/Makefile
+++ b/Makefile
@@ -814,7 +814,6 @@ LIB_OBJS += merge.o
LIB_OBJS += merge-blobs.o
LIB_OBJS += merge-recursive.o
LIB_OBJS += mergesort.o
-LIB_OBJS += mru.o
LIB_OBJS += name-hash.o
LIB_OBJS += notes.o
LIB_OBJS += notes-cache.o
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..4064e35 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,8 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
off_t offset;
if (p == *found_pack)
@@ -1030,8 +1029,9 @@ static int want_object_in_pack(const unsigned char *sha1,
*found_pack = p;
}
want = want_found_object(exclude, p);
- if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ if (!exclude && want > 0) {
+ list_move_to_front(>mru, _git_mru);
+ }
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..1a275ae 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,10 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
+
struct pack_entry {
off_t offset;
diff --git a/list.h b/list.h
index eb60119..5129b0a 100644
--- a/list.h
+++ b/list.h
@@ -93,6 +93,13 @@ static inline void list_move(struct list_head *elem, struct
list_head *head)
list_add(elem, head);
}
+/* Move to the front of the list. */
+static inline void list_move_to_front(struct list_head *elem, struct list_head
*head)
+{
+ list_del(elem);
+ list_add(elem, head);
+}
+
/* Replace an old entry. */
static inline void list_replace(struct list_head *old, struct list_head *newp)
{
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to put at the front of the list. */
- list_del(>list);
- list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
- struct list_head *pos;
- struct list_head *tmp;
-
- list_for_each_safe(pos, tmp, >list) {
- free(list_entry(pos, struct
Re: [PATCH] mru: Replace mru.[ch] with list.h implementation
On Sun, Nov 12, 2017 at 9:54 AM, Jeff King <p...@peff.net> wrote:
> On Sat, Nov 11, 2017 at 01:06:46PM -0500, Gargi Sharma wrote:
>
>> Replace custom allocation in mru.[ch] with generic calls
>> to list.h API.
>>
>> Signed-off-by: Gargi Sharma <gs051...@gmail.com>
>
> Thanks, and welcome to the git list. :)
>
> This looks like a good start on the topic, but I have a few comments.
>
> It's a good idea to explain in the commit message not just what we're
> doing, but why we want to do it, to help later readers of "git log". I
> know that you picked this up from the discussion in the thread at:
>
> https://public-inbox.org/git/xmqq8tgz13x7@gitster.mtv.corp.google.com/
>
> so it might be a good idea to summarize the ideas there (and add your
> own thoughts, of course).
>
>> ---
>> builtin/pack-objects.c | 14 --
>> cache.h| 9 +
>> mru.c | 27 ---
>> mru.h | 40
>> packfile.c | 28 +++-
>> 5 files changed, 32 insertions(+), 86 deletions(-)
>> delete mode 100644 mru.c
>> delete mode 100644 mru.h
>
> After the "---" line, you can put any information that people on the
> list might want to know but that doesn't need to go into the commit
> message. The big thing the maintainer would want to know here is that
> your patch is prepared on top of the ot/mru-on-list topic, so he knows
> where to apply it.
>
> The diffstat is certainly encouraging so far. :)
>
>> @@ -1012,9 +1012,9 @@ static int want_object_in_pack(const unsigned char
>> *sha1,
>> return want;
>> }
>>
>> - list_for_each(pos, _git_mru.list) {
>> - struct mru *entry = list_entry(pos, struct mru, list);
>> - struct packed_git *p = entry->item;
>> + list_for_each(pos, _git_mru) {
>> + struct packed_git *p = list_entry(pos, struct packed_git, mru);
>> + struct list_head *entry = &(p->mru);
>> off_t offset;
>>
>> if (p == *found_pack)
>
> I think "entry" here is going to be the same as "pos". That said, I
> think it's only use is in bumping us to the front of the mru list later:
>
>> @@ -1030,8 +1030,10 @@ static int want_object_in_pack(const unsigned char
>> *sha1,
>> *found_pack = p;
>> }
>> want = want_found_object(exclude, p);
>> - if (!exclude && want > 0)
>> - mru_mark(_git_mru, entry);
>> + if (!exclude && want > 0) {
>> + list_del(entry);
>> + list_add(entry, _git_mru);
>> + }
>
> And I think this might be more obvious if we drop "entry" entirely and
> just do:
>
> list_del(>mru);
> list_add(>mru, _git_mru);
>
> It might merit a comment like "/* bump to the front of the mru list */"
> or similar to make it clear what's going on (or even adding a
> list_move_to_front() helper).
I will add a helper to list.h, for doing this :)
>
>> @@ -1566,6 +1566,7 @@ struct pack_window {
>>
>> extern struct packed_git {
>> struct packed_git *next;
>> + struct list_head mru;
>> struct pack_window *windows;
>> off_t pack_size;
>> const void *index_data;
>
> Sort of a side note, but seeing these two list pointers together makes
> me wonder what we should do with the list created by the "next" pointer.
> It seems like there are three options:
>
> 1. Convert it to "struct list_head", too, for consistency.
>
> 2. Leave it as-is. We never delete from the list nor do any fancy
> manipulation, so it doesn't benefit from the reusable code.
>
> 3. I wonder if we could drop it entirely, and just keep a single list
> of packs, ordered by mru. I'm not sure if anybody actually cares
> about accessing them in the "original" order. That order is
> reverse-chronological (by prepare_packed_git()), but I think that
> was mostly out of a sense that recent packs would be accessed more
> than older ones (but having a real mru strategy replaces that
> anyway).
>
> What do you think?
I think in the long run, it'll be easier if there is only a single
list of packs given
that no one needs to access the list in order.
If we go
[PATCH] mru: Replace mru.[ch] with list.h implementation
Replace custom allocation in mru.[ch] with generic calls
to list.h API.
Signed-off-by: Gargi Sharma <gs051...@gmail.com>
---
builtin/pack-objects.c | 14 --
cache.h| 9 +
mru.c | 27 ---
mru.h | 40
packfile.c | 28 +++-
5 files changed, 32 insertions(+), 86 deletions(-)
delete mode 100644 mru.c
delete mode 100644 mru.h
diff --git a/builtin/pack-objects.c b/builtin/pack-objects.c
index ba81234..26717c5 100644
--- a/builtin/pack-objects.c
+++ b/builtin/pack-objects.c
@@ -24,7 +24,7 @@
#include "reachable.h"
#include "sha1-array.h"
#include "argv-array.h"
-#include "mru.h"
+#include "list.h"
#include "packfile.h"
static const char *pack_usage[] = {
@@ -1012,9 +1012,9 @@ static int want_object_in_pack(const unsigned char *sha1,
return want;
}
- list_for_each(pos, _git_mru.list) {
- struct mru *entry = list_entry(pos, struct mru, list);
- struct packed_git *p = entry->item;
+ list_for_each(pos, _git_mru) {
+ struct packed_git *p = list_entry(pos, struct packed_git, mru);
+ struct list_head *entry = &(p->mru);
off_t offset;
if (p == *found_pack)
@@ -1030,8 +1030,10 @@ static int want_object_in_pack(const unsigned char *sha1,
*found_pack = p;
}
want = want_found_object(exclude, p);
- if (!exclude && want > 0)
- mru_mark(_git_mru, entry);
+ if (!exclude && want > 0) {
+ list_del(entry);
+ list_add(entry, _git_mru);
+ }
if (want != -1)
return want;
}
diff --git a/cache.h b/cache.h
index 49b083e..1a275ae 100644
--- a/cache.h
+++ b/cache.h
@@ -4,7 +4,7 @@
#include "git-compat-util.h"
#include "strbuf.h"
#include "hashmap.h"
-#include "mru.h"
+#include "list.h"
#include "advice.h"
#include "gettext.h"
#include "convert.h"
@@ -1566,6 +1566,7 @@ struct pack_window {
extern struct packed_git {
struct packed_git *next;
+ struct list_head mru;
struct pack_window *windows;
off_t pack_size;
const void *index_data;
@@ -1587,10 +1588,10 @@ extern struct packed_git {
} *packed_git;
/*
- * A most-recently-used ordered version of the packed_git list, which can
- * be iterated instead of packed_git (and marked via mru_mark).
+ * A most-recently-used ordered version of the packed_git list.
*/
-extern struct mru packed_git_mru;
+extern struct list_head packed_git_mru;
+
struct pack_entry {
off_t offset;
diff --git a/mru.c b/mru.c
deleted file mode 100644
index 8f3f34c..000
--- a/mru.c
+++ /dev/null
@@ -1,27 +0,0 @@
-#include "cache.h"
-#include "mru.h"
-
-void mru_append(struct mru *head, void *item)
-{
- struct mru *cur = xmalloc(sizeof(*cur));
- cur->item = item;
- list_add_tail(>list, >list);
-}
-
-void mru_mark(struct mru *head, struct mru *entry)
-{
- /* To mark means to put at the front of the list. */
- list_del(>list);
- list_add(>list, >list);
-}
-
-void mru_clear(struct mru *head)
-{
- struct list_head *pos;
- struct list_head *tmp;
-
- list_for_each_safe(pos, tmp, >list) {
- free(list_entry(pos, struct mru, list));
- }
- INIT_LIST_HEAD(>list);
-}
diff --git a/mru.h b/mru.h
deleted file mode 100644
index 80a589e..000
--- a/mru.h
+++ /dev/null
@@ -1,40 +0,0 @@
-#ifndef MRU_H
-#define MRU_H
-
-#include "list.h"
-
-/**
- * A simple most-recently-used cache, backed by a doubly-linked list.
- *
- * Usage is roughly:
- *
- * // Create a list. Zero-initialization is required.
- * static struct mru cache;
- * INIT_LIST_HEAD();
- *
- * // Add new item to the end of the list.
- * void *item;
- * ...
- * mru_append(, item);
- *
- * // Mark an item as used, moving it to the front of the list.
- * mru_mark(, item);
- *
- * // Reset the list to empty, cleaning up all resources.
- * mru_clear();
- *
- * Note that you SHOULD NOT call mru_mark() and then continue traversing the
- * list; it reorders the marked item to the front of the list, and therefore
- * you will begin traversing the whole list again.
- */
-
-struct mru {
- struct list_head list;
- void *item;
-};
-
-void mru_append(struct mru *head, void *item);
-void mru_mark(struct mru *head, struct mru *entry);
-void mru_clear(struct mru *head);
