From: Junio C Hamano <gitster@pobox•com>
To: Christian Couder <christian.couder@gmail•com>
Cc: git <git@vger•kernel.org>, "Jeff King" <peff@peff•net>,
"Ævar Arnfjörð" <avarab@gmail•com>,
"Nguyen Thai Ngoc Duy" <pclouds@gmail•com>,
"David Turner" <dturner@twopensource•com>,
"Eric Sunshine" <sunshine@sunshineco•com>,
"Torsten Bögershausen" <tboegi@web•de>,
"Christian Couder" <chriscool@tuxfamily•org>
Subject: Re: [PATCH v4 09/10] config: add core.untrackedCache
Date: Wed, 30 Dec 2015 15:23:36 -0800 [thread overview]
Message-ID: <xmqqbn97cyh3.fsf@gitster.mtv.corp.google.com> (raw)
In-Reply-To: <CAP8UFD0pOiqa5ZxwM0Vfzr_wMJ+HDrXyhzJ+TmRDED5GH+koMw@mail.gmail.com> (Christian Couder's message of "Wed, 30 Dec 2015 23:47:44 +0100")
Christian Couder <christian.couder@gmail•com> writes:
> 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 ;-).
>> 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.
>> 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.
next prev parent reply other threads:[~2015-12-30 23:23 UTC|newest]
Thread overview: 24+ messages / expand[flat|nested] mbox.gz Atom feed top
2015-12-29 7:09 [PATCH v4 00/10] Untracked cache improvements Christian Couder
2015-12-29 7:09 ` [PATCH v4 01/10] dir: free untracked cache when removing it Christian Couder
2015-12-29 22:26 ` Junio C Hamano
2015-12-29 7:09 ` [PATCH v4 02/10] update-index: use enum for untracked cache options Christian Couder
2015-12-29 7:09 ` [PATCH v4 03/10] update-index: add --test-untracked-cache Christian Couder
2015-12-29 22:28 ` Junio C Hamano
2015-12-29 7:09 ` [PATCH v4 04/10] update-index: add untracked cache notifications Christian Couder
2015-12-29 7:09 ` [PATCH v4 05/10] update-index: move 'uc' var declaration Christian Couder
2015-12-29 7:09 ` [PATCH v4 06/10] dir: add {new,add}_untracked_cache() Christian Couder
2015-12-29 7:09 ` [PATCH v4 07/10] dir: add remove_untracked_cache() Christian Couder
2015-12-29 7:09 ` [PATCH v4 08/10] dir: simplify untracked cache "ident" field Christian Couder
2015-12-29 22:32 ` Junio C Hamano
2015-12-30 22:01 ` Christian Couder
2015-12-30 11:47 ` Torsten Bögershausen
2015-12-30 17:20 ` Christian Couder
2015-12-29 7:09 ` [PATCH v4 09/10] config: add core.untrackedCache Christian Couder
2015-12-29 22:35 ` Junio C Hamano
2015-12-30 22:47 ` Christian Couder
2015-12-30 23:23 ` Junio C Hamano [this message]
2015-12-31 12:33 ` Christian Couder
2016-01-04 18:09 ` Junio C Hamano
2016-01-08 17:52 ` Christian Couder
2016-01-08 19:43 ` Junio C Hamano
2015-12-29 7:09 ` [PATCH v4 10/10] t7063: add tests for core.untrackedCache Christian Couder
Reply instructions:
You may reply publicly to this message via plain-text email
using any one of the following methods:
* Save the following mbox file, import it into your mail client,
and reply-to-all from there: mbox
Avoid top-posting and favor interleaved quoting:
https://en.wikipedia.org/wiki/Posting_style#Interleaved_style
* Reply using the --to, --cc, and --in-reply-to
switches of git-send-email(1):
git send-email \
--in-reply-to=xmqqbn97cyh3.fsf@gitster.mtv.corp.google.com \
--to=gitster@pobox$(echo .)com \
--cc=avarab@gmail$(echo .)com \
--cc=chriscool@tuxfamily$(echo .)org \
--cc=christian.couder@gmail$(echo .)com \
--cc=dturner@twopensource$(echo .)com \
--cc=git@vger$(echo .)kernel.org \
--cc=pclouds@gmail$(echo .)com \
--cc=peff@peff$(echo .)net \
--cc=sunshine@sunshineco$(echo .)com \
--cc=tboegi@web$(echo .)de \
/path/to/YOUR_REPLY
https://kernel.org/pub/software/scm/git/docs/git-send-email.html
Be sure your reply has a Subject: header at the top and a blank line
before the message body.
This is a public inbox, see mirroring instructions
for how to clone and mirror all data and code used for this inbox