Re: [PATCH v4] untracked-cache: support '--untracked-files=all' if configured

["Ævar Arnfjörð Bjarmason" <avarab@gmail.com>]

On Sun, Feb 27 2022, Tao Klerks via GitGitGadget wrote:

> From: Tao Klerks <tao@klerks.biz>
> [...]
>     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;
 
 	/*