Re: [PATCH v5 09/11] config: add core.untrackedCache

From: Junio C Hamano <gitster@pobox.com>
Date: Jan 15, 2016, 20:15 UTC
Message-ID: <xmqqlh7qha5k.fsf@gitster.mtv.corp.google.com>
In-Reply-To: <1452841192-620-10-git-send-email-chriscool@tuxfamily.org>

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

Show 8 quoted lines

> 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,
> we might want the untracked-cache to be automatically remove from
> the index.

I'd say "fully supported" and "not supported" are bad phrasing. Network file system may give you mtime, but for the purpose of uc, it may not be usable, and that is when you do not want to rely on it. So perhaps "When we know that mtime on directory the environment gives us is usable for the purpose of untracked cache", and "Also when we know that mtime is not usable" or something like that?

I think "we might want" can become a stronger "we may want" in both paragraphs. The second paragraph needs s/remove/removed/.

Show 5 quoted lines

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

The above makes a reader wonder what are abnormal ways ;-).

    Allow the user to express such preference by setting the
    'core.untrackedCache' configuration variable, which can take
    'keep', 'false', or 'true'.

would be clearer without being muddied by an unnecessary value judgement.

Show 5 quoted lines

> When read_index_from() is called, 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`.

OK.

> `git update-index --[no-|force-]untracked-cache` still adds or
> removes the untracked cache from the index,

The above is meant to be a contraction of "... still adds UC to the index or removes UC from the index", but reads as if the original were "... still adds UC from the index or removes UC from the index".

"... still adds UC to or removes from the index" perhaps?

> but this shows a
> warning if it goes against the value of core.untrackedCache,
> as the untracked cache will go back to its previous state the
> next time read_index_from() is called.

Not incorrect per-se, but

    ... shows a warning ... because the next time the index is read
    UC will be added or removed if the configuration is set to do so.

may make it more clear that `keep` in core.untrackedCache does not have to issue a warning (I saw your code correctly handles the warning in this patch).

Show 9 quoted lines

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

    But because those tests take a long time, `--untracked-cache` no
    longer performs them.  Instead, there is now `--test-untracked-cache`
    to perform the tests.  This change makes `--untracked-cache` the
    same as `--force-untracked-cache`.

Show 5 quoted lines

> +Untracked cache
> +---------------
> +
> +This cache could speed up commands that involve determining untracked
> +files such as `git status`.

You are being a bit too modest by saying "could" here, I think. If you do not want to be overly assertive, perhaps say "is meant to"?

Show 6 quoted lines

> +This feature works by recording the mtime of the working tree
> +directories and then omitting reading directories and stat calls
> +against files in those directories whose mtime hasn't changed. For
> +this to work the underlying operating system and file system must
> +change the `st_mtime` field of directories if files in the directory
> +are added, modified or deleted.

A reader who read only this without reading the code would wonder if the code works correctly after a untracked path is added to a diretory A/B (the timestamp of B would be updated, while A would stay the same) and we ask what are the untracked things inside the whole tree. Of course we shouldn't "omit" scanning A down to B, but it is a bit unclear that we didn't have such a design bug from this description.

Unfortunately I don't have a good rephrasing suggestion.

Thanks.