On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@xxxxxxxxx> wrote:
> Christian Couder <christian.couder@xxxxxxxxx> 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).
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.
>> 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.
Ok, I will use "feature".
>> +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.
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.
>> @@ -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.
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".
> 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.
Ok, I will use something like that.
>> +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)?
Yeah, that's what the code does.
> 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".
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?
>> + 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.
Scripts users might just not look at the warnings and expect things to
work if the exit code is 0.
> 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?".
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-".
--
To unsubscribe from this list: send the line "unsubscribe git" in
the body of a message to majordomo@xxxxxxxxxxxxxxx
More majordomo info at http://vger.kernel.org/majordomo-info.html