revision: free diff options

There is a todo comment in `release_revisions()` that mentions that we
need to free the diff options, which was added via 54c8a7c (revisions
API: add a TODO for diff_free(&revs->diffopt), 2022-04-14). Releasing
the diff options wasn't quite feasible at that time because some call
sites rely on its contents to remain even after the revisions have been
released.

In fact, there really only are a couple of callsites that misbehave
here:

  - `cmd_shortlog()` releases the revisions, but continues to access its
    file pointer.

  - `do_diff_cache()` creates a shallow copy of `struct diff_options`,
    but does not set the `no_free` member. Consequently, we end up
    releasing resources of the caller-provided diff options.

  - `diff_free()` and friends do not play nice when being called
    multiple times as they don't unset data structures that they have
    just released.

Fix all of those cases and enable the call to `diff_free()`, which plugs
a bunch of memory leaks.

Signed-off-by: Patrick Steinhardt <[email protected]>
Signed-off-by: Junio C Hamano <[email protected]>

Author: pks-t

builtin/shortlog.c

@@ -460,11 +460,8 @@ int cmd_shortlog(int argc, const char **argv, const char *prefix)
 	else
 		get_from_rev(&rev, &log);
 
-	release_revisions(&rev);
-
 	shortlog_output(&log);
-	if (log.file != stdout)
-		fclose(log.file);
+	release_revisions(&rev);
 	return 0;
 }
 

diff-lib.c

@@ -662,9 +662,11 @@ int do_diff_cache(const struct object_id *tree_oid, struct diff_options *opt)
 	repo_init_revisions(opt->repo, &revs, NULL);
 	copy_pathspec(&revs.prune_data, &opt->pathspec);
 	revs.diffopt = *opt;
+	revs.diffopt.no_free = 1;
 
 	if (diff_cache(&revs, tree_oid, NULL, 1))
 		exit(128);
+
 	release_revisions(&revs);
 	return 0;
 }

diff.c

@@ -6649,8 +6649,10 @@ static void diff_flush_patch_all_file_pairs(struct diff_options *o)
 
 static void diff_free_file(struct diff_options *options)
 {
-	if (options->close_file)
+	if (options->close_file && options->file) {
 		fclose(options->file);
+		options->file = NULL;
+	}
 }
 
 static void diff_free_ignore_regex(struct diff_options *options)
@@ -6661,7 +6663,9 @@ static void diff_free_ignore_regex(struct diff_options *options)
 		regfree(options->ignore_regex[i]);
 		free(options->ignore_regex[i]);
 	}
-	free(options->ignore_regex);
+
+	FREE_AND_NULL(options->ignore_regex);
+	options->ignore_regex_nr = 0;
 }
 
 void diff_free(struct diff_options *options)

revision.c

@@ -3191,7 +3191,7 @@ void release_revisions(struct rev_info *revs)
 	release_revisions_mailmap(revs->mailmap);
 	free_grep_patterns(&revs->grep_filter);
 	graph_clear(revs->graph);
-	/* TODO (need to handle "no_free"): diff_free(&revs->diffopt) */
+	diff_free(&revs->diffopt);
 	diff_free(&revs->pruning);
 	reflog_walk_info_release(revs->reflog_info);
 	release_revisions_topo_walk_info(revs->topo_walk_info);

t/t4208-log-magic-pathspec.sh

@@ -5,6 +5,7 @@ test_description='magic pathspec tests using git-log'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success 'setup' '

t/t6000-rev-list-misc.sh

@@ -5,6 +5,7 @@ test_description='miscellaneous rev-list tests'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

t/t6001-rev-list-graft.sh

@@ -5,6 +5,7 @@ test_description='Revision traversal vs grafts and path limiter'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 test_expect_success setup '

t/t6013-rev-list-reverse-parents.sh

@@ -5,6 +5,7 @@ test_description='--reverse combines with --parents'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 

t/t6017-rev-list-stdin.sh

@@ -8,6 +8,7 @@ test_description='log family learns --stdin'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./test-lib.sh
 
 check () {

t/t9500-gitweb-standalone-no-errors.sh

@@ -13,6 +13,7 @@ or warnings to log.'
 GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME=main
 export GIT_TEST_DEFAULT_INITIAL_BRANCH_NAME
 
+TEST_PASSES_SANITIZE_LEAK=true
 . ./lib-gitweb.sh
 
 # ----------------------------------------------------------------------