Re: [PATCH 18/25] list-files: delete redundant cached entries

Eric Sunshine [off-list ref] writes:

On Mon, Apr 6, 2015 at 9:52 AM, Nguyễn Thái Ngọc Duy [off-list ref] wrote:

quoted

When both --cached and one of -amdAMD is used together we may have two
entries of the same path, e.g. "  foo" and "MM foo". In this case it's
pretty clear that "foo" must be tracked, no need to display "   foo".
The new function does that.

Helped-by: Eric Sunshine [off-list ref]
Signed-off-by: Nguyễn Thái Ngọc Duy <redacted>
---

diff --git a/builtin/list-files.c b/builtin/list-files.c
index 14ffd62..31c2336 100644
--- a/builtin/list-files.c
+++ b/builtin/list-files.c

@@ -93,7 +93,10 @@ static int compare_item(const void *a_, const void *b_)
 {
        const struct item *a = a_;
        const struct item *b = b_;
-       return strcmp(a->path, b->path);
+       int ret = strcmp(a->path, b->path);
+       if (ret)
+               return ret;
+       return strncmp(a->tag, b->tag, 2);
 }

 static void free_item(struct item *item)

@@ -132,7 +135,12 @@ static void remove_duplicates(struct item_list *list)
        for (src = dst = 1; src < list->nr; src++) {
                if (!compare_item(list->items + dst - 1, list->items + src))
                        free_item(list->items + src);
-               else
+               else if ((list->items[dst - 1].tag[0] == ' ' &&
+                         list->items[dst - 1].tag[1] == ' ' &&
+                         !strcmp(list->items[src].path, list->items[dst - 1].path))) {
+                       free_item(list->items + dst - 1);
+                       list->items[dst - 1] = list->items[src];

I was wondering if you could drop this backward-patching case by
having tag=="  " items sort after tag="xx" items and just fold out the
tag=="  " items normally in the preceding 'if', however, when I
started coding it, I found that the resulting code wasn't any more
pleasant.

You may read from wt-status and then index (or the other way around)
into the set of files to be listed---instead of deduping at the end,
you should logically do "add in from index only for entries we did
not see in wt-status output".  So some deduping is necessary.

But I actually think that the worst aspect of the series this part
of the code shows is the same issue I raised in the previous review
round in the approach, which has not been addressed at all as far as
I can see in this round.

Why does tag[] record the semantic meaning in textual form?  Isn't
this a sympotom that shows the code is turning the information into
textual form way too early, which makes the code very unpleasant to
read and hard to maintain and modify.