Re: [PATCH v4] untracked-cache: support '--untracked-files=all' if configured
From: Ævar Arnfjörð Bjarmason <hidden>
Date: 2022-02-28 14:08:42
Subsystem:
the rest · Maintainer:
Linus Torvalds
On Sun, Feb 27 2022, Tao Klerks via GitGitGadget wrote:
From: Tao Klerks <redacted>
[...]
The test suite passes, but as a n00b I do have questions:
* Is there any additional validation that could/should be done to
confirm that "-uall" untracked data can be cached safely?I haven't given this any substantial review, sorry.
* It looks safe from following the code
Yes, at a glance it looks safe to me.
* It seems from discussing briefly with Elijah Newren in the thread
above that thare are no obvious red flags
* Manual testing, explicitly and implicitly through months of use,
yields no issues
* Is it OK to check the repo configuration in the body of processing?
It seems to be a rare patternYes, it's not very common, but it's fine.
* Can anyone think of a way to test this change?
Well, if we set "flags" to 0 in your new helper the existing tests will fail, so that's something at least.
-static void new_untracked_cache(struct index_state *istate)
+static unsigned configured_default_dir_flags(struct repository *repo)
+{
+ /*
+ * 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()
+ */
+ char *status_untracked_files_config_value;
+ int config_outcome = repo_config_get_string(repo,
+ "status.showuntrackedfiles",
+ &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;
+ }
+}
+
[...]In reviewing this I found the addition of very long lines & indentation made this a bit harder to read. I came up with the below which should be squashable on top, and perhaps makes this easier to read. I.e. the one caller that needs custom flags passes them, others pass -1 now. For throwaway "char *" variables we usually use "char *p", "char *val" or whatever. A "status_untracked_files_config_value" is a bit much for something function local whose body is <10 lines of code. And if you drop the "int config_outcome" + rename the variable the repo_config_get_string() fits on one line:
diff --git a/dir.c b/dir.c
index 57a7d42482f..22a27d7780f 100644
--- a/dir.c
+++ b/dir.c@@ -2746,34 +2746,33 @@ static void set_untracked_ident(struct untracked_cache *uc) strbuf_addch(&uc->ident, 0); } -static unsigned configured_default_dir_flags(struct repository *repo) +static unsigned new_untracked_cache_flags(struct index_state *istate) { + struct repository *repo = istate->repo; + char *val; + /* * 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() */ - char *status_untracked_files_config_value; - int config_outcome = repo_config_get_string(repo, - "status.showuntrackedfiles", - &status_untracked_files_config_value); - if (!config_outcome && !strcmp(status_untracked_files_config_value, "all")) { + if (!repo_config_get_string(repo, "status.showuntrackedfiles", &val) && + !strcmp(val, "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; - } + + /* + * 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; } -static void new_untracked_cache(struct index_state *istate, unsigned flags) +static void new_untracked_cache(struct index_state *istate, int flags) { struct untracked_cache *uc = xcalloc(1, sizeof(*uc)); strbuf_init(&uc->ident, 100); uc->exclude_per_dir = ".gitignore"; - uc->dir_flags = flags; + uc->dir_flags = flags >= 0 ? flags : new_untracked_cache_flags(istate); set_untracked_ident(uc); istate->untracked = uc; istate->cache_changed |= UNTRACKED_CHANGED;
@@ -2782,13 +2781,11 @@ static void new_untracked_cache(struct index_state *istate, unsigned flags) void add_untracked_cache(struct index_state *istate) { if (!istate->untracked) { - new_untracked_cache(istate, - configured_default_dir_flags(istate->repo)); + new_untracked_cache(istate, -1); } else { if (!ident_in_untracked(istate->untracked)) { free_untracked_cache(istate->untracked); - new_untracked_cache(istate, - configured_default_dir_flags(istate->repo)); + new_untracked_cache(istate, -1); } } }
@@ -2866,7 +2863,7 @@ static struct untracked_cache_dir *validate_untracked_cache(struct dir_struct *d * the current effective flags don't match the configured * flags. */ - if (dir->flags != configured_default_dir_flags(istate->repo)) + if (dir->flags != new_untracked_cache_flags(istate)) return NULL; /*