v4, 10 partsUntracked cache improvements

Here is a new version of a patch series to improve the untracked cache feature.

This v4 implements core.untrackedCache as a tristate config variable. When it's `true`, Git commands, especially `git status`, should always add the untracked cache and use it, and when `false`, Git commands should remove it. The default though is now `keep` in which case the untracked cache is neither removed nor added, and used if it is there.

Patch 1/10 is a small bugfix that has not changed since v3.

Patch 2/10 to 4/10 add some small features that are missing. The only chqnge there since v3 is that we are now using `report()` to display verbose information, thanks to Duy.

Patchs 5/10 to 7/10 are some refactoring to prepare for the following patchs. Among them 6/10 is the result of merging two patchs from v3, thanks to Eric.

Patch 8/10 deals with the "ident" field in "struct untracked_cache" and is mostly the same as in v3. The difference is just a small bug fix to prevent a crash.

Patch 9/10 adds core.untrackedCache. It has been changed compared to
v3 in the following ways:
  - the config variable is now a tristate, thanks to Junio,
  - we use `switch` to deal with different values, thanks to Torsten,
  - documentation for --test-untracked-cache is improved, thanks to
    David.

Patch 10/10, which contains tests, has been changed to reflect changes in 9/10 and to add a few tests.

So the changes compared to v3 are mostly small updates, and patchs 6/10, 9/10 and 10/10.

The patch series is also available there:

https://github.com/chriscool/git/tree/uc-notifs40

Thanks to the reviewers and helpers.

Christian Couder (10):
  dir: free untracked cache when removing it
  update-index: use enum for untracked cache options
  update-index: add --test-untracked-cache
  update-index: add untracked cache notifications
  update-index: move 'uc' var declaration
  dir: add {new,add}_untracked_cache()
  dir: add remove_untracked_cache()
  dir: simplify untracked cache "ident" field
  config: add core.untrackedCache
  t7063: add tests for core.untrackedCache

 Documentation/config.txt               |  7 +++
 Documentation/git-update-index.txt     | 65 +++++++++++++++++++++++-----
 builtin/update-index.c                 | 62 ++++++++++++++++----------
 cache.h                                |  1 +
 config.c                               | 11 +++++
 contrib/completion/git-completion.bash |  1 +
 dir.c                                  | 70 ++++++++++++++++++++++++------
 dir.h                                  |  2 +
 environment.c                          |  1 +
 t/t7063-status-untracked-cache.sh      | 79 +++++++++++++++++++++++++++++++---
 wt-status.c                            | 13 ++++++
 11 files changed, 260 insertions(+), 52 deletions(-)

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 1 +
 1 file changed, 1 insertion(+)

Christian Couder <christian.couder@gmail.com> writes:

Looks good.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 18 +++++++++++++-----
 1 file changed, 13 insertions(+), 5 deletions(-)

It is nice to just be able to test if untracked cache is supported without enabling it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 Documentation/git-update-index.txt | 12 +++++++++++-
 builtin/update-index.c             |  5 +++++
 2 files changed, 16 insertions(+), 1 deletion(-)

Christian Couder <christian.couder@gmail.com> writes:

Looks good.

Attempting to flip the untracked-cache feature on for a random index file with

    cd /random/unrelated/place
    git --git-dir=/somewhere/else/.git update-index --untracked-cache

would not work as you might expect. Because flipping the feature on in the index also records the location of the corresponding working tree (/random/unrelated/place in the above example), when the index is subsequently used to keep track of files in the working tree in /somewhere/else, the feature is disabled.

With this patch "git update-index --[test-]untracked-cache" tells the user in which directory tests are performed. This makes it easy to spot any problem.

Also in verbose mode, let's tell the user when the cache is enabled or disabled.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 14 +++++++++-----
 1 file changed, 9 insertions(+), 5 deletions(-)

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 4 +---
 1 file changed, 1 insertion(+), 3 deletions(-)

Factor out code into new_untracked_cache() and add_untracked_cache(), which will be used in later commits.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 11 +----------
 dir.c                  | 18 ++++++++++++++++++
 dir.h                  |  1 +
 3 files changed, 20 insertions(+), 10 deletions(-)

Factor out code into remove_untracked_cache(), which will be used in a later commit.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 builtin/update-index.c | 6 +-----
 dir.c                  | 9 +++++++++
 dir.h                  | 1 +
 3 files changed, 11 insertions(+), 5 deletions(-)

It is not a good idea to compare kernel versions and disable the untracked cache if it changes as people may upgrade and still want the untracked cache to work. So let's just compare work tree locations to decide if we should disable it.

Also it's not useful to store many locations in the ident field and compare to any of them. It can even be dangerous if GIT_WORK_TREE is used with different values. So let's just store one location, the location of the current work tree.

If this location changed and we still want an untracked cache, let's delete the cache and recreate it.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 dir.c | 49 ++++++++++++++++++++++++++++++++++---------------
 1 file changed, 34 insertions(+), 15 deletions(-)

Christian Couder <christian.couder@gmail.com> writes:

The ", system " part looks funny. I think I know what you are trying to do, though.

If the path to the working tree has ", system " as a substring, I wonder if funny things may happen with this starts_with()?

On Tue, Dec 29, 2015 at 11:32 PM, Junio C Hamano <gitster@pobox.com> wrote:

Yeah, I think in the next iteration I will do what Torsten suggests. It looks like the only sane solution, other than just not using the "ident" field anymore and relying only on config variables like the example I give in my reply to Torsten.

On 2015-12-29 08.09, Christian Couder wrote:

OK with that.

I don't think I fully agree in all details. The list is there to distinguish between different clients when sharing a Git workspace over a network to different clients:

A file system is exported from Linux to Linux via NFS, including a Git workspace) The same Workspace is exported via SAMBA to Windows and, in an extreme case, AFS to Mac OS.

Other combinations could be Linux - SAMBA - Linux Linux - AFP - Linux

Mac OS - NFS - Linux Mac OS - AFP - Mac OS Mac OS - SMB - Linux, Mac OS, Windows The list is incomplete (BSD and other Unix is missing), there are other combinations of network protocols, there are NAS boxes which mostly Linux under the hood, but may have vendor specific tweaks.

Now we want to know, which of the combinations work. The different clients need to test separately. E.g. for a given server:

NFS - Linux SMB - Linux SMB Windows.

At this point I would agree that the old code from dir.c:

static const char *get_ident_string(void)
{
	static struct strbuf sb = STRBUF_INIT;
	struct utsname uts;

	if (sb.len)
		return sb.buf;
	if (uname(&uts) < 0)
		die_errno(_("failed to get kernel name and information"));
	strbuf_addf(&sb, "Location %s, system %s %s %s", get_git_work_tree(),
		    uts.sysname, uts.release, uts.version);
	return sb.buf;
}
------------------
is unneccessary picky, and the sysname should be enough:

strbuf_addf(&sb, "Location %s, system %s", get_git_work_tree(), uts.sysname);

The old code did not find out, which network protocol that we used, (you need to call "mount", and grep for the directory, and try to get the FS information, which may be ext4, btrfs, smbfs, nfs....) This is unnecessary complicated, so we endet up in using the path.

If I was a system administrator, (I sometimes am), I would set up a bunch of Linux boxes in a similar way, so that the repo is under /nfs/server1/projects/myproject (and the same path is used for all Linux boxes)

The Windows machines may use something like N:/projects/myproject or //server1/projects/myproject

and Mac OS may use /Volumes/projects/myproject (If mounted from the finder) or /nfs/projects/myproject (When autofs is used)

I may have missed the discussion somewhat, could you explain why having different uname/location combinations are not usable at your site ?

How much burden is actually put on your system, and is there a way to keep a list of different clients ?

What I understand is that a kernel update of a Linux machine shouldn't matter, but if a machine runs Windows or Linux should matter.

On Wed, Dec 30, 2015 at 12:47 PM, Torsten Bögershausen <tboegi@web.de> wrote:

Suppose I use "git update-index --untracked-cache" first with GIT_WORK_TREE=/foo and then with GIT_WORK_TREE=/bar. It will store both /foo and /bar as valid locations for the worktree in the "ident" field.

Then I work a bit with GIT_WORK_TREE=/foo and then a bit with GIT_WORK_TREE=/bar. Now does the untracked cache info in the index reflect the state of /foo or the state of /bar?

With a config variable, the problem is worse, because, as we automatically add the untracked cache when git status is used, we are even less likely to pay attention to GIT_WORK_TREE when the cache is added.

In my opinion at this point it is a bit insane to want untracked cache to work when it is accessed by different clients over the network. It is much safer to just disable it (with a config option) in this case. But I am ok to try to improve things for your use case to work if it doesn't complicate things too much.

I am ok with the change to compare only the location and the system name. I wonder a bit though if this change is completely backward compatible.

I tried to explain above why using a config option to set the untracked cache implies that you have to be careful at what happens when people use GIT_WORK_TREE.

But it should be usable if indeed we put only the system name as you suggest. So I think I will do that.

About a list of different client, instead of just one, I don't think it is sane given the GIT_WORK_TREE problem.

If you really have many clients that should use the untracked cache with different paths, then what you might want is a set of config options like this:

[untrackedCache "windows7"]
        enable = true
        system = Windows
        systemVersion = 7
        path = C:/my/work/tree
[untrackedCache "windows8"]
        enable = true
        system = Windows
        systemVersion = 8
        path = C:/my/windows8/work/tree
[untrackedCache "linux"]
        enable = true
        system = Linux
        path = ~/work/tree

This is much more flexible and visible than a custom string list inside the untracked cache. But it is overkill for Booking.com's use case (where the machines use Linux and behave well regarding mtime). So if you are willing to implement that, then that would be great!

Everything could matter. Some people might want to enable it or disable it depending on the value of an environment variable or something.

Instead of trying to think about all that could matter, we should have, and still should, think about generic and flexible config based solutions.

Thanks, Christian.

When we know that mtime is fully supported by the environment, we might want the untracked cache to be always used by default without any mtime test or kernel version check being performed.

Also when we know that mtime is not supported by the environment, for example because the repo is shared over a network file system, then we might want 'git update-index --untracked-cache' to fail immediately instead of performing tests (because it might work on some systems using the repo over the network file system but not others).

The normal way to achieve the above in Git is to use a config variable. That's why this patch introduces "core.untrackedCache".

This variable is a tristate, `keep`, `false` and `true`, which default to `keep`.

When "git status" is run, it now adds or removes the untracked cache in the index to respect the value of this variable. So it does nothing if the value is `keep` or if the variable is unset; it adds the untracked cache if the value is `true`; and it removes the cache if the value is `false`.

`git update-index --[no-|force-]untracked-cache` still adds or removes the untracked cache from the index, but this now fails if it goes against the value of core.untrackedCache.

Also `--untracked-cache` used to check that the underlying operating system and file system change `st_mtime` field of a directory if files are added or deleted in that directory. But those tests take a long time and there is now `--test-untracked-cache` to perform them.

That's why, to be more consistent with other git commands, this patch prevents `--untracked-cache` to perform tests, so that after this patch there is no difference any more between `--untracked-cache` and `--force-untracked-cache`.

This last change is backward incompatible and should be mentioned in the release notes.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
---
 Documentation/config.txt               |  7 ++++
 Documentation/git-update-index.txt     | 65 ++++++++++++++++++++++++++--------
 builtin/update-index.c                 | 35 ++++++++++++------
 cache.h                                |  1 +
 config.c                               | 11 ++++++
 contrib/completion/git-completion.bash |  1 +
 environment.c                          |  1 +
 t/t7063-status-untracked-cache.sh      |  4 +--
 wt-status.c                            | 13 +++++++
 9 files changed, 109 insertions(+), 29 deletions(-)

Christian Couder <christian.couder@gmail.com> writes:

Before "Before setting it to `true`", the reader needs to be told what would happen when this is set to each of these three values (as well as what happens when this is not set at all).

"extension" is a term that is fairly close to the machinery. In other parts of the documentation (like the added text below in this patch), it is called "untracked cache FEATURE", which probably is a better word to use here as well.

While the above is not wrong per-se, from the point of those who looked for these options (that is, those who wanted to do a one-shot enabling or disabling of the feature, perhaps to try it out to see how well it helps on their system), I think the explanation of the interaction between the option and the config is backwards. For their purpose, setting it to `true` or `false` will be hinderance, because the options are made no-op by such a setting. IOW, the advertisement "once you decided that you want to use the feature, configuration is a better place to set it" does not belong here.

If I were writing this documentation, I'd probably rephrase the second paragraph like so:

	These options take effect only when the
	`core.untrackedCache` configuration variable (see
	linkgit:git-config[1]) is set to `keep` (or it is left
	unset).  When the configuration variable is set to `true`,
	the untracked cache feature is always enabled (and when it
	is set to `false`, it is always disabled), making these
	options no-op.

perhaps.

s/persistant/persistent/; but more importantly, I do not think persistence has much to do with the choice between the option and configuration. Once it is set with `--untracked-cache`, it should persist until you explicitly use `--no-untracked-cache` to disable it, no? Otherwise you have a bug that needs to be fixed.

The reason you'd want to use a configuration is because the effect of using the `--untracked-cache` option is per repository (rather, per index-file).

    If you want to enable (or disable) this feature, it is easier to
    use the `core.untrackedCache` configuration variable than using
    the `--untracked-cache` option to `git update-index` in each
    repository, especially if you want to do so across all
    repositories you use, because you can set the configuration
    variable to `true` (or `false`) in your `$HOME/.gitconfig` just
    once and have it affect all repositories you touch.

or something, perhaps.

Is it only "git status", or anything that writes/updates the index file? The above makes it sound as if you are saying "If you change the variable, you must run `git status` for the change to take effect", and if that is indeed the case, perhaps you should say so more explicitly. My cursory reading of the code suggests that the user must run `git status` in a mode that shows untracked files (i.e. "git status -uno" does not fix)?

I somehow thought, per the previous discussion with Duy, that the check and addition of an empty one (if missing in the index and configuration is set to `true`) or removal of an existing one (if configuration is set to `false`) were to be done when the index is read by read_index_from(), though. If you have to say "After setting the configuration, you must run this command", that does not sound like a huge improvement over "If you want to enable this feature, you must run this command".

I do buy the decision to make these no-op when the configuration says `true` or `false`, but I am not sure if these deserve die().

Exiting with 0 (= success) after issuing a warning() might be more appropriate. I'd especially anticipate that some newbies will complain that they got "fatal:" when they used the "--force-" variant, saying "I know what I am doing, that is why I said 'force', why does stupid Git refuse?".

On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:

Ok, then I think I will write something like:

core.untrackedCache::
     Determines what to do about the untracked cache feature of the
index. It will
     be kept, if this variable is unset or set to `keep`. It will
automatically be added
     if set to `true`. And it will automatically be removed, if set to
`false`. Before
     setting it to `true`, you should check that mtime is working
properly on your
     system.
     See linkgit:git-update-index[1]. `keep` by default.

Ok, I will use "feature".

Yeah, but "no-op" is not technically true, as if you just set the config variable to true for example and then use "--untracked-cache" then the cache will be added immediately. Also it does not explain that for example "git update-index --untracked-cache" will die() if the config variable is set to false.

Yeah, it should persist except if you clone and copy the config file. I agree that it is not clear what "persistent" means, so maybe I can just remove "as it is more persistant and visible with a configuration variable".

Ok, I will use something like that.

Yeah, that's what the code does.

The untracked-cache feature, as I tried to explain in an email in the previous discussion, has an effect only on git status when it has to show untracked files. Otherwise the untracked-cache is simply not used. It might be a goal to use it more often, but it's not my patch series' goal.

So why should we have a check to see if maybe the cache should be added or removed in all the other cases when the cache is not used?

Scripts users might just not look at the warnings and expect things to work if the exit code is 0.

I think this should be taken care of in the "--force-" documentation. Maybe we should say that it is deprecated and make it clear, if we don't already, that it does nothing more than without "force-".

Christian Couder <christian.couder@gmail.com> writes:

The "update-index --[no-]untracked-cache" command is made to no longer follow the user's immediate desire (i.e. enable or disable) expressed by the invocation of the command. That is what I meant by 'no-op'.

As I think it is a BUG to make these die(), it is an irrelevant objection ;-).

In the future we may extend the system to allow "git add somedir/" to take advantage of the untracked cache (i.e. we may already know what untracked paths there are without letting readdir(3) to scan it in somedir/), for example. Is it reasonable to force users to say "git status", in such a future? And more importantly, when that future comes, is it reasonable to force users to re-learn Git to do something else to do things differently at that point?

Itt sounds like somewhat a short-sighted mindset to design the system, and I was hoping that by now you would have become better than that.

The real question is what are the problems in implementing this in the way Duy suggested in the previous discussion. The answer may fall into somewhere between "that approach does not work in such and such cases, so this is the best I could come up with" and "I know that approach is far superiour, but I have invested too much in this inferiour approach and refuse to rework it further to make it better."

I unfortunately am not getting the sense that I know where your answer would fall into that spectrum from your response.

That is exactly why I said it is inappropriate to error out.

Using of not using untracked-cache is (supposed to be) purely performance and not correctness thing, and I do not think the users and the scripts do not deserve to see a failure from "update-index --untracked-cache" only because there is a stray core.untrackedCache set to 'false' somewhere.

On Thu, Dec 31, 2015 at 12:23 AM, Junio C Hamano <gitster@pobox.com> wrote:

Ok I think I will just use what you suggest except the "no-op" part:

      These options take effect only when the
      `core.untrackedCache` configuration variable (see
      linkgit:git-config[1]) is set to `keep` (or it is left
      unset).  When the configuration variable is set to `true`,
      the untracked cache feature is always enabled (and when it
      is set to `false`, it is always disabled).

If a new feature takes advantage of the untracked cache, it is reasonable that this feature enables or disables it if should be enabled or disabled. Maybe we can just say in the doc something like:

    When the `core.untrackedCache` configuration variable is changed, the
    untracked cache is added to or removed from the index the next time
    a command that can use the untracked cache is run; while when
    `--[no-|force-]untracked-cache` are used, the untracked cache is
    immediately added to or removed from the index. Currently only
    "git status" can use the untracked cache.

I don't think people will have to relearn anything or do things differently especially if things are explained like above.

If they want to use the untracked cache by setting a config option, and then want to enable it right away, they can still use git status for that, so things will continue to work.

What scenario do you have in mind where people would have to do things differently?

My question is why should I invest time thinking about and testing another approach when the current approach seems simpler, less bug prone, faster and without any downside?

Suppose that we do as you suggest. This means that when "git update-index --untracked-cache" is called while the config variable is false, we issue a warning, add the UC to the index and exit 0.

So a legacy script calling "git update-index --untracked-cache" and getting a 0 exit code from it will assume that every time "git status" is called the UC will be used because it has been enabled in the index. But that would be wrong if the config variable is false, because in this case when git status is called, it will remove the UC.

This "stray core.untrackedCache" could not have been put there by users of previous git versions because it has no meaning before this patch series. So I don't understand why you call it "a stray core.untrackedCache".

It is no more "stray" to me than the call to "update-index --untracked-cache".

If it has been put in /etc/git.config by an admin and if the user thinks he knows better, the user can still change the config locally to override the system one.

Christian Couder <christian.couder@gmail.com> writes:

They eventually will see a system in which that they do not have do anything after flipping the configuration, yet will still see stale "you must run 'git status'" on their websearches and SO questions, which would be a cost for them to remember that they no longer have to do the extra 'git status'.

As to the downside, I think Duy's "at the time we read the index" is the least problematic route from the maintainability point of view. What are you doing to help people who make changes in the future to the system to make "git add $dir" or "git clean $dir" aware of the untracked cache not to forget doing the "oh, the config says we want to add the untracked cache if missing, so we do it here" in their new codepath? Whey you say "This is only about status", you are essentially saying "It's outside the scope of my job, I was hired to improve the usability of 'git status' with untracked cache and I do not care about the longer term overall health of the system".

Now, you said something about "simpler", "less bug prone" and "faster" (I doubt you can make such statements that involve comparison without investing time thinking about other approaches, though, but that is a separate topic). That would mean that you see "complexity", "error prone-ness", and "slowness" in the way Duy suggested---that is exactly the question I asked you in the message you are responding to. What are the problems in implementing this in the way Duy suggested? What kind of "complexity" do you see? Which part is more "error prone"? Why does it have to be "slower"?

You are assuming that everybody constantly looks at /etc/git.config to make sure evil admins won't do things that affect their repositories and use of Git in potentially negative way. I doubt anybody does.

By the way, I understand that this "stray one affects without user being aware of it" is the primary the reason why Ævar wants this 'configuration automatically adds untracked cache even to the existing index' feature---everybody will get it without even be aware of the change their admins make. Which may be a good thing for those who use the configuration variable.

On Mon, Jan 4, 2016 at 7:09 PM, Junio C Hamano <gitster@pobox.com> wrote:

I just tried Duy's approach by moving into read_index_from() the code I had put in wt_status_collect_untracked() and it doesn't work because "git status" calls read_index_from() before calling git_default_core_config().

The interesting parts of the backtraces for both calls are the following:

#0 read_index_from (istate=0x8660a0 <the_index>, path=0x868b90 ".git/index") at read-cache.c:1626 #1 0x0000000000530ba3 in read_index (istate=0x8660a0 <the_index>) at read-cache.c:1404 #2 0x00000000005743e1 in gitmodules_config () at submodule.c:188 #3 0x0000000000431ed0 in status_init_config (s=0x83aa20 <s>, fn=0x434edd <git_status_config>) at builtin/commit.c:186

#0 git_default_core_config (var=0x86ac00 "core.untrackedcache", value=0x86ac60 "true") at config.c:695 #1 0x00000000004be761 in git_default_config (var=0x86ac00 "core.untrackedcache", value=0x86ac60 "true", dummy=0x0) at config.c:1016 #2 0x00000000004cfeba in git_diff_basic_config (var=0x86ac00 "core.untrackedcache", value=0x86ac60 "true", cb=0x0) at diff.c:277 #3 0x00000000004cfc9b in git_diff_ui_config (var=0x86ac00 "core.untrackedcache", value=0x86ac60 "true", cb=0x0) at diff.c:230 #4 0x000000000043524a in git_status_config (k=0x86ac00 "core.untrackedcache", v=0x86ac60 "true", cb=0x83aa20 <s>) at builtin/commit.c:1313 #5 0x00000000004bf183 in configset_iter (cs=0x8495e0 <the_config_set>, fn=0x434edd <git_status_config>, data=0x83aa20 <s>) at config.c:1305 #6 0x00000000004bf206 in git_config (fn=0x434edd <git_status_config>, data=0x83aa20 <s>) at config.c:1317 #7 0x0000000000431ee3 in status_init_config (s=0x83aa20 <s>, fn=0x434edd <git_status_config>) at builtin/commit.c:187

You can see that in status_init_config(), gitmodules_config() is called just before git_config(). And unfortunately gitmodules_config() indirectly calls read_index_from().

So indeed it seems very bug prone to me to rely on read_index_from() being called before git_default_core_config().

Christian Couder <christian.couder@gmail.com> writes:

I didn't expect to (and didn't want to) see the configuration be dead by git_default_core_config() in the first place, though. I instead was expecting that it would be done through the caching layer, e.g. by calling git_config_get_maybe_bool() interface when read_index_from() wants to see if it needs to muck with the existing (or missing) extension.

Signed-off-by: Christian Couder <chriscool@tuxfamily.org>
---
 t/t7063-status-untracked-cache.sh | 75 ++++++++++++++++++++++++++++++++++++---
 1 file changed, 71 insertions(+), 4 deletions(-)