Re: [PATCH v4 09/10] config: add core.untrackedCache

> Christian Couder <christian.couder@gmail.com> writes:
>
>> +core.untrackedCache::
>> +     Determines if untracked cache will be automatically enabled or
>> +     disabled. It can be `keep`, `true` or `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.
>> +
>
> 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).

>> diff --git a/Documentation/git-update-index.txt b/Documentation/git-update-index.txt
>> index a0afe17..813f6cc 100644
>> --- a/Documentation/git-update-index.txt
>> +++ b/Documentation/git-update-index.txt
>> @@ -174,27 +174,31 @@ may not support it yet.
>>
>>  --untracked-cache::
>>  --no-untracked-cache::
>> -     Enable or disable untracked cache extension. This could speed
>> -     up for commands that involve determining untracked files such
>> -     as `git status`. The underlying operating system and file
>> -     system must change `st_mtime` field of a directory if files
>> -     are added or deleted in that directory.
>> +     Enable or disable untracked cache extension. Please use
>> +     `--test-untracked-cache` before enabling it.
>
> "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.

>> +These options cannot override the `core.untrackedCache` configuration
>> +variable when it is set to `true` or `false` (see
>> +linkgit:git-config[1]). They can override it only when it is unset or
>> +set to `keep`. If you want untracked cache to persist, it is better
>> +anyway to just set this configuration variable to `true` or `false`
>> +directly.
>
> 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.

>> @@ -385,6 +389,34 @@ Although this bit looks similar to assume-unchanged bit, its goal is
>>  different from assume-unchanged bit's. Skip-worktree also takes
>>  precedence over assume-unchanged bit when both are set.
>>
>> +Untracked cache
>> +---------------
>> +
>> +This cache could speed up commands that involve determining untracked
>> +...
>> +It is recommended to use the `core.untrackedCache` configuration
>> +variable (see linkgit:git-config[1]) to enable or disable this feature
>> +instead of using the `--[no-|force-]untracked-cache`, as it is more
>> +persistant and visible with a configuration variable.
>
> 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.

>> +When the `core.untrackedCache` configuration variable is changed, the
>> +untracked cache is added to or removed from the index the next time
>> +"git status" is run; while when `--[no-|force-]untracked-cache` are
>> +used, the untracked cache is immediately added to or removed from the
>> +index.
>
> 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".

>> +     switch (untracked_cache) {
>> +     case UC_UNSPECIFIED:
>> +             break;
>> +     case UC_DISABLE:
>> +             if (use_untracked_cache == 1)
>> +                     die("core.untrackedCache is set to true; "
>> +                         "remove or change it, if you really want to "
>> +                         "disable the untracked cache");
>>               remove_untracked_cache();
>>               report(_("Untracked cache disabled"));
>> +             break;
>> +     case UC_TEST:
>> +             setup_work_tree();
>> +             return !test_if_untracked_cache_is_supported();
>> +     case UC_ENABLE:
>> +     case UC_FORCE:
>> +             if (use_untracked_cache == 0)
>> +                     die("core.untrackedCache is set to false; "
>> +                         "remove or change it, if you really want to "
>> +                         "enable the untracked cache");
>> +             add_untracked_cache();
>> +             report(_("Untracked cache enabled for '%s'"), get_git_work_tree());
>> +             break;
>
> 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.