[PATCH 3/3] setup: add `clear_repository_format()`

[Martin Ågren <martin.agren@xxxxxxxxx>]

After we set up a `struct repository_format`, it owns various pieces of
allocated memory. We then either use those members, because we decide we
want to use the "candidate" repository format, or we discard the
candidate / scratch space. In the first case, we transfer ownership of
the memory to a few global variables. In the latter case, we just
silently drop the struct and end up leaking memory.

Introduce a function `clear_repository_format()` which frees the memory
the struct holds on to. Call it in the code paths where we currently
leak the memory. Also call it in the error path of
`read_repository_format()` to clean up any partial result.

For hygiene, we need to at least set the pointers that we free to NULL.
For future-proofing, let's zero the entire struct instead. It just means
that in the error path of `read_...()` we need to restore the error
sentinel in the `version` field.

We could take this opportunity to stop claiming that all fields except
`version` are undefined in case of an error. On the other hand, having
them defined as zero is not much better than having them undefined. We
could define them to some fallback configuration (`is_bare = -1` and
`hash_algo = GIT_HASH_SHA1`?), but "clear()" and/or "read()" seem like
the wrong places to enforce fallback configurations. Let's leave things
as "undefined" instead to encourage users to check `version`.

Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx>
---
 The error state can always be defined later. Defining it now, then
 trying to backpedal, is probably not so fun. Filling the struct with
 non-zero values might help flush out bugs like the one fixed in the
 previous patch, but I'm wary of going that far in this patch.

 cache.h      |  6 ++++++
 repository.c |  1 +
 setup.c      | 14 ++++++++++++++
 3 files changed, 21 insertions(+)

diff --git a/cache.h b/cache.h
index 8b9e592c65..53ac01efa7 100644
--- a/cache.h
+++ b/cache.h
@@ -979,6 +979,12 @@ struct repository_format {
  */
 void read_repository_format(struct repository_format *format, const char *path);
 
+/*
+ * Free the memory held onto by `format`, but not the struct itself.
+ * (No need to use this after `read_repository_format()` fails.)
+ */
+void clear_repository_format(struct repository_format *format);
+
 /*
  * Verify that the repository described by repository_format is something we
  * can read. If it is, return 0. Otherwise, return -1, and "err" will describe
diff --git a/repository.c b/repository.c
index 5dd1486718..efa9d1d960 100644
--- a/repository.c
+++ b/repository.c
@@ -159,6 +159,7 @@ int repo_init(struct repository *repo,
 	if (worktree)
 		repo_set_worktree(repo, worktree);
 
+	clear_repository_format(&format);
 	return 0;
 
 error:
diff --git a/setup.c b/setup.c
index 52c3c9d31f..babe5ea156 100644
--- a/setup.c
+++ b/setup.c
@@ -517,6 +517,18 @@ void read_repository_format(struct repository_format *format, const char *path)
 	format->hash_algo = GIT_HASH_SHA1;
 	string_list_init(&format->unknown_extensions, 1);
 	git_config_from_file(check_repo_format, path, format);
+	if (format->version == -1) {
+		clear_repository_format(format);
+		format->version = -1;
+	}
+}
+
+void clear_repository_format(struct repository_format *format)
+{
+	string_list_clear(&format->unknown_extensions, 0);
+	free(format->work_tree);
+	free(format->partial_clone);
+	memset(format, 0, sizeof(*format));
 }
 
 int verify_repository_format(const struct repository_format *format,
@@ -1043,9 +1055,11 @@ int discover_git_directory(struct strbuf *commondir,
 		strbuf_release(&err);
 		strbuf_setlen(commondir, commondir_offset);
 		strbuf_setlen(gitdir, gitdir_offset);
+		clear_repository_format(&candidate);
 		return -1;
 	}
 
+	clear_repository_format(&candidate);
 	return 0;
 }
 
-- 
2.20.1