On Sun, Feb 27 2022, Tao Klerks via GitGitGadget wrote:
> From: Tao Klerks <tao@xxxxxxxxxx>
> [...]
> 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 pattern
Yes, 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;
/*
