Re: [PATCH 3/5] mv: move src_dir cleanup to end of cmd_mv()

[Jeff King <peff@peff.net>]

On Thu, May 30, 2024 at 09:24:34AM +0200, Patrick Steinhardt wrote:

> > Looks like no. The obvious candidate would be t7002-mv-sparse-checkout,
> > but it looks like the sparse-checkout code has minor leaks itself.
> 
> Okay, thanks for double checking! I was mostly asking because I plan to
> send another leak fixes series to the mailing list later this week.

OK, good news. t7002 _does_ trigger the leak fixed in my patch. You just
can't tell because of all of the sparse-checkout leaks. ;)

The (messy) patch below gets it to a leak-free state when applied on
top. Do you want me to do another mini-series with it, or do you want to
just roll it into what you're doing (I won't be surprised if you've
already found some of these).

-Peff

diff --git a/builtin/sparse-checkout.c b/builtin/sparse-checkout.c
index 0f52e25249..1ed9dfa886 100644
--- a/builtin/sparse-checkout.c
+++ b/builtin/sparse-checkout.c
@@ -311,6 +311,8 @@ static void write_cone_to_file(FILE *fp, struct pattern_list *pl)
 		fprintf(fp, "%s/\n", pattern);
 		free(pattern);
 	}
+
+	string_list_clear(&sl, 0);
 }
 
 static int write_patterns_and_update(struct pattern_list *pl)
@@ -471,6 +473,7 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)
 	/* If we already have a sparse-checkout file, use it. */
 	if (res >= 0) {
 		free(sparse_filename);
+		clear_pattern_list(&pl);
 		return update_working_directory(NULL);
 	}
 
@@ -486,6 +489,7 @@ static int sparse_checkout_init(int argc, const char **argv, const char *prefix)
 			die(_("failed to open '%s'"), sparse_filename);
 
 		free(sparse_filename);
+		clear_pattern_list(&pl);
 		fprintf(fp, "/*\n!/*/\n");
 		fclose(fp);
 		return 0;
@@ -525,6 +529,10 @@ static void insert_recursive_pattern(struct pattern_list *pl, struct strbuf *pat
 
 		if (!hashmap_get_entry(&pl->parent_hashmap, e, ent, NULL))
 			hashmap_add(&pl->parent_hashmap, &e->ent);
+		else {
+			free(e->pattern);
+			free(e);
+		}
 	}
 }
 
@@ -891,7 +899,6 @@ static int sparse_checkout_disable(int argc, const char **argv,
 		OPT_END(),
 	};
 	struct pattern_list pl;
-	struct strbuf match_all = STRBUF_INIT;
 
 	/*
 	 * We do not exit early if !core_apply_sparse_checkout; due to the
@@ -917,8 +924,7 @@ static int sparse_checkout_disable(int argc, const char **argv,
 	pl.use_cone_patterns = 0;
 	core_apply_sparse_checkout = 1;
 
-	strbuf_addstr(&match_all, "/*");
-	add_pattern(strbuf_detach(&match_all, NULL), empty_base, 0, &pl, 0);
+	add_pattern("/*", empty_base, 0, &pl, 0);
 
 	prepare_repo_settings(the_repository);
 	the_repository->settings.sparse_index = 0;
diff --git a/dir.c b/dir.c
index 2d83f3311a..5769c4e693 100644
--- a/dir.c
+++ b/dir.c
@@ -799,6 +799,8 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 
 	if (given->patternlen > 2 &&
 	    !strcmp(given->pattern + given->patternlen - 2, "/*")) {
+		struct pattern_entry *old;
+
 		if (!(given->flags & PATTERN_FLAG_NEGATIVE)) {
 			/* Not a cone pattern. */
 			warning(_("unrecognized pattern: '%s'"), given->pattern);
@@ -824,7 +826,11 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 		}
 
 		hashmap_add(&pl->parent_hashmap, &translated->ent);
-		hashmap_remove(&pl->recursive_hashmap, &translated->ent, &data);
+		old = hashmap_remove_entry(&pl->recursive_hashmap, translated, ent, &data);
+		if (old) {
+			free(old->pattern);
+			free(old);
+		}
 		free(data);
 		return;
 	}
@@ -855,6 +861,7 @@ static void add_pattern_to_hashsets(struct pattern_list *pl, struct path_pattern
 
 clear_hashmaps:
 	warning(_("disabling cone pattern matching"));
+	/* should free ent->pattern too; refactor clear_pattern_list? */
 	hashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);
 	hashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);
 	pl->use_cone_patterns = 0;
@@ -950,13 +957,20 @@ static int read_skip_worktree_file_from_index(struct index_state *istate,
  */
 void clear_pattern_list(struct pattern_list *pl)
 {
+	struct hashmap_iter iter;
+	struct pattern_entry *entry;
 	int i;
 
 	for (i = 0; i < pl->nr; i++)
 		free(pl->patterns[i]);
 	free(pl->patterns);
 	free(pl->filebuf);
+
+	hashmap_for_each_entry(&pl->recursive_hashmap, &iter, entry, ent)
+		free(entry->pattern);
 	hashmap_clear_and_free(&pl->recursive_hashmap, struct pattern_entry, ent);
+	hashmap_for_each_entry(&pl->parent_hashmap, &iter, entry, ent)
+		free(entry->pattern);
 	hashmap_clear_and_free(&pl->parent_hashmap, struct pattern_entry, ent);
 
 	memset(pl, 0, sizeof(*pl));
-- 
2.45.1.692.gbe047d9c60