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

From: Junio C Hamano <gitster@pobox.com>
Date: Dec 30, 2015, 23:23 UTC
Message-ID: <xmqqbn97cyh3.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <CAP8UFD0pOiqa5ZxwM0Vfzr_wMJ+HDrXyhzJ+TmRDED5GH+koMw@mail.gmail.com>

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

Show 28 quoted lines

> On Tue, Dec 29, 2015 at 11:35 PM, Junio C Hamano <gitster@pobox.com> wrote:
> ...
>> 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.

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'.

> ... for example "git update-index --untracked-cache" will die() if
> the config variable is set to false.

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

Show 14 quoted lines

>> 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.

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.

Show 5 quoted lines

>> 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.

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.