Re: [PATCH 7/8] config: add core.untrackedCache

From: Christian Couder <christian.couder@gmail.com>
Date: Dec 15, 2015, 09:34 UTC
Message-ID: <CAP8UFD0ex4JuU=FMDwnSnf9io16=D2_m3x1ajXtCu25OU_DLAw@mail.gmail.com>
In-Reply-To: <xmqqa8pciuqq.fsf@gitster.mtv.corp.google.com>

On Mon, Dec 14, 2015 at 10:30 PM, Junio C Hamano <gitster@pobox.com> wrote:

Show 15 quoted lines

> Junio C Hamano <gitster@pobox.com> writes:
>
> The primary reason why I do not like your "configuration decides" is
> it will be a huge source of confusions and bugs.  Imagine what
> should happen in this sequence, and when should a stale cached
> information be discarded?
>
>  - the configuration is set to 'yes'.
>  - the index is updated and written by various commands.
>  - more work is done in the working tree without updating the index.
>  - the configuration is set to 'no'.
>  - more work is done in the working tree without updating the index.
>  - the configuration is set to 'yes'.
>  - more work is done in the working tree without updating the index.
>  - somebody asks "what untracked paths are there?"

As far as I understand the UC just stores the mtime of the directories in the working tree to avoid the need of lstat'ing all the files in the directories.

When somebody asks "what untracked paths are there", if the UC has not been discarded when the configuration was set to no, then git will just ask for the mtimes of the directories in the working tree and compare them with what is in the UC.

I don't see how it can create confusion and bugs, as the work done in the working tree should anyway have changed the mtime of the directories where work has been done.

Maybe as the UC has not been updated for a long time, there will be a lot of mtimes that are different, so there will not be a big speed up or it could be even slower than if the UC was not used, but that's all.

Show 8 quoted lines

> In the "configuration decides" world, I am not sure how a sane
> implementation efficiently invalidates the cache as needed, without
> the config subsystem having intimate knowledge with the untracked
> cache (so far, the config subsystem is merely a key-value store and
> does not care _what_ it stores; you would want to invalidate the
> cache in the index when somebody sets the variable to 'no', which
> means the config subsystem needs to know that this variable is
> special).

In the current patch series and in the one I am preparing and will probably send soon, this hunk takes care of removing or addind the UC if needed:

diff --git a/wt-status.c b/wt-status.c
index 435fc28..3e0fe02 100644
--- a/wt-status.c
+++ b/wt-status.c
@@ -586,6 +586,15 @@ static void wt_status_collect_untracked(struct
wt_status *s)
                dir.flags |= DIR_SHOW_IGNORED_TOO;
        else
                dir.untracked = the_index.untracked;
+
+       if (!dir.untracked && use_untracked_cache == 1) {
+               add_untracked_cache();
+               dir.untracked = the_index.untracked;
+       } else if (dir.untracked && use_untracked_cache == 0) {
+               remove_untracked_cache();
+               dir.untracked = NULL;
+       }
+
        setup_standard_excludes(&dir);

        fill_directory(&dir, &s->pathspec);

So when the config option is changed, the UC is removed or recreated
only the next time "git status" (and maybe also "git commit" and a few
other commands) is called.

And anyway I don't think people will change the UC config very often.
Maybe they will play with it a bit when they discover it, but
afterwards they will just set it or not and be done with it. So I
think it is not worth trying to optimize what happens when the config
is set or unset. We should just make sure that it works and it is well
documented.