git/list[1] front-page[2] threads[3] people[4] search[5] about
 

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

From
Ævar Arnfjörð Bjarmason <avarab@gmail.com>
Date
Feb 28, 2022, 14:02 UTC
Message-ID
<220228.86y21v3wyj.gmgdl@evledraar.gmail.com>
In-Reply-To
<pull.985.v4.git.1645974782256.gitgitgadget@gmail.com>
On Sun, Feb 27 2022, Tao Klerks via GitGitGadget wrote:
Show 6 quoted lines
> 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.
Show 6 quoted lines
>        * 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.

Show 24 quoted lines
> -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;
 
 	/*
Previous: Tao Klerks via GitGitGadgetNext: Tao Klerks
Message 9 of 22 in “Support untracked cache with '--untracked-files=all' if configured”
  1. Support untracked cache with '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Jun 23, 2021
  2. untracked-cache: support '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Feb 25, 2022
  3. Junio C HamanoFeb 25, 2022
  4. Tao KlerksFeb 27, 2022
  5. Junio C HamanoFeb 27, 2022
  6. untracked-cache: support '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Feb 27, 2022
  7. Tao KlerksFeb 27, 2022
  8. untracked-cache: support '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Feb 27, 2022
  9. Ævar Arnfjörð BjarmasonFeb 28, 2022
  10. Tao KlerksMar 2, 2022
  11. 0/2 Support untracked cache with '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Mar 29, 2022
  12. 1/2 untracked-cache: test untracked-cache-bypassing behavior with -uallTao Klerks via GitGitGadget, Mar 29, 2022
  13. Junio C HamanoMar 29, 2022
  14. Tao KlerksMar 30, 2022
  15. Junio C HamanoMar 30, 2022
  16. Tao KlerksMar 31, 2022
  17. 2/2 untracked-cache: support '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Mar 29, 2022
  18. Junio C HamanoMar 29, 2022
  19. Tao KlerksMar 30, 2022
  20. 0/2 Support untracked cache with '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Mar 31, 2022
  21. 1/2 untracked-cache: test untracked-cache-bypassing behavior with -uallTao Klerks via GitGitGadget, Mar 31, 2022
  22. 2/2 untracked-cache: support '--untracked-files=all' if configuredTao Klerks via GitGitGadget, Mar 31, 2022

Read the whole thread, see it on lore, or plain text.

$ cat FOOTERMessages come from the public archive at lore.kernel.org/git, fetched every hour. The front page is chosen and written each morning by an AI editor and can be wrong; the threads themselves are the record. About and API. For agents: an MCP server at https://gitlist.dev/mcp, and any thread, story or person page as Markdown by adding .md to its URL (or sending Accept: text/markdown). Details in /llms.txt.