"Tao Klerks via GitGitGadget" <gitgitgadget@xxxxxxxxx> writes:
> If the previously stored flags no longer match the current
> configuration, but the currently-applicable flags do match the current
> configuration, then the previously stored untracked cache data is
> discarded.
>
> For most users there will be no change in behavior. Users who need
> '--untracked-files=all' to perform well will have the option of
> setting "status.showuntrackedfiles" to "all".
>
> Users who need '--untracked-files=all' to perform well for their
> tooling AND prefer to avoid the verbosity of "all" when running
> git status explicitly without options... are out of luck for now (no
> change).
So, in short, the root of the problem is that untracked cache can be
used to serve only one mode (between normal and all) of operation,
and the real solution to that problem must come in a different form,
i.e. allowing a single unified untracked cache to be usable in both
modes, perhaps by maintaining all the untracked ones in the cache,
but filter out upon output when the 'normal' mode is asked for?
> Users who set "status.showuntrackedfiles" to "all" and yet most
> frequently explicitly call 'git status --untracked-files=normal' (and
> use the untracked cache) are the only ones who would be disadvantaged
> by this change. It seems unlikely there are any such users.
Given how widely used we are these days, I am afraid that the days
we can safely say "users with such a strange use pattern would not
exist at all" is long gone.
> +static unsigned configured_default_dir_flags(struct index_state *istate)
> +{
> + /* This logic is coordinated with the setting of these flags in
> + * wt-status.c#wt_status_collect_untracked(), and the evaluation
> + * of the config setting in commit.c#git_status_config()
> + */
Good thing to note here.
Style.
/*
* Our multi-line comments reads more like this, with
* slash-asterisk that opens, and asterisk-slash that closes,
* sitting on their own lines.
*/
> + char *status_untracked_files_config_value;
> + int config_outcome = repo_config_get_string(istate->repo,
> + "status.showuntrackedfiles",
The indentation looks a bit off.
In this project, tab width is 8. The beginning of the second
parameter to the function call on the second line should align with
the beginning of the first parameter that immediately follows the
open parenthesis '('.
> + &status_untracked_files_config_value);
> + if (!config_outcome && !strcmp(status_untracked_files_config_value, "all")) {
> + return 0;
> + } else {
> + /*
> + * The default, if "all" is not set, is "normal" - leading us here.
> + * If the value is "none" then it really doesn't matter.
> + */
> + return DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
> + }
> +}
I didn't see the need to pass istate to this function, though.
Shouldn't it take a repository instead?
> -static void new_untracked_cache(struct index_state *istate)
> +static void new_untracked_cache(struct index_state *istate, unsigned flags)
> {
> struct untracked_cache *uc = xcalloc(1, sizeof(*uc));
> strbuf_init(&uc->ident, 100);
> uc->exclude_per_dir = ".gitignore";
> - /* should be the same flags used by git-status */
> - uc->dir_flags = DIR_SHOW_OTHER_DIRECTORIES | DIR_HIDE_EMPTY_DIRECTORIES;
> + uc->dir_flags = flags;
So we used to hardcode these two flags to match what is done in
wt-status.c when show_untracked_files != SHOW_ALL_UNTRACKEDFILES;
we allow a different set of flags to be given by the caller.
> @@ -2761,11 +2781,13 @@ static void new_untracked_cache(struct index_state *istate)
> void add_untracked_cache(struct index_state *istate)
> {
> if (!istate->untracked) {
> - new_untracked_cache(istate);
> + new_untracked_cache(istate,
> + configured_default_dir_flags(istate));
> } else {
> if (!ident_in_untracked(istate->untracked)) {
> free_untracked_cache(istate->untracked);
> - new_untracked_cache(istate);
> + new_untracked_cache(istate,
> + configured_default_dir_flags(istate));
> }
> }
> }
OK. That's quite straight-forward to see how the tweak is made.
> @@ -2781,10 +2803,12 @@ void remove_untracked_cache(struct index_state *istate)
>
> static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *dir,
> int base_len,
> - const struct pathspec *pathspec)
> + const struct pathspec *pathspec,
> + struct index_state *istate)
> {
> struct untracked_cache_dir *root;
> static int untracked_cache_disabled = -1;
> + unsigned configured_dir_flags;
>
> if (!dir->untracked)
> return NULL;
> @@ -2812,17 +2836,9 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
> if (base_len || (pathspec && pathspec->nr))
> return NULL;
>
> - /* Different set of flags may produce different results */
> - if (dir->flags != dir->untracked->dir_flags ||
This is removed because we are making sure that dir->flags and
dir->untracked->dir_flags match?
> - /*
> - * See treat_directory(), case index_nonexistent. Without
> - * this flag, we may need to also cache .git file content
> - * for the resolve_gitlink_ref() call, which we don't.
> - */
> - !(dir->flags & DIR_SHOW_OTHER_DIRECTORIES) ||
This is removed because...?
> - /* We don't support collecting ignore files */
> - (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |
> - DIR_COLLECT_IGNORED)))
> + /* We don't support collecting ignore files */
> + if (dir->flags & (DIR_SHOW_IGNORED | DIR_SHOW_IGNORED_TOO |
> + DIR_COLLECT_IGNORED))
> return NULL;
>
> /*
> @@ -2845,6 +2861,40 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d
> return NULL;
> }
>
> + /* We don't support using or preparing the untracked cache if
> + * the current effective flags don't match the configured
> + * flags.
> + */
Style (another one in large comment below).
> + configured_dir_flags = configured_default_dir_flags(istate);
> + if (dir->flags != configured_dir_flags)
> + return NULL;
Hmph. If this weren't necessary, this function does not need to
call configured_default_dir_flags(), and it can lose the
configured_dir_flags variable, too. Which means that
new_untracked_cache() function does not need to take the flags word
as a caller-supplied parameter. Instead, it can make a call to
configured_dir_flags() and assign the result to uc->dir_flags
itself, which would have been much nicer.
> + /* If the untracked structure we received does not have the same flags
> + * as configured, but the configured flags do match the effective flags,
> + * then we need to reset / create a new "untracked" structure to match
> + * the new config.
> + * Keeping the saved and used untracked cache in-line with the
> + * configuration provides an opportunity for frequent users of
> + * "git status -uall" to leverage the untracked cache by aligning their
> + * configuration (setting "status.showuntrackedfiles" to "all" or
> + * "normal" as appropriate), where previously this option was
> + * incompatible with untracked cache and *consistently* caused
> + * surprisingly bad performance (with fscache and fsmonitor enabled) on
> + * Windows.
> + *
> + * IMPROVEMENT OPPORTUNITY: If we reworked the untracked cache storage
> + * to not be as bound up with the desired output in a given run,
> + * and instead iterated through and stored enough information to
> + * correctly serve both "modes", then users could get peak performance
> + * with or without '-uall' regardless of their
> + * "status.showuntrackedfiles" config.
> + */
> + if (dir->flags != dir->untracked->dir_flags) {
> + free_untracked_cache(istate->untracked);
> + new_untracked_cache(istate, configured_dir_flags);
> + dir->untracked = istate->untracked;
> + }
This compensates what we lost in the validate_untracked_cache()
above by making them match. Looking reasonable.
> if (!dir->untracked->root)
> FLEX_ALLOC_STR(dir->untracked->root, name, "");
>
> @@ -2916,7 +2966,7 @@ int read_directory(struct dir_struct *dir, struct index_state *istate,
> return dir->nr;
> }
>
> - untracked = validate_untracked_cache(dir, len, pathspec);
> + untracked = validate_untracked_cache(dir, len, pathspec, istate);
> if (!untracked)
> /*
> * make sure untracked cache code path is disabled,
>
> base-commit: dab1b7905d0b295f1acef9785bb2b9cbb0fdec84
