diff.[ch]: have diff_free() call clear_pathspec(opts.pathspec)

avar · gitster · commit 244c27242f44 · 2022-02-16T13:50:13.000-08:00
Have the diff_free() function call clear_pathspec(). Since the diff_flush() function calls this all its callers can be simplified to rely on it instead. When I added the diff_free() function in e900d49 (diff: add an API for deferred freeing, 2021-02-11) I simply missed this, or wasn't interested in it. Let's consolidate this now. This means that any future callers (and I've got revision.c in mind) that embed a "struct diff_options" can simply call diff_free() instead of needing know that it has an embedded pathspec. This does fix a bunch of leaks, but I can't mark any test here as passing under the SANITIZE=leak testing mode because in 886e108 (builtin/: add UNLEAKs, 2017-10-01) an UNLEAK(rev) was added, which plasters over the memory leak. E.g. "t4011-diff-symlink.sh" would report fewer leaks with this fix, but because of the UNLEAK() reports none. I'll eventually loop around to removing that UNLEAK(rev) annotation as I'll fix deeper issues with the revisions API leaking. This is one small step on the way there, a new freeing function in revisions.c will want to call this diff_free(). Signed-off-by: Ævar Arnfjörð Bjarmason <avarab@gmail.com> Reviewed-by: Elijah Newren <newren@gmail.com> Signed-off-by: Junio C Hamano <gitster@pobox.com>
diff --git a/add-interactive.c b/add-interactive.c
@@ -797,14 +797,14 @@ static int run_revert(struct add_i_state *s, const struct pathspec *ps,
 	diffopt.flags.override_submodule_config = 1;
 	diffopt.repo = s->r;
 
-	if (do_diff_cache(&oid, &diffopt))
+	if (do_diff_cache(&oid, &diffopt)) {
+		diff_free(&diffopt);
 		res = -1;
-	else {
+	} else {
 		diffcore_std(&diffopt);
 		diff_flush(&diffopt);
 	}
 	free(paths);
-	clear_pathspec(&diffopt.pathspec);
 
 	if (!res && write_locked_index(s->r->index, &index_lock,
 				       COMMIT_LOCK) < 0)
diff --git a/blame.c b/blame.c
@@ -1403,7 +1403,6 @@ static struct blame_origin *find_origin(struct repository *r,
 		}
 	}
 	diff_flush(&diff_opts);
-	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -1447,7 +1446,6 @@ static struct blame_origin *find_rename(struct repository *r,
 		}
 	}
 	diff_flush(&diff_opts);
-	clear_pathspec(&diff_opts.pathspec);
 	return porigin;
 }
 
@@ -2328,7 +2326,6 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,
 	} while (unblamed);
 	target->suspects = reverse_blame(leftover, NULL);
 	diff_flush(&diff_opts);
-	clear_pathspec(&diff_opts.pathspec);
 }
 
 /*
diff --git a/builtin/reset.c b/builtin/reset.c
@@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,
 		return 1;
 	diffcore_std(&opt);
 	diff_flush(&opt);
-	clear_pathspec(&opt.pathspec);
 
 	return 0;
 }
diff --git a/diff.c b/diff.c
@@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)
 
 	diff_free_file(options);
 	diff_free_ignore_regex(options);
+	clear_pathspec(&options->pathspec);
 }
 
 void diff_flush(struct diff_options *options)
diff --git a/notes-merge.c b/notes-merge.c
@@ -175,7 +175,6 @@ static struct notes_merge_pair *diff_tree_remote(struct notes_merge_options *o,
 		       oid_to_hex(&mp->remote));
 	}
 	diff_flush(&opt);
-	clear_pathspec(&opt.pathspec);
 
 	*num_changes = len;
 	return changes;
@@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,
 		       oid_to_hex(&mp->local));
 	}
 	diff_flush(&opt);
-	clear_pathspec(&opt.pathspec);
 }
 
 static void check_notes_merge_worktree(struct notes_merge_options *o)

Original file line number	Diff line number	Diff line change
`@@ -1403,7 +1403,6 @@ static struct blame_origin find_origin(struct repository r,`
`1403`	`1403`	`}`
`1404`	`1404`	`}`
`1405`	`1405`	`diff_flush(&diff_opts);`
`1406`		`- clear_pathspec(&diff_opts.pathspec);`
`1407`	`1406`	`return porigin;`
`1408`	`1407`	`}`
`1409`	`1408`
`@@ -1447,7 +1446,6 @@ static struct blame_origin find_rename(struct repository r,`
`1447`	`1446`	`}`
`1448`	`1447`	`}`
`1449`	`1448`	`diff_flush(&diff_opts);`
`1450`		`- clear_pathspec(&diff_opts.pathspec);`
`1451`	`1449`	`return porigin;`
`1452`	`1450`	`}`
`1453`	`1451`
`@@ -2328,7 +2326,6 @@ static void find_copy_in_parent(struct blame_scoreboard *sb,`
`2328`	`2326`	`} while (unblamed);`
`2329`	`2327`	`target->suspects = reverse_blame(leftover, NULL);`
`2330`	`2328`	`diff_flush(&diff_opts);`
`2331`		`- clear_pathspec(&diff_opts.pathspec);`
`2332`	`2329`	`}`
`2333`	`2330`
`2334`	`2331`	`/*`
Original file line number	Diff line number	Diff line change
`@@ -274,7 +274,6 @@ static int read_from_tree(const struct pathspec *pathspec,`
`274`	`274`	`return 1;`
`275`	`275`	`diffcore_std(&opt);`
`276`	`276`	`diff_flush(&opt);`
`277`		`- clear_pathspec(&opt.pathspec);`
`278`	`277`
`279`	`278`	`return 0;`
`280`	`279`	`}`
Original file line number	Diff line number	Diff line change
`@@ -6345,6 +6345,7 @@ void diff_free(struct diff_options *options)`
`6345`	`6345`
`6346`	`6346`	`diff_free_file(options);`
`6347`	`6347`	`diff_free_ignore_regex(options);`
	`6348`	`+ clear_pathspec(&options->pathspec);`
`6348`	`6349`	`}`
`6349`	`6350`
`6350`	`6351`	`void diff_flush(struct diff_options *options)`
Original file line number	Diff line number	Diff line change
`@@ -175,7 +175,6 @@ static struct notes_merge_pair diff_tree_remote(struct notes_merge_options o,`
`175`	`175`	`oid_to_hex(&mp->remote));`
`176`	`176`	`}`
`177`	`177`	`diff_flush(&opt);`
`178`		`- clear_pathspec(&opt.pathspec);`
`179`	`178`
`180`	`179`	`*num_changes = len;`
`181`	`180`	`return changes;`
`@@ -261,7 +260,6 @@ static void diff_tree_local(struct notes_merge_options *o,`
`261`	`260`	`oid_to_hex(&mp->local));`
`262`	`261`	`}`
`263`	`262`	`diff_flush(&opt);`
`264`		`- clear_pathspec(&opt.pathspec);`
`265`	`263`	`}`
`266`	`264`
`267`	`265`	`static void check_notes_merge_worktree(struct notes_merge_options *o)`