Re: [PATCH v4 06/27] revision.[ch]: provide and start using a release_revisions()

> The users of the revision.[ch] API's "struct rev_info" are a major
> source of memory leaks in the test suite under SANITIZE=leak, which in
> turn adds a lot of noise when trying to mark up tests with
> "TEST_PASSES_SANITIZE_LEAK=true".
> 
> The users of that API are largely one-shot, e.g. "git log", so
> arguably freeing the memory is a waste of time, but in many cases
> they've actually been trying to free the memory, and just doing that
> in a buggy manner.

> Let's provide a release_revisions() function for these users, and
> start migrating them over per the plan outlined in [1]. Right now this
> only handles the "pending" member of the struct, but more will be
> added in subsequent commits.
> 
> Even though we only clear the "pending" member now, let's not leave a
> trap in code like the pre-image of index_differs_from(), where we'd
> start doing the wrong thing as soon as the release_revisions() learned
> to clear its "diffopt". I.e. we need to call release_revisions() after
> we've inspected any state in "struct rev_info".
> 
> This leaves in place e.g. clear_pathspec(&rev.prune_data) in
> stash_working_tree() in builtin/stash.c, subsequent commits will teach
> release_revisions() to free "prune_data" and other members that in
> some cases are individually cleared by users of "struct rev_info" by
> reaching into its members. Those subsequent commits will remove the
> relevant calls to e.g. clear_pathspec().
> 
> We avoid amending code in index_differs_from() in diff-lib.c as well
> as wt_status_collect_changes_index(), has_unstaged_changes() and
> has_uncommitted_changes() in wt-status.c in a way that assumes that we
> are already clearing the "diffopt" member. That will be handled in a
> subsequent commit.
> 
> 1. https://lore.kernel.org/git/87a6k8daeu.fsf@evledraar.gmail.com/
> 
> Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com>
> ---
>   builtin/checkout.c | 2 +-
>   builtin/stash.c    | 3 +--
>   diff-lib.c         | 2 +-
>   range-diff.c       | 2 +-
>   revision.c         | 5 +++++
>   revision.h         | 6 ++++++
>   wt-status.c        | 5 +++--
>   7 files changed, 18 insertions(+), 7 deletions(-)
> 
> diff --git a/builtin/checkout.c b/builtin/checkout.c
> index 797681481d1..4d9e0bd3ac1 100644
> --- a/builtin/checkout.c
> +++ b/builtin/checkout.c
> @@ -629,7 +629,7 @@ static void show_local_changes(struct object *head,
>   	diff_setup_done(&rev.diffopt);
>   	add_pending_object(&rev, head, NULL);
>   	run_diff_index(&rev, 0);
> -	object_array_clear(&rev.pending);
> +	release_revisions(&rev);
>   }
>   
>   static void describe_detached_head(const char *msg, struct commit *commit)
> diff --git a/builtin/stash.c b/builtin/stash.c
> index ccdfdab44be..ad74624c2f7 100644
> --- a/builtin/stash.c
> +++ b/builtin/stash.c
> @@ -1266,9 +1266,8 @@ static int stash_working_tree(struct stash_info *info, const struct pathspec *ps
>   
>   done:
>   	discard_index(&istate);
> -	UNLEAK(rev);
> -	object_array_clear(&rev.pending);
>   	clear_pathspec(&rev.prune_data);
> +	release_revisions(&rev);
>   	strbuf_release(&diff_output);
>   	remove_path(stash_index_path.buf);
>   	return ret;
> diff --git a/diff-lib.c b/diff-lib.c
> index ca085a03efc..d6800274bd5 100644
> --- a/diff-lib.c
> +++ b/diff-lib.c
> @@ -662,7 +662,7 @@ int index_differs_from(struct repository *r,
>   		diff_flags_or(&rev.diffopt.flags, flags);
>   	rev.diffopt.ita_invisible_in_index = ita_invisible_in_index;
>   	run_diff_index(&rev, 1);
> -	object_array_clear(&rev.pending);
> +	release_revisions(&rev);
>   	return (rev.diffopt.flags.has_changes != 0);
>   }
>   
> diff --git a/range-diff.c b/range-diff.c
> index b72eb9fdbee..39cc010c628 100644
> --- a/range-diff.c
> +++ b/range-diff.c
> @@ -596,6 +596,6 @@ int is_range_diff_range(const char *arg)
>   	}
>   
>   	free(copy);
> -	object_array_clear(&revs.pending);
> +	release_revisions(&revs);
>   	return negative > 0 && positive > 0;
>   }
> diff --git a/revision.c b/revision.c
> index 29f102cb83d..9a3c05b69a5 100644
> --- a/revision.c
> +++ b/revision.c
> @@ -2931,6 +2931,11 @@ static void release_revisions_commit_list(struct rev_info *revs)
>   	}
>   }
>   
> +void release_revisions(struct rev_info *revs)
> +{
> +	object_array_clear(&revs->pending);
> +}
> +
>   static void add_child(struct rev_info *revs, struct commit *parent, struct commit *child)
>   {
>   	struct commit_list *l = xcalloc(1, sizeof(*l));
> diff --git a/revision.h b/revision.h
> index 5bc59c7bfe1..61c780fc4cd 100644
> --- a/revision.h
> +++ b/revision.h
> @@ -377,6 +377,12 @@ void repo_init_revisions(struct repository *r,
>   int setup_revisions(int argc, const char **argv, struct rev_info *revs,
>   		    struct setup_revision_opt *);
>   
> +/**
> + * Free data allocated in a "struct rev_info" after it's been
> + * initialized with repo_init_revisions().
> + */
> +void release_revisions(struct rev_info *revs);
> +
>   void parse_revision_opt(struct rev_info *revs, struct parse_opt_ctx_t *ctx,
>   			const struct option *options,
>   			const char * const usagestr[]);
> diff --git a/wt-status.c b/wt-status.c
> index d33f9272b72..922cf787f95 100644
> --- a/wt-status.c
> +++ b/wt-status.c
> @@ -662,7 +662,7 @@ static void wt_status_collect_changes_index(struct wt_status *s)
>   
>   	copy_pathspec(&rev.prune_data, &s->pathspec);
>   	run_diff_index(&rev, 1);
> -	object_array_clear(&rev.pending);
> +	release_revisions(&rev);
>   	clear_pathspec(&rev.prune_data);
>   }
>   
> @@ -2545,6 +2545,7 @@ int has_unstaged_changes(struct repository *r, int ignore_submodules)
>   	rev_info.diffopt.flags.quick = 1;
>   	diff_setup_done(&rev_info.diffopt);
>   	result = run_diff_files(&rev_info, 0);
> +	release_revisions(&rev_info);
>   	return diff_result_code(&rev_info.diffopt, result);
>   }
>   
> @@ -2577,7 +2578,7 @@ int has_uncommitted_changes(struct repository *r,
>   
>   	diff_setup_done(&rev_info.diffopt);
>   	result = run_diff_index(&rev_info, 1);
> -	object_array_clear(&rev_info.pending);
> +	release_revisions(&rev_info);
>   	return diff_result_code(&rev_info.diffopt, result);
>   }
>