From: Junio C Hamano <gitster@pobox•com>
To: Eric Sunshine <sunshine@sunshineco•com>
Cc: "Nguyễn Thái Ngọc Duy" <pclouds@gmail•com>,
"Git List" <git@vger•kernel.org>
Subject: Re: [PATCH 18/25] list-files: delete redundant cached entries
Date: Tue, 07 Apr 2015 19:39:01 -0700 [thread overview]
Message-ID: <xmqqzj6jbayi.fsf@gitster.dls.corp.google.com> (raw)
In-Reply-To: <CAPig+cTDHf5Wh92SLHfQ0_GRY0cbpz+ALfiukJJHed6d=QmLdg@mail.gmail.com> (Eric Sunshine's message of "Mon, 6 Apr 2015 17:35:24 -0400")
Eric Sunshine <sunshine@sunshineco•com> writes:
> On Mon, Apr 6, 2015 at 9:52 AM, Nguyễn Thái Ngọc Duy <pclouds@gmail•com> wrote:
>> 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 <sunshine@sunshineco•com>
>> Signed-off-by: Nguyễn Thái Ngọc Duy <pclouds@gmail•com>
>> ---
>> 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.
next prev parent reply other threads:[~2015-04-08 2:39 UTC|newest]
Thread overview: 36+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-04-06 13:52 [PATCH v2 00/25] list-files redesign Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 01/25] ls_colors.c: add $LS_COLORS parsing code Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 02/25] ls_colors.c: parse color.ls.* from config file Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 03/25] ls_colors.c: add a function to color a file name Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 04/25] ls_colors.c: highlight submodules like directories Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 05/25] list-files: command skeleton Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 06/25] list-files: show paths relative to cwd Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 07/25] list-files: add tag to each entry, filter duplicate tags Nguyễn Thái Ngọc Duy
2015-04-06 21:32 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 08/25] list-files: add --[no-]column, -C and -1 Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 09/25] list-files: add --max-depth, -R and default to --max-depth=0 Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 10/25] list-files: show directories as well as files Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 11/25] list-files: add --color Nguyễn Thái Ngọc Duy
2015-04-06 21:33 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 12/25] list-files: add -F/--classify Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 13/25] list-files: new indicator '&' for submodules when -F is used Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 14/25] list-files: add --cached and --others Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 15/25] list-files: add --ignored Nguyễn Thái Ngọc Duy
2015-04-06 21:34 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 16/25] list-files: add --unmerged Nguyễn Thái Ngọc Duy
2015-04-06 21:34 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 17/25] list-files: add file modification options -[admADM] Nguyễn Thái Ngọc Duy
2015-04-06 21:34 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 18/25] list-files: delete redundant cached entries Nguyễn Thái Ngọc Duy
2015-04-06 21:35 ` Eric Sunshine
2015-04-08 2:39 ` Junio C Hamano [this message]
2015-04-06 13:52 ` [PATCH 19/25] list-files: make alias 'ls' default to 'list-files' Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 20/25] list-files: preload index Nguyễn Thái Ngọc Duy
2015-04-06 21:35 ` Eric Sunshine
2015-04-06 13:52 ` [PATCH 21/25] list-files: reduce match_pathspec calls in matched() Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 22/25] list-files: only do diff that is actually useful Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 23/25] pathspec: move getenv() code out of prefix_pathspec() Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 24/25] list-files: make :(glob) pathspec default Nguyễn Thái Ngọc Duy
2015-04-06 13:52 ` [PATCH 25/25] list-files: documentation Nguyễn Thái Ngọc Duy
2015-04-06 21:37 ` Eric Sunshine
2015-04-06 13:58 ` [PATCH v2 00/25] list-files redesign Duy Nguyen
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqzj6jbayi.fsf@gitster.dls.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
--cc=sunshine@sunshineco$(echo .)com \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox