This is a follow-up to v3 [1] from about a month ago. Patch 1 is unchanged; patch 2 provides some additional documentation of the initialization that is required, plus I've gotten rid of the compound literal. Range-diff below. Thanks Peff and brian for very helpful comments and discussion. Martin [1] https://public-inbox.org/git/cover.1548186510.git.martin.agren@xxxxxxxxx/ Martin Ågren (2): setup: free old value before setting `work_tree` setup: fix memory leaks with `struct repository_format` cache.h | 31 ++++++++++++++++++++++++++++--- builtin/init-db.c | 3 ++- repository.c | 3 ++- setup.c | 40 ++++++++++++++++++++++++++++------------ worktree.c | 4 +++- 5 files changed, 63 insertions(+), 18 deletions(-) Range-diff against v3: 1: 13019979b8 = 1: 13019979b8 setup: free old value before setting `work_tree` 2: e0c4a73119 ! 2: b21704c1e4 setup: fix memory leaks with `struct repository_format` @@ -16,15 +16,16 @@ they take from it, rather than stealing the pointers. Call `clear_...()` at the start of `read_...()` instead of just zeroing - the struct, since we sometimes enter the function multiple times. This - means that it is important to initialize the struct before calling - `read_...()`, so document that. Teach `read_...()` to clear the struct - upon an error, so that it is reset to a safe state, and document this. + the struct, since we sometimes enter the function multiple times. Thus, + it is important to initialize the struct before calling `read_...()`, so + document that. It's also important because we might not even call + `read_...()` before we call `clear_...()`, see, e.g., builtin/init-db.c. - About that very last point: In `setup_git_directory_gently()`, we look - at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we + Teach `read_...()` to clear the struct on error, so that it is reset to + a safe state, and document this. (In `setup_git_directory_gently()`, we + look at `repo_fmt.hash_algo` even if `repo_fmt.version` is -1, which we weren't actually supposed to do per the API. After this commit, that's - ok. + ok.) We inherit the existing code's combining "error" and "no version found". Both are signalled through `version == -1` and now both cause us to @@ -34,11 +35,21 @@ non-negative version number before using them. Signed-off-by: Martin Ågren <martin.agren@xxxxxxxxx> - Signed-off-by: Junio C Hamano <gitster@xxxxxxxxx> diff --git a/cache.h b/cache.h --- a/cache.h +++ b/cache.h +@@ + extern const char *core_partial_clone_filter_default; + extern int repository_format_worktree_config; + ++/* ++ * You _have_ to initialize a `struct repository_format` using ++ * `= REPOSITORY_FORMAT_INIT` before calling `read_repository_format()`. ++ */ + struct repository_format { + int version; + int precious_objects; @@ struct string_list unknown_extensions; }; @@ -146,8 +157,15 @@ } return 0; -@@ + } ++static void init_repository_format(struct repository_format *format) ++{ ++ const struct repository_format fresh = REPOSITORY_FORMAT_INIT; ++ ++ memcpy(format, &fresh, sizeof(fresh)); ++} ++ int read_repository_format(struct repository_format *format, const char *path) { - memset(format, 0, sizeof(*format)); @@ -167,7 +185,7 @@ + string_list_clear(&format->unknown_extensions, 0); + free(format->work_tree); + free(format->partial_clone); -+ *format = (struct repository_format)REPOSITORY_FORMAT_INIT; ++ init_repository_format(format); +} + int verify_repository_format(const struct repository_format *format, -- 2.21.0
